* [PATCH bpf-next 1/2] bpf: Fix missed rcu read lock in bpf_task_under_cgroup()
@ 2023-10-05 8:39 Yafang Shao
2023-10-05 8:39 ` [PATCH bpf-next 2/2] selftests/bpf: Add selftest for sleepable bpf_task_under_cgroup() Yafang Shao
0 siblings, 1 reply; 4+ messages in thread
From: Yafang Shao @ 2023-10-05 8:39 UTC (permalink / raw)
To: ast, daniel, john.fastabend, andrii, martin.lau, song,
yonghong.song, kpsingh, sdf, haoluo, jolsa
Cc: bpf, Yafang Shao, Feng Zhou
When employed within a sleepable program not under RCU protection, the use
of 'bpf_task_under_cgroup()' may trigger a warning in the kernel log,
particularly when CONFIG_PROVE_RCU is enabled.
[ 1259.662354] =============================
[ 1259.662357] WARNING: suspicious RCU usage
[ 1259.662358] 6.5.0+ #33 Not tainted
[ 1259.662360] -----------------------------
[ 1259.662361] include/linux/cgroup.h:423 suspicious rcu_dereference_check() usage!
[ 1259.662364]
other info that might help us debug this:
[ 1259.662366]
rcu_scheduler_active = 2, debug_locks = 1
[ 1259.662368] 1 lock held by trace/72954:
[ 1259.662369] #0: ffffffffb5e3eda0 (rcu_read_lock_trace){....}-{0:0}, at: __bpf_prog_enter_sleepable+0x0/0xb0
[ 1259.662383]
stack backtrace:
[ 1259.662385] CPU: 50 PID: 72954 Comm: trace Kdump: loaded Not tainted 6.5.0+ #33
[ 1259.662391] Call Trace:
[ 1259.662393] <TASK>
[ 1259.662395] dump_stack_lvl+0x6e/0x90
[ 1259.662401] dump_stack+0x10/0x20
[ 1259.662404] lockdep_rcu_suspicious+0x163/0x1b0
[ 1259.662412] task_css_set.part.0+0x23/0x30
[ 1259.662417] bpf_task_under_cgroup+0xe7/0xf0
[ 1259.662422] bpf_prog_7fffba481a3bcf88_lsm_run+0x5c/0x93
[ 1259.662431] bpf_trampoline_6442505574+0x60/0x1000
[ 1259.662439] bpf_lsm_bpf+0x5/0x20
[ 1259.662443] ? security_bpf+0x32/0x50
[ 1259.662452] __sys_bpf+0xe6/0xdd0
[ 1259.662463] __x64_sys_bpf+0x1a/0x30
[ 1259.662467] do_syscall_64+0x38/0x90
[ 1259.662472] entry_SYSCALL_64_after_hwframe+0x6e/0xd8
[ 1259.662479] RIP: 0033:0x7f487baf8e29
...
[ 1259.662504] </TASK>
This issue can be reproduced by executing a straightforward program, as
demonstrated below:
SEC("lsm.s/bpf")
int BPF_PROG(lsm_run, int cmd, union bpf_attr *attr, unsigned int size)
{
struct cgroup *cgrp = NULL;
struct task_struct *task;
int ret = 0;
if (cmd != BPF_LINK_CREATE)
return 0;
// The cgroup2 should be mounted first
cgrp = bpf_cgroup_from_id(1);
if (!cgrp)
goto out;
task = bpf_get_current_task_btf();
if (bpf_task_under_cgroup(task, cgrp))
ret = -1;
bpf_cgroup_release(cgrp);
out:
return ret;
}
After running the program, if you subsequently execute another BPF program,
you will encounter the warning. It's worth noting that
task_under_cgroup_hierarchy() is also utilized by
bpf_current_task_under_cgroup(). However, bpf_current_task_under_cgroup()
doesn't exhibit this issue because it cannot be used in sleepable BPF
programs.
Fixes: b5ad4cdc46c7 ("bpf: Add bpf_task_under_cgroup() kfunc")
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Feng Zhou <zhoufeng.zf@bytedance.com>
---
kernel/bpf/helpers.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index dd1c69e..bb521b1 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2212,7 +2212,12 @@ __bpf_kfunc struct cgroup *bpf_cgroup_from_id(u64 cgid)
__bpf_kfunc long bpf_task_under_cgroup(struct task_struct *task,
struct cgroup *ancestor)
{
- return task_under_cgroup_hierarchy(task, ancestor);
+ long ret;
+
+ rcu_read_lock();
+ ret = task_under_cgroup_hierarchy(task, ancestor);
+ rcu_read_unlock();
+ return ret;
}
#endif /* CONFIG_CGROUPS */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: Add selftest for sleepable bpf_task_under_cgroup()
2023-10-05 8:39 [PATCH bpf-next 1/2] bpf: Fix missed rcu read lock in bpf_task_under_cgroup() Yafang Shao
@ 2023-10-05 8:39 ` Yafang Shao
2023-10-05 17:13 ` Stanislav Fomichev
0 siblings, 1 reply; 4+ messages in thread
From: Yafang Shao @ 2023-10-05 8:39 UTC (permalink / raw)
To: ast, daniel, john.fastabend, andrii, martin.lau, song,
yonghong.song, kpsingh, sdf, haoluo, jolsa
Cc: bpf, Yafang Shao
The result as follows,
$ tools/testing/selftests/bpf/test_progs --name=task_under_cgroup
#237 task_under_cgroup:OK
Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
And no error messages in dmesg.
Without the prev patch, there will be RCU warnings in dmesg.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
.../selftests/bpf/prog_tests/task_under_cgroup.c | 8 +++++--
.../selftests/bpf/progs/test_task_under_cgroup.c | 28 +++++++++++++++++++++-
2 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c b/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
index 4224727..d1a5a5c 100644
--- a/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
+++ b/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
@@ -30,8 +30,12 @@ void test_task_under_cgroup(void)
if (!ASSERT_OK(ret, "test_task_under_cgroup__load"))
goto cleanup;
- ret = test_task_under_cgroup__attach(skel);
- if (!ASSERT_OK(ret, "test_task_under_cgroup__attach"))
+ skel->links.lsm_run = bpf_program__attach_lsm(skel->progs.lsm_run);
+ if (!ASSERT_OK_PTR(skel->links.lsm_run, "attach_lsm"))
+ goto cleanup;
+
+ skel->links.tp_btf_run = bpf_program__attach_trace(skel->progs.tp_btf_run);
+ if (!ASSERT_OK_PTR(skel->links.tp_btf_run, "attach_tp_btf"))
goto cleanup;
pid = fork();
diff --git a/tools/testing/selftests/bpf/progs/test_task_under_cgroup.c b/tools/testing/selftests/bpf/progs/test_task_under_cgroup.c
index 56cdc0a..7e750309 100644
--- a/tools/testing/selftests/bpf/progs/test_task_under_cgroup.c
+++ b/tools/testing/selftests/bpf/progs/test_task_under_cgroup.c
@@ -18,7 +18,7 @@
int remote_pid;
SEC("tp_btf/task_newtask")
-int BPF_PROG(handle__task_newtask, struct task_struct *task, u64 clone_flags)
+int BPF_PROG(tp_btf_run, struct task_struct *task, u64 clone_flags)
{
struct cgroup *cgrp = NULL;
struct task_struct *acquired;
@@ -48,4 +48,30 @@ int BPF_PROG(handle__task_newtask, struct task_struct *task, u64 clone_flags)
return 0;
}
+SEC("lsm.s/bpf")
+int BPF_PROG(lsm_run, int cmd, union bpf_attr *attr, unsigned int size)
+{
+ struct cgroup *cgrp = NULL;
+ struct task_struct *task;
+ int ret = 0;
+
+ task = bpf_get_current_task_btf();
+ if (local_pid != task->pid)
+ return 0;
+
+ if (cmd != BPF_LINK_CREATE)
+ return 0;
+
+ /* 1 is the root cgroup */
+ cgrp = bpf_cgroup_from_id(1);
+ if (!cgrp)
+ goto out;
+ if (!bpf_task_under_cgroup(task, cgrp))
+ ret = -1;
+ bpf_cgroup_release(cgrp);
+
+out:
+ return ret;
+}
+
char _license[] SEC("license") = "GPL";
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Add selftest for sleepable bpf_task_under_cgroup()
2023-10-05 8:39 ` [PATCH bpf-next 2/2] selftests/bpf: Add selftest for sleepable bpf_task_under_cgroup() Yafang Shao
@ 2023-10-05 17:13 ` Stanislav Fomichev
2023-10-06 2:17 ` Yafang Shao
0 siblings, 1 reply; 4+ messages in thread
From: Stanislav Fomichev @ 2023-10-05 17:13 UTC (permalink / raw)
To: Yafang Shao
Cc: ast, daniel, john.fastabend, andrii, martin.lau, song,
yonghong.song, kpsingh, haoluo, jolsa, bpf
On 10/05, Yafang Shao wrote:
> The result as follows,
>
> $ tools/testing/selftests/bpf/test_progs --name=task_under_cgroup
> #237 task_under_cgroup:OK
> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
>
> And no error messages in dmesg.
>
> Without the prev patch, there will be RCU warnings in dmesg.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
> .../selftests/bpf/prog_tests/task_under_cgroup.c | 8 +++++--
> .../selftests/bpf/progs/test_task_under_cgroup.c | 28 +++++++++++++++++++++-
> 2 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c b/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
> index 4224727..d1a5a5c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
> +++ b/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
> @@ -30,8 +30,12 @@ void test_task_under_cgroup(void)
> if (!ASSERT_OK(ret, "test_task_under_cgroup__load"))
> goto cleanup;
>
> - ret = test_task_under_cgroup__attach(skel);
> - if (!ASSERT_OK(ret, "test_task_under_cgroup__attach"))
> + skel->links.lsm_run = bpf_program__attach_lsm(skel->progs.lsm_run);
> + if (!ASSERT_OK_PTR(skel->links.lsm_run, "attach_lsm"))
> + goto cleanup;
> +
So we rely on the second attach here to trigger the program above?
Maybe add a comment? Otherwise we might risk loosing this dependency
after some refactoring...
Other than that, both patches look good to me, feel free to use for both
if/when you resend:
Acked-by: Stanislav Fomichev <sdf@google.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Add selftest for sleepable bpf_task_under_cgroup()
2023-10-05 17:13 ` Stanislav Fomichev
@ 2023-10-06 2:17 ` Yafang Shao
0 siblings, 0 replies; 4+ messages in thread
From: Yafang Shao @ 2023-10-06 2:17 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: ast, daniel, john.fastabend, andrii, martin.lau, song,
yonghong.song, kpsingh, haoluo, jolsa, bpf
On Fri, Oct 6, 2023 at 1:13 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> On 10/05, Yafang Shao wrote:
> > The result as follows,
> >
> > $ tools/testing/selftests/bpf/test_progs --name=task_under_cgroup
> > #237 task_under_cgroup:OK
> > Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> >
> > And no error messages in dmesg.
> >
> > Without the prev patch, there will be RCU warnings in dmesg.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> > .../selftests/bpf/prog_tests/task_under_cgroup.c | 8 +++++--
> > .../selftests/bpf/progs/test_task_under_cgroup.c | 28 +++++++++++++++++++++-
> > 2 files changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c b/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
> > index 4224727..d1a5a5c 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
> > @@ -30,8 +30,12 @@ void test_task_under_cgroup(void)
> > if (!ASSERT_OK(ret, "test_task_under_cgroup__load"))
> > goto cleanup;
> >
> > - ret = test_task_under_cgroup__attach(skel);
> > - if (!ASSERT_OK(ret, "test_task_under_cgroup__attach"))
> > + skel->links.lsm_run = bpf_program__attach_lsm(skel->progs.lsm_run);
> > + if (!ASSERT_OK_PTR(skel->links.lsm_run, "attach_lsm"))
> > + goto cleanup;
> > +
>
> So we rely on the second attach here to trigger the program above?
Right.
> Maybe add a comment? Otherwise we might risk loosing this dependency
> after some refactoring...
Sure. will add a comment.
>
> Other than that, both patches look good to me, feel free to use for both
> if/when you resend:
>
> Acked-by: Stanislav Fomichev <sdf@google.com>
Thanks for your review.
--
Regards
Yafang
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-10-06 2:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-05 8:39 [PATCH bpf-next 1/2] bpf: Fix missed rcu read lock in bpf_task_under_cgroup() Yafang Shao
2023-10-05 8:39 ` [PATCH bpf-next 2/2] selftests/bpf: Add selftest for sleepable bpf_task_under_cgroup() Yafang Shao
2023-10-05 17:13 ` Stanislav Fomichev
2023-10-06 2:17 ` Yafang Shao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).