The selftest I provided can reproduce a panic: './test_progs -a cgroup_storage_update'
When we attach a program to cgroup and if prog->aux->cgroup_storage exists, which means the cgroup_storage map is used in the program, we will then allocate storage by bpf_cgroup_storages_alloc() and assign it to pl->storage.
At the end, pl->storage will be assigned to cgrp->bpf.effective[atype]->cgroup_storage by xxx_effective_progs().
But when we attach a program without the cgroup_storage map being used (prog->aux->cgroup_storage is empty), the cgroup_storage in struct bpf_prog_array_item is empty.
Then, if we use BPF_LINK_UPDATE to replace the old program with a new one that uses the cgroup_storage map, we miss the cgroup_storage being initialized.
This causes a panic when accessing storage in bpf_get_local_storage.
Jiayuan Chen (2): bpf: Create cgroup storage if needed when updating link selftests/bpf: Add link update test for cgroup_storage
kernel/bpf/cgroup.c | 24 +++++++--- .../selftests/bpf/prog_tests/cgroup_storage.c | 45 +++++++++++++++++++ .../selftests/bpf/progs/cgroup_storage.c | 6 +++ 3 files changed, 70 insertions(+), 5 deletions(-)
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); 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; }
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; }
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 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`.
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`.
…
This cause a painc when accessing stroage in bpf_get_local_storage.
Please avoid typos in such a change description.
How do you think about to append parentheses to function names?
See also: https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/...
Regards, Markus
Add link update test for cgroup_storage.
'./test_progs -a cgroup_storage_update' test_cgroup_storage_update:PASS:create cgroup 0 nsec setup_network:PASS:ip netns add cgroup_storage_ns 0 nsec setup_network:PASS:open netns 0 nsec setup_network:PASS:ip link set lo up 0 nsec test_cgroup_storage_update:PASS:setup network 0 nsec test_cgroup_storage_update:PASS:load program 0 nsec test_cgroup_storage_update:PASS:attach no map program 0 nsec test_cgroup_storage_update:PASS:bpf_link_update 0 nsec test_cgroup_storage_update:PASS:first ping 0 nsec test_cgroup_storage_update:PASS:second ping 0 nsec test_cgroup_storage_update:PASS:third ping 0 nsec 61 cgroup_storage_update:OK Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
Signed-off-by: Jiayuan Chen jiayuan.chen@linux.dev --- .../selftests/bpf/prog_tests/cgroup_storage.c | 45 +++++++++++++++++++ .../selftests/bpf/progs/cgroup_storage.c | 6 +++ 2 files changed, 51 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c b/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c index cf395715ced4..8478b08aa62a 100644 --- a/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c @@ -94,3 +94,48 @@ void test_cgroup_storage(void) close(cgroup_fd); cleanup_cgroup_environment(); } + +void test_cgroup_storage_update(void) +{ + struct cgroup_storage *skel; + struct nstoken *ns = NULL; + int cgroup_fd; + int err; + + cgroup_fd = cgroup_setup_and_join(TEST_CGROUP); + if (!ASSERT_OK_FD(cgroup_fd, "create cgroup")) + return; + + if (!ASSERT_OK(setup_network(&ns), "setup network")) + goto cleanup_cgroup; + + skel = cgroup_storage__open_and_load(); + if (!ASSERT_OK_PTR(skel, "load program")) + goto cleanup_network; + + skel->links.bpf_prog_no_map = + bpf_program__attach_cgroup(skel->progs.bpf_prog_no_map, + cgroup_fd); + if (!ASSERT_OK_PTR(skel->links.bpf_prog_no_map, "attach no map prog")) + goto cleanup_progs; + + err = bpf_link_update(bpf_link__fd(skel->links.bpf_prog_no_map), + bpf_program__fd(skel->progs.bpf_prog), NULL); + if (!ASSERT_OK(err, "bpf_link_update")) + goto cleanup_progs; + + err = SYS_NOFAIL(PING_CMD); + ASSERT_OK(err, "first ping"); + err = SYS_NOFAIL(PING_CMD); + ASSERT_NEQ(err, 0, "second ping"); + err = SYS_NOFAIL(PING_CMD); + ASSERT_OK(err, "third ping"); + +cleanup_progs: + cgroup_storage__destroy(skel); +cleanup_network: + cleanup_network(ns); +cleanup_cgroup: + close(cgroup_fd); + cleanup_cgroup_environment(); +} diff --git a/tools/testing/selftests/bpf/progs/cgroup_storage.c b/tools/testing/selftests/bpf/progs/cgroup_storage.c index db1e4d2d3281..33a6013ca806 100644 --- a/tools/testing/selftests/bpf/progs/cgroup_storage.c +++ b/tools/testing/selftests/bpf/progs/cgroup_storage.c @@ -21,4 +21,10 @@ int bpf_prog(struct __sk_buff *skb) return (*counter & 1); }
+SEC("cgroup_skb/egress") +int bpf_prog_no_map(struct __sk_buff *skb) +{ + return 1; +} + char _license[] SEC("license") = "GPL";
linux-kselftest-mirror@lists.linaro.org
-
Jiayuan Chen -
Markus Elfring -
Martin KaFai Lau