[PATCH] checksum: Fix calculation for ICMP checksum on IPv4

older
[PATCH 0/8] Add support for log...

Stefano Brivio

25 Oct 2022 25 Oct '22

6:07 p.m.

We need to zero out the checksum field before calculating the checksum, of course. I have no idea how this passed the "icmp" test set, looking into it. Reported-by: Paul Holzinger Fixes: 67ab6171729c ("Add csum_icmp4() helper for calculating ICMP checksums") Signed-off-by: Stefano Brivio --- checksum.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/checksum.c b/checksum.c index 09d2c7c..c59869c 100644 --- a/checksum.c +++ b/checksum.c @@ -160,10 +160,13 @@ void csum_udp4(struct udphdr *udp4hr, in_addr_t saddr, in_addr_t daddr, */ void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t len) { - /* Partial checksum for ICMP header alone */ - uint32_t psum = sum_16b(icmp4hr, sizeof(*icmp4hr)); + uint32_t psum; icmp4hr->checksum = 0; + + /* Partial checksum for ICMP header alone */ + psum = sum_16b(icmp4hr, sizeof(*icmp4hr)); + icmp4hr->checksum = csum_unaligned(payload, len, psum); } -- 2.35.1

Show replies by date

Stefano Brivio

25 Oct 25 Oct

7:25 p.m.

On Tue, 25 Oct 2022 18:07:13 +0200 Stefano Brivio wrote:

...

We need to zero out the checksum field before calculating the checksum, of course. I have no idea how this passed the "icmp" test set, looking into it.

That's because the version of ping(8) I use on my test machine, from 'iputils', ignores a failed checksum in the reply: https://github.com/iputils/iputils/blob/59908434d7505ef574c2e0811ad1d5edb671... ...the parameter 'csfailed' in gather_statistics() is just passed as 0 here. The checksum was actually wrong in the CI test run. Forcing a specific version (e.g. from GNU inetutils) looks rather messy and non-portable, so I'm afraid there isn't much we can do for this, other than being careful. -- Stefano

David Gibson

26 Oct 26 Oct

2:06 a.m.

On Tue, Oct 25, 2022 at 06:07:13PM +0200, Stefano Brivio wrote:

...

We need to zero out the checksum field before calculating the checksum, of course. I have no idea how this passed the "icmp" test set, looking into it.

Reported-by: Paul Holzinger Fixes: 67ab6171729c ("Add csum_icmp4() helper for calculating ICMP checksums") Signed-off-by: Stefano Brivio

Oops, that's embarrassing. Reviewed-by: David Gibson

...

--- checksum.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/checksum.c b/checksum.c index 09d2c7c..c59869c 100644 --- a/checksum.c +++ b/checksum.c @@ -160,10 +160,13 @@ void csum_udp4(struct udphdr *udp4hr, in_addr_t saddr, in_addr_t daddr, */ void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t len) { - /* Partial checksum for ICMP header alone */ - uint32_t psum = sum_16b(icmp4hr, sizeof(*icmp4hr)); + uint32_t psum;

icmp4hr->checksum = 0; + + /* Partial checksum for ICMP header alone */ + psum = sum_16b(icmp4hr, sizeof(*icmp4hr)); + icmp4hr->checksum = csum_unaligned(payload, len, psum); }

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Paul Holzinger

2:44 p.m.

Hi, thanks for the quick patch. I can confirm that it works but there is still an issue. The first ICMP package is always dropped: $ podman run --net=pasta alpine ping -c 2 192.168.188.1 PING 192.168.188.1 (192.168.188.1): 56 data bytes 64 bytes from 192.168.188.1: seq=1 ttl=42 time=0.550 ms --- 192.168.188.1 ping statistics --- 2 packets transmitted, 1 packets received, 50% packet loss round-trip min/avg/max = 0.550/0.550/0.550 ms This is a problem in pasta (it works from my host and with slirp4nents), I also see the reply on the host with tcpdump: listening on enp9s0u2u1u2, link-type EN10MB (Ethernet), snapshot length 262144 bytes 14:41:09.457147 IP 192.168.188.22 > 192.168.188.1: ICMP echo request, id 31, seq 0, length 64 14:41:09.457481 IP 192.168.188.1 > 192.168.188.22: ICMP echo reply, id 31, seq 0, length 64 14:41:10.456972 IP 192.168.188.22 > 192.168.188.1: ICMP echo request, id 31, seq 1, length 64 14:41:10.457339 IP 192.168.188.1 > 192.168.188.22: ICMP echo reply, id 31, seq 1, length 64 Paul On Wed, Oct 26, 2022 at 2:16 AM David Gibson wrote:

