Re: [PATCH v4 net] net: udp: fix Fast/frag0 UDP GRO

[Willem de Bruijn]

On Wed, Nov 11, 2020 at 6:29 AM Alexander Lobakin <alobakin@xxxxx> wrote:
>
> From: Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx>
> Date: Tue, 10 Nov 2020 13:49:56 -0500
>
> > On Mon, Nov 9, 2020 at 7:29 PM Alexander Lobakin <alobakin@xxxxx> wrote:
> >>
> >> From: Alexander Lobakin <alobakin@xxxxx>
> >> Date: Tue, 10 Nov 2020 00:17:18 +0000
> >>
> >>> While testing UDP GSO fraglists forwarding through driver that uses
> >>> Fast GRO (via napi_gro_frags()), I was observing lots of out-of-order
> >>> iperf packets:
> >>>
> >>> [ ID] Interval           Transfer     Bitrate         Jitter
> >>> [SUM]  0.0-40.0 sec  12106 datagrams received out-of-order
> >>>
> >>> Simple switch to napi_gro_receive() or any other method without frag0
> >>> shortcut completely resolved them.
> >>>
> >>> I've found that UDP GRO uses udp_hdr(skb) in its .gro_receive()
> >>> callback. While it's probably OK for non-frag0 paths (when all
> >>> headers or even the entire frame are already in skb->data), this
> >>> inline points to junk when using Fast GRO (napi_gro_frags() or
> >>> napi_gro_receive() with only Ethernet header in skb->data and all
> >>> the rest in shinfo->frags) and breaks GRO packet compilation and
> >>> the packet flow itself.
> >>> To support both modes, skb_gro_header_fast() + skb_gro_header_slow()
> >>> are typically used. UDP even has an inline helper that makes use of
> >>> them, udp_gro_udphdr(). Use that instead of troublemaking udp_hdr()
> >>> to get rid of the out-of-order delivers.
> >>>
> >>> Present since the introduction of plain UDP GRO in 5.0-rc1.
> >>>
> >>> Since v3 [1]:
> >>>  - restore the original {,__}udp{4,6}_lib_lookup_skb() and use
> >>>    private versions of them inside GRO code (Willem).
> >>
> >> Note: this doesn't cover a support for nested tunnels as it's out of
> >> the subject and requires more invasive changes. It will be handled
> >> separately in net-next series.
> >
> > Thanks for looking into that.
>
> Thank you (and Eric) for all your comments and reviews :)
>
> > In that case, should the p->data + off change be deferred to that,
> > too? It adds some risk unrelated to the bug fix.
>
> Change to p->data + off is absolutely safe and even can prevent from
> any other potentional problems with Fast/frag0 GRO of UDP fraglists.
> I find them pretty fragile currently.

Especially for fixes that go to net and eventually stable branches,
I'm in favor of the smallest possible change, minimizing odds of
unintended side effects.

Skipping this would also avoid the int to u32 change.

But admittedly at some point it is a matter of preference. Overall
makes sense to me. Thanks for the fix!

Acked-by: Willem de Bruijn <willemb@xxxxxxxxxx>