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

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

[Stefano Brivio]

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 <pholzing@redhat.com>
Fixes: 67ab6171729c ("Add csum_icmp4() helper for calculating ICMP checksums")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 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);
 }
 
-- 
@@ -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

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

[Stefano Brivio]

On Tue, 25 Oct 2022 18:07:13 +0200
Stefano Brivio <sbrivio@redhat.com> 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/59908434d7505ef574c2e0811ad1d5edb671845a/ping/ping.c#L1544

...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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1438 bytes --]

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 <pholzing@redhat.com>
> Fixes: 67ab6171729c ("Add csum_icmp4() helper for calculating ICMP checksums")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Oops, that's embarrassing.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Stefano Brivio]

Hi Paul,

On Wed, 26 Oct 2022 14:44:23 +0200
Paul Holzinger <pholzing@redhat.com> 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

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

[Stefano Brivio]

On Wed, 26 Oct 2022 15:07:42 +0200
Paul Holzinger <pholzing@redhat.com> 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

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

[Stefano Brivio]

On Wed, 26 Oct 2022 15:40:56 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Wed, 26 Oct 2022 15:07:42 +0200
> Paul Holzinger <pholzing@redhat.com> 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