* [PATCH bpf-next] selftests/bpf: Fix stat probe in d_path test
@ 2020-09-16 11:24 Jiri Olsa
2020-09-16 17:25 ` KP Singh
2020-09-17 1:45 ` Alexei Starovoitov
0 siblings, 2 replies; 6+ messages in thread
From: Jiri Olsa @ 2020-09-16 11:24 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.
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)
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;
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Fix stat probe in d_path test
2020-09-16 11:24 [PATCH bpf-next] selftests/bpf: Fix stat probe in d_path test Jiri Olsa
@ 2020-09-16 17:25 ` KP Singh
2020-09-17 1:45 ` Alexei Starovoitov
1 sibling, 0 replies; 6+ messages in thread
From: KP Singh @ 2020-09-16 17:25 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Networking,
bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend
On Wed, Sep 16, 2020 at 1:24 PM 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.
>
> 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>
Acked-by: KP Singh <kpsingh@google.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)
>
> 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))
optional:
maybe it's better to add two separate checks with specific error messages?
"stat", "trampoline for security_inode_getattr was not called\n"
"close", "trampoline for filp_close was not called\n"
I think this would make the output more readable.
- KP
> + goto cleanup;
> +
> for (int i = 0; i < MAX_FILES; i++) {
> CHECK(strncmp(src.paths[i], bss->paths_stat[i], MAX_PATH_LEN),
> "check",
[...]
> if (pid != my_pid)
> return 0;
>
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Fix stat probe in d_path test
2020-09-16 11:24 [PATCH bpf-next] selftests/bpf: Fix stat probe in d_path test Jiri Olsa
2020-09-16 17:25 ` KP Singh
@ 2020-09-17 1:45 ` Alexei Starovoitov
2020-09-17 8:25 ` Jiri Olsa
1 sibling, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2020-09-17 1:45 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, netdev, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Fix stat probe in d_path test
2020-09-17 1:45 ` Alexei Starovoitov
@ 2020-09-17 8:25 ` Jiri Olsa
2020-09-17 21:14 ` Alexei Starovoitov
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Olsa @ 2020-09-17 8:25 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
netdev, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Fix stat probe in d_path test
2020-09-17 8:25 ` Jiri Olsa
@ 2020-09-17 21:14 ` Alexei Starovoitov
2020-09-18 10:22 ` Jiri Olsa
0 siblings, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2020-09-17 21:14 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Network Development, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Fix stat probe in d_path test
2020-09-17 21:14 ` Alexei Starovoitov
@ 2020-09-18 10:22 ` Jiri Olsa
0 siblings, 0 replies; 6+ messages in thread
From: Jiri Olsa @ 2020-09-18 10:22 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 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-18 10:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-16 11:24 [PATCH bpf-next] selftests/bpf: Fix stat probe in d_path test Jiri Olsa
2020-09-16 17:25 ` KP Singh
2020-09-17 1:45 ` Alexei Starovoitov
2020-09-17 8:25 ` Jiri Olsa
2020-09-17 21:14 ` Alexei Starovoitov
2020-09-18 10:22 ` 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).