On Mon, Feb 27, 2023 at 08:01:21PM -0800, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@xxxxxxxxxx>
>
> Adjust cgroup kfunc test to dereference RCU protected cgroup pointer
> as PTR_TRUSTED and pass into KF_TRUSTED_ARGS kfunc.
>
> Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
This LGTM, but I noticed that another cgrp test was failing with this
patch set:
[root@archbig bpf]# ./test_progs -t cgrp_local_storage/recursion
test_cgrp_local_storage:PASS:join_cgroup /cgrp_local_storage 0 nsec
libbpf: prog 'on_lookup': BPF program load failed: Permission denied
libbpf: prog 'on_lookup': -- BEGIN PROG LOAD LOG --
reg type unsupported for arg#0 function on_lookup#16
0: R1=ctx(off=0,imm=0) R10=fp0
; struct task_struct *task = bpf_get_current_task_btf();
0: (85) call bpf_get_current_task_btf#158 ; R0_w=trusted_ptr_task_struct(off=0,imm=0)
1: (bf) r6 = r0 ; R0_w=trusted_ptr_task_struct(off=0,imm=0) R6_w=trusted_ptr_task_struct(off=0,imm=0)
; bpf_cgrp_storage_delete(&map_a, task->cgroups->dfl_cgrp);
2: (79) r1 = *(u64 *)(r6 +2296) ; R1_w=rcu_ptr_or_null_css_set(off=0,imm=0) R6_w=trusted_ptr_task_struct(off=0,imm=0)
; bpf_cgrp_storage_delete(&map_a, task->cgroups->dfl_cgrp);
3: (79) r2 = *(u64 *)(r1 +120)
R1 invalid mem access 'rcu_ptr_or_null_'
processed 4 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
-- END PROG LOAD LOG --
libbpf: prog 'on_lookup': failed to load: -13
libbpf: failed to load object 'cgrp_ls_recursion'
libbpf: failed to load BPF skeleton 'cgrp_ls_recursion': -13
test_recursion:FAIL:skel_open_and_load unexpected error: -13
#43/3 cgrp_local_storage/recursion:FAIL
#43 cgrp_local_storage:FAIL
All error logs:
test_cgrp_local_storage:PASS:join_cgroup /cgrp_local_storage 0 nsec
libbpf: prog 'on_lookup': BPF program load failed: Permission denied
libbpf: prog 'on_lookup': -- BEGIN PROG LOAD LOG --
reg type unsupported for arg#0 function on_lookup#16
0: R1=ctx(off=0,imm=0) R10=fp0
; struct task_struct *task = bpf_get_current_task_btf();
0: (85) call bpf_get_current_task_btf#158 ; R0_w=trusted_ptr_task_struct(off=0,imm=0)
1: (bf) r6 = r0 ; R0_w=trusted_ptr_task_struct(off=0,imm=0) R6_w=trusted_ptr_task_struct(off=0,imm=0)
; bpf_cgrp_storage_delete(&map_a, task->cgroups->dfl_cgrp);
2: (79) r1 = *(u64 *)(r6 +2296) ; R1_w=rcu_ptr_or_null_css_set(off=0,imm=0) R6_w=trusted_ptr_task_struct(off=0,imm=0)
; bpf_cgrp_storage_delete(&map_a, task->cgroups->dfl_cgrp);
3: (79) r2 = *(u64 *)(r1 +120)
R1 invalid mem access 'rcu_ptr_or_null_'
processed 4 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
-- END PROG LOAD LOG --
libbpf: prog 'on_lookup': failed to load: -13
libbpf: failed to load object 'cgrp_ls_recursion'
libbpf: failed to load BPF skeleton 'cgrp_ls_recursion': -13
test_recursion:FAIL:skel_open_and_load unexpected error: -13
#43/3 cgrp_local_storage/recursion:FAIL
#43 cgrp_local_storage:FAIL
Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
[root@archbig bpf]#
The ptr type looks correct, so I assumed that the arg type for the proto
needed to be updated to expect NULL. This doesn't seem to fix it though:
diff --git a/kernel/bpf/bpf_cgrp_storage.c b/kernel/bpf/bpf_cgrp_storage.c
index 6cdf6d9ed91d..9d5d47c8e820 100644
--- a/kernel/bpf/bpf_cgrp_storage.c
+++ b/kernel/bpf/bpf_cgrp_storage.c
@@ -241,6 +241,6 @@ const struct bpf_func_proto bpf_cgrp_storage_delete_proto = {
.gpl_only = false,
.ret_type = RET_INTEGER,
.arg1_type = ARG_CONST_MAP_PTR,
- .arg2_type = ARG_PTR_TO_BTF_ID,
+ .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
.arg2_btf_id = &bpf_cgroup_btf_id[0],
};
> ---
> tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c | 2 +-
> tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c | 7 ++++++-
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c b/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
> index 4ad7fe24966d..d5a53b5e708f 100644
> --- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
> +++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c
> @@ -205,7 +205,7 @@ int BPF_PROG(cgrp_kfunc_get_unreleased, struct cgroup *cgrp, const char *path)
> }
>
> SEC("tp_btf/cgroup_mkdir")
> -__failure __msg("arg#0 is untrusted_ptr_or_null_ expected ptr_ or socket")
> +__failure __msg("bpf_cgroup_release expects refcounted")
> int BPF_PROG(cgrp_kfunc_release_untrusted, struct cgroup *cgrp, const char *path)
> {
> struct __cgrps_kfunc_map_value *v;
> diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c b/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c
> index 42e13aebdd62..85becaa8573b 100644
> --- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c
> +++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c
> @@ -61,7 +61,7 @@ int BPF_PROG(test_cgrp_acquire_leave_in_map, struct cgroup *cgrp, const char *pa
> SEC("tp_btf/cgroup_mkdir")
> int BPF_PROG(test_cgrp_xchg_release, struct cgroup *cgrp, const char *path)
> {
> - struct cgroup *kptr;
> + struct cgroup *kptr, *cg;
> struct __cgrps_kfunc_map_value *v;
> long status;
>
> @@ -80,6 +80,11 @@ int BPF_PROG(test_cgrp_xchg_release, struct cgroup *cgrp, const char *path)
> return 0;
> }
>
> + kptr = v->cgrp;
> + cg = bpf_cgroup_ancestor(kptr, 1);
> + if (cg) /* verifier only check */
> + bpf_cgroup_release(cg);
> +
> kptr = bpf_kptr_xchg(&v->cgrp, NULL);
> if (!kptr) {
> err = 3;
> --
> 2.30.2
>
