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

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Mon, 9 Nov 2020 09:36:12 -0500

On Sat, Nov 7, 2020 at 8:11 PM Alexander Lobakin [off-list ref] wrote:

quoted

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() 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.

Fixes: e20cf8d3f1f7 ("udp: implement GRO for plain UDP sockets.")
Signed-off-by: Alexander Lobakin <redacted>
---
 net/ipv4/udp_offload.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index e67a66fbf27b..13740e9fe6ec 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c

@@ -366,7 +366,7 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
 static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
                                               struct sk_buff *skb)
 {
-       struct udphdr *uh = udp_hdr(skb);
+       struct udphdr *uh = udp_gro_udphdr(skb);
        struct sk_buff *pp = NULL;
        struct udphdr *uh2;
        struct sk_buff *p;

Good catch. skb_gro_header_slow may fail and return NULL. Need to
check that before dereferencing uh below in

Ah, sure. It's very unlikely condition, but "better safe than sorry"
(c). Thanks, will do v2 soon.

quoted

        /* requires non zero csum, for symmetry with GSO */
        if (!uh->check) {
                NAPI_GRO_CB(skb)->flush = 1;
                return NULL;
        }

Al