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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] bpf: dead code in bpf_d_path_allowed for BPF_TRACE_FENTRY_MULTI
- [Low] bpf: build failure due to missing filp_close_sync BTF ID
- [Low] selftests/bpf: use of deprecated CHECK() macro when updating existing test
--

commit 83314c61560bbf42baf321997dbfd6e5b66818d7
Author: Ihor Solodrai <ihor.solodrai@linux.dev>
bpf: Allow bpf_d_path() from filp_close_sync()

This commit allows bpf_d_path() to be called from the filp_close_sync()
fentry program. It updates the allowlist and related selftests to reflect
the target function change introduced by recent VFS updates.

Link: https://lore.kernel.org/bpf/20260924204226.190315-1-ihor.solodrai@linux.dev/

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 29260951aa871..c3b869646baeb 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -974,6 +974,10 @@ BTF_ID(func, vfs_getattr)
>  BTF_ID(func, filp_close)
>  BTF_SET_END(btf_allowlist_d_path)
>  
> +BTF_SET_START(btf_allowlist_d_path_fentry)
> +BTF_ID(func, filp_close_sync)

[Severity: Low]
Is this function available in the current tree? The commit message mentions
VFS commit c1ed65ea167b, but without it being merged, resolve_btfids will
abort the kernel build because it cannot find the target function
filp_close_sync.

> +BTF_SET_END(btf_allowlist_d_path_fentry)

[ ... ]

> -	return btf_id_set_contains(&btf_allowlist_d_path,
> -				   prog->aux->attach_btf_id);
> +	if (btf_id_set_contains(&btf_allowlist_d_path,
> +				prog->aux->attach_btf_id))
> +		return true;
> +
> +	if (btf_id_set_contains(&btf_allowlist_d_path_fentry,
> +				prog->aux->attach_btf_id))
> +		return prog->expected_attach_type == BPF_TRACE_FENTRY ||
> +		       prog->expected_attach_type == BPF_TRACE_FENTRY_MULTI;

[Severity: Low]
Could this check for BPF_TRACE_FENTRY_MULTI be unreachable? For multi-attach
programs, prog->aux->attach_btf_id is always initialized to a dummy ID
(bpf_multi_func_btf_id[0]) at program load time. Because this dummy ID is not
present in the btf_allowlist_d_path_fentry set, the btf_id_set_contains()
check evaluates to false before reaching the expected_attach_type condition.

> +
> +	return false;
>  }

[ ... ]

> 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 test is being updated, should this deprecated CHECK() macro be
migrated to a modern ASSERT_*() macro? The BPF subsystem selftest standards
mandate using modern assertion macros when updating existing tests.

>  		goto cleanup;

[ ... ]

> 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: Low]
Similar to the BTF ID issue above, will this test fail to load because
filp_close_sync does not exist in the current kernel tree?

>  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/20260925210347.1364191-1-ihor.solodrai@linux.dev?part=1