linux-kernel - Re: [PATCH bpf-next v1 1/2] bpf: Create cgroup storage if needed when updating link

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]

Message-ID: <cbb82d78518c251000e8b52e3f3799b0df438210@linux.dev>
Date: Wed, 23 Apr 2025 02:21:37 +0000
From: "Jiayuan Chen" <jiayuan.chen@...ux.dev>
To: "Martin KaFai Lau" <martin.lau@...ux.dev>
Cc: andrii@...nel.org, alexis.lothore@...tlin.com, mrpre@....com,
 syzbot+e6e8f6618a2d4b35e4e0@...kaller.appspotmail.com, "Alexei
 Starovoitov" <ast@...nel.org>, "Daniel Borkmann" <daniel@...earbox.net>,
 "Eduard Zingerman" <eddyz87@...il.com>, "Song Liu" <song@...nel.org>,
 "Yonghong Song" <yonghong.song@...ux.dev>, "John Fastabend"
 <john.fastabend@...il.com>, "KP Singh" <kpsingh@...nel.org>, "Stanislav
 Fomichev" <sdf@...ichev.me>, "Hao Luo" <haoluo@...gle.com>, "Jiri Olsa"
 <jolsa@...nel.org>, "Mykola Lysenko" <mykolal@...com>, "Shuah Khan"
 <shuah@...nel.org>, "Alan Maguire" <alan.maguire@...cle.com>,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
 bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v1 1/2] bpf: Create cgroup storage if needed
 when updating link

April 23, 2025 at 08:13, "Martin KaFai Lau" <martin.lau@...ux.dev> wrote:

> 
> On 4/16/25 9:40 PM, Jiayuan Chen wrote:
> 
> > 
> > when we attach a prog without cgroup_storage map being used,
> > 
> >  cgroup_storage in struct bpf_prog_array_item is empty. Then, if we use
> > 
> >  BPF_LINK_UPDATE to replace old prog with a new one that uses the
> > 
> >  cgroup_storage map, we miss cgroup_storage being initiated.
> > 
> >  This cause a painc when accessing stroage in bpf_get_local_storage.
> > 
> >  Reported-by: syzbot+e6e8f6618a2d4b35e4e0@...kaller.appspotmail.com
> > 
> >  Closes: https://lore.kernel.org/all/67fc867e.050a0220.2970f9.03b8.GAE@google.com/T/
> > 
> >  Fixes: 0c991ebc8c69 ("bpf: Implement bpf_prog replacement for an active bpf_cgroup_link")
> > 
> >  Signed-off-by: Jiayuan Chen <jiayuan.chen@...ux.dev>
> > 
> >  ---
> > 
> >  kernel/bpf/cgroup.c | 24 +++++++++++++++++++-----
> > 
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> > 
> >  diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> > 
> >  index 84f58f3d028a..cdf0211ddc79 100644
> > 
> >  --- a/kernel/bpf/cgroup.c
> > 
> >  +++ b/kernel/bpf/cgroup.c
> > 
> >  @@ -770,12 +770,14 @@ static int cgroup_bpf_attach(struct cgroup *cgrp,
> > 
> >  }
> > 
> >  > /* Swap updated BPF program for given link in effective program arrays across
> > 
> >  - * all descendant cgroups. This function is guaranteed to succeed.
> > 
> >  + * all descendant cgroups.
> > 
> >  */
> > 
> >  -static void replace_effective_prog(struct cgroup *cgrp,
> > 
> >  - enum cgroup_bpf_attach_type atype,
> > 
> >  - struct bpf_cgroup_link *link)
> > 
> >  +static int replace_effective_prog(struct cgroup *cgrp,
> > 
> >  + enum cgroup_bpf_attach_type atype,
> > 
> >  + struct bpf_cgroup_link *link)
> > 
> >  {
> > 
> >  + struct bpf_cgroup_storage *new_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {};
> > 
> >  + struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = {};
> > 
> >  struct bpf_prog_array_item *item;
> > 
> >  struct cgroup_subsys_state *css;
> > 
> >  struct bpf_prog_array *progs;
> > 
> >  @@ -784,6 +786,10 @@ static void replace_effective_prog(struct cgroup *cgrp,
> > 
> >  struct cgroup *cg;
> > 
> >  int pos;
> > 
> >  > + if (bpf_cgroup_storages_alloc(storage, new_storage, link->type,
> > 
> >  + link->link.prog, cgrp))
> > 
> >  + return -ENOMEM;
> > 
> >  +
> > 
> >  css_for_each_descendant_pre(css, &cgrp->self) {
> > 
> >  struct cgroup *desc = container_of(css, struct cgroup, self);
> > 
> >  > @@ -810,8 +816,11 @@ static void replace_effective_prog(struct cgroup *cgrp,
> > 
> >  desc->bpf.effective[atype],
> > 
> >  lockdep_is_held(&cgroup_mutex));
> > 
> >  item = &progs->items[pos];
> > 
> >  + bpf_cgroup_storages_assign(item->cgroup_storage, storage);
> > 
> 
> I am still recalling my memory on this older cgroup storage, so I think it will be faster to ask questions.
> 
> What is in the pl->storage (still NULL?), and will the future compute_effective_progs() work?
> 

For non-link path:
cgroup_bpf_attach
	bpf_cgroup_storages_assign(pl->storage, storage); // allocate and set
	update_effective_progs
		compute_effective_progs
			bpf_cgroup_storages_assign(item->cgroup_storage, pl->storage);


pl->storage is just as a temporary holder, never freed, and its value will
eventually be assigned to `item->cgroup_storage`.