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 for Android: free password hash cracker in your pocket

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

Message-ID: <6601c4f27529_11bc25294b@willemb.c.googlers.com.notmuch>
Date: Mon, 25 Mar 2024 14:39:46 -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-22 18:29:40)
> > Antoine Tenart wrote:
> > > UDP GRO validates checksums and in udp4/6_gro_complete fraglist packets
> > > are converted to CHECKSUM_UNNECESSARY to avoid later checks. However
> > > this is an issue for CHECKSUM_PARTIAL packets as they can be looped in
> > > an egress path and then their partial checksums are not fixed.
> > > 
> > > Different issues can be observed, from invalid checksum on packets to
> > > traces like:
> > > 
> > >   gen01: hw csum failure
> > >   skb len=3008 headroom=160 headlen=1376 tailroom=0
> > >   mac=(106,14) net=(120,40) trans=160
> > >   shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0))
> > >   csum(0xffff232e ip_summed=2 complete_sw=0 valid=0 level=0)
> > >   hash(0x77e3d716 sw=1 l4=1) proto=0x86dd pkttype=0 iif=12
> > >   ...
> > > 
> > > Fix this by only converting CHECKSUM_NONE packets to
> > > CHECKSUM_UNNECESSARY by reusing __skb_incr_checksum_unnecessary.
> > > 
> > > Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
> > > Signed-off-by: Antoine Tenart <atenart@...nel.org>
> > 
> > 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.

> It seems non-fraglist handles csum as it should
> already, except for tunneled packets but that's why patch 4 prevents
> those packets from being GROed.

You mean that on segmentation, the segments are restored and thus
skb->csum of each segment is again correct, right?

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.

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.

> 
> > Second, this leaves CHECKSUM_COMPLETE as is. Is that intentional? I
> > don't immediately see where GSO skb->csum would be updated.
> 
> That is intentional, fraglist GSO packets aren't modified and csums
> don't need to be updated. The issues are with converting the checksum
> type: partial checksum information can be lost.
> 
> Thanks,
> Antoine