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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 24 Feb 2023 15:10:52 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...nel.org>,
        David Vernet <void@...ifault.com>,
        Dave Marchevsky <davemarchevsky@...a.com>,
        Tejun Heo <tj@...nel.org>,
        Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 4/4] selftests/bpf: Tweak cgroup kfunc test.

On Thu, Feb 23, 2023 at 1:48 PM Stanislav Fomichev <sdf@...gle.com> wrote:
>
> On 02/22, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@...nel.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@...nel.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.