[PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[Sean Anderson]

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 */
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[Willem de Bruijn]

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);
>  	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 */
> -- 
> 2.35.1.1320.gc452695387.dirty
> 

Re: [PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[Sean Anderson]

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);
>>  	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 */
>> -- 
>> 2.35.1.1320.gc452695387.dirty
>> 
> 
> 

Re: [PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[Willem de Bruijn]

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);
> >>  	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 */
> >> -- 
> >> 2.35.1.1320.gc452695387.dirty
> >> 
> > 
> > 

Re: [PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[Eric Dumazet]

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);
> >>      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 */
> >> --
> >> 2.35.1.1320.gc452695387.dirty
> >>
> >
> >

Re: [PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[Willem de Bruijn]

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.

Re: [PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[Jakub Kicinski]

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?

Re: [PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[Willem de Bruijn]

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.

Re: [PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[Sean Anderson]

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

Re: [PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[Willem de Bruijn]

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

Re: [PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[Sean Anderson]

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

Re: [PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[Willem de Bruijn]

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.

Re: [PATCH net] selftests: net: csum: Fix checksums for packets with non-zero padding

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html