From: Cong Wang <xiyou.wangcong@gmail.com>
To: Roman Gushchin <guro@fb.com>
Cc: "Zefan Li" <lizefan@huawei.com>,
"Linux Kernel Network Developers" <netdev@vger.kernel.org>,
"Cameron Berkenpas" <cam@neo-zeon.de>,
"Peter Geis" <pgwipeout@gmail.com>,
"Lu Fengqi" <lufq.fnst@cn.fujitsu.com>,
"Daniël Sonck" <dsonck92@gmail.com>,
"Daniel Borkmann" <daniel@iogearbox.net>,
"Tejun Heo" <tj@kernel.org>
Subject: Re: [Patch net] cgroup: fix cgroup_sk_alloc() for sk_clone_lock()
Date: Mon, 22 Jun 2020 11:14:20 -0700 [thread overview]
Message-ID: <CAM_iQpVTwkxep3RCcwqCE0rypwj5prLdbE4oEUTyev+RxQq37Q@mail.gmail.com> (raw)
In-Reply-To: <20200620155751.GJ237539@carbon.dhcp.thefacebook.com>
On Sat, Jun 20, 2020 at 8:58 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Fri, Jun 19, 2020 at 08:00:41PM -0700, Cong Wang wrote:
> > On Fri, Jun 19, 2020 at 6:14 PM Roman Gushchin <guro@fb.com> wrote:
> > >
> > > On Sat, Jun 20, 2020 at 09:00:40AM +0800, Zefan Li wrote:
> > > > I think so, though I'm not familiar with the bfp cgroup code.
> > > >
> > > > > If so, we might wanna fix it in a different way,
> > > > > just checking if (!(css->flags & CSS_NO_REF)) in cgroup_bpf_put()
> > > > > like in cgroup_put(). It feels more reliable to me.
> > > > >
> > > >
> > > > Yeah I also have this idea in my mind.
> > >
> > > I wonder if the following patch will fix the issue?
> >
> > Interesting, AFAIU, this refcnt is for bpf programs attached
> > to the cgroup. By this suggestion, do you mean the root
> > cgroup does not need to refcnt the bpf programs attached
> > to it? This seems odd, as I don't see how root is different
> > from others in terms of bpf programs which can be attached
> > and detached in the same way.
> >
> > I certainly understand the root cgroup is never gone, but this
> > does not mean the bpf programs attached to it too.
> >
> > What am I missing?
>
> It's different because the root cgroup can't be deleted.
>
> All this reference counting is required to automatically detach bpf programs
> from a _deleted_ cgroup (look at cgroup_bpf_offline()). It's required
> because a cgroup can be in dying state for a long time being pinned by a
> pagecache page, for example. Only a user can detach a bpf program from
> an existing cgroup.
Yeah, but users can still detach the bpf programs from root cgroup.
IIUC, after detaching, the pointer in the bpf array will be empty_prog_array
which is just an array of NULL. Then __cgroup_bpf_run_filter_skb() will
deref it without checking NULL (as check_non_null == false).
This matches the 0000000000000010 pointer seen in the bug reports,
the 0x10, that is 16, is the offset of items[] in struct bpf_prog_array.
So looks like we have to add a NULL check there regardless of refcnt.
Also, I am not sure whether your suggested patch makes a difference
for percpu refcnt, as percpu_ref_put() will never call ->release() until
percpu_ref_kill(), which is never called on root cgroup?
Thanks.
next prev parent reply other threads:[~2020-06-22 18:14 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-16 18:03 [Patch net] cgroup: fix cgroup_sk_alloc() for sk_clone_lock() Cong Wang
2020-06-18 1:44 ` Zefan Li
2020-06-18 19:19 ` Cong Wang
2020-06-18 19:36 ` Roman Gushchin
2020-06-18 21:09 ` Cong Wang
2020-06-18 21:26 ` Roman Gushchin
2020-06-18 22:45 ` Peter Geis
2020-06-19 6:40 ` Zefan Li
2020-06-19 19:51 ` Cong Wang
2020-06-20 0:45 ` Zefan Li
2020-06-20 0:51 ` Zefan Li
2020-06-20 3:31 ` Cong Wang
2020-06-20 7:52 ` Zefan Li
2020-06-20 16:04 ` Roman Gushchin
2020-06-23 22:21 ` Roman Gushchin
2020-06-26 5:23 ` Cameron Berkenpas
2020-06-26 17:58 ` Cong Wang
2020-06-26 22:03 ` Cameron Berkenpas
2020-06-27 22:59 ` Cameron Berkenpas
2020-06-30 22:16 ` Cong Wang
2020-06-27 23:41 ` Roman Gushchin
2020-06-30 22:22 ` Cong Wang
2020-06-30 22:48 ` Roman Gushchin
2020-07-01 1:18 ` Zefan Li
2020-07-02 4:48 ` Cong Wang
2020-07-02 8:12 ` Thomas Lamprecht
2020-07-02 16:02 ` Roman Gushchin
2020-07-02 16:24 ` Peter Geis
2020-07-03 1:17 ` Zefan Li
2020-06-20 0:51 ` Roman Gushchin
2020-06-20 1:00 ` Zefan Li
2020-06-20 1:14 ` Roman Gushchin
2020-06-20 2:48 ` Zefan Li
2020-06-20 3:00 ` Cong Wang
2020-06-20 15:57 ` Roman Gushchin
2020-06-22 18:14 ` Cong Wang [this message]
2020-06-22 20:39 ` Roman Gushchin
2020-06-23 8:45 ` Zhang,Qiang
2020-06-23 17:56 ` Cong Wang
2020-06-23 8:54 ` Zhang,Qiang
2020-06-23 9:01 ` Zhang,Qiang
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=CAM_iQpVTwkxep3RCcwqCE0rypwj5prLdbE4oEUTyev+RxQq37Q@mail.gmail.com \
--to=xiyou.wangcong@gmail.com \
--cc=cam@neo-zeon.de \
--cc=daniel@iogearbox.net \
--cc=dsonck92@gmail.com \
--cc=guro@fb.com \
--cc=lizefan@huawei.com \
--cc=lufq.fnst@cn.fujitsu.com \
--cc=netdev@vger.kernel.org \
--cc=pgwipeout@gmail.com \
--cc=tj@kernel.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 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 NNTP newsgroup(s).