[PATCHv2 bpf-next] selftests/bpf: Fix d_path test

[PATCHv2 bpf-next] selftests/bpf: Fix d_path test

[Jiri Olsa]

Recent commit [1] broken d_path test, because now filp_close is not called
directly from sys_close, but eventually later when the file is finally
released.

As suggested by Hou Tao we don't need to re-hook the bpf program, but just
instead we can use sys_close_range to trigger filp_close synchronously.

[1] 021a160abf62 ("fs: use __fput_sync in close(2)")
Suggested-by: Hou Tao <houtao@huaweicloud.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../testing/selftests/bpf/prog_tests/d_path.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

  v2: call close_range through syscall call [Hou Tao]

diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
index 911345c526e6..ccc768592e66 100644
--- a/tools/testing/selftests/bpf/prog_tests/d_path.c
+++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
@@ -12,6 +12,17 @@
 #include "test_d_path_check_rdonly_mem.skel.h"
 #include "test_d_path_check_types.skel.h"
 
+/* sys_close_range is not around for long time, so let's
+ * make sure we can call it on systems with older glibc
+ */
+#ifndef __NR_close_range
+#ifdef __alpha__
+#define __NR_close_range 546
+#else
+#define __NR_close_range 436
+#endif
+#endif
+
 static int duration;
 
 static struct {
@@ -90,7 +101,11 @@ static int trigger_fstat_events(pid_t pid)
 	fstat(indicatorfd, &fileStat);
 
 out_close:
-	/* triggers filp_close */
+	/* sys_close no longer triggers filp_close, but we can
+	 * call sys_close_range instead which still does
+	 */
+#define close(fd) syscall(__NR_close_range, fd, fd, 0)
+
 	close(pipefd[0]);
 	close(pipefd[1]);
 	close(sockfd);
@@ -98,6 +113,8 @@ static int trigger_fstat_events(pid_t pid)
 	close(devfd);
 	close(localfd);
 	close(indicatorfd);
+
+#undef close
 	return ret;
 }
 
-- 
2.41.0

Re: [PATCHv2 bpf-next] selftests/bpf: Fix d_path test

[Daniel Borkmann]

On 8/31/23 4:11 PM, Jiri Olsa wrote:
> Recent commit [1] broken d_path test, because now filp_close is not called
> directly from sys_close, but eventually later when the file is finally
> released.
> 
> As suggested by Hou Tao we don't need to re-hook the bpf program, but just
> instead we can use sys_close_range to trigger filp_close synchronously.
> 
> [1] 021a160abf62 ("fs: use __fput_sync in close(2)")
> Suggested-by: Hou Tao <houtao@huaweicloud.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

That did the trick, thanks everyone, applied!

Re: [PATCHv2 bpf-next] selftests/bpf: Fix d_path test

[Song Liu]

On Thu, Aug 31, 2023 at 8:21 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 8/31/23 4:11 PM, Jiri Olsa wrote:
> > Recent commit [1] broken d_path test, because now filp_close is not called
> > directly from sys_close, but eventually later when the file is finally
> > released.
> >
> > As suggested by Hou Tao we don't need to re-hook the bpf program, but just
> > instead we can use sys_close_range to trigger filp_close synchronously.
> >
> > [1] 021a160abf62 ("fs: use __fput_sync in close(2)")
> > Suggested-by: Hou Tao <houtao@huaweicloud.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>
> That did the trick, thanks everyone, applied!

I guess I am a bit late. But how about we use something like the following?
I like this one better because it tests bpf_d_path() from retval at fexit.

Thanks,
Song




diff --git i/kernel/trace/bpf_trace.c w/kernel/trace/bpf_trace.c
index a7264b2c17ad..fe91836cedcd 100644
--- i/kernel/trace/bpf_trace.c
+++ w/kernel/trace/bpf_trace.c
@@ -941,6 +941,7 @@ BTF_ID(func, vfs_fallocate)
 BTF_ID(func, dentry_open)
 BTF_ID(func, vfs_getattr)
 BTF_ID(func, filp_close)
+BTF_ID(func, close_fd_get_file)
 BTF_SET_END(btf_allowlist_d_path)

 static bool bpf_d_path_allowed(const struct bpf_prog *prog)
diff --git i/tools/testing/selftests/bpf/progs/test_d_path.c
w/tools/testing/selftests/bpf/progs/test_d_path.c
index 84e1f883f97b..c880cfc95737 100644
--- i/tools/testing/selftests/bpf/progs/test_d_path.c
+++ w/tools/testing/selftests/bpf/progs/test_d_path.c
@@ -40,8 +40,8 @@ int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
        return 0;
 }

