| Message ID | 20230831141103.359810-1-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | [PATCHv2,bpf-next] selftests/bpf: Fix d_path test | expand |
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!
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;
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;
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
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; }
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]