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