April 23, 2025 at 08:13, "Martin KaFai Lau" <martin....@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+e6e8f6618a2d4b35e...@syzkaller.appspotmail.com > > > > Closes: > > https://lore.kernel.org/all/67fc867e.050a0220.2970f9.03b8....@google.com/T/ > > > > Fixes: 0c991ebc8c69 ("bpf: Implement bpf_prog replacement for an active > > bpf_cgroup_link") > > > > Signed-off-by: Jiayuan Chen <jiayuan.c...@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`.
