Re: [PATCH v2 bpf-next 4/4] selftests/bpf: Tweak cgroup kfunc test.

[Stanislav Fomichev <sdf@google.com>]

On 02/22, 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>
> ---
>   tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h  | 2 +-
>   tools/testing/selftests/bpf/progs/cgrp_kfunc_failure.c | 2 +-
>   tools/testing/selftests/bpf/progs/cgrp_kfunc_success.c | 7 ++++++-
>   3 files changed, 8 insertions(+), 3 deletions(-)

> diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h  
> b/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
> index 50d8660ffa26..eb5bf3125816 100644
> --- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
> +++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
> @@ -10,7 +10,7 @@
>   #include <bpf/bpf_tracing.h>

>   struct __cgrps_kfunc_map_value {
> -	struct cgroup __kptr * cgrp;
> +	struct cgroup __kptr_rcu * cgrp;
>   };

>   struct hash_map {
> 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 0c23ea32df9f..37ed73186fba 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)
> +		bpf_cgroup_release(cg);

I went through the series, it all makes sense, I'm assuming Kumar
will have another look eventually? (since he did for v1).

One question here, should we have something like the following?

if (cg) {
	bpf_cgroup_release(cg);
} else {
	err = 4;
	return 0;
}

Or are we just making sure here that the verifier is not complaining
about bpf_cgroup_ancestor(v->cgrp) and don't really care whether
bpf_cgroup_ancestor returns something useful or not?

> +
>   	kptr = bpf_kptr_xchg(&v->cgrp, NULL);
>   	if (!kptr) {
>   		err = 3;
> --
> 2.30.2