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