netdev - Re: [PATCH net v2 3/4] udp: do not transition UDP fraglist to unnecessary checksum

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <65fb4a8b1389_1faab3294c8@willemb.c.googlers.com.notmuch>
Date: Wed, 20 Mar 2024 16:43:55 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Antoine Tenart <atenart@...nel.org>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 davem@...emloft.net, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com
Cc: steffen.klassert@...unet.com, 
 willemdebruijn.kernel@...il.com, 
 netdev@...r.kernel.org
Subject: Re: [PATCH net v2 3/4] udp: do not transition UDP fraglist to
 unnecessary checksum

Antoine Tenart wrote:
> Quoting Willem de Bruijn (2024-03-20 14:00:48)
> > Antoine Tenart wrote:
> > > Quoting Willem de Bruijn (2024-03-19 14:38:20)
> > > > 
> > > > The original patch converted to CHECKSUM_UNNECESSARY for a reason.
> > > > The skb->csum of the main gso_skb is not valid?
> > > > 
> > > > Should instead only the csum_level be adjusted, to always keep
> > > > csum_level == 0?
> > > 
> > > The above trace is an ICMPv6 packet being tunneled and GROed at the UDP
> > > level, thus we have:
> > >   UDP(CHECKSUM_PARTIAL)/Geneve/ICMPv6(was CHECKSUM_NONE)
> > > csum_level would need to be 1 here; but we can't know that.
> > 
> > Is this a packet looped internally? Else it is not CHECKSUM_PARTIAL.
> 
> I'm not sure to follow, CHECKSUM_NONE packets going in a tunnel will be
> encapsulated and the outer UDP header will be CHECKSUM_PARTIAL. The
> packet can be looped internally or going to a remote host.

That is on transmit. To come into contact with UDP_GRO while having
CHECKSUM_PARTIAL the packet will have to loop into the receive path,
in some way that triggers GRO. Perhaps through gro_cells, as other
GRO paths are hardware NIC drivers.
 
> > > There is another issue (no kernel trace): if a packet has partial csum
> > > and is being GROed that information is lost and the packet ends up with
> > > an invalid csum.
> > 
> > CHECKSUM_PARTIAL should be converted to CHECKSUM_UNNECESSARY for this
> > reason. CHECKSUM_PARTIAL implies the header is prepared with pseudo
> > header checksum. Similarly CHECKSUM_COMPLETE implies skb csum is valid.
> > CHECKSUM_UNNECESSARY has neither expectations.
> 
> But not if the packet is sent to a remote host. Otherwise an inner
> partial csum is never fixed by the stack/NIC before going out.

The stack will only offload a single checksum. With local checksum
offload, this can be the inner checksum and the outer can be cheaply
computed in software. udp_set_csum() handles this. It indeed sets lco
if the inner packet has CHECKSUM_PARTIAL. Otherwise it sets ip_summed
to CHECKSUM_PARTIAL, now pointing to the outer UDP header.

You're right. Regardless of whether it points to the inner or outer
checksum, a conversion of CHECKSUM_PARTIAL to CHECKSUM_UNNECESSARY
will break checksum offload in the forwarding case.

> > > Packets with CHECKSUM_UNNECESSARY should end up with the same info. My
> > > impression is this checksum conversion is at best setting the same info
> > > and otherwise is overriding valuable csum information.
> > > 
> > > Or would packets with CSUM_NONE being GROed would benefit from the
> > > CHECKSUM_UNNECESSARY conversion?
> > 
> > Definitely. If the packet has CHECKSUM_NONE and GRO checks its
> > validity in software, converting it to CHECKSUM_UNNECESSARY avoids
> > potential additional checks at later stages in the packet path.
> 
> Makes sense. The current code really looks like
> __skb_incr_checksum_unnecessary, w/o the CHECKSUM_NONE check to only
> convert those packets.
> 
> Thanks!
> Antoine