Thread (6 messages) flat view 6 messages, 3 authors, 2020-11-16

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

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2020-11-09 22:25:11
Also in: lkml

On Mon, Nov 9, 2020 at 4:15 PM Alexander Lobakin [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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.

Since v2 [1]:
 - dropped redundant check introduced in v2 as it's performed right
   before (thanks to Eric);
 - udp_hdr() switched to data + off for skbs from list (also Eric);
 - fixed possible malfunction of {,__}udp{4,6}_lib_lookup_skb() with
   Fast/frag0 due to ip{,v6}_hdr() usage (Willem).

Since v1 [2]:
 - added a NULL pointer check for "uh" as suggested by Willem.

[1] https://lore.kernel.org/netdev/0eaG8xtbtKY1dEKCTKUBubGiC9QawGgB3tVZtNqVdY@cp4-web-030.plabs.ch (local)
[2] https://lore.kernel.org/netdev/YazU6GEzBdpyZMDMwJirxDX7B4sualpDG68ADZYvJI@cp4-web-034.plabs.ch (local)

Fixes: e20cf8d3f1f7 ("udp: implement GRO for plain UDP sockets.")
Signed-off-by: Alexander Lobakin <redacted>
---
 net/ipv4/udp.c         | 4 ++--
 net/ipv4/udp_offload.c | 9 ++++++---
 net/ipv6/udp.c         | 4 ++--
 3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 09f0a23d1a01..948ddc9a0212 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -534,7 +534,7 @@ static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb,
                                                 __be16 sport, __be16 dport,
                                                 struct udp_table *udptable)
 {
-       const struct iphdr *iph = ip_hdr(skb);
+       const struct iphdr *iph = skb_gro_network_header(skb);
This function is called from the normal UDP stack, not the GRO stack.
It's not safe to use this helper here.
quoted hunk ↗ jump to hunk
        return __udp4_lib_lookup(dev_net(skb->dev), iph->saddr, sport,
                                 iph->daddr, dport, inet_iif(skb),
@@ -544,7 +544,7 @@ static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb,
 struct sock *udp4_lib_lookup_skb(struct sk_buff *skb,
                                 __be16 sport, __be16 dport)
 {
-       const struct iphdr *iph = ip_hdr(skb);
+       const struct iphdr *iph = skb_gro_network_header(skb);
This one is, but I think it would be preferable to avoid leaking this
frag0 optimization stuff outside of the core GRO code if we can help
it.

Also haven't checked whether that helper is safe to call from
.gro_complete handlers such as udp_gro_complete. It's not needed
there, in any case.

Instead, perhaps we can call __udp4_lib_lookup which takes the exact
fields as arguments, and do the network header lookup in
udp_gro_complete itself.

Less important (because it's not working before), does the use of
skb_gro_network_header break any nested tunnel support that the
p->data + off change would add?
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help