Padding is not included in UDP and TCP checksums. Therefore, reduce the length of the checksummed data to include only the data in the IP payload. This fixes spurious reported checksum failures like
rx: pkt: sport=33000 len=26 csum=0xc850 verify=0xf9fe pkt: bad csum
Technically it is possible for there to be trailing bytes after the UDP data but before the Ethernet padding (e.g. if sizeof(ip) + sizeof(udp) + udp.len < ip.len). However, we don't generate such packets.
Fixes: 91a7de85600d ("selftests/net: add csum offload test") Signed-off-by: Sean Anderson sean.anderson@linux.dev --- Found while testing for this very bug in hardware checksum offloads.
tools/testing/selftests/net/lib/csum.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/lib/csum.c b/tools/testing/selftests/net/lib/csum.c index b9f3fc3c3426..e0a34e5e8dd5 100644 --- a/tools/testing/selftests/net/lib/csum.c +++ b/tools/testing/selftests/net/lib/csum.c @@ -654,10 +654,16 @@ static int recv_verify_packet_ipv4(void *nh, int len) { struct iphdr *iph = nh; uint16_t proto = cfg_encap ? IPPROTO_UDP : cfg_proto; + uint16_t ip_len;
if (len < sizeof(*iph) || iph->protocol != proto) return -1;
+ ip_len = ntohs(iph->tot_len); + if (ip_len > len || ip_len < sizeof(*iph)) + return -1; + + len = ip_len; iph_addr_p = &iph->saddr; if (proto == IPPROTO_TCP) return recv_verify_packet_tcp(iph + 1, len - sizeof(*iph)); @@ -669,16 +675,22 @@ static int recv_verify_packet_ipv6(void *nh, int len) { struct ipv6hdr *ip6h = nh; uint16_t proto = cfg_encap ? IPPROTO_UDP : cfg_proto; + uint16_t ip_len;
if (len < sizeof(*ip6h) || ip6h->nexthdr != proto) return -1;
+ ip_len = ntohs(ip6h->payload_len); + if (ip_len > len - sizeof(*ip6h)) + return -1; + + len = ip_len; iph_addr_p = &ip6h->saddr;
if (proto == IPPROTO_TCP) - return recv_verify_packet_tcp(ip6h + 1, len - sizeof(*ip6h)); + return recv_verify_packet_tcp(ip6h + 1, len); else - return recv_verify_packet_udp(ip6h + 1, len - sizeof(*ip6h)); + return recv_verify_packet_udp(ip6h + 1, len); }
/* return whether auxdata includes TP_STATUS_CSUM_VALID */
Sean Anderson wrote:
Padding is not included in UDP and TCP checksums. Therefore, reduce the length of the checksummed data to include only the data in the IP payload. This fixes spurious reported checksum failures like
rx: pkt: sport=33000 len=26 csum=0xc850 verify=0xf9fe pkt: bad csum
Are you using this test as receiver for other input?
The packet builder in the test doesn't generate these, does it?
Just trying to understand if this is a bug fix or a new use for csum.c as receiver.
Technically it is possible for there to be trailing bytes after the UDP data but before the Ethernet padding (e.g. if sizeof(ip) + sizeof(udp) + udp.len < ip.len). However, we don't generate such packets.
More likely is that L3 and L4 length fields agree, as both are generated at the sender, but that some trailer is attached in the network. Such as a timestamp trailer.
Fixes: 91a7de85600d ("selftests/net: add csum offload test") Signed-off-by: Sean Anderson sean.anderson@linux.dev
Found while testing for this very bug in hardware checksum offloads.
tools/testing/selftests/net/lib/csum.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/lib/csum.c b/tools/testing/selftests/net/lib/csum.c index b9f3fc3c3426..e0a34e5e8dd5 100644 --- a/tools/testing/selftests/net/lib/csum.c +++ b/tools/testing/selftests/net/lib/csum.c @@ -654,10 +654,16 @@ static int recv_verify_packet_ipv4(void *nh, int len) { struct iphdr *iph = nh; uint16_t proto = cfg_encap ? IPPROTO_UDP : cfg_proto;
- uint16_t ip_len;
if (len < sizeof(*iph) || iph->protocol != proto) return -1;
- ip_len = ntohs(iph->tot_len);
- if (ip_len > len || ip_len < sizeof(*iph))
return -1;- len = ip_len; iph_addr_p = &iph->saddr; if (proto == IPPROTO_TCP) return recv_verify_packet_tcp(iph + 1, len - sizeof(*iph));
@@ -669,16 +675,22 @@ static int recv_verify_packet_ipv6(void *nh, int len) { struct ipv6hdr *ip6h = nh; uint16_t proto = cfg_encap ? IPPROTO_UDP : cfg_proto;
- uint16_t ip_len;
nit: payload_len, as it never includes sizeof ipv6hdr
if (len < sizeof(*ip6h) || ip6h->nexthdr != proto) return -1;
- ip_len = ntohs(ip6h->payload_len);
- if (ip_len > len - sizeof(*ip6h))
return -1;- len = ip_len; iph_addr_p = &ip6h->saddr;
if (proto == IPPROTO_TCP)
return recv_verify_packet_tcp(ip6h + 1, len - sizeof(*ip6h));
return recv_verify_packet_tcp(ip6h + 1, len);
return recv_verify_packet_udp(ip6h + 1, len - sizeof(*ip6h));
return recv_verify_packet_udp(ip6h + 1, len);} /* return whether auxdata includes TP_STATUS_CSUM_VALID */ -- 2.35.1.1320.gc452695387.dirty
On 9/6/24 22:05, Willem de Bruijn wrote:
Sean Anderson wrote:
Padding is not included in UDP and TCP checksums. Therefore, reduce the length of the checksummed data to include only the data in the IP payload. This fixes spurious reported checksum failures like
rx: pkt: sport=33000 len=26 csum=0xc850 verify=0xf9fe pkt: bad csum
Are you using this test as receiver for other input?
The packet builder in the test doesn't generate these, does it?
It's added by the MAC before transmission.
This is permitted by the standard, but in this case it actually appears to be due to the MAC using 32-bit reads for the data and not masking off the end. Not sure whether this is a bug in the driver/device, since technically we may leak up to 3 bytes of memory.
That said, it would be a nice enhancement to generate packets with non-zero padding as well, since they are an interesting edge case.
Just trying to understand if this is a bug fix or a new use for csum.c as receiver.
Bug fix.
Technically it is possible for there to be trailing bytes after the UDP data but before the Ethernet padding (e.g. if sizeof(ip) + sizeof(udp) + udp.len < ip.len). However, we don't generate such packets.
More likely is that L3 and L4 length fields agree, as both are generated at the sender, but that some trailer is attached in the network. Such as a timestamp trailer.
Yes, as noted above we don't generate packets with differing L3 and L4 lengths.
Fixes: 91a7de85600d ("selftests/net: add csum offload test") Signed-off-by: Sean Anderson sean.anderson@linux.dev
Found while testing for this very bug in hardware checksum offloads.
tools/testing/selftests/net/lib/csum.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/lib/csum.c b/tools/testing/selftests/net/lib/csum.c index b9f3fc3c3426..e0a34e5e8dd5 100644 --- a/tools/testing/selftests/net/lib/csum.c +++ b/tools/testing/selftests/net/lib/csum.c @@ -654,10 +654,16 @@ static int recv_verify_packet_ipv4(void *nh, int len) { struct iphdr *iph = nh; uint16_t proto = cfg_encap ? IPPROTO_UDP : cfg_proto;
- uint16_t ip_len;
if (len < sizeof(*iph) || iph->protocol != proto) return -1;
- ip_len = ntohs(iph->tot_len);
- if (ip_len > len || ip_len < sizeof(*iph))
return -1;- len = ip_len; iph_addr_p = &iph->saddr; if (proto == IPPROTO_TCP) return recv_verify_packet_tcp(iph + 1, len - sizeof(*iph));
@@ -669,16 +675,22 @@ static int recv_verify_packet_ipv6(void *nh, int len) { struct ipv6hdr *ip6h = nh; uint16_t proto = cfg_encap ? IPPROTO_UDP : cfg_proto;
- uint16_t ip_len;
nit: payload_len, as it never includes sizeof ipv6hdr
OK
--Sean
if (len < sizeof(*ip6h) || ip6h->nexthdr != proto) return -1;
- ip_len = ntohs(ip6h->payload_len);
- if (ip_len > len - sizeof(*ip6h))
return -1;- len = ip_len; iph_addr_p = &ip6h->saddr;
if (proto == IPPROTO_TCP)
return recv_verify_packet_tcp(ip6h + 1, len - sizeof(*ip6h));
return recv_verify_packet_tcp(ip6h + 1, len);
return recv_verify_packet_udp(ip6h + 1, len - sizeof(*ip6h));
return recv_verify_packet_udp(ip6h + 1, len);} /* return whether auxdata includes TP_STATUS_CSUM_VALID */ -- 2.35.1.1320.gc452695387.dirty
Sean Anderson wrote:
On 9/6/24 22:05, Willem de Bruijn wrote:
Sean Anderson wrote:
Padding is not included in UDP and TCP checksums. Therefore, reduce the length of the checksummed data to include only the data in the IP payload. This fixes spurious reported checksum failures like
rx: pkt: sport=33000 len=26 csum=0xc850 verify=0xf9fe pkt: bad csum
Are you using this test as receiver for other input?
The packet builder in the test doesn't generate these, does it?
It's added by the MAC before transmission.
This is permitted by the standard, but in this case it actually appears to be due to the MAC using 32-bit reads for the data and not masking off the end. Not sure whether this is a bug in the driver/device, since technically we may leak up to 3 bytes of memory.
That said, it would be a nice enhancement to generate packets with non-zero padding as well, since they are an interesting edge case.
Thanks for that context.
Just trying to understand if this is a bug fix or a new use for csum.c as receiver.
Bug fix.
Technically it is possible for there to be trailing bytes after the UDP data but before the Ethernet padding (e.g. if sizeof(ip) + sizeof(udp) + udp.len < ip.len). However, we don't generate such packets.
More likely is that L3 and L4 length fields agree, as both are generated at the sender, but that some trailer is attached in the network. Such as a timestamp trailer.
Yes, as noted above we don't generate packets with differing L3 and L4 lengths.
Fixes: 91a7de85600d ("selftests/net: add csum offload test") Signed-off-by: Sean Anderson sean.anderson@linux.dev
Reviewed-by: Willem de Bruijn willemb@google.com
Found while testing for this very bug in hardware checksum offloads.
tools/testing/selftests/net/lib/csum.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/lib/csum.c b/tools/testing/selftests/net/lib/csum.c index b9f3fc3c3426..e0a34e5e8dd5 100644 --- a/tools/testing/selftests/net/lib/csum.c +++ b/tools/testing/selftests/net/lib/csum.c @@ -654,10 +654,16 @@ static int recv_verify_packet_ipv4(void *nh, int len) { struct iphdr *iph = nh; uint16_t proto = cfg_encap ? IPPROTO_UDP : cfg_proto;
- uint16_t ip_len;
if (len < sizeof(*iph) || iph->protocol != proto) return -1;
- ip_len = ntohs(iph->tot_len);
- if (ip_len > len || ip_len < sizeof(*iph))
return -1;- len = ip_len; iph_addr_p = &iph->saddr; if (proto == IPPROTO_TCP) return recv_verify_packet_tcp(iph + 1, len - sizeof(*iph));
@@ -669,16 +675,22 @@ static int recv_verify_packet_ipv6(void *nh, int len) { struct ipv6hdr *ip6h = nh; uint16_t proto = cfg_encap ? IPPROTO_UDP : cfg_proto;
- uint16_t ip_len;
nit: payload_len, as it never includes sizeof ipv6hdr
OK
--Sean
if (len < sizeof(*ip6h) || ip6h->nexthdr != proto) return -1;
- ip_len = ntohs(ip6h->payload_len);
- if (ip_len > len - sizeof(*ip6h))
return -1;- len = ip_len; iph_addr_p = &ip6h->saddr;
if (proto == IPPROTO_TCP)
return recv_verify_packet_tcp(ip6h + 1, len - sizeof(*ip6h));
return recv_verify_packet_tcp(ip6h + 1, len);
return recv_verify_packet_udp(ip6h + 1, len - sizeof(*ip6h));
return recv_verify_packet_udp(ip6h + 1, len);} /* return whether auxdata includes TP_STATUS_CSUM_VALID */ -- 2.35.1.1320.gc452695387.dirty
On Mon, Sep 9, 2024 at 5:02 PM Sean Anderson sean.anderson@linux.dev wrote:
On 9/6/24 22:05, Willem de Bruijn wrote:
Sean Anderson wrote:
Padding is not included in UDP and TCP checksums. Therefore, reduce the length of the checksummed data to include only the data in the IP payload. This fixes spurious reported checksum failures like
rx: pkt: sport=33000 len=26 csum=0xc850 verify=0xf9fe pkt: bad csum
Are you using this test as receiver for other input?
The packet builder in the test doesn't generate these, does it?
It's added by the MAC before transmission.
This is permitted by the standard, but in this case it actually appears to be due to the MAC using 32-bit reads for the data and not masking off the end. Not sure whether this is a bug in the driver/device, since technically we may leak up to 3 bytes of memory.
This seems to be a bug in the driver.
A call to skb_put_padto(skb, ETH_ZLEN) should be added.
That said, it would be a nice enhancement to generate packets with non-zero padding as well, since they are an interesting edge case.
Just trying to understand if this is a bug fix or a new use for csum.c as receiver.
Bug fix.
Technically it is possible for there to be trailing bytes after the UDP data but before the Ethernet padding (e.g. if sizeof(ip) + sizeof(udp) + udp.len < ip.len). However, we don't generate such packets.
More likely is that L3 and L4 length fields agree, as both are generated at the sender, but that some trailer is attached in the network. Such as a timestamp trailer.
Yes, as noted above we don't generate packets with differing L3 and L4 lengths.
Fixes: 91a7de85600d ("selftests/net: add csum offload test") Signed-off-by: Sean Anderson sean.anderson@linux.dev
Found while testing for this very bug in hardware checksum offloads.
tools/testing/selftests/net/lib/csum.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/lib/csum.c b/tools/testing/selftests/net/lib/csum.c index b9f3fc3c3426..e0a34e5e8dd5 100644 --- a/tools/testing/selftests/net/lib/csum.c +++ b/tools/testing/selftests/net/lib/csum.c @@ -654,10 +654,16 @@ static int recv_verify_packet_ipv4(void *nh, int len) { struct iphdr *iph = nh; uint16_t proto = cfg_encap ? IPPROTO_UDP : cfg_proto;
uint16_t ip_len;
if (len < sizeof(*iph) || iph->protocol != proto) return -1;
ip_len = ntohs(iph->tot_len);
if (ip_len > len || ip_len < sizeof(*iph))
return -1;len = ip_len; iph_addr_p = &iph->saddr; if (proto == IPPROTO_TCP) return recv_verify_packet_tcp(iph + 1, len - sizeof(*iph));
@@ -669,16 +675,22 @@ static int recv_verify_packet_ipv6(void *nh, int len) { struct ipv6hdr *ip6h = nh; uint16_t proto = cfg_encap ? IPPROTO_UDP : cfg_proto;
- uint16_t ip_len;
nit: payload_len, as it never includes sizeof ipv6hdr
OK
--Sean
if (len < sizeof(*ip6h) || ip6h->nexthdr != proto) return -1;
ip_len = ntohs(ip6h->payload_len);
if (ip_len > len - sizeof(*ip6h))
return -1;len = ip_len; iph_addr_p = &ip6h->saddr;
if (proto == IPPROTO_TCP)
return recv_verify_packet_tcp(ip6h + 1, len - sizeof(*ip6h));
return recv_verify_packet_tcp(ip6h + 1, len);
return recv_verify_packet_udp(ip6h + 1, len - sizeof(*ip6h));
return recv_verify_packet_udp(ip6h + 1, len);}
/* return whether auxdata includes TP_STATUS_CSUM_VALID */
2.35.1.1320.gc452695387.dirty
Eric Dumazet wrote:
On Mon, Sep 9, 2024 at 5:02 PM Sean Anderson sean.anderson@linux.dev wrote:
On 9/6/24 22:05, Willem de Bruijn wrote:
Sean Anderson wrote:
Padding is not included in UDP and TCP checksums. Therefore, reduce the length of the checksummed data to include only the data in the IP payload. This fixes spurious reported checksum failures like
rx: pkt: sport=33000 len=26 csum=0xc850 verify=0xf9fe pkt: bad csum
Are you using this test as receiver for other input?
The packet builder in the test doesn't generate these, does it?
It's added by the MAC before transmission.
This is permitted by the standard, but in this case it actually appears to be due to the MAC using 32-bit reads for the data and not masking off the end. Not sure whether this is a bug in the driver/device, since technically we may leak up to 3 bytes of memory.
This seems to be a bug in the driver.
A call to skb_put_padto(skb, ETH_ZLEN) should be added.
In which case this test detecting it may be nice to have, for lack of a more targeted test.
On Mon, 09 Sep 2024 13:26:42 -0400 Willem de Bruijn wrote:
This seems to be a bug in the driver.
A call to skb_put_padto(skb, ETH_ZLEN) should be added.
In which case this test detecting it may be nice to have, for lack of a more targeted test.
IIUC we're basically saying that we don't need to trim because pad should be 0? In that case maybe let's keep the patch but add a check on top which scans the pad for non-zero bytes, and print an informative warning?
Jakub Kicinski wrote:
On Mon, 09 Sep 2024 13:26:42 -0400 Willem de Bruijn wrote:
This seems to be a bug in the driver.
A call to skb_put_padto(skb, ETH_ZLEN) should be added.
In which case this test detecting it may be nice to have, for lack of a more targeted test.
IIUC we're basically saying that we don't need to trim because pad should be 0? In that case maybe let's keep the patch but add a check on top which scans the pad for non-zero bytes, and print an informative warning?
Data arriving with padding probably deserves a separate test.
We can use this csum test as stand-in, I suppose.
Is it safe to assume that all padding is wrong on ingress, not just non-zero padding. The ip stack itself treats it as benign and trims the trailing bytes silently.
I do know of legitimate cases of trailer data lifting along.
On 9/9/24 21:01, Willem de Bruijn wrote:
Jakub Kicinski wrote:
On Mon, 09 Sep 2024 13:26:42 -0400 Willem de Bruijn wrote:
This seems to be a bug in the driver.
A call to skb_put_padto(skb, ETH_ZLEN) should be added.
In which case this test detecting it may be nice to have, for lack of a more targeted test.
IIUC we're basically saying that we don't need to trim because pad should be 0? In that case maybe let's keep the patch but add a check on top which scans the pad for non-zero bytes, and print an informative warning?
Data arriving with padding probably deserves a separate test.
We can use this csum test as stand-in, I suppose.
Is it safe to assume that all padding is wrong on ingress, not just non-zero padding. The ip stack itself treats it as benign and trims the trailing bytes silently.
I do know of legitimate cases of trailer data lifting along.
Ideally we would test that
- Ingress padding is ignored. - Egress padding does not leak past the buffer. The easiest way to handle this would be to check that it is constant (e.g. all the padding uses the same value), but this could have false-positives for e.g. timestamps.
--Sean
Sean Anderson wrote:
On 9/9/24 21:01, Willem de Bruijn wrote:
Jakub Kicinski wrote:
On Mon, 09 Sep 2024 13:26:42 -0400 Willem de Bruijn wrote:
This seems to be a bug in the driver.
A call to skb_put_padto(skb, ETH_ZLEN) should be added.
In which case this test detecting it may be nice to have, for lack of a more targeted test.
IIUC we're basically saying that we don't need to trim because pad should be 0? In that case maybe let's keep the patch but add a check on top which scans the pad for non-zero bytes, and print an informative warning?
Data arriving with padding probably deserves a separate test.
We can use this csum test as stand-in, I suppose.
Is it safe to assume that all padding is wrong on ingress, not just non-zero padding. The ip stack itself treats it as benign and trims the trailing bytes silently.
I do know of legitimate cases of trailer data lifting along.
Ideally we would test that
- Ingress padding is ignored.
I think the goal of a hardware padding test is to detect when padding leaks onto the wire.
If not adding a new test, detect in csum and fail anytime padding is detected (i.e., not only non-zero)?
- Egress padding does not leak past the buffer. The easiest way to handle this would be to check that it is constant (e.g. all the padding uses the same value), but this could have false-positives for e.g. timestamps.
--Sean
On 9/10/24 13:42, Willem de Bruijn wrote:
Sean Anderson wrote:
On 9/9/24 21:01, Willem de Bruijn wrote:
Jakub Kicinski wrote:
On Mon, 09 Sep 2024 13:26:42 -0400 Willem de Bruijn wrote:
This seems to be a bug in the driver.
A call to skb_put_padto(skb, ETH_ZLEN) should be added.
In which case this test detecting it may be nice to have, for lack of a more targeted test.
IIUC we're basically saying that we don't need to trim because pad should be 0? In that case maybe let's keep the patch but add a check on top which scans the pad for non-zero bytes, and print an informative warning?
Data arriving with padding probably deserves a separate test.
We can use this csum test as stand-in, I suppose.
Is it safe to assume that all padding is wrong on ingress, not just non-zero padding. The ip stack itself treats it as benign and trims the trailing bytes silently.
I do know of legitimate cases of trailer data lifting along.
Ideally we would test that
- Ingress padding is ignored.
I think the goal of a hardware padding test is to detect when padding leaks onto the wire.
Which is the subject of my second bullet.
If not adding a new test, detect in csum and fail anytime padding is detected (i.e., not only non-zero)?
As noted below, this is only a problem if we leak kernel memory in the padding. Otherwise, any kind of padding at all is completely standard conformant.
- Egress padding does not leak past the buffer. The easiest way to handle this would be to check that it is constant (e.g. all the padding uses the same value), but this could have false-positives for e.g. timestamps.
--Sean
Sean Anderson wrote:
On 9/10/24 13:42, Willem de Bruijn wrote:
Sean Anderson wrote:
On 9/9/24 21:01, Willem de Bruijn wrote:
Jakub Kicinski wrote:
On Mon, 09 Sep 2024 13:26:42 -0400 Willem de Bruijn wrote:
> This seems to be a bug in the driver. > > A call to skb_put_padto(skb, ETH_ZLEN) should be added.
In which case this test detecting it may be nice to have, for lack of a more targeted test.
IIUC we're basically saying that we don't need to trim because pad should be 0? In that case maybe let's keep the patch but add a check on top which scans the pad for non-zero bytes, and print an informative warning?
Data arriving with padding probably deserves a separate test.
We can use this csum test as stand-in, I suppose.
Is it safe to assume that all padding is wrong on ingress, not just non-zero padding. The ip stack itself treats it as benign and trims the trailing bytes silently.
I do know of legitimate cases of trailer data lifting along.
Ideally we would test that
- Ingress padding is ignored.
I think the goal of a hardware padding test is to detect when padding leaks onto the wire.
Which is the subject of my second bullet.
If not adding a new test, detect in csum and fail anytime padding is detected (i.e., not only non-zero)?
As noted below, this is only a problem if we leak kernel memory in the padding. Otherwise, any kind of padding at all is completely standard conformant.
Ack. I actually was not clear on that.
Hello:
This patch was applied to netdev/net.git (main) by Jakub Kicinski kuba@kernel.org:
On Fri, 6 Sep 2024 17:07:43 -0400 you wrote:
Padding is not included in UDP and TCP checksums. Therefore, reduce the length of the checksummed data to include only the data in the IP payload. This fixes spurious reported checksum failures like
rx: pkt: sport=33000 len=26 csum=0xc850 verify=0xf9fe pkt: bad csum
[...]
Here is the summary with links: - [net] selftests: net: csum: Fix checksums for packets with non-zero padding https://git.kernel.org/netdev/net/c/e8a63d473b49
You are awesome, thank you!
linux-kselftest-mirror@lists.linaro.org
-
Eric Dumazet -
Jakub Kicinski -
patchwork-bot+netdevbpf＠kernel.org -
Sean Anderson -
Willem de Bruijn