Re: [RFC/PATCH bpf-next] bpf: Fix d_path test after last fs update

[Song Liu <song@xxxxxxxxxx>]

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
 fi

 mksysmap vmlinux System.map ${kallsymso}
diff --git i/tools/bpf/resolve_btfids/main.c w/tools/bpf/resolve_btfids/main.c
index 27a23196d58e..2940fe004220 100644
--- i/tools/bpf/resolve_btfids/main.c
+++ w/tools/bpf/resolve_btfids/main.c
@@ -599,8 +599,10 @@ static int id_patch(struct object *obj, struct btf_id *id)
        int i;

        /* For set, set8, id->id may be 0 */
-       if (!id->id && !id->is_set && !id->is_set8)
-               pr_err("WARN: resolve_btfids: unresolved symbol %s\n",
id->name);
+       if (!id->id && !id->is_set && !id->is_set8) {
+               pr_err("FAILED resolve_btfids: unresolved symbol
%s\n", id->name);
+               return -1;
+       }

        for (i = 0; i < id->addr_cnt; i++) {
                unsigned long addr = id->addr[i];

Thanks,
Song