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

[Yonghong Song <yonghong.song@linux.dev>]

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