| Message ID | 20200916112416.2321204-1-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] selftests/bpf: Fix stat probe in d_path test | expand |
On Wed, Sep 16, 2020 at 01:24:16PM +0200, Jiri Olsa wrote: > Some kernels builds might inline vfs_getattr call within fstat > syscall code path, so fentry/vfs_getattr trampoline is not called. > > Alexei suggested [1] we should use security_inode_getattr instead, > because it's less likely to get inlined. > > Adding security_inode_getattr to the d_path allowed list and > switching the stat trampoline to security_inode_getattr. > > Adding flags that indicate trampolines were called and failing > the test if any of them got missed, so it's easier to identify > the issue next time. > > [1] https://lore.kernel.org/bpf/CAADnVQJ0FchoPqNWm+dEppyij-MOvvEG_trEfyrHdabtcEuZGg@mail.gmail.com/ > Fixes: e4d1af4b16f8 ("selftests/bpf: Add test for d_path helper") > Signed-off-by: Jiri Olsa <jolsa@redhat.com> > --- > kernel/trace/bpf_trace.c | 1 + > tools/testing/selftests/bpf/prog_tests/d_path.c | 6 ++++++ > tools/testing/selftests/bpf/progs/test_d_path.c | 9 ++++++++- > 3 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index b2a5380eb187..1001c053ebb3 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -1122,6 +1122,7 @@ BTF_ID(func, vfs_truncate) > BTF_ID(func, vfs_fallocate) > BTF_ID(func, dentry_open) > BTF_ID(func, vfs_getattr) > +BTF_ID(func, security_inode_getattr) > BTF_ID(func, filp_close) > BTF_SET_END(btf_allowlist_d_path) I think it's concealing the problem instead of fixing it. bpf is difficult to use for many reasons. Let's not make it harder. The users will have a very hard time debugging why vfs_getattr bpf probe is not called in all cases. Let's replace: vfs_truncate -> security_path_truncate vfs_fallocate -> security_file_permission vfs_getattr -> security_inode_getattr For dentry_open also add security_file_open. dentry_open and filp_close are in its own files, so unlikely to be inlined. Ideally resolve_btfids would parse dwarf info and check whether any of the funcs in allowlist were inlined. That would be more reliable, but not pretty to drag libdw dependency into resolve_btfids. > > diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c > index fc12e0d445ff..f507f1a6fa3a 100644 > --- a/tools/testing/selftests/bpf/prog_tests/d_path.c > +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c > @@ -120,6 +120,12 @@ void test_d_path(void) > if (err < 0) > goto cleanup; > > + if (CHECK(!bss->called_stat || !bss->called_close, +1 to KP's comment. > + "check", > + "failed to call trampolines called_stat %d, bss->called_close %d\n", > + bss->called_stat, bss->called_close)) > + goto cleanup;
On Wed, Sep 16, 2020 at 06:45:31PM -0700, Alexei Starovoitov wrote: > On Wed, Sep 16, 2020 at 01:24:16PM +0200, Jiri Olsa wrote: > > Some kernels builds might inline vfs_getattr call within fstat > > syscall code path, so fentry/vfs_getattr trampoline is not called. > > > > Alexei suggested [1] we should use security_inode_getattr instead, > > because it's less likely to get inlined. > > > > Adding security_inode_getattr to the d_path allowed list and > > switching the stat trampoline to security_inode_getattr. > > > > Adding flags that indicate trampolines were called and failing > > the test if any of them got missed, so it's easier to identify > > the issue next time. > > > > [1] https://lore.kernel.org/bpf/CAADnVQJ0FchoPqNWm+dEppyij-MOvvEG_trEfyrHdabtcEuZGg@mail.gmail.com/ > > Fixes: e4d1af4b16f8 ("selftests/bpf: Add test for d_path helper") > > Signed-off-by: Jiri Olsa <jolsa@redhat.com> > > --- > > kernel/trace/bpf_trace.c | 1 + > > tools/testing/selftests/bpf/prog_tests/d_path.c | 6 ++++++ > > tools/testing/selftests/bpf/progs/test_d_path.c | 9 ++++++++- > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index b2a5380eb187..1001c053ebb3 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -1122,6 +1122,7 @@ BTF_ID(func, vfs_truncate) > > BTF_ID(func, vfs_fallocate) > > BTF_ID(func, dentry_open) > > BTF_ID(func, vfs_getattr) > > +BTF_ID(func, security_inode_getattr) > > BTF_ID(func, filp_close) > > BTF_SET_END(btf_allowlist_d_path) > > I think it's concealing the problem instead of fixing it. > bpf is difficult to use for many reasons. Let's not make it harder. > The users will have a very hard time debugging why vfs_getattr bpf probe > is not called in all cases. > Let's replace: > vfs_truncate -> security_path_truncate > vfs_fallocate -> security_file_permission > vfs_getattr -> security_inode_getattr > > For dentry_open also add security_file_open. > dentry_open and filp_close are in its own files, > so unlikely to be inlined. ok > Ideally resolve_btfids would parse dwarf info and check > whether any of the funcs in allowlist were inlined. > That would be more reliable, but not pretty to drag libdw > dependency into resolve_btfids. hm, we could add some check to perf|bpftrace that would show you all the places where function is called from and if it was inlined or is a regular call.. so user is aware what probe calls to expect > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c > > index fc12e0d445ff..f507f1a6fa3a 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/d_path.c > > +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c > > @@ -120,6 +120,12 @@ void test_d_path(void) > > if (err < 0) > > goto cleanup; > > > > + if (CHECK(!bss->called_stat || !bss->called_close, > > +1 to KP's comment. ok thanks, jirka
On Thu, Sep 17, 2020 at 1:25 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > Ideally resolve_btfids would parse dwarf info and check > > whether any of the funcs in allowlist were inlined. > > That would be more reliable, but not pretty to drag libdw > > dependency into resolve_btfids. > > hm, we could add some check to perf|bpftrace that would > show you all the places where function is called from and > if it was inlined or is a regular call.. so user is aware > what probe calls to expect The check like this belongs in some library, but making libbpf depend on dwarf is not great. I think we're at the point where we need to break libbpf into many libraries. This one could be called libbpftrace. It would potentially include symbolizer and other dwarf related operations. Such inlining check would be good to do not only for d_path allowlist, but for any kprobe/fentry function.
On Thu, Sep 17, 2020 at 02:14:38PM -0700, Alexei Starovoitov wrote: > On Thu, Sep 17, 2020 at 1:25 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > Ideally resolve_btfids would parse dwarf info and check > > > whether any of the funcs in allowlist were inlined. > > > That would be more reliable, but not pretty to drag libdw > > > dependency into resolve_btfids. > > > > hm, we could add some check to perf|bpftrace that would > > show you all the places where function is called from and > > if it was inlined or is a regular call.. so user is aware > > what probe calls to expect > > The check like this belongs in some library, > but making libbpf depend on dwarf is not great. > I think we're at the point where we need to break libbpf > into many libraries. This one could be called libbpftrace. > It would potentially include symbolizer and other dwarf > related operations. ok > Such inlining check would be good to do not only for d_path > allowlist, but for any kprobe/fentry function. yes, that's what I meant jirka
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index b2a5380eb187..1001c053ebb3 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -1122,6 +1122,7 @@ BTF_ID(func, vfs_truncate) BTF_ID(func, vfs_fallocate) BTF_ID(func, dentry_open) BTF_ID(func, vfs_getattr) +BTF_ID(func, security_inode_getattr) BTF_ID(func, filp_close) BTF_SET_END(btf_allowlist_d_path) diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c index fc12e0d445ff..f507f1a6fa3a 100644 --- a/tools/testing/selftests/bpf/prog_tests/d_path.c +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c @@ -120,6 +120,12 @@ void test_d_path(void) if (err < 0) goto cleanup; + if (CHECK(!bss->called_stat || !bss->called_close, + "check", + "failed to call trampolines called_stat %d, bss->called_close %d\n", + bss->called_stat, bss->called_close)) + goto cleanup; + for (int i = 0; i < MAX_FILES; i++) { CHECK(strncmp(src.paths[i], bss->paths_stat[i], MAX_PATH_LEN), "check", diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c index 61f007855649..84e1f883f97b 100644 --- a/tools/testing/selftests/bpf/progs/test_d_path.c +++ b/tools/testing/selftests/bpf/progs/test_d_path.c @@ -15,7 +15,10 @@ char paths_close[MAX_FILES][MAX_PATH_LEN] = {}; int rets_stat[MAX_FILES] = {}; int rets_close[MAX_FILES] = {}; -SEC("fentry/vfs_getattr") +int called_stat = 0; +int called_close = 0; + +SEC("fentry/security_inode_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. Alexei suggested [1] we should use security_inode_getattr instead, because it's less likely to get inlined. Adding security_inode_getattr to the d_path allowed list and switching the stat trampoline to security_inode_getattr. Adding flags that indicate trampolines were called and failing the test if any of them got missed, so it's easier to identify the issue next time. [1] https://lore.kernel.org/bpf/CAADnVQJ0FchoPqNWm+dEppyij-MOvvEG_trEfyrHdabtcEuZGg@mail.gmail.com/ Fixes: e4d1af4b16f8 ("selftests/bpf: Add test for d_path helper") Signed-off-by: Jiri Olsa <jolsa@redhat.com> --- kernel/trace/bpf_trace.c | 1 + tools/testing/selftests/bpf/prog_tests/d_path.c | 6 ++++++ tools/testing/selftests/bpf/progs/test_d_path.c | 9 ++++++++- 3 files changed, 15 insertions(+), 1 deletion(-)