| Message ID | 20231005083953.1281-2-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,1/2] bpf: Fix missed rcu read lock in bpf_task_under_cgroup() | expand |
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>
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.
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";
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(-)