netdev - Re: [PATCH net v3 3/4] udp: do not transition UDP GRO fraglist partial checksums to unnecessary

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] [day] [month] [year] [list]

Message-ID: <6602cf8c2c62b_13d9ab29440@willemb.c.googlers.com.notmuch>
Date: Tue, 26 Mar 2024 09:37:16 -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 v3 3/4] udp: do not transition UDP GRO fraglist
 partial checksums to unnecessary

Antoine Tenart wrote:
> Quoting Willem de Bruijn (2024-03-25 19:39:46)
> > Antoine Tenart wrote:
> > > Quoting Willem de Bruijn (2024-03-22 18:29:40)
> > > > 
> > > > Should fraglist UDP GRO and non-fraglist (udp_gro_complete_segment)
> > > > have the same checksumming behavior?
> > > 
> > > They can't as non-fraglist GRO packets can be aggregated, csum can't
> > > just be converted there.
> > 
> > I suppose this could be done. But it is just simpler to convert to
> > CHECKSUM_UNNECESSARY.
> 
> Oh, do you mean using the non-fraglist behavior in fraglist?
> udp_gro_complete_segment converts all packets to CHECKSUM_PARTIAL (as
> packets could have been aggregated) but that's not required in fraglist.
> 
> To say it another way: my understanding is packets in the non-fraglist
> case have to be converted to CHECKSUM_PARTIAL, while the fraglist case
> can keep the checksum info as-is (and have the conversion to unnecessary
> as an optimization when applicable).

That makes sense. Thanks.
 
> > You mean that on segmentation, the segments are restored and thus
> > skb->csum of each segment is again correct, right?
> 
> In the fraglist case, yes.
> 
> > I suppose this could be converted to CHECKSUM_UNNECESSARY if just
> > for equivalence between the two UDP_GRO methods and simplicity.
> > 
> > But also fine to leave as is.
> 
> I'm not sure I got your suggestion as I don't see how non-fraglist
> packets could be converted to CHECKSUM_UNNECESSARY when being aggregated
> (uh->len can change there).
> 
> This series is aiming at fixing known issues and unifying the behavior
> would be net-next material IMO. I'll send a v4 for those fixes, but then
> I'm happy to discuss the above suggestion and investigate; so let's
> continue the discussion here in parallel.

The above explains why the solutions are distinct. No need to try to
unify more in a follow-up, then.

> > Can you at least summarize this in the commit message? Currently
> > CHECKSUM_COMPLETE is not mentioned, but the behavior is not trivial.
> > It may be helpful next time we again stumble on this code and do a
> > git blame.
> 
> Sure, I'll try to improve the commit log.
> 
> Thanks!
> Antoine