* [PATCH bpf-next v1 0/2] bpf: Fix panic in bpf_get_local_storage
@ 2025-04-17 4:40 Jiayuan Chen
2025-04-17 4:40 ` [PATCH bpf-next v1 1/2] bpf: Create cgroup storage if needed when updating link Jiayuan Chen
2025-04-17 4:40 ` [PATCH bpf-next v1 2/2] selftests/bpf: Add link update test for cgroup_storage Jiayuan Chen
0 siblings, 2 replies; 7+ messages in thread
From: Jiayuan Chen @ 2025-04-17 4:40 UTC (permalink / raw)
To: andrii, martin.lau, bpf
Cc: alexis.lothore, mrpre, Jiayuan Chen, Alexei Starovoitov,
Daniel Borkmann, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, Alan Maguire, linux-kernel,
linux-kselftest
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(-)
--
2.47.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf-next v1 1/2] bpf: Create cgroup storage if needed when updating link
2025-04-17 4:40 [PATCH bpf-next v1 0/2] bpf: Fix panic in bpf_get_local_storage Jiayuan Chen
@ 2025-04-17 4:40 ` Jiayuan Chen
2025-04-23 0:13 ` Martin KaFai Lau
2025-04-23 8:45 ` [PATCH bpf-next " Markus Elfring
2025-04-17 4:40 ` [PATCH bpf-next v1 2/2] selftests/bpf: Add link update test for cgroup_storage Jiayuan Chen
1 sibling, 2 replies; 7+ messages in thread
From: Jiayuan Chen @ 2025-04-17 4:40 UTC (permalink / raw)
To: andrii, martin.lau, bpf
Cc: alexis.lothore, mrpre, Jiayuan Chen, syzbot+e6e8f6618a2d4b35e4e0,
Alexei Starovoitov, Daniel Borkmann, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, Alan Maguire,
linux-kernel, linux-kselftest
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;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf-next v1 2/2] selftests/bpf: Add link update test for cgroup_storage
2025-04-17 4:40 [PATCH bpf-next v1 0/2] bpf: Fix panic in bpf_get_local_storage Jiayuan Chen
2025-04-17 4:40 ` [PATCH bpf-next v1 1/2] bpf: Create cgroup storage if needed when updating link Jiayuan Chen
@ 2025-04-17 4:40 ` Jiayuan Chen
1 sibling, 0 replies; 7+ messages in thread
From: Jiayuan Chen @ 2025-04-17 4:40 UTC (permalink / raw)
To: andrii, martin.lau, bpf
Cc: alexis.lothore, mrpre, Jiayuan Chen, Alexei Starovoitov,
Daniel Borkmann, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, Alan Maguire, linux-kernel,
linux-kselftest
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";
--
2.47.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v1 1/2] bpf: Create cgroup storage if needed when updating link
2025-04-17 4:40 ` [PATCH bpf-next v1 1/2] bpf: Create cgroup storage if needed when updating link Jiayuan Chen
@ 2025-04-23 0:13 ` Martin KaFai Lau
2025-04-23 2:21 ` Jiayuan Chen
2025-04-23 8:45 ` [PATCH bpf-next " Markus Elfring
1 sibling, 1 reply; 7+ messages in thread
From: Martin KaFai Lau @ 2025-04-23 0:13 UTC (permalink / raw)
To: Jiayuan Chen
Cc: andrii, alexis.lothore, mrpre, syzbot+e6e8f6618a2d4b35e4e0,
Alexei Starovoitov, Daniel Borkmann, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, Alan Maguire,
linux-kernel, linux-kselftest, bpf
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;
> }
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v1 1/2] bpf: Create cgroup storage if needed when updating link
2025-04-23 0:13 ` Martin KaFai Lau
@ 2025-04-23 2:21 ` Jiayuan Chen
2025-04-23 21:07 ` Martin KaFai Lau
0 siblings, 1 reply; 7+ messages in thread
From: Jiayuan Chen @ 2025-04-23 2:21 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: andrii, alexis.lothore, mrpre, syzbot+e6e8f6618a2d4b35e4e0,
Alexei Starovoitov, Daniel Borkmann, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, Alan Maguire,
linux-kernel, linux-kselftest, bpf
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`.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Create cgroup storage if needed when updating link
2025-04-17 4:40 ` [PATCH bpf-next v1 1/2] bpf: Create cgroup storage if needed when updating link Jiayuan Chen
2025-04-23 0:13 ` Martin KaFai Lau
@ 2025-04-23 8:45 ` Markus Elfring
1 sibling, 0 replies; 7+ messages in thread
From: Markus Elfring @ 2025-04-23 8:45 UTC (permalink / raw)
To: Jiayuan Chen, bpf, linux-kselftest
Cc: Jiayuan Chen, syzbot+e6e8f6618a2d4b35e4e0, yonghong.song, LKML,
Alan Maguire, Alexei Starovoitov, Alexis Lothoré,
Andrii Nakryiko, Daniel Borkmann, Eduard Zingerman, Hao Luo,
Jiri Olsa, John Fastabend, KP Singh, Martin KaFai Lau,
Mykola Lysenko, Shuah Khan, Song Liu, Stanislav Fomichev
…
> 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/Documentation/process/submitting-patches.rst?h=v6.15-rc3#n94
Regards,
Markus
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v1 1/2] bpf: Create cgroup storage if needed when updating link
2025-04-23 2:21 ` Jiayuan Chen
@ 2025-04-23 21:07 ` Martin KaFai Lau
0 siblings, 0 replies; 7+ messages in thread
From: Martin KaFai Lau @ 2025-04-23 21:07 UTC (permalink / raw)
To: Jiayuan Chen
Cc: andrii, alexis.lothore, mrpre, syzbot+e6e8f6618a2d4b35e4e0,
Alexei Starovoitov, Daniel Borkmann, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan, Alan Maguire,
linux-kernel, linux-kselftest, bpf
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`.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-04-23 21:08 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-17 4:40 [PATCH bpf-next v1 0/2] bpf: Fix panic in bpf_get_local_storage Jiayuan Chen
2025-04-17 4:40 ` [PATCH bpf-next v1 1/2] bpf: Create cgroup storage if needed when updating link Jiayuan Chen
2025-04-23 0:13 ` Martin KaFai Lau
2025-04-23 2:21 ` Jiayuan Chen
2025-04-23 21:07 ` Martin KaFai Lau
2025-04-23 8:45 ` [PATCH bpf-next " Markus Elfring
2025-04-17 4:40 ` [PATCH bpf-next v1 2/2] selftests/bpf: Add link update test for cgroup_storage Jiayuan Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox