Re: [PATCH bpf-next] bpf/selftests: Test bpf_d_path on rdonly_mem.

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Dec 21, 2021 at 12:16 PM Hao Luo <haoluo@google.com> wrote:
>
> On Mon, Dec 20, 2021 at 8:28 PM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 12/20/21 12:12 PM, Hao Luo wrote:
> > > The second parameter of bpf_d_path() can only accept writable
> > > memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not
> > > be passed into bpf_d_path for modification. This patch adds
> > > a selftest to verify this behavior.
> > >
> > > Signed-off-by: Hao Luo <haoluo@google.com>
> > > ---
> > >   .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++-
> > >   .../bpf/progs/test_d_path_check_rdonly_mem.c  | 30 +++++++++++++++++++
> > >   2 files changed, 51 insertions(+), 1 deletion(-)
> > >   create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > > index 0a577a248d34..f8d8c5a5dfba 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/d_path.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > > @@ -9,6 +9,7 @@
> > >   #define MAX_FILES           7
> > >
> > >   #include "test_d_path.skel.h"
> > > +#include "test_d_path_check_rdonly_mem.skel.h"
> > >
> > >   static int duration;
> > >
> > > @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid)
> > >       return ret;
> > >   }
> > >
> > > -void test_d_path(void)
> > > +static void test_d_path_basic(void)
> > >   {
> > >       struct test_d_path__bss *bss;
> > >       struct test_d_path *skel;
> > > @@ -155,3 +156,22 @@ void test_d_path(void)
> > >   cleanup:
> > >       test_d_path__destroy(skel);
> > >   }
> > > +
> > > +static void test_d_path_check_rdonly_mem(void)
> > > +{
> > > +     struct test_d_path_check_rdonly_mem *skel;
> > > +
> > > +     skel = test_d_path_check_rdonly_mem__open_and_load();
> > > +     ASSERT_ERR_PTR(skel, "unexpected load of a prog using d_path to write rdonly_mem\n");
> > > +
> > > +     test_d_path_check_rdonly_mem__destroy(skel);
> >
> > You shouldn't call test_d_path_check_rdonly_mem__destroy(skel) if skel
> > is an ERR_PTR. Maybe
> >         if (!ASSERT_ERR_PTR(...))
> >                 test_d_path_check_rdonly_mem__destroy(skel);
> >
>
> Ack. Will change that.

no need, __destroy() handles NULLs and ERR_PTR just fine, the way you
wrote it is totally correct (that's a deliberate nice feature of
libbpf's "destructor" APIs)

>
> I don't know if it's only me: I find it confusing when figuring out
> what ASSERT_ERR_PTR(ptr) returns. Is the returned value 'ptr'? or 'ptr
> != NULL'? or 'err != 0'? I used to think ASSERT-like function/macro
> returns nothing.
>

You haven't looked at many other selftests, I presume. All the
ASSERT_xxx() macros return true/false depending whether the assertion
holds or not. ASSERT_ERR_PTR() checks that ptr *is* erroneous (which
is NULL and ERR_PTR). If it's not, it returns false. So

if (!ASSERT_ERR_PTR(ptr, "short_descriptor"))
   /* do something if assertion failed */

is a common pattern.

Note also "short_descriptor", it's not supposed to be a long
descriptive sentences, it's sort of a "codename" of the particular
check. It's not illegal to use space-separated sentence, but better to
keep it short and identifier-like.

> I noticed that xxx__destroy has a check for NULL, so I put the destroy
> function unconditionally.
>
> > > +}
> > > +
> > > +void test_d_path(void)
> > > +{
> > > +     if (test__start_subtest("basic"))
> > > +             test_d_path_basic();
> > > +
> > > +     if (test__start_subtest("check_rdonly_mem"))
> > > +             test_d_path_check_rdonly_mem();
> > > +}
> > > diff --git a/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> > > new file mode 100644
> > > index 000000000000..c7a9655d5850
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> > > @@ -0,0 +1,30 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/* Copyright (c) 2021 Google */
> > > +
> > > +#include "vmlinux.h"
> > > +
> > > +#include "vmlinux.h"
> >
> > duplicated vmlinux.h.
> >
>
> Thanks. Will fix that.
>
> > > +#include <bpf/bpf_helpers.h>
> > > +#include <bpf/bpf_tracing.h>
> > > +
> > > +extern const int bpf_prog_active __ksym;
> > > +
> > > +SEC("fentry/security_inode_getattr")
> > > +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat,
> > > +          __u32 request_mask, unsigned int query_flags)
> > > +{
> > > +     char *active;
> >
> > int *active?
> > It may not matter since the program is rejected by the kernel but
> > with making it conforms to kernel definition we have one less thing
> > to worry about the verification.
> >
>
> Because bpf_d_path() accepts 'char *' instead of 'int *', I need to
> cast 'active' to 'char *' somewhere, otherwise the compiler will issue
> a warning. To combine with your comment, maybe the following:
>
> int *active;
> active = (int *)bpf_per_cpu_ptr(...);
> ...
> bpf_d_path(path, (char *)active, sizeof(int));
>

why not `void *`?

> > > +     __u32 cpu;
> > > +
> > > +     cpu = bpf_get_smp_processor_id();
> > > +     active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu);
> >
> > int *
> >
> > > +     if (active) {
> > > +             /* FAIL here! 'active' is a rdonly_mem. bpf helpers that
> >
> > 'active' points to readonly memory.
> >
>
> Ack.
>
> > > +              * update its arguments can not write into it.
> > > +              */
> > > +             bpf_d_path(path, active, sizeof(int));
> > > +     }
> > > +     return 0;
> > > +}
> > > +
> > > +char _license[] SEC("license") = "GPL";