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/22/25 7:21 PM, Jiayuan Chen wrote:
> 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

fwiw, I don't think this details matter here, but it is not only for non-link 
path. cgroup_bpf_link_attach also calls 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);

The pl, that the __cgroup_bpf_replace is xchg()-ing its pl->link->link.prog with 
new_prog, still has a NULL in pl->storage. When another "different" bpf prog is 
added and attached to the same cgroup "later", compute_effective_progs will be 
called and it will have the same bug, no?

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