Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/lwip: fix wrong icmp chksum in IPv4 #19853

Closed

Conversation

krzysztof-cabaj
Copy link
Contributor

Contribution description

Device with RIOT OS and LWIP IPv4 stack did not respond to ping from Windows hosts. Linux hosts could
ping device without problems. In the network trace icmp responses with chksum set to 0 could be observed.
Additionally Wireshark flag icmp requests as "no response found!". The cause of such behavior is icmp chksum
set to 0.

This PR enables calculation of icmp chksum in the LWIP stack.

I tested this behavior and PR using LWIP 2.2.0 provided by PR #19780 - the stack behaves in this same manner and
this PR fix problem.

Testing procedure

Run any LWIP code, for e.g. examples/coap with enabled IPv4 stack. To do this:

  1. in Makefile, line 17 change:
LWIP_IPV4 ?= 0

to

LWIP_IPV4 ?= 1
  1. add to main.c code which configure IPv4 address
include "lwip/netif.h"

and in main function:

#define _TEST_ADDR4_LOCAL  (0x964fa8c0U)   /* 192.168.79.150 */
#define _TEST_ADDR4_MASK   (0x00ffffffU)   /* 255.255.255.0 */
sys_lock_tcpip_core();
struct netif *iface = netif_find("ET0");

ip4_addr_t ip, subnet;
ip.addr = _TEST_ADDR4_LOCAL;
subnet.addr = _TEST_ADDR4_MASK;
netif_set_addr(iface, &ip, &subnet, NULL);
sys_unlock_tcpip_core();

Ping 192.168.79.150 from Windows machine - without this PR ping should fail.
I tested this PR code in hardware - nucleo-f207zg.

Issues/PRs references

Tested with PR #19780 .

@github-actions github-actions bot added Area: network Area: Networking Area: pkg Area: External package ports labels Aug 1, 2023
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 1, 2023
@benpicco benpicco requested a review from yarrick August 1, 2023 21:10
@riot-ci
Copy link

riot-ci commented Aug 1, 2023

Murdock results

✔️ PASSED

f7f16f8 pkg/lwip: enables flags only for choosen Nucleo boards

Success Failures Total Runtime
7937 0 7937 14m:15s

Artifacts

@yarrick
Copy link
Contributor

yarrick commented Sep 17, 2023

Without this change I am seeing ICMP checksums working for IPv4 and IPv6.

I used $ LWIP_IPV4=1 make -C tests/pkg/lwip flash
and then pinged the IP received by DHCP (shown with ifconfig on the device)

@krzysztof-cabaj
Copy link
Contributor Author

Hi!

I perform in the lab experiment like you mentioned, on a most recent RIOT code (current master branch) and I still cannot ping my board. Please looked at the attached screenshot.

What kind of board you use? I tested on STM nucleo-207zg - so maybe this bahaviour concerns only this family of boards?

Experiment screenshot - RIOT shell, windows shell and wireshark. Board IP 192.168.143.65, windows IP 192.168.143.77.

obraz

#These flags enable calculating ICMP chksum in LWIP software.
#Without these flags RIOT device respond with ICMP chsum equal to 0, and
#cannot be successfully "pinged" from Windows hosts.
CFLAGS +=-DCHECKSUM_GEN_ICMP=0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is strange. According to https://www.nongnu.org/lwip/2_1_x/group__lwip__opts__checksum.html#ga2291ec5bec0a551545da6d5f9f9316b2 this would disable the generation of ICMP checksums, rather than enabling it.

I haven't looked at the code yet, but I kind of doubt that this is such an obvious bug in lwIP that CHECKSUM_GEN_ICMP is checked for being zero to generate the checksum. I fear that there is some other bug lurking that only gets hidden by the flags. But as said: I didn't look at the code, so these are just guesses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these flags are very weird but I found them in a some forum post concerning micro-controllers and similar issue - now after a more than month I cannot find this page :( .

When I used this solution, my understanding is that used in Nucleo board ethernet controller calculate IP checksum in hardware when received from micro-controller packet set this field to 0. I try to check this in board documentation.

#Without these flags RIOT device respond with ICMP chsum equal to 0, and
#cannot be successfully "pinged" from Windows hosts.
CFLAGS +=-DCHECKSUM_GEN_ICMP=0
CFLAGS +=-DLWIP_CHECKSUM_CTRL_PER_NETIF=1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would add runtime overhead so that checksum generation and validation can be enabled/disabled for each network interface. IMO it would be better to just enable ICMP checksum generation for all interface at compile time by default. If there really is need to fine-tune checksum generation and validation, this could be exposed as a module that users would have to enable explicitly in order to trade some RAM and CPU cycles for the ability to fine-tune checksum settings at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand idea of special module you mentioned. Could you send link to similar module which is currently in RIOT?

After @yarrick tests I suspect that this issue concerns only subset of boards - probably Nucleo boards. Next week I should heva access to other Nucleo boards - I perform additional experiments. But now, I have other solution - maybe conditional addition of these flags - only for required boards is simpler solution than additional module about which programmer must remember.

@yarrick
Copy link
Contributor

yarrick commented Sep 27, 2023

yarrick added a commit to yarrick/RIOT that referenced this pull request Sep 27, 2023
lwIP will fill them in already.

Having this enabled causes empty checksums to be sent: RIOT-OS#19853
@krzysztof-cabaj
Copy link
Contributor Author

@yarrick thank you for info and PR. I did some additional investigations.

  1. The flag LWIP_CHECKSUM_CTRL_PER_NETIF=1 is not needed for this issue - my solution works also when only CHECKSUM_GEN_ICMP=0 is used - i checked this using nucleo-f207zg,
  2. I add some conditions to the Makefile (please see most recent commit) - so this flags is enabled only for chosen boards. Now I'm working on more general condition,
  3. In next week I should have access to other nucleo board (probably nucleo-f429zi and nucleo-f767zi) so I could test this PR as well as PR cpu/stm32/periph/eth: Disable hardware checksums #19952,
  4. PR cpu/stm32/periph/eth: Disable hardware checksums #19952 works on nucleo-f207zg - however I have some doubts if disabling hardware chsum calculation is a best idea.

@yarrick
Copy link
Contributor

yarrick commented Sep 28, 2023

Instead of adding checks on board names inside lwIP we should make network interfaces that use hw checksumming enable some pseudomodule and then lwIP would act on that and disable the relevant calculations. Of course this could still cause issues for boards with multiple different interfaces (which is why calculating them always is safest).

@krzysztof-cabaj
Copy link
Contributor Author

I don't understand idea of such pseudomodule (wchich could enable some features on request). Could you send me a link to such pseudomodule in RIOT?

bors bot added a commit that referenced this pull request Sep 28, 2023
19952: cpu/stm32/periph/eth: Disable hardware checksums r=maribu a=yarrick

lwIP will fill them in already.

Having this enabled causes empty checksums to be sent: #19853



Co-authored-by: Erik Ekman <eekman@google.com>
@yarrick
Copy link
Contributor

yarrick commented Sep 28, 2023

I added pseudomodules in #16966

@krzysztof-cabaj
Copy link
Contributor Author

@yarrick thanks for a link.

@krzysztof-cabaj
Copy link
Contributor Author

Due to fact that PR #19952 is merged tomaster I close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants