netdev - Re: [BUG bpf-next] bpf/net: Hitting gpf when running selftests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 1 Sep 2023 11:32:24 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Xu Kuohai <xukuohai@...wei.com>
Cc: John Fastabend <john.fastabend@...il.com>,
	Jiri Olsa <olsajiri@...il.com>, Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Andrii Nakryiko <andrii@...nel.org>, netdev@...r.kernel.org,
	bpf@...r.kernel.org, Martin KaFai Lau <kafai@...com>,
	Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
	KP Singh <kpsingh@...omium.org>,
	Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>,
	Hou Tao <houtao1@...wei.com>,
	Bobby Eshleman <bobby.eshleman@...edance.com>
Subject: Re: [BUG bpf-next] bpf/net: Hitting gpf when running selftests

On Fri, Sep 01, 2023 at 05:10:43PM +0800, Xu Kuohai wrote:

SNIP

> > > Trying to come up with some nice fix now.
> > 
> > Something like this it fixes the splat, but need to think if it
> > introduces anything or some better way to do this. Basic idea
> > is to bump user->refcnt because we have two references to the
> > skb and want to ensure we really only kfree_skb() the skb
> > after both references are dropped.
> > 
> > diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> > index a0659fc29bcc..6c31eefbd777 100644
> > --- a/net/core/skmsg.c
> > +++ b/net/core/skmsg.c
> > @@ -612,12 +612,18 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
> >   static int sk_psock_handle_skb(struct sk_psock *psock, struct sk_buff *skb,
> >                                 u32 off, u32 len, bool ingress)
> >   {
> > +       int err = 0;
> > +
> >          if (!ingress) {
> >                  if (!sock_writeable(psock->sk))
> >                          return -EAGAIN;
> >                  return skb_send_sock(psock->sk, skb, off, len);
> >          }
> > -       return sk_psock_skb_ingress(psock, skb, off, len);
> > +       skb_get(skb);
> > +       err = sk_psock_skb_ingress(psock, skb, off, len);
> > +       if (err < 0)
> > +               kfree_skb(skb);
> > +       return err;
> >   }
> >   static void sk_psock_skb_state(struct sk_psock *psock,
> > @@ -685,9 +691,7 @@ static void sk_psock_backlog(struct work_struct *work)
> >                  } while (len);
> >                  skb = skb_dequeue(&psock->ingress_skb);
> > -               if (!ingress) {
> > -                       kfree_skb(skb);
> > -               }
> > +               kfree_skb(skb);
> >          }
> >   end:
> >          mutex_unlock(&psock->work_mutex);
> > .
> 
> With this fix, the crash is gone.

+1, same on my setup

jirka

> 
> I am worried that the skb might be inserted into another skb list before
> skb_dequeue is called, but I can’t find such code, it seems this worry
> is unnecessary.