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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
>
> Acked-by: Andrii Nakryiko <andriin@xxxxxx>
>
> > .../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