...

On Tue, Oct 25, 2022 at 06:07:13PM +0200, Stefano Brivio wrote:

...
We need to zero out the checksum field before calculating the checksum, of course. I have no idea how this passed the "icmp" test set, looking into it.

Reported-by: Paul Holzinger Fixes: 67ab6171729c ("Add csum_icmp4() helper for calculating ICMP checksums") Signed-off-by: Stefano Brivio

Oops, that's embarrassing.

Reviewed-by: David Gibson

...
--- checksum.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/checksum.c b/checksum.c index 09d2c7c..c59869c 100644 --- a/checksum.c +++ b/checksum.c @@ -160,10 +160,13 @@ void csum_udp4(struct udphdr *udp4hr, in_addr_t saddr, in_addr_t daddr, */ void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t len) { - /* Partial checksum for ICMP header alone */ - uint32_t psum = sum_16b(icmp4hr, sizeof(*icmp4hr)); + uint32_t psum;

icmp4hr->checksum = 0; + + /* Partial checksum for ICMP header alone */ + psum = sum_16b(icmp4hr, sizeof(*icmp4hr)); + icmp4hr->checksum = csum_unaligned(payload, len, psum); }

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

Stefano Brivio

2:57 p.m.

Hi Paul, On Wed, 26 Oct 2022 14:44:23 +0200 Paul Holzinger wrote:

...

Hi,

thanks for the quick patch. I can confirm that it works but there is still an issue.

Thanks for having a look!

...

The first ICMP package is always dropped:

$ podman run --net=pasta alpine ping -c 2 192.168.188.1 PING 192.168.188.1 (192.168.188.1): 56 data bytes 64 bytes from 192.168.188.1: seq=1 ttl=42 time=0.550 ms

--- 192.168.188.1 ping statistics --- 2 packets transmitted, 1 packets received, 50% packet loss round-trip min/avg/max = 0.550/0.550/0.550 ms

This is a problem in pasta (it works from my host and with slirp4nents), I also see the reply on the host with tcpdump: listening on enp9s0u2u1u2, link-type EN10MB (Ethernet), snapshot length 262144 bytes 14:41:09.457147 IP 192.168.188.22 > 192.168.188.1: ICMP echo request, id 31, seq 0, length 64 14:41:09.457481 IP 192.168.188.1 > 192.168.188.22: ICMP echo reply, id 31, seq 0, length 64 14:41:10.456972 IP 192.168.188.22 > 192.168.188.1: ICMP echo request, id 31, seq 1, length 64 14:41:10.457339 IP 192.168.188.1 > 192.168.188.22: ICMP echo reply, id 31, seq 1, length 64

This difference is due to the fact that pasta allows any IP address to be used by the container, and it will learn that on the first packet. I see the same exact behaviour. We might be able to improve this, but I'm not entirely sure. To make sure I have the right "problem" in mind, would you mind sharing a packet capture (including ARP requests) from the container side? That would be --net=pasta,-p,your.pcap. Thanks, -- Stefano

Paul Holzinger

3:07 p.m.

Pcap file is attached. This difference is due to the fact that pasta allows any IP address to

...

be used by the container, and it will learn that on the first packet. I see the same exact behaviour. We might be able to improve this, but I'm not entirely sure.

I might misunderstand how passt/pasta work but it already configured the interface in the netns with the correct ip, no? Why does it need to learn it? On Wed, Oct 26, 2022 at 2:57 PM Stefano Brivio wrote:

...

Hi Paul,

On Wed, 26 Oct 2022 14:44:23 +0200 Paul Holzinger wrote:

...
Hi,

thanks for the quick patch. I can confirm that it works but there is still an issue.