-SEC("fentry/filp_close")
-int BPF_PROG(prog_close, struct file *file, void *id)
+SEC("fexit/close_fd_get_file")
+int BPF_PROG(close_fd_get_file, int fd, struct file *file /* retval */)
 {
        pid_t pid = bpf_get_current_pid_tgid() >> 32;
        __u32 cnt = cnt_close;

Re: [PATCHv2 bpf-next] selftests/bpf: Fix d_path test

[Jiri Olsa]

On Fri, Sep 01, 2023 at 04:09:31PM -0700, Song Liu wrote:
> On Thu, Aug 31, 2023 at 8:21 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 8/31/23 4:11 PM, Jiri Olsa wrote:
> > > Recent commit [1] broken d_path test, because now filp_close is not called
> > > directly from sys_close, but eventually later when the file is finally
> > > released.
> > >
> > > As suggested by Hou Tao we don't need to re-hook the bpf program, but just
> > > instead we can use sys_close_range to trigger filp_close synchronously.
> > >
> > > [1] 021a160abf62 ("fs: use __fput_sync in close(2)")
> > > Suggested-by: Hou Tao <houtao@huaweicloud.com>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> >
> > That did the trick, thanks everyone, applied!
> 
> I guess I am a bit late. But how about we use something like the following?
> I like this one better because it tests bpf_d_path() from retval at fexit.

right, that would have been an option as well

> 
> Thanks,
> Song
> 
> 
> 
> 
> diff --git i/kernel/trace/bpf_trace.c w/kernel/trace/bpf_trace.c
> index a7264b2c17ad..fe91836cedcd 100644
> --- i/kernel/trace/bpf_trace.c
> +++ w/kernel/trace/bpf_trace.c
> @@ -941,6 +941,7 @@ BTF_ID(func, vfs_fallocate)
>  BTF_ID(func, dentry_open)
>  BTF_ID(func, vfs_getattr)
>  BTF_ID(func, filp_close)
> +BTF_ID(func, close_fd_get_file)

I liked using the close_range syscall because we did not need to
add new allowed function.. however close_fd_get_file looks safe
enough so I wouldn't mind changing that if you insist ;-)

jirka


>  BTF_SET_END(btf_allowlist_d_path)
> 
>  static bool bpf_d_path_allowed(const struct bpf_prog *prog)
> diff --git i/tools/testing/selftests/bpf/progs/test_d_path.c
> w/tools/testing/selftests/bpf/progs/test_d_path.c
> index 84e1f883f97b..c880cfc95737 100644
> --- i/tools/testing/selftests/bpf/progs/test_d_path.c
> +++ w/tools/testing/selftests/bpf/progs/test_d_path.c
> @@ -40,8 +40,8 @@ int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
>         return 0;
>  }
> 
> -SEC("fentry/filp_close")
> -int BPF_PROG(prog_close, struct file *file, void *id)
> +SEC("fexit/close_fd_get_file")
> +int BPF_PROG(close_fd_get_file, int fd, struct file *file /* retval */)
>  {
>         pid_t pid = bpf_get_current_pid_tgid() >> 32;
>         __u32 cnt = cnt_close;

Re: [PATCHv2 bpf-next] selftests/bpf: Fix d_path test

[Song Liu]

On Mon, Sep 4, 2023 at 12:10 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Fri, Sep 01, 2023 at 04:09:31PM -0700, Song Liu wrote:
> > On Thu, Aug 31, 2023 at 8:21 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > >
> > > On 8/31/23 4:11 PM, Jiri Olsa wrote:
> > > > Recent commit [1] broken d_path test, because now filp_close is not called
> > > > directly from sys_close, but eventually later when the file is finally
> > > > released.
> > > >
> > > > As suggested by Hou Tao we don't need to re-hook the bpf program, but just
> > > > instead we can use sys_close_range to trigger filp_close synchronously.
> > > >
> > > > [1] 021a160abf62 ("fs: use __fput_sync in close(2)")
> > > > Suggested-by: Hou Tao <houtao@huaweicloud.com>
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > >
> > > That did the trick, thanks everyone, applied!
> >
> > I guess I am a bit late. But how about we use something like the following?
> > I like this one better because it tests bpf_d_path() from retval at fexit.
>
> right, that would have been an option as well
>
> >
> > Thanks,
> > Song
> >
> >
> >
> >
> > diff --git i/kernel/trace/bpf_trace.c w/kernel/trace/bpf_trace.c
> > index a7264b2c17ad..fe91836cedcd 100644
> > --- i/kernel/trace/bpf_trace.c
> > +++ w/kernel/trace/bpf_trace.c
> > @@ -941,6 +941,7 @@ BTF_ID(func, vfs_fallocate)
> >  BTF_ID(func, dentry_open)
> >  BTF_ID(func, vfs_getattr)
> >  BTF_ID(func, filp_close)
> > +BTF_ID(func, close_fd_get_file)
>
> I liked using the close_range syscall because we did not need to
> add new allowed function.. however close_fd_get_file looks safe
> enough so I wouldn't mind changing that if you insist ;-)

Let's use close_range() then. I will send another patch to test
something with "struct file *" as retval on fexit.

Thanks,
Song