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`.