| Message ID | 20200910122224.1683258-1-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] selftests/bpf: Check trampoline execution in d_path test | expand |
On Thu, Sep 10, 2020 at 5:25 AM Jiri Olsa <jolsa@kernel.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@redhat.com> > --- Acked-by: Andrii Nakryiko <andriin@fb.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 >
On Thu, Sep 10, 2020 at 5:22 AM Jiri Olsa <jolsa@kernel.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. It's great that we had the test and it failed. Doing the test skipping will only hide the problem. Please don't do it here and in the future. Instead let's figure out the real solution. Assuming that vfs_getattr was added to btf_allowlist_d_path for a reason we have to make this introspection place reliable regardless of compiler inlining decisions. We can mark it as 'noinline', but that's undesirable. I suggest we remove it from the allowlist and replace it with security_inode_getattr. I think that is a better long term fix. While at it I would apply the same critical thinking to other functions in the allowlist. They might suffer the same issue. So s/vfs_truncate/security_path_truncate/ and so on? Things won't work when CONFIG_SECURITY is off, but that is a rare kernel config? Or add both security_* and vfs_* variants and switch tests to use security_* ? but it feels fragile to allow inline-able funcs in allowlist.
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@kernel.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@redhat.com> > > --- > > Acked-by: Andrii Nakryiko <andriin@fb.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
On Thu, Sep 10, 2020 at 05:46:21PM -0700, Alexei Starovoitov wrote: > On Thu, Sep 10, 2020 at 5:22 AM Jiri Olsa <jolsa@kernel.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. > > It's great that we had the test and it failed. > Doing the test skipping will only hide the problem. > Please don't do it here and in the future. > Instead let's figure out the real solution. > Assuming that vfs_getattr was added to btf_allowlist_d_path > for a reason we have to make this introspection place > reliable regardless of compiler inlining decisions. > We can mark it as 'noinline', but that's undesirable. > I suggest we remove it from the allowlist and replace it with > security_inode_getattr. > I think that is a better long term fix. in my case vfs_getattr got inlined in vfs_statx_fd and both of them are defined in fs/stat.c so the idea is that inlining will not happen if the function is defined in another object? or less likely..? we should be safe when it's called from module > While at it I would apply the same critical thinking to other > functions in the allowlist. They might suffer the same issue. > So s/vfs_truncate/security_path_truncate/ and so on? > Things won't work when CONFIG_SECURITY is off, but that is a rare kernel config? > Or add both security_* and vfs_* variants and switch tests to use security_* ? > but it feels fragile to allow inline-able funcs in allowlist. hm, what's the difference between vfs_getattr and security_inode_getattr in this regard? I'd expect compiler could inline it same way as for vfs_getattr jirka
On Fri, Sep 11, 2020 at 6:16 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Thu, Sep 10, 2020 at 05:46:21PM -0700, Alexei Starovoitov wrote: > > On Thu, Sep 10, 2020 at 5:22 AM Jiri Olsa <jolsa@kernel.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. > > > > It's great that we had the test and it failed. > > Doing the test skipping will only hide the problem. > > Please don't do it here and in the future. > > Instead let's figure out the real solution. > > Assuming that vfs_getattr was added to btf_allowlist_d_path > > for a reason we have to make this introspection place > > reliable regardless of compiler inlining decisions. > > We can mark it as 'noinline', but that's undesirable. > > I suggest we remove it from the allowlist and replace it with > > security_inode_getattr. > > I think that is a better long term fix. > > in my case vfs_getattr got inlined in vfs_statx_fd and both > of them are defined in fs/stat.c > > so the idea is that inlining will not happen if the function > is defined in another object? or less likely..? when it's in a different .o file. yes. Very few folks build LTO kernels, so I propose to cross that bridge when we get there. Eventually we can replace security_inode_getattr with bpf_lsm_inode_getattr or simply add noinline to security_inode_getattr. > we should be safe when it's called from module what do you mean? > > While at it I would apply the same critical thinking to other > > functions in the allowlist. They might suffer the same issue. > > So s/vfs_truncate/security_path_truncate/ and so on? > > Things won't work when CONFIG_SECURITY is off, but that is a rare kernel config? > > Or add both security_* and vfs_* variants and switch tests to use security_* ? > > but it feels fragile to allow inline-able funcs in allowlist. > > hm, what's the difference between vfs_getattr and security_inode_getattr > in this regard? I'd expect compiler could inline it same way as for vfs_getattr not really because they're in different files and LTO is not on. Even with LTO the chances of inlining are small. The compiler will consider profitability of it. Since there is a loop inside, it's unlikely.
On Mon, Sep 14, 2020 at 07:30:33PM -0700, Alexei Starovoitov wrote: > On Fri, Sep 11, 2020 at 6:16 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Thu, Sep 10, 2020 at 05:46:21PM -0700, Alexei Starovoitov wrote: > > > On Thu, Sep 10, 2020 at 5:22 AM Jiri Olsa <jolsa@kernel.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. > > > > > > It's great that we had the test and it failed. > > > Doing the test skipping will only hide the problem. > > > Please don't do it here and in the future. > > > Instead let's figure out the real solution. > > > Assuming that vfs_getattr was added to btf_allowlist_d_path > > > for a reason we have to make this introspection place > > > reliable regardless of compiler inlining decisions. > > > We can mark it as 'noinline', but that's undesirable. > > > I suggest we remove it from the allowlist and replace it with > > > security_inode_getattr. > > > I think that is a better long term fix. > > > > in my case vfs_getattr got inlined in vfs_statx_fd and both > > of them are defined in fs/stat.c > > > > so the idea is that inlining will not happen if the function > > is defined in another object? or less likely..? > > when it's in a different .o file. yes. > Very few folks build LTO kernels, so I propose to cross that bridge when > we get there. > Eventually we can replace security_inode_getattr > with bpf_lsm_inode_getattr or simply add noinline to security_inode_getattr. > > > we should be safe when it's called from module > > what do you mean? it's external call, so it will not get inlined > > > > While at it I would apply the same critical thinking to other > > > functions in the allowlist. They might suffer the same issue. > > > So s/vfs_truncate/security_path_truncate/ and so on? > > > Things won't work when CONFIG_SECURITY is off, but that is a rare kernel config? > > > Or add both security_* and vfs_* variants and switch tests to use security_* ? > > > but it feels fragile to allow inline-able funcs in allowlist. > > > > hm, what's the difference between vfs_getattr and security_inode_getattr > > in this regard? I'd expect compiler could inline it same way as for vfs_getattr > > not really because they're in different files and LTO is not on. > Even with LTO the chances of inlining are small. The compiler will > consider profitability of it. Since there is a loop inside, it's unlikely. ok, thanks for info I'll use that security_inode_getattr instead of vfs_getattr jirka
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; + } + 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]); } 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;
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@redhat.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(-)