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

Message-ID: <171144629575.5526.17151762059541054429@kwain>
Date: Tue, 26 Mar 2024 10:44:55 +0100
From: Antoine Tenart <atenart@...nel.org>
To: 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

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

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

> 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