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: <171086409633.4835.11427072260403202761@kwain>
Date: Tue, 19 Mar 2024 17:01:36 +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 v2 3/4] udp: do not transition UDP fraglist to unnecessary checksum

Quoting Willem de Bruijn (2024-03-19 14:38:20)
> Antoine Tenart wrote:
> > udp4/6_gro_complete transition fraglist packets to CHECKSUM_UNNECESSARY
> > and sets their checksum level based on if the packet is recognized to be
> > a tunneled one. However there is no safe way to detect a packet is a
> > tunneled one and in case such packet is GROed at the UDP level, setting
> > a wrong checksum level will lead to later errors. For example if those
> > packets are forwarded to the Tx path they could produce the following
> > dump:
> > 
> >   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
> >   ...
> > 
> > Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
> > Signed-off-by: Antoine Tenart <atenart@...nel.org>
> 
> 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.

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.

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?

For reference, original commit says:
"""
After validating the csum,  we mark ip_summed as
CHECKSUM_UNNECESSARY for fraglist GRO packets to
make sure that the csum is not touched.
"""

But I'm failing to see where that would happen and how the none to
unnecessary conversion would help. WDYT?

Thanks,
Antoine