Re: [PATCH vfs v1] bpf: Allow bpf_d_path() from filp_close_sync()

[Ihor Solodrai <ihor.solodrai@linux.dev>]

On 9/24/26 1:52 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] bpf: missing filp_close_sync dependency breaks resolve_btfids and selftest
> - [Low] bpf: failure to migrate CHECK() to ASSERT_*() when updating selftest
> --
> 
> commit f620a29b55750df4341321e76f9065c834f91fa3
> Author: Ihor Solodrai <ihor.solodrai@linux.dev>
> 
> bpf: Allow bpf_d_path() from filp_close_sync()
> 
> The d_path selftest closes its descriptors with close_range() so that
> its fentry program on filp_close() runs. The vfs commit
> 46ace7e4dc4b ("fs: make close_range() synchronous") switched
> close_range() to filp_close_sync(), so the program still attaches but
> is never called. This commit allows bpf_d_path() from filp_close_sync()
> and updates the selftest.
> 
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index 29260951aa871..09a27810ca27f 100644
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -972,6 +972,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_close_sync)
> 
> [Severity: High]
> Does filp_close_sync exist in the current tree? Since the referenced VFS commit
> (46ace7e4dc4b) might not be present yet, adding its BTF ID here will break the
> kernel build during resolve_btfids due to a missing dependency.

The patch obviously depends on 46ace7e4dc4b ("fs: make close_range() synchronous")
being in the tree.

> 
> [ ... ]
> 
>> diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
>> index 1a2a2f1abf033..7665e1d28a641 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/d_path.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
> [ ... ]
>> @@ -165,7 +166,7 @@ static void test_d_path_basic(void)
>>  
>>  	if (CHECK(!bss->called_close,
>>  		  "close",
>> -		  "trampoline for filp_close was not called\n"))
>> +		  "trampoline for filp_close_sync was not called\n"))
> 
> [Severity: Low]
> Since this CHECK() macro in test_d_path_basic() is being modified, shouldn't
> it be migrated to the modern ASSERT_*() macros? BPF subsystem guidelines require
> replacing deprecated CHECK() macros when updating existing tests.

That's a selftests/bpf nit, not relevant to the issue addressed by the patch.

> 
>>  		goto cleanup;
>>  
>>  	for (int i = 0; i < MAX_FILES; i++) {
> 
> [ ... ]
> 
>> diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c
>> index 561b2f861808e..edb5494a47f74 100644
>> --- a/tools/testing/selftests/bpf/progs/test_d_path.c
>> +++ b/tools/testing/selftests/bpf/progs/test_d_path.c
>> @@ -41,7 +41,7 @@ int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
>>  	return 0;
>>  }
>>  
>> -SEC("fentry/filp_close")
>> +SEC("fentry/filp_close_sync")
> 
> [Severity: High]
> Will the test load fail unconditionally here? Attaching a BPF program to
> fentry/filp_close_sync when the function doesn't yet exist in the tree will
> cause an error. Applying this patch without its VFS dependency introduces a
> cross-tree dependency failure regression.

See above.

> 
>>  int BPF_PROG(prog_close, struct file *file, void *id)
>>  {
>>  	pid_t pid = bpf_get_current_pid_tgid() >> 32;
>