* [PATCHv2 bpf-next] selftests/bpf: Fix stat probe in d_path test
@ 2020-09-18 11:23 Jiri Olsa
2020-09-21 23:32 ` Alexei Starovoitov
0 siblings, 1 reply; 3+ messages in thread
From: Jiri Olsa @ 2020-09-18 11:23 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh
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)
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;
--
2.26.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCHv2 bpf-next] selftests/bpf: Fix stat probe in d_path test
2020-09-18 11:23 [PATCHv2 bpf-next] selftests/bpf: Fix stat probe in d_path test Jiri Olsa
@ 2020-09-21 23:32 ` Alexei Starovoitov
2020-09-22 18:39 ` Jiri Olsa
0 siblings, 1 reply; 3+ messages in thread
From: Alexei Starovoitov @ 2020-09-21 23:32 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Network Development, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCHv2 bpf-next] selftests/bpf: Fix stat probe in d_path test
2020-09-21 23:32 ` Alexei Starovoitov
@ 2020-09-22 18:39 ` Jiri Olsa
0 siblings, 0 replies; 3+ messages in thread
From: Jiri Olsa @ 2020-09-22 18:39 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Network Development, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-09-22 18:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-18 11:23 [PATCHv2 bpf-next] selftests/bpf: Fix stat probe in d_path test Jiri Olsa
2020-09-21 23:32 ` Alexei Starovoitov
2020-09-22 18:39 ` Jiri Olsa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).