netdev - Re: [PATCH bpf-next 05/15] bpf: Fix incorrect mem_cgroup

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, 12 Aug 2022 08:31:50 +0800
From:   Yafang Shao <laoar.shao@...il.com>
To:     Roman Gushchin <roman.gushchin@...ux.dev>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, jolsa@...nel.org,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Shakeel Butt <shakeelb@...gle.com>,
        Muchun Song <songmuchun@...edance.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>
Subject: Re: [PATCH bpf-next 05/15] bpf: Fix incorrect mem_cgroup_put

On Fri, Aug 12, 2022 at 12:48 AM Roman Gushchin
<roman.gushchin@...ux.dev> wrote:
>
> On Wed, Aug 10, 2022 at 03:18:30PM +0000, Yafang Shao wrote:
> > The memcg may be the root_mem_cgroup, in which case we shouldn't put it.
> > So a new helper bpf_map_put_memcg() is introduced to pair with
> > bpf_map_get_memcg().
> >
> > Fixes: 4201d9ab3e42 ("bpf: reparent bpf maps on memcg offlining")
> > Cc: Roman Gushchin <roman.gushchin@...ux.dev>
> > Cc: Shakeel Butt <shakeelb@...gle.com>
> > Signed-off-by: Yafang Shao <laoar.shao@...il.com>
> > ---
> >  kernel/bpf/syscall.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 83c7136..51ab8b1 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -441,6 +441,14 @@ static struct mem_cgroup *bpf_map_get_memcg(const struct bpf_map *map)
> >       return root_mem_cgroup;
> >  }
> >
> > +static void bpf_map_put_memcg(struct mem_cgroup *memcg)
> > +{
> > +     if (mem_cgroup_is_root(memcg))
> > +             return;
> > +
> > +     mem_cgroup_put(memcg);
> > +}
>
> +1 to what Shakeel said. mem_cgroup_put(root_mem_cgroup) is totally valid.
> So this change does absolutely nothing.
>

Do you mean that we can mem_cgroup_put(root_mem_cgroup) without
mem_cgroup_get(root_mem_cgroup) ?
Am I missing something ?

> The fixes tag assumes there is a bug in the existing code. If so, please,
> describe the problem and how to reproduce it.
>

It is found by code review.  The root_mem_cgroup's css will break. But
I don't know what it may cause to the user.
If you think the fixes tag is unproper, I will remove it.

> Also, if it's not related to the rest of the patchset, please, send it
> separately.
>

I want to introduce a bpf_map_put_memcg() helper to pair with
bpf_map_get_memcg().
This new helper will be used by other patches.

-- 
Regards
Yafang