modem_shell/serial_lte_modem: Replace IPPROTO_RAW with ETH_P_ALL #20977

rlubos · 2025-03-17T14:23:03Z

It turns out that AF_PACKET and IPPROTO_RAW is not a valid combination (see zephyrproject-rtos/zephyr#86827) and will most likely be removed from Zephyr. An alternative for this would be to use AF_PACKET datagram socket with some extra tweaks specific for IP packet recognition in PPP L2 as proposed in this PR.

@nrfconnect/ncs-modem-tre Is PPP functionality in MoSH covered in Jenkins CI? I've tested the change on desk and it seems to work fine, although I would be more confident if this got some extra testing.

@nrfconnect/ncs-iot-oulu Same question - is PPP functionality in SLM covered in Jenkins? Unfortunately SLM refused to cooperate with me and I did not manage to verify this on desk, the change set is similar to MoSH though. It would be good to actually verify this however.

NordicBuilder · 2025-03-17T14:25:28Z

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
zephyr	nrfconnect/sdk-zephyr@`5319cfc`	nrfconnect/sdk-zephyr#2628	nrfconnect/sdk-zephyr#2628/files

⛔ DNM label due to: 1 project with PR revision

Note: This message is automatically posted and updated by the Manifest GitHub Action.

NordicBuilder · 2025-03-17T14:29:10Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 9

Inputs:

Sources:

sdk-nrf: PR head: 5b1426208a8791d910551f0edb87279a114e4369
zephyr: PR head: 11ad60a2970b192c86e86ae91f21da3de98e6f43

more details

sdk-nrf:

PR head: 5b1426208a8791d910551f0edb87279a114e4369
merge base: ac06821f5c71138aff195069a19001af0d847cc2
target head (main): 6f6a3c2e51bc982acf0c735f973a8dcbfd830c4b
Diff

zephyr:

PR head: 11ad60a2970b192c86e86ae91f21da3de98e6f43
merge base: 5319cfc0718146a1bb21a856415d9b50abbf979e
target head (main): 6bf2702919b980f7a825f6fdc965cfaf81d9ac70
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (15)

applications
│  ├── serial_lte_modem
│  │  ├── src
│  │  │  │ slm_ppp.c
samples
│  ├── cellular
│  │  ├── modem_shell
│  │  │  ├── overlay-ppp.conf
│  │  │  ├── src
│  │  │  │  ├── ppp
│  │  │  │  │  ├── ppp_ctrl.c
│  │  │  │  │  │ ppp_mdm_data_rcv.c
west.yml
zephyr
│  ├── subsys
│  │  ├── net
│  │  │  ├── ip
│  │  │  │  ├── ipv4.c
│  │  │  │  ├── ipv6.c
│  │  │  │  ├── ipv6_nbr.c
│  │  │  │  │ net_context.c
│  │  │  ├── l2
│  │  │  │  ├── ethernet
│  │  │  │  │  ├── arp.c
│  │  │  │  │  ├── ethernet.c
│  │  │  │  │  ├── gptp
│  │  │  │  │  │  │ gptp_messages.c
│  │  │  │  │  ├── lldp
│  │  │  │  │  │  │ lldp.c
│  │  │  │  ├── ppp
│  │  │  │  │  │ ppp_l2.c
│  ├── tests
│  │  ├── net
│  │  │  ├── arp
│  │  │  │  ├── src
│  │  │  │  │  │ main.c

Outputs:

Toolchain

Version: 4ffa2202d5
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:4ffa2202d5_e579f9fbe6

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister - Skipped: Skipping Build & Test as it succeeded in a previous run: 7
✅ Integration tests
- ✅ test-fw-nrfconnect-chip - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nfc - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_cloud - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_serial_lte_modem - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_zephyr_lwm2m - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_samples - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_thingy91 - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-rs - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-fem - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-thread - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-find-my
- ✅ test-fw-nrfconnect-nrf-iot_mosh - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_positioning - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-wifi - Skipped: Job was skipped as it succeeded in a previous run

Disabled integration tests

- desktop52_verification
- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-boot
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-proprietary_esb
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-zigbee
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-mcuboot
- test-sdk-pmic-samples
- test-sdk-sidewalk
- test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

github-actions · 2025-03-17T14:45:53Z

You can find the documentation preview for this PR here.

MarkusLassila

SLM has tests for PPP, but only in nightly environment. Tried out these changes manually and it seems that PPP link can be brought up and data will flow through.

So seems to work correctly. In any case, now is a good time for this kind of change as we are actively working with SLM PPP.

applications/serial_lte_modem/src/slm_ppp.c

The combination of AF_PACKET and IPPROTO_RAW is not correct as packet sockets should only be used with IEEE 802.3 protocol numbers. As the support for this combination will likely be soon removed from Zephyr, switch to use DGRAM packet socket instead. As PPP L2 only supports IP/IPv6 packets, specify the L2 protocol type field accordingly so that the L2 can interpret the packet data correctly. Signed-off-by: Robert Lubos <[email protected]>

trantanen · 2025-03-18T10:10:50Z

It turns out that AF_PACKET and IPPROTO_RAW is not a valid combination (see zephyrproject-rtos/zephyr#86827) and will most likely be removed from Zephyr.

Outside of PPP, MOSH is using raw sockets by specifying AF_PACKET, SOCK_RAW and leaving proto=0, i.e., not defining any IPPROTO_*.

@nrfconnect/ncs-modem-tre Is PPP functionality in MoSH covered in Jenkins CI? I've tested the change on desk and it seems to work fine, although I would be more confident if this got some extra testing.

PPP is not covered in the integration CI but there is something in the nightly for MOSH PPP. We'll try to see if we can get that run somehow for a PPP image.

@jhirsi, who would be more knowledgeable on this topic in our side, is out for this and next week but we'll try to handle this.

rlubos · 2025-03-18T10:34:57Z

It turns out that AF_PACKET and IPPROTO_RAW is not a valid combination (see zephyrproject-rtos/zephyr#86827) and will most likely be removed from Zephyr.

Outside of PPP, MOSH is using raw sockets by specifying AF_PACKET, SOCK_RAW and leaving proto=0, i.e., not defining any IPPROTO_*.

Hmm, but from what I see this is only for modem sockets? Those shouldn't be affected by whatever changes are made in Zephyr.

A separate question is whether such a combination is a valid one for the expected use case. After doing some research the conclusion is those RAW sockets are not well standardized and rather unportable. @jukkar has prepared a nice summary based on various sources: zephyrproject-rtos/zephyr#86827 (comment)
So in Linux, such socket would only allow for TX, but I assume in modem it just forwards all packets in and out.

Pull changes needed to make datagram packet sockets work with PPP L2. Signed-off-by: Robert Lubos <[email protected]>

sonarqubecloud · 2025-03-18T11:06:44Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

trantanen · 2025-03-18T11:09:50Z

Outside of PPP, MOSH is using raw sockets by specifying AF_PACKET, SOCK_RAW and leaving proto=0, i.e., not defining any IPPROTO_*.

Hmm, but from what I see this is only for modem sockets? Those shouldn't be affected by whatever changes are made in Zephyr.

Yep, that's only for modem sockets at the moment.

A separate question is whether such a combination is a valid one for the expected use case. After doing some research the conclusion is those RAW sockets are not well standardized and rather unportable. @jukkar has prepared a nice summary based on various sources: zephyrproject-rtos/zephyr#86827 (comment)

Nice summary.

So in Linux, such socket would only allow for TX, but I assume in modem it just forwards all packets in and out.

Yep, modem should be just a passthrough. Good to know that this wouldn't work in a general manner and not without offloading.

trantanen

Approving from modem_shell side. PPP nightly test passes.

rlubos · 2025-03-18T12:58:29Z

Approving from modem_shell side. PPP nightly test passes.

Awesome, thanks 👍

lemrey · 2025-03-18T16:09:08Z

There is a documentation page for how the user is supposed to use RAW sockets with the modem: https://github.com/nrfconnect/sdk-nrfxlib/blob/main//nrf_modem/doc/sockets/raw_sockets.rst.

For a while 0 has been the documented protocol for opening RAW sockets on the modem. If I am not mistaken there was a recent patch to nrf_modem to allow NRF_IPPROTO_RAW (with NRF_AF_PACKET), which is exactly the combination that is being removed in Zephyr..

Does it make sense for nrf_socket() to accept NRF_AF_PACKET and NRF_IPPROTO_RAW or is it something we need to adjust?

rlubos · 2025-03-19T08:57:05Z

Does it make sense for nrf_socket() to accept NRF_AF_PACKET and NRF_IPPROTO_RAW or is it something we need to adjust?

IMHO it'd be better to stick to the current behavior (0 as protocol wildcard). As I mentioned earlier, packet sockets are not really standardized so we can only rely on well-known implementations (like Linux), and according to their documentation of AF_PACKET (https://man7.org/linux/man-pages/man7/packet.7.html), protocol field should be:

protocol is the IEEE 802.3 protocol number in network byte order.

And IPPROTO_RAW is not a valid protocol number according to this.

NordicBuilder · 2025-04-21T00:38:55Z

This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 7 days. Note, that you can always re-open a closed pull request at any time.

rlubos · 2025-04-28T10:35:42Z

Will come with the upmerge.

rlubos requested review from a team as code owners March 17, 2025 14:23

github-actions bot added manifest changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Mar 17, 2025

rlubos force-pushed the mosh-ppp-socket-swap branch from 18b98d6 to 2f91c34 Compare March 17, 2025 14:24

NordicBuilder added manifest-zephyr DNM labels Mar 17, 2025

MarkusLassila approved these changes Mar 18, 2025

View reviewed changes

applications/serial_lte_modem/src/slm_ppp.c Outdated Show resolved Hide resolved

rlubos added 2 commits March 18, 2025 09:12

rlubos force-pushed the mosh-ppp-socket-swap branch from 2f91c34 to 5cda357 Compare March 18, 2025 08:13

manifest: Update sdk-zephyr with networking changes

5b14262

Pull changes needed to make datagram packet sockets work with PPP L2. Signed-off-by: Robert Lubos <[email protected]>

rlubos force-pushed the mosh-ppp-socket-swap branch from 5cda357 to 5b14262 Compare March 18, 2025 11:02

trantanen approved these changes Mar 18, 2025

View reviewed changes

SeppoTakalo approved these changes Mar 20, 2025

View reviewed changes

NordicBuilder added the Stale label Apr 21, 2025

rlubos closed this Apr 28, 2025

modem_shell/serial_lte_modem: Replace IPPROTO_RAW with ETH_P_ALL #20977

modem_shell/serial_lte_modem: Replace IPPROTO_RAW with ETH_P_ALL #20977

Uh oh!

Conversation

rlubos commented Mar 17, 2025

Uh oh!

NordicBuilder commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NordicBuilder commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

Uh oh!

github-actions bot commented Mar 17, 2025

Uh oh!

MarkusLassila left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

trantanen commented Mar 18, 2025

Uh oh!

rlubos commented Mar 18, 2025

Uh oh!

sonarqubecloud bot commented Mar 18, 2025

Quality Gate passed

Uh oh!

trantanen commented Mar 18, 2025

Uh oh!

trantanen left a comment

Choose a reason for hiding this comment

Uh oh!

rlubos commented Mar 18, 2025

Uh oh!

lemrey commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rlubos commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NordicBuilder commented Apr 21, 2025

Uh oh!

rlubos commented Apr 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

NordicBuilder commented Mar 17, 2025 •

edited

Loading

NordicBuilder commented Mar 17, 2025 •

edited

Loading

MarkusLassila left a comment •

edited

Loading

lemrey commented Mar 18, 2025 •

edited

Loading

rlubos commented Mar 19, 2025 •

edited

Loading