From: Eric Dumazet <eric.dumazet@gmail.com>
To: Alexander Lobakin <alobakin@pm.me>,
Eric Dumazet <eric.dumazet@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
Paolo Abeni <pabeni@redhat.com>,
Willem de Bruijn <willemb@google.com>,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 net] net: udp: fix Fast/frag0 UDP GRO
Date: Mon, 9 Nov 2020 20:29:03 +0100 [thread overview]
Message-ID: <0a7af3fb-d1c4-bedf-4931-5f22f0481491@gmail.com> (raw)
In-Reply-To: <Nc6hn1Qaui1C7hTlHl8CdsNV00CdlHtyjQYv36ZYA@cp4-web-040.plabs.ch>
On 11/9/20 7:26 PM, Alexander Lobakin wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 9 Nov 2020 18:37:36 +0100
>
>> On 11/9/20 5:56 PM, Alexander Lobakin wrote:
>>> 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:
>>>
.
>>>
>>> Since v1 [1]:
>>> - added a NULL pointer check for "uh" as suggested by Willem.
>>>
>>> [1] https://lore.kernel.org/netdev/YazU6GEzBdpyZMDMwJirxDX7B4sualpDG68ADZYvJI@cp4-web-034.plabs.ch
>>>
>>> Fixes: e20cf8d3f1f7 ("udp: implement GRO for plain UDP sockets.")
>>> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
>>> ---
>>> net/ipv4/udp_offload.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>>> index e67a66fbf27b..7f6bd221880a 100644
>>> --- a/net/ipv4/udp_offload.c
>>> +++ b/net/ipv4/udp_offload.c
>>> @@ -366,13 +366,18 @@ 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;
>>> unsigned int ulen;
>>> int ret = 0;
>>>
>>> + if (unlikely(!uh)) {
>>
>> How uh could be NULL here ?
>>
>> My understanding is that udp_gro_receive() is called
>> only after udp4_gro_receive() or udp6_gro_receive()
>> validated that udp_gro_udphdr(skb) was not NULL.
>
> Right, but only after udp{4,6}_lib_lookup_skb() in certain cases.
> I don't know for sure if their logic can actually edit skb->data,
> so it's better to check from my point of view.
Not really. This would send a wrong signal to readers of this code.
I do not think these functions can mess with GRO internals.
This would be a nightmare, GRO is already way too complex.
In fact these functions should use a const qualifier
for their " struct sk_buff *skb" argument to prevent future bugs.
I will test and submit this patch :
diff --git a/include/net/ip.h b/include/net/ip.h
index 2d6b985d11ccaa75827b3a15ac3f898d7a193242..e20874059f826eb0f9e899aed556bfbc9c9d71e8 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -99,7 +99,7 @@ static inline void ipcm_init_sk(struct ipcm_cookie *ipcm,
#define PKTINFO_SKB_CB(skb) ((struct in_pktinfo *)((skb)->cb))
/* return enslaved device index if relevant */
-static inline int inet_sdif(struct sk_buff *skb)
+static inline int inet_sdif(const struct sk_buff *skb)
{
#if IS_ENABLED(CONFIG_NET_L3_MASTER_DEV)
if (skb && ipv4_l3mdev_skb(IPCB(skb)->flags))
diff --git a/include/net/udp.h b/include/net/udp.h
index 295d52a73598277dc5071536f777d1a87e7df1d1..877832bed4713a011a514a2f6f522728c8c89e20 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -164,7 +164,7 @@ static inline void udp_csum_pull_header(struct sk_buff *skb)
UDP_SKB_CB(skb)->cscov -= sizeof(struct udphdr);
}
-typedef struct sock *(*udp_lookup_t)(struct sk_buff *skb, __be16 sport,
+typedef struct sock *(*udp_lookup_t)(const struct sk_buff *skb, __be16 sport,
__be16 dport);
INDIRECT_CALLABLE_DECLARE(struct sk_buff *udp4_gro_receive(struct list_head *,
@@ -313,7 +313,7 @@ struct sock *udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr, __be16 sport,
__be32 daddr, __be16 dport, int dif, int sdif,
struct udp_table *tbl, struct sk_buff *skb);
-struct sock *udp4_lib_lookup_skb(struct sk_buff *skb,
+struct sock *udp4_lib_lookup_skb(const struct sk_buff *skb,
__be16 sport, __be16 dport);
struct sock *udp6_lib_lookup(struct net *net,
const struct in6_addr *saddr, __be16 sport,
@@ -324,7 +324,7 @@ struct sock *__udp6_lib_lookup(struct net *net,
const struct in6_addr *daddr, __be16 dport,
int dif, int sdif, struct udp_table *tbl,
struct sk_buff *skb);
-struct sock *udp6_lib_lookup_skb(struct sk_buff *skb,
+struct sock *udp6_lib_lookup_skb(const struct sk_buff *skb,
__be16 sport, __be16 dport);
/* UDP uses skb->dev_scratch to cache as much information as possible and avoid
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 09f0a23d1a01741d335ce45f25fe70a4e00698c7..8b8dadfea6c9854e6bfaa0fabcb774db39976da3 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -541,7 +541,7 @@ static inline struct sock *__udp4_lib_lookup_skb(struct sk_buff *skb,
inet_sdif(skb), udptable, skb);
}
-struct sock *udp4_lib_lookup_skb(struct sk_buff *skb,
+struct sock *udp4_lib_lookup_skb(const struct sk_buff *skb,
__be16 sport, __be16 dport)
{
const struct iphdr *iph = ip_hdr(skb);
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 29d9691359b9c49ccb56a11f79e3658b1a76700d..adfe9ca6f516612b5aad6d6c654c7da1dd56a50e 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -276,7 +276,7 @@ static struct sock *__udp6_lib_lookup_skb(struct sk_buff *skb,
inet6_sdif(skb), udptable, skb);
}
-struct sock *udp6_lib_lookup_skb(struct sk_buff *skb,
+struct sock *udp6_lib_lookup_skb(const struct sk_buff *skb,
__be16 sport, __be16 dport)
{
const struct ipv6hdr *iph = ipv6_hdr(skb);
next prev parent reply other threads:[~2020-11-09 19:29 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-09 16:56 [PATCH v2 net] net: udp: fix Fast/frag0 UDP GRO Alexander Lobakin
2020-11-09 17:08 ` Willem de Bruijn
2020-11-09 17:37 ` Eric Dumazet
2020-11-09 18:26 ` Alexander Lobakin
2020-11-09 19:29 ` Eric Dumazet [this message]
2020-11-09 19:56 ` Alexander Lobakin
2020-11-09 18:28 ` Willem de Bruijn
2020-11-09 18:54 ` Eric Dumazet
2020-11-09 19:06 ` Willem de Bruijn
2020-11-09 19:48 ` Alexander Lobakin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0a7af3fb-d1c4-bedf-4931-5f22f0481491@gmail.com \
--to=eric.dumazet@gmail.com \
--cc=alobakin@pm.me \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=kuznet@ms2.inr.ac.ru \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=willemb@google.com \
--cc=yoshfuji@linux-ipv6.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.