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

[Jiri Olsa <olsajiri@gmail.com>]

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;