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

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

[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.

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.

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?
jirka


[1] 021a160abf62 ("fs: use __fput_sync in close(2)")
---
 kernel/trace/bpf_trace.c                        | 1 +
 tools/testing/selftests/bpf/progs/test_d_path.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index a7264b2c17ad..c829e24af246 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, filp_flush)
 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..3467d1b8098c 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/filp_flush")
+int BPF_PROG(prog_close, struct file *file)
 {
 	pid_t pid = bpf_get_current_pid_tgid() >> 32;
 	__u32 cnt = cnt_close;
-- 
2.41.0

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

[Jiri Olsa]

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?
> jirka
> 
> 
> [1] 021a160abf62 ("fs: use __fput_sync in close(2)")
> ---
>  kernel/trace/bpf_trace.c                        | 1 +
>  tools/testing/selftests/bpf/progs/test_d_path.c | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index a7264b2c17ad..c829e24af246 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, filp_flush)
>  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..3467d1b8098c 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/filp_flush")
> +int BPF_PROG(prog_close, struct file *file)
>  {
>  	pid_t pid = bpf_get_current_pid_tgid() >> 32;
>  	__u32 cnt = cnt_close;
> -- 
> 2.41.0
> 

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

[Song Liu]

On Wed, Aug 30, 2023 at 9:27 AM Jiri Olsa <olsajiri@gmail.com> 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

I played with it for a bit, but haven't got a good solution. Maybe we should
just remove the test for close()?

Thanks,
Song
>
> >
> > 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.

[...]

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

[Jiri Olsa]

On Wed, Aug 30, 2023 at 02:35:49PM -0400, Song Liu wrote:
> On Wed, Aug 30, 2023 at 9:27 AM Jiri Olsa <olsajiri@gmail.com> 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
> 
> I played with it for a bit, but haven't got a good solution. Maybe we should
> just remove the test for close()?

I was thinking the same.. also we have some example with filp_close in bpftrace
docs, I think we'll need to add some note with explanation in there 

jirka

> 
> Thanks,
> Song
> >
> > >
> > > 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.
> 
> [...]

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

[Alexei Starovoitov]

On Wed, Aug 30, 2023 at 12:04 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Aug 30, 2023 at 02:35:49PM -0400, Song Liu wrote:
> > On Wed, Aug 30, 2023 at 9:27 AM Jiri Olsa <olsajiri@gmail.com> 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
> >
> > I played with it for a bit, but haven't got a good solution. Maybe we should
> > just remove the test for close()?
>
> I was thinking the same.. also we have some example with filp_close in bpftrace
> docs, I think we'll need to add some note with explanation in there

Maybe use __x64_sys_close in the test and recommend bpftrace scripts
to do the same?

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

[Jiri Olsa]

On Wed, Aug 30, 2023 at 01:29:18PM -0700, Alexei Starovoitov wrote:
> On Wed, Aug 30, 2023 at 12:04 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Wed, Aug 30, 2023 at 02:35:49PM -0400, Song Liu wrote:
> > > On Wed, Aug 30, 2023 at 9:27 AM Jiri Olsa <olsajiri@gmail.com> 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
> > >
> > > I played with it for a bit, but haven't got a good solution. Maybe we should
> > > just remove the test for close()?
> >
> > I was thinking the same.. also we have some example with filp_close in bpftrace
> > docs, I think we'll need to add some note with explanation in there
> 
> Maybe use __x64_sys_close in the test and recommend bpftrace scripts
> to do the same?

we need struct file pointer as an argument, __x64_sys_close has pt_regs pointer

jirka

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

[Yonghong Song]

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;

>> jirka
>>
>>
>> [1] 021a160abf62 ("fs: use __fput_sync in close(2)")
>> ---
>>   kernel/trace/bpf_trace.c                        | 1 +
>>   tools/testing/selftests/bpf/progs/test_d_path.c | 4 ++--
>>   2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index a7264b2c17ad..c829e24af246 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, filp_flush)
>>   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..3467d1b8098c 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/filp_flush")
>> +int BPF_PROG(prog_close, struct file *file)
>>   {
>>   	pid_t pid = bpf_get_current_pid_tgid() >> 32;
>>   	__u32 cnt = cnt_close;
>> -- 
>> 2.41.0
>>
> 

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

[Song Liu]

On Wed, Aug 30, 2023 at 6:17 PM Yonghong Song <yonghong.song@linux.dev> 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

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

[Jiri Olsa]

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@linux.dev> 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

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

[Jiri Olsa]

On Wed, Aug 30, 2023 at 06:11:52PM -0400, Yonghong Song 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)