Thanks for having a look!

...
The first ICMP package is always dropped:

$ podman run --net=pasta alpine ping -c 2 192.168.188.1 PING 192.168.188.1 (192.168.188.1): 56 data bytes 64 bytes from 192.168.188.1: seq=1 ttl=42 time=0.550 ms

--- 192.168.188.1 ping statistics --- 2 packets transmitted, 1 packets received, 50% packet loss round-trip min/avg/max = 0.550/0.550/0.550 ms

This is a problem in pasta (it works from my host and with slirp4nents), I also see the reply on the host with tcpdump: listening on enp9s0u2u1u2, link-type EN10MB (Ethernet), snapshot length 262144 bytes 14:41:09.457147 IP 192.168.188.22 > 192.168.188.1: ICMP echo request, id 31, seq 0, length 64 14:41:09.457481 IP 192.168.188.1 > 192.168.188.22: ICMP echo reply, id 31, seq 0, length 64 14:41:10.456972 IP 192.168.188.22 > 192.168.188.1: ICMP echo request, id 31, seq 1, length 64 14:41:10.457339 IP 192.168.188.1 > 192.168.188.22: ICMP echo reply, id 31, seq 1, length 64

This difference is due to the fact that pasta allows any IP address to be used by the container, and it will learn that on the first packet. I see the same exact behaviour. We might be able to improve this, but I'm not entirely sure.

To make sure I have the right "problem" in mind, would you mind sharing a packet capture (including ARP requests) from the container side? That would be --net=pasta,-p,your.pcap.

Thanks,

-- Stefano

Stefano Brivio

3:40 p.m.

On Wed, 26 Oct 2022 15:07:42 +0200 Paul Holzinger wrote:

...

Pcap file is attached.

Thanks, it's not the issue I had in mind. Here the ARP exchange already happened, and the ICMP proxy is not tracking the first reply. We might be using this kind of mechanism here, if bind() for ICMP echo sockets is not allowed on the host: https://passt.top/passt/commit/?id=9663378d6d6dcd8275d60b826356cc4be0538231 (this issue was seen in KubeVirt with passt). But I don't have a clear explanation as to why that first reply is ignored, yet. I'll need to look further into this.

...

...
This difference is due to the fact that pasta allows any IP address to be used by the container, and it will learn that on the first packet. I see the same exact behaviour. We might be able to improve this, but I'm not entirely sure.

I might misunderstand how passt/pasta work but it already configured the interface in the netns with the correct ip, no? Why does it need to learn it?

Not in general. Podman is passing --config-net, so we can be reasonably (but not totally) sure that that's going to be the address used in the future -- the user could still change it manually. In other cases, we might be offering zero or more of DHCP, NDP, DHCPv6, depending on configuration, and nobody guarantees that the container or a guest is actually implementing that. -- Stefano

Stefano Brivio

4:28 p.m.

On Wed, 26 Oct 2022 15:40:56 +0200 Stefano Brivio wrote:

...

On Wed, 26 Oct 2022 15:07:42 +0200 Paul Holzinger wrote:

...
Pcap file is attached.

Thanks, it's not the issue I had in mind. Here the ARP exchange already happened, and the ICMP proxy is not tracking the first reply.

Somewhat interestingly: $ ./pasta --config-net -- sh -c 'sleep 0.05; ping -c1 88.198.0.161' PING 88.198.0.161 (88.198.0.161) 56(84) bytes of data. 64 bytes from 88.198.0.161: icmp_seq=1 ttl=255 time=0.593 ms --- 88.198.0.161 ping statistics --- 1 packets transmitted, 1 received, 0% packet loss, time 0ms rtt min/avg/max/mdev = 0.593/0.593/0.593/0.000 ms $ ./pasta --config-net -- sh -c 'ping -c1 88.198.0.161' ping: connect: Network is unreachable ...but that's not the same that happens with Podman, I'm still debugging that. -- Stefano

950

Age (days ago)

951

Last active (days ago)

List overview

Download

7 comments

3 participants

participants (3)

David Gibson
Paul Holzinger
Stefano Brivio

[PATCH] checksum: Fix calculation for ICMP checksum on IPv4

tags

participants (3)