On Thu, Aug 31, 2023 at 01:24:10AM -0400, Song Liu wrote:
> On Wed, Aug 30, 2023 at 6:17 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
> >
> >
> >
> > On 8/30/23 9:27 AM, Jiri Olsa wrote:
> > > On Wed, Aug 30, 2023 at 11:35:02AM +0200, 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.
> > >>
> > >> I can't see any other solution than to hook filp_flush function and
> > >> that also means we need to add it to btf_allowlist_d_path list, so
> > >> it can use the d_path helper.
> > >>
> > >> But it's probably not very stable because filp_flush is static so it
> > >> could be potentially inlined.
> > >
> > > looks like llvm makes it inlined (from CI)
> > >
> > > Error: #68/1 d_path/basic
> > > libbpf: prog 'prog_close': failed to find kernel BTF type ID of 'filp_flush': -3
> > >
> > > jirka
> > >
> > >>
> > >> Also if we'd keep the current filp_close hook and find a way how to 'wait'
> > >> for it to be called so user space can go with checks, then it looks
> > >> like d_path might not work properly when the task is no longer around.
> > >>
> > >> thoughts?
> >
> > Jiri,
> >
> > The following patch works fine for me:
> >
> > $ git diff
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index a7264b2c17ad..fdeec712338f 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/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, __fput_sync)
> > BTF_SET_END(btf_allowlist_d_path)
> >
> > static bool bpf_d_path_allowed(const struct bpf_prog *prog)
> > diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c
> > b/tools/testing/selftests/bpf/progs/test_d_path.c
> > index 84e1f883f97b..672897197c2a 100644
> > --- a/tools/testing/selftests/bpf/progs/test_d_path.c
> > +++ b/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("fentry/__fput_sync")
> > +int BPF_PROG(prog_close, struct file *file)
> > {
> > pid_t pid = bpf_get_current_pid_tgid() >> 32;
> > __u32 cnt = cnt_close;
>
> Yeah, I guess this is the easiest fix at the moment.
>
> Related, shall we have resolve_btfids fail for missing ID? Something
> like:
>
> diff --git i/scripts/link-vmlinux.sh w/scripts/link-vmlinux.sh
> index a432b171be82..9a194152da49 100755
> --- i/scripts/link-vmlinux.sh
> +++ w/scripts/link-vmlinux.sh
> @@ -274,7 +274,10 @@ vmlinux_link vmlinux "${kallsymso}" ${btf_vmlinux_bin_o}
> # fill in BTF IDs
> if is_enabled CONFIG_DEBUG_INFO_BTF && is_enabled CONFIG_BPF; then
> info BTFIDS vmlinux
> - ${RESOLVE_BTFIDS} vmlinux
> + if ! ${RESOLVE_BTFIDS} vmlinux ; then
> + echo >&2 Failed to resolve BTF IDs
> + exit 1
> + fi
IIUC link-vmlinux.sh will fail when ${RESOLVE_BTFIDS} returns != 0,
and now the unresolved symbol is just a warning
we used to have that but we decided to just warn:
5aad03685185 tools/resolve_btfids: Emit warnings and patch zero id for missing symbols
jirka
