Re: [PATCH v3 bpf-next 5/5] selftests/bpf: Tweak cgroup kfunc test.

[David Vernet <void@manifault.com>]

On Mon, Feb 27, 2023 at 08:01:21PM -0800, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> 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@kernel.org>

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
>