netdev - Re: [PATCH net 1/2] udp: Mark GSO packets as 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: <CAF=yD-LLPPg77MUhdXrHUVJj4o2+rnOC_qsHc_8tKurTsAGkYw@mail.gmail.com>
Date: Thu, 25 Jul 2024 10:21:50 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jakub Sitnicki <jakub@...udflare.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Willem de Bruijn <willemb@...gle.com>, kernel-team@...udflare.com, 
	syzbot+e15b7e15b8a751a91d9a@...kaller.appspotmail.com
Subject: Re: [PATCH net 1/2] udp: Mark GSO packets as CHECKSUM_UNNECESSARY
 early on on output

On Thu, Jul 25, 2024 at 5:56 AM Jakub Sitnicki <jakub@...udflare.com> wrote:
>
> In commit 10154dbded6d ("udp: Allow GSO transmit from devices with no
> checksum offload") we have added a tweak in the UDP GSO code to mark GSO
> packets being sent out as CHECKSUM_UNNECESSARY when the egress device
> doesn't support checksum offload. This was done to satisfy the offload
> checks in the gso stack.
>
> However, when sending a UDP GSO packet from a tunnel device, we will go
> through the TX path and the GSO offload twice. Once for the tunnel device,
> which acts as a passthru for GSO packets, and once for the underlying
> egress device.
>
> Even though a tunnel device acts as a passthru for a UDP GSO packet, GSO
> offload checks still happen on transmit from a tunnel device. So if the skb
> is not marked as CHECKSUM_UNNECESSARY or CHECKSUM_PARTIAL, we will get a
> warning from the gso stack.

I don't entirely understand. The check should not hit on pass through,
where segs == skb:

        if (segs != skb && unlikely(skb_needs_check(skb, tx_path) &&
!IS_ERR(segs)))
                skb_warn_bad_offload(skb);

> Today this can occur in two situations, which we check for in
> __ip_append_data() and __ip6_append_data():
>
> 1) when the tunnel device does not advertise checksum offload, or
> 2) when there are IPv6 extension headers present.
>
> To fix it mark UDP_GSO packets as CHECKSUM_UNNECESSARY early on the TX
> path, when still in the udp layer, since we need to have ip_summed set up
> correctly for GSO processing by tunnel devices.

The previous patch converted segments post segmentation to
CHECKSUM_UNNECESSARY, which is fine as they had
already been checksummed in software, and CHECKSUM_NONE
packets on egress are common.

This creates GSO packets without CHECKSUM_PARTIAL.
Segmentation offload always requires checksum offload. So these
would be weird new packets. And having CHECKSUM_NONE (or
equivalent), but entering software checksumming is also confusing.

The crux is that I don't understand why the warning fires on tunnel
exit when no segmentation takes place there. Hopefully we can fix
in a way that does not introduce these weird GSO packets (but if
not, so be it).