On Thu, Feb 23, 2023 at 1:48 PM Stanislav Fomichev [off-list ref] wrote:

On 02/22, Alexei Starovoitov wrote:

quoted

From: Alexei Starovoitov <ast@kernel.org>

quoted

Adjust cgroup kfunc test to dereference RCU protected cgroup pointer
as PTR_TRUSTED and pass into KF_TRUSTED_ARGS kfunc.

quoted

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(-)

quoted

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>

quoted

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

quoted

  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)
  }

quoted

  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;

quoted

@@ -80,6 +80,11 @@ int BPF_PROG(test_cgrp_xchg_release, struct cgroup

*cgrp, const char *path)
              return 0;
      }

[..]

quoted

+     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.