diff mbox series

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

Message ID 20230223030717.58668-5-alexei.starovoitov@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf: Introduce kptr_rcu. | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-7 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-17
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 13 maintainers not CCed: linux-kselftest@vger.kernel.org john.fastabend@gmail.com sdf@google.com shuah@kernel.org jolsa@kernel.org song@kernel.org mykolal@fb.com martin.lau@linux.dev haoluo@google.com yhs@fb.com joannelkoong@gmail.com roberto.sassu@huawei.com kpsingh@kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 35 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 fail Logs for test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-19 fail Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32 on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 fail Logs for test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_progs_parallel on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for test_verifier on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-37 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for test_progs_parallel on s390x with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_maps on s390x with gcc

Commit Message

Alexei Starovoitov Feb. 23, 2023, 3:07 a.m. UTC 
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(-)

Comments

Stanislav Fomichev Feb. 23, 2023, 9:48 p.m. UTC | #1 
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
Alexei Starovoitov Feb. 24, 2023, 11:10 p.m. UTC | #2 
On Thu, Feb 23, 2023 at 1:48 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> 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?

It's the verifier only check.
See other bpf_cgroup_ancestor() related tests in the same file.
They check the run-time component quite well.
No need to duplicate.
diff mbox series

Patch

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);
+
 	kptr = bpf_kptr_xchg(&v->cgrp, NULL);
 	if (!kptr) {
 		err = 3;