Re: [PATCH net 1/2] udp: Mark GSO packets as CHECKSUM_UNNECESSARY early on on output

[Jakub Sitnicki <jakub@cloudflare.com>]

On Fri, Jul 26, 2024 at 09:58 AM -04, Willem de Bruijn wrote:
> On Fri, Jul 26, 2024 at 7:23 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> On Thu, Jul 25, 2024 at 10:21 AM -04, Willem de Bruijn wrote:
>> > On Thu, Jul 25, 2024 at 5:56 AM Jakub Sitnicki <jakub@cloudflare.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);
>> >
>>
>> That's something I should have explained better. Let me try to shed some
>> light on it now. We're hitting the skb_warn_bad_offload warning because
>> skb_mac_gso_segment doesn't return any segments (segs == NULL).
>>
>> And that's because we bail out early out of __udp_gso_segment when we
>> detect that the tunnel device is capable of tx-udp-segmentation
>> (GSO_UDP_L4):
>>
>>         if (skb_gso_ok(gso_skb, features | NETIF_F_GSO_ROBUST)) {
>>                 /* Packet is from an untrusted source, reset gso_segs. */
>>                 skb_shinfo(gso_skb)->gso_segs = DIV_ROUND_UP(gso_skb->len - sizeof(*uh),
>>                                                              mss);
>>                 return NULL;
>>         }
>
> Oh I see. Thanks.
>
>> It has not occurred to me before, but in the spirit of commit
>> 8d74e9f88d65 "net: avoid skb_warn_bad_offload on IS_ERR" [1], we could
>> tighten the check to exclude cases when segs == NULL. I'm thinking of:
>>
>>         if (segs != skb && !IS_ERR_OR_NULL(segs) && unlikely(skb_needs_check(skb, tx_path)))
>>                 skb_warn_bad_offload(skb);
>
> That looks sensible to me. And nicer than the ip_summed conversion in
> udp_send_skb.

I've audited all existing ->gso_segment callbacks. skb_mac_gso_segment()
returns no segments, that is segs == NULL, if the callback chain ends
with either of these:

… → udp[46]_ufo_fragment → __udp_gso_segment → skb_gso_ok == true
… → tcp[46]_gso_segment → tcp_gso_segment → skb_gso_ok == true
… → sctp_gso_segment → skb_gso_ok == true

IOW when the device advertises that it can handle the desired GSO kind
(skb_gso_ok() returns true).

Considering that a device offering HW GSO and no checksum offload at the
same time makes no sense, I also think that tweaking the bad offload
detection to exclude the !segs case doesn't deprive us of diagnostics.

I will change to that in v2.