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>
---
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
On Tue, 25 Oct 2022 18:07:13 +0200 Stefano Brivio <sbrivio(a)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/59908434d7505ef574c2e0811ad1d5edb67… ...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
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
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 ICMPchecksums")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_tsaddr, in_addr_t daddr,*/ void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_tlen){ - /* 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
Hi Paul, On Wed, 26 Oct 2022 14:44:23 +0200 Paul Holzinger <pholzing(a)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 64This 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
Pcap file is attached. This difference is due to the fact that pasta allows any IP address tobe 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 <sbrivio(a)redhat.com> wrote: > Hi Paul, > > On Wed, 26 Oct 2022 14:44:23 +0200 > Paul Holzinger <pholzing(a)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 tobe 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 > >
On Wed, 26 Oct 2022 15:07:42 +0200 Paul Holzinger <pholzing(a)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.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. -- StefanoThis 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, 26 Oct 2022 15:40:56 +0200 Stefano Brivio <sbrivio(a)redhat.com> wrote:On Wed, 26 Oct 2022 15:07:42 +0200 Paul Holzinger <pholzing(a)redhat.com> wrote: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. -- StefanoPcap 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.