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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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