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

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 <david(a)gibson.dropbear.id.au>
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 <pholzing(a)redhat.com>
 Fixes: 67ab6171729c ("Add csum_icmp4() helper for calculating ICMP 
checksums")
  Signed-off-by: Stefano Brivio
<sbrivio(a)redhat.com> 
 Oops, that's embarrassing.

 Reviewed-by: David Gibson <david(a)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