hum right, that should work.. there are several callers of that,
but AFAICT they all seem ok (not holding mount_lock or rename_lock)
to run d_path helper

I'll send patch with the change

thanks,
jirka


>  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;
> 
> > > jirka
> > > 
> > > 
> > > [1] 021a160abf62 ("fs: use __fput_sync in close(2)")
> > > ---
> > >   kernel/trace/bpf_trace.c                        | 1 +
> > >   tools/testing/selftests/bpf/progs/test_d_path.c | 4 ++--
> > >   2 files changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index a7264b2c17ad..c829e24af246 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, filp_flush)
> > >   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..3467d1b8098c 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/filp_flush")
> > > +int BPF_PROG(prog_close, struct file *file)
> > >   {
> > >   	pid_t pid = bpf_get_current_pid_tgid() >> 32;
> > >   	__u32 cnt = cnt_close;
> > > -- 
> > > 2.41.0
> > > 
> > 

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

[Hou Tao]

Hi

On 8/30/2023 5:35 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.

To make test_d_path self-test pass, beside attaching to a different
function (e.g., __fput_sync or filp_flush), we could also use
close_range() or even dup2() to close the created fd, because these
syscalls still use filp_close() to close the opened file.

>
> 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.
>
> 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.

It seems there is no need to wait for it to be called, because
filp_close() is still called synchronously by some syscall (e.g.,
close_range or io_uring). So if the bpf program tries to collect many
close event as possible, it should be attach to both filp_close() and
__fput_sync(), right ?

>
> thoughts?
> jirka
>
>
> [1] 021a160abf62 ("fs: use __fput_sync in close(2)")
> ---
>  kernel/trace/bpf_trace.c                        | 1 +
>  tools/testing/selftests/bpf/progs/test_d_path.c | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index a7264b2c17ad..c829e24af246 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, filp_flush)
>  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..3467d1b8098c 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/filp_flush")
> +int BPF_PROG(prog_close, struct file *file)
>  {
>  	pid_t pid = bpf_get_current_pid_tgid() >> 32;
>  	__u32 cnt = cnt_close;

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

[Jiri Olsa]

On Thu, Aug 31, 2023 at 03:37:37PM +0800, Hou Tao wrote:
> Hi
> 
> On 8/30/2023 5:35 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.
> 
> To make test_d_path self-test pass, beside attaching to a different
> function (e.g., __fput_sync or filp_flush), we could also use
> close_range() or even dup2() to close the created fd, because these
> syscalls still use filp_close() to close the opened file.

nice, I like the close_range solution ;-) patch below fixes the test

> 
> >
> > 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.
> >
> > 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.
> 
> It seems there is no need to wait for it to be called, because
> filp_close() is still called synchronously by some syscall (e.g.,
> close_range or io_uring). So if the bpf program tries to collect many
> close event as possible, it should be attach to both filp_close() and
> __fput_sync(), right ?
> 

right, when we hook to close_range it's still synchronous

thanks,
jirka


---
diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
index 911345c526e6..8a558c980753 100644
--- a/tools/testing/selftests/bpf/prog_tests/d_path.c
+++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
@@ -91,6 +91,7 @@ static int trigger_fstat_events(pid_t pid)
 
 out_close:
 	/* triggers filp_close */
+#define close(fd) close_range(fd, fd, 0)
 	close(pipefd[0]);
 	close(pipefd[1]);
 	close(sockfd);
@@ -98,6 +99,7 @@ static int trigger_fstat_events(pid_t pid)
 	close(devfd);
 	close(localfd);
 	close(indicatorfd);
+#undef close
 	return ret;
 }