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

["Jiayuan Chen" <jiayuan.chen@linux.dev>]

April 23, 2025 at 08:13, "Martin KaFai Lau" <martin.lau@linux.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@syzkaller.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@linux.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`.