| Message ID | 20200918112338.2618444-1-jolsa@kernel.org |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [PATCHv2,bpf-next] selftests/bpf: Fix stat probe in d_path test | expand |
On Fri, Sep 18, 2020 at 4:23 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. > > Alexei suggested [1] we should use security_inode_getattr instead, > because it's less likely to get inlined. Using this idea also for > vfs_truncate (replaced with security_path_truncate) and vfs_fallocate > (replaced with security_file_permission). > > Keeping dentry_open and filp_close, because they are in their own > files, so unlikely to be inlined, but in case they are, adding > security_file_open. > > Switching the d_path test 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. > > Suggested-by: Alexei Starovoitov <ast@kernel.org> > [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> > --- > v2 changes: > - replaced vfs_* function with security_* in d_path allow list > vfs_truncate -> security_path_truncate > vfs_fallocate -> security_file_permission > vfs_getattr -> security_inode_getattr > - added security_file_open to d_path allow list > - split verbose output for trampoline flags > > kernel/trace/bpf_trace.c | 7 ++++--- > tools/testing/selftests/bpf/prog_tests/d_path.c | 10 ++++++++++ > tools/testing/selftests/bpf/progs/test_d_path.c | 9 ++++++++- > 3 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index b2a5380eb187..e24323d72cac 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -1118,10 +1118,11 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz) > } > > BTF_SET_START(btf_allowlist_d_path) > -BTF_ID(func, vfs_truncate) > -BTF_ID(func, vfs_fallocate) > +BTF_ID(func, security_path_truncate) > +BTF_ID(func, security_file_permission) > +BTF_ID(func, security_inode_getattr) > +BTF_ID(func, security_file_open) > BTF_ID(func, dentry_open) > -BTF_ID(func, vfs_getattr) > BTF_ID(func, filp_close) > BTF_SET_END(btf_allowlist_d_path) bpf CI system flagged the build error: FAILED unresolved symbol security_path_truncate because CONFIG_SECURITY_PATH wasn't set. Which points to the issue with this patch that the above security_* funcs have to be guarded with appropriate #ifdef. I don't have a use case for tracing vfs_truncate, but security_path_unlink I would want to do in the future. Unfortunately it's under the same SECURITY_PATH ifdef. So my earlier desire to make it fool proof is not feasible at this point. Adding 'was_probed_func_inlined' check to libbpftrace.a would solve it eventually. For now I think we have to live with this function probing fragility. So I've modified the patch to add these few security_* funcs and kept vfs_* equivalents. Also reworded commit log and applied to bpf-next. Thanks https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=a8a717963fe5ecfd274eb93dd1285ee9428ffca7
On Mon, Sep 21, 2020 at 04:32:13PM -0700, Alexei Starovoitov wrote: > On Fri, Sep 18, 2020 at 4:23 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. > > > > Alexei suggested [1] we should use security_inode_getattr instead, > > because it's less likely to get inlined. Using this idea also for > > vfs_truncate (replaced with security_path_truncate) and vfs_fallocate > > (replaced with security_file_permission). > > > > Keeping dentry_open and filp_close, because they are in their own > > files, so unlikely to be inlined, but in case they are, adding > > security_file_open. > > > > Switching the d_path test 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. > > > > Suggested-by: Alexei Starovoitov <ast@kernel.org> > > [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> > > --- > > v2 changes: > > - replaced vfs_* function with security_* in d_path allow list > > vfs_truncate -> security_path_truncate > > vfs_fallocate -> security_file_permission > > vfs_getattr -> security_inode_getattr > > - added security_file_open to d_path allow list > > - split verbose output for trampoline flags > > > > kernel/trace/bpf_trace.c | 7 ++++--- > > tools/testing/selftests/bpf/prog_tests/d_path.c | 10 ++++++++++ > > tools/testing/selftests/bpf/progs/test_d_path.c | 9 ++++++++- > > 3 files changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index b2a5380eb187..e24323d72cac 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -1118,10 +1118,11 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz) > > } > > > > BTF_SET_START(btf_allowlist_d_path) > > -BTF_ID(func, vfs_truncate) > > -BTF_ID(func, vfs_fallocate) > > +BTF_ID(func, security_path_truncate) > > +BTF_ID(func, security_file_permission) > > +BTF_ID(func, security_inode_getattr) > > +BTF_ID(func, security_file_open) > > BTF_ID(func, dentry_open) > > -BTF_ID(func, vfs_getattr) > > BTF_ID(func, filp_close) > > BTF_SET_END(btf_allowlist_d_path) > > bpf CI system flagged the build error: > FAILED unresolved symbol security_path_truncate > because CONFIG_SECURITY_PATH wasn't set. > Which points to the issue with this patch that the above > security_* funcs have to be guarded with appropriate #ifdef. ugh, sry > I don't have a use case for tracing vfs_truncate, but > security_path_unlink I would want to do in the future. > Unfortunately it's under the same SECURITY_PATH ifdef. > So my earlier desire to make it fool proof is not feasible at this point. > Adding 'was_probed_func_inlined' check to libbpftrace.a would > solve it eventually. > For now I think we have to live with this function probing fragility. > So I've modified the patch to add these few security_* funcs > and kept vfs_* equivalents. > Also reworded commit log and applied to bpf-next. Thanks > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=a8a717963fe5ecfd274eb93dd1285ee9428ffca7 > ok, looks good, thanks, jirka
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index b2a5380eb187..e24323d72cac 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -1118,10 +1118,11 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz) } BTF_SET_START(btf_allowlist_d_path) -BTF_ID(func, vfs_truncate) -BTF_ID(func, vfs_fallocate) +BTF_ID(func, security_path_truncate) +BTF_ID(func, security_file_permission) +BTF_ID(func, security_inode_getattr) +BTF_ID(func, security_file_open) BTF_ID(func, dentry_open) -BTF_ID(func, vfs_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..0a577a248d34 100644 --- a/tools/testing/selftests/bpf/prog_tests/d_path.c +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c @@ -120,6 +120,16 @@ void test_d_path(void) if (err < 0) goto cleanup; + if (CHECK(!bss->called_stat, + "stat", + "trampoline for security_inode_getattr was not called\n")) + goto cleanup; + + if (CHECK(!bss->called_close, + "close", + "trampoline for filp_close was 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", 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. Using this idea also for vfs_truncate (replaced with security_path_truncate) and vfs_fallocate (replaced with security_file_permission). Keeping dentry_open and filp_close, because they are in their own files, so unlikely to be inlined, but in case they are, adding security_file_open. Switching the d_path test 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. Suggested-by: Alexei Starovoitov <ast@kernel.org> [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> --- v2 changes: - replaced vfs_* function with security_* in d_path allow list vfs_truncate -> security_path_truncate vfs_fallocate -> security_file_permission vfs_getattr -> security_inode_getattr - added security_file_open to d_path allow list - split verbose output for trampoline flags kernel/trace/bpf_trace.c | 7 ++++--- tools/testing/selftests/bpf/prog_tests/d_path.c | 10 ++++++++++ tools/testing/selftests/bpf/progs/test_d_path.c | 9 ++++++++- 3 files changed, 22 insertions(+), 4 deletions(-)