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

[Martin KaFai Lau <martin.lau@linux.dev>]

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?

>   		WRITE_ONCE(item->prog, link->link.prog);
>   	}
> +	bpf_cgroup_storages_link(new_storage, cgrp, link->type);
> +	return 0;
>   }
>   
>   /**
> @@ -833,6 +842,7 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp,
>   	struct bpf_prog_list *pl;
>   	struct hlist_head *progs;
>   	bool found = false;
> +	int err;
>   
>   	atype = bpf_cgroup_atype_find(link->type, new_prog->aux->attach_btf_id);
>   	if (atype < 0)
> @@ -853,7 +863,11 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp,
>   		return -ENOENT;
>   
>   	old_prog = xchg(&link->link.prog, new_prog);
> -	replace_effective_prog(cgrp, atype, link);
> +	err = replace_effective_prog(cgrp, atype, link);
> +	if (err) {
> +		xchg(&link->link.prog, old_prog);
> +		return err;
> +	}
>   	bpf_prog_put(old_prog);
>   	return 0;
>   }