netdev - Re: [PATCH bpf-next] selftests/bpf: Check trampoline execution in d

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, 11 Sep 2020 15:04:21 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>
Subject: Re: [PATCH bpf-next] selftests/bpf: Check trampoline execution in
 d_path test

On Thu, Sep 10, 2020 at 03:22:10PM -0700, Andrii Nakryiko wrote:
> On Thu, Sep 10, 2020 at 5:25 AM Jiri Olsa <jolsa@...nel.org> wrote:
> >
> > Some kernels builds might inline vfs_getattr call within
> > fstat syscall code path, so fentry/vfs_getattr trampoline
> > is not called.
> >
> > I'm not sure how to handle this in some generic way other
> > than use some other function, but that might get inlined at
> > some point as well.
> >
> > Adding flags that indicate trampolines were called and failing
> > the test if neither of them got called.
> >
> >   $ sudo ./test_progs -t d_path
> >   test_d_path:PASS:setup 0 nsec
> >   ...
> >   trigger_fstat_events:PASS:trigger 0 nsec
> >   test_d_path:FAIL:124 trampolines not called
> >   #22 d_path:FAIL
> >   Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
> >
> > If only one trampoline is called, it's still enough to test
> > the helper, so only warn about missing trampoline call and
> > continue in test.
> >
> >   $ sudo ./test_progs -t d_path -v
> >   test_d_path:PASS:setup 0 nsec
> >   ...
> >   trigger_fstat_events:PASS:trigger 0 nsec
> >   fentry/vfs_getattr not called
> >   #22 d_path:OK
> >   Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> >
> > Signed-off-by: Jiri Olsa <jolsa@...hat.com>
> > ---
> 
> Acked-by: Andrii Nakryiko <andriin@...com>
> 
> >  .../testing/selftests/bpf/prog_tests/d_path.c | 25 +++++++++++++++----
> >  .../testing/selftests/bpf/progs/test_d_path.c |  7 ++++++
> >  2 files changed, 27 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > index fc12e0d445ff..ec15f7d1dd0a 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/d_path.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > @@ -120,26 +120,41 @@ void test_d_path(void)
> >         if (err < 0)
> >                 goto cleanup;
> >
> > +       if (!bss->called_stat && !bss->called_close) {
> > +               PRINT_FAIL("trampolines not called\n");
> > +               goto cleanup;
> > +       }
> > +
> > +       if (!bss->called_stat) {
> > +               fprintf(stdout, "fentry/vfs_getattr not called\n");
> > +               goto cleanup;
> > +       }
> > +
> > +       if (!bss->called_close) {
> > +               fprintf(stdout, "fentry/filp_close not called\n");
> > +               goto cleanup;
> > +       }
> 
> not sure why you didn't go with `if (CHECK(!bss->called_close, ...`
> for these checks, would even save you some typing.

ok

> 
> > +
> >         for (int i = 0; i < MAX_FILES; i++) {
> > -               CHECK(strncmp(src.paths[i], bss->paths_stat[i], MAX_PATH_LEN),
> > +               CHECK(bss->called_stat && strncmp(src.paths[i], bss->paths_stat[i], MAX_PATH_LEN),
> >                       "check",
> >                       "failed to get stat path[%d]: %s vs %s\n",
> >                       i, src.paths[i], bss->paths_stat[i]);
> > -               CHECK(strncmp(src.paths[i], bss->paths_close[i], MAX_PATH_LEN),
> > +               CHECK(bss->called_close && strncmp(src.paths[i], bss->paths_close[i], MAX_PATH_LEN),
> >                       "check",
> >                       "failed to get close path[%d]: %s vs %s\n",
> >                       i, src.paths[i], bss->paths_close[i]);
> >                 /* The d_path helper returns size plus NUL char, hence + 1 */
> > -               CHECK(bss->rets_stat[i] != strlen(bss->paths_stat[i]) + 1,
> > +               CHECK(bss->called_stat && bss->rets_stat[i] != strlen(bss->paths_stat[i]) + 1,
> >                       "check",
> >                       "failed to match stat return [%d]: %d vs %zd [%s]\n",
> >                       i, bss->rets_stat[i], strlen(bss->paths_stat[i]) + 1,
> >                       bss->paths_stat[i]);
> > -               CHECK(bss->rets_close[i] != strlen(bss->paths_stat[i]) + 1,
> > +               CHECK(bss->called_close && bss->rets_close[i] != strlen(bss->paths_close[i]) + 1,
> >                       "check",
> >                       "failed to match stat return [%d]: %d vs %zd [%s]\n",
> >                       i, bss->rets_close[i], strlen(bss->paths_close[i]) + 1,
> > -                     bss->paths_stat[i]);
> > +                     bss->paths_close[i]);
> 
> 
> those `bss->called_xxx` guard conditions are a bit lost on reading, if
> you reordered CHECKs, you could be more explicit:
> 
> if (bss->called_stat) {
>     CHECK(...);
>     CHECK(...);
> }
> if (bss->called_close) { ... }

ok, will change

thanks,
jirka