From: "Maciej Żenczykowski" <maze@google.com>
To: "Lena Wang (王娜)" <Lena.Wang@mediatek.com>
Cc: "steffen.klassert@secunet.com" <steffen.klassert@secunet.com>,
"kuba@kernel.org" <kuba@kernel.org>,
"Shiming Cheng (成诗明)" <Shiming.Cheng@mediatek.com>,
"pabeni@redhat.com" <pabeni@redhat.com>,
"edumazet@google.com" <edumazet@google.com>,
"willemdebruijn.kernel@gmail.com"
<willemdebruijn.kernel@gmail.com>,
"matthias.bgg@gmail.com" <matthias.bgg@gmail.com>,
"davem@davemloft.net" <davem@davemloft.net>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH net] udp: fix segmentation crash for GRO packet without fraglist
Date: Mon, 15 Apr 2024 19:53:48 -0700 [thread overview]
Message-ID: <CANP3RGdkxT4TjeSvv1ftXOdFQd5Z4qLK1DbzwATq_t_Dk+V8ig@mail.gmail.com> (raw)
In-Reply-To: <65e3e88a53d466cf5bad04e5c7bc3f1648b82fd7.camel@mediatek.com>
On Mon, Apr 15, 2024 at 7:14 PM Lena Wang (王娜) <Lena.Wang@mediatek.com> wrote:
>
> On Mon, 2024-04-15 at 16:53 -0400, Willem de Bruijn wrote:
> >
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > shiming.cheng@ wrote:
> > > From: Shiming Cheng <shiming.cheng@mediatek.com>
> > >
> > > A GRO packet without fraglist is crashed and backtrace is as below:
> > > [ 1100.812205][ C3] CPU: 3 PID: 0 Comm: swapper/3 Tainted:
> > > G W OE 6.6.17-android15-0-g380371ea9bf1 #1
> > > [ 1100.812317][ C3] __udp_gso_segment+0x298/0x4d4
> > > [ 1100.812335][ C3] __skb_gso_segment+0xc4/0x120
> > > [ 1100.812339][ C3] udp_rcv_segment+0x50/0x134
> > > [ 1100.812344][ C3] udp_queue_rcv_skb+0x74/0x114
> > > [ 1100.812348][ C3] udp_unicast_rcv_skb+0x94/0xac
> > > [ 1100.812358][ C3] udp_rcv+0x20/0x30
> > >
> > > The reason that the packet loses its fraglist is that in ingress
> > bpf
> > > it makes a test pull with to make sure it can read packet headers
> > > via direct packet access: In bpf_progs/offload.c
> > > try_make_writable -> bpf_skb_pull_data -> pskb_may_pull ->
> > > __pskb_pull_tail This operation pull the data in fraglist into
> > linear
> > > and set the fraglist to null.
> >
> > What is the right behavior from BPF with regard to SKB_GSO_FRAGLIST
> > skbs?
> >
> > Some, like SCTP, cannot be linearized ever, as the do not have a
> > single gso_size.
> >
> > Should this BPF operation just fail?
> >
> In most situation for big gso size packet, it indeed fails but BPF
> doesn't check the result. It seems the udp GRO packet can't be pulled/
> trimed/condensed or else it can't be segmented correctly.
>
> As the BPF function comments it doesn't matter if the data pull failed
> or pull less. It just does a blind best effort pull.
>
> A patch to modify bpf pull length is upstreamed to Google before and
> below are part of Google BPF expert maze's reply:
> maze@google.com<maze@google.com> #5Apr 13, 2024 02:30AM
> I *think* if that patch fixes anything, then it's really proving that
> there's a bug in the kernel that needs to be fixed instead.
> It should be legal to call try_make_writable(skb, X) with *any* value
> of X.
>
> I add maze in loop and we could start more discussion here.
Personally, I think bpf_skb_pull_data() should have automatically
(ie. in kernel code) reduced how much it pulls so that it would pull
headers only,
and not packet content.
(This is assuming the rest of the code isn't ready to deal with a longer pull,
which I think is the case atm. Pulling too much, and then crashing or forcing
the stack to drop packets because of them being malformed seems wrong...)
In general it would be nice if there was a way to just say pull all headers...
(or possibly all L2/L3/L4 headers)
You in general need to pull stuff *before* you've even looked at the packet,
so that you can look at the packet,
so it's relatively hard/annoying to pull the correct length from bpf
code itself.
> > > BPF needs to modify a proper length to do pull data. However kernel
> > > should also improve the flow to avoid crash from a bpf function
> > call.
> > > As there is no split flow and app may not decode the merged UDP
> > packet,
> > > we should drop the packet without fraglist in skb_segment_list
> > here.
> > >
> > > Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
> > > Signed-off-by: Shiming Cheng <shiming.cheng@mediatek.com>
> > > Signed-off-by: Lena Wang <lena.wang@mediatek.com>
> > > ---
> > > net/core/skbuff.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > > index b99127712e67..f68f2679b086 100644
> > > --- a/net/core/skbuff.c
> > > +++ b/net/core/skbuff.c
> > > @@ -4504,6 +4504,9 @@ struct sk_buff *skb_segment_list(struct
> > sk_buff *skb,
> > > if (err)
> > > goto err_linearize;
> > >
> > > +if (!list_skb)
> > > +goto err_linearize;
> > > +
> > > skb_shinfo(skb)->frag_list = NULL;
> >
> > In absense of plugging the issue in BPF, dropping here is the best
> > we can do indeed, I think.
> >
--
Maciej Żenczykowski, Kernel Networking Developer @ Google
next prev parent reply other threads:[~2024-04-16 2:54 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-15 15:01 [PATCH net] udp: fix segmentation crash for GRO packet without fraglist shiming.cheng
2024-04-15 20:53 ` Willem de Bruijn
2024-04-16 2:14 ` Lena Wang (王娜)
2024-04-16 2:53 ` Maciej Żenczykowski [this message]
2024-04-16 17:16 ` Willem de Bruijn
2024-04-16 17:51 ` Maciej Żenczykowski
2024-04-16 17:57 ` Maciej Żenczykowski
2024-04-16 23:14 ` Willem de Bruijn
2024-04-17 7:19 ` Lena Wang (王娜)
2024-04-17 19:48 ` Willem de Bruijn
2024-04-18 2:52 ` Lena Wang (王娜)
2024-04-18 4:15 ` Maciej Żenczykowski
2024-04-19 8:36 ` Lena Wang (王娜)
2024-04-19 14:17 ` Willem de Bruijn
2024-04-19 17:29 ` Maciej Żenczykowski
2024-04-19 17:41 ` Willem de Bruijn
2024-04-23 14:47 ` Lena Wang (王娜)
2024-04-23 18:35 ` Willem de Bruijn
2024-04-24 12:22 ` Lena Wang (王娜)
2024-04-24 14:28 ` Willem de Bruijn
2024-04-25 4:32 ` Lena Wang (王娜)
2024-04-25 14:07 ` Willem de Bruijn
2024-04-26 9:52 ` Lena Wang (王娜)
2024-04-26 21:08 ` Daniel Borkmann
2024-04-27 13:28 ` Willem de Bruijn
2024-04-28 7:48 ` Lena Wang (王娜)
2024-04-28 13:19 ` Willem de Bruijn
2024-04-29 10:15 ` Daniel Borkmann
2024-04-29 11:45 ` Lena Wang (王娜)
2024-04-29 15:11 ` Daniel Borkmann
2024-04-29 21:14 ` Willem de Bruijn
2024-04-26 0:16 ` Maciej Żenczykowski
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=CANP3RGdkxT4TjeSvv1ftXOdFQd5Z4qLK1DbzwATq_t_Dk+V8ig@mail.gmail.com \
--to=maze@google.com \
--cc=Lena.Wang@mediatek.com \
--cc=Shiming.Cheng@mediatek.com \
--cc=bpf@vger.kernel.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=matthias.bgg@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=steffen.klassert@secunet.com \
--cc=willemdebruijn.kernel@gmail.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).