Message-ID: <CAEf4BzbVT+DmjPXLrcFG0ZFMCw0P_cb0W9abiaygfBAFu+nh7Q@mail.gmail.com>
Date:   Thu, 10 Sep 2020 15:22:10 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <jolsa@...nel.org>
Cc:     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 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.

> +
>         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) { ... }

>         }
>
>  cleanup:
> diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c
> index 61f007855649..9e7223b4a555 100644
> --- a/tools/testing/selftests/bpf/progs/test_d_path.c
> +++ b/tools/testing/selftests/bpf/progs/test_d_path.c
> @@ -15,6 +15,9 @@ char paths_close[MAX_FILES][MAX_PATH_LEN] = {};
>  int rets_stat[MAX_FILES] = {};
>  int rets_close[MAX_FILES] = {};
>
> +int called_stat = 0;
> +int called_close = 0;
> +
>  SEC("fentry/vfs_getattr")
>  int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
>              __u32 request_mask, unsigned int query_flags)
> @@ -23,6 +26,8 @@ int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
>         __u32 cnt = cnt_stat;
>         int ret;
>
> +       called_stat = 1;
> +
>         if (pid != my_pid)
>                 return 0;
>
> @@ -42,6 +47,8 @@ int BPF_PROG(prog_close, struct file *file, void *id)
>         __u32 cnt = cnt_close;
>         int ret;
>
> +       called_close = 1;
> +
>         if (pid != my_pid)
>                 return 0;
>
> --
> 2.26.2
>

Powered by blists - more mailing lists