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

[sashiko-bot@kernel.org]

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.

[ ... ]

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

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924204226.190315-1-ihor.solodrai@linux.dev?part=1