Re: [PATCH] selftests/bpf: fix token tests for bpf_object__prepare() probe_loading

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The patch masks a libbpf bug by inappropriately expanding token privileges in tests, defeating the purpose of testing fine-grained token delegation.
- [Medium] The patch silently adds BPF_MAP_TYPE_ARRAY and BPF_PROG_LOAD permissions to test options without documenting them in the commit message.
--

commit f600b2d49f35c6ad7110d3da0ab207e0df841447
Author: Yuan Chen <chenyuan@kylinos.cn>

selftests/bpf: fix token tests for bpf_object__prepare() probe_loading

This patch modifies BPF token selftests to add SOCKET_FILTER, TRACEPOINT,
and ARRAY map permissions to various subtests. This is intended to fix test
failures caused by libbpf's internal environment probe loading.

> The bpf_object__prepare() function internally calls bpf_object__probe_loading(),
> which attempts to load SOCKET_FILTER and TRACEPOINT programs to verify the BPF
> environment. Without these permissions in the token, the probe fails and causes
> the tests to fail.
> 
> Fix the following test cases by adding SOCKET_FILTER and TRACEPOINT permissions:

[Severity: Medium]
The commit message explicitly mentions adding SOCKET_FILTER and TRACEPOINT
permissions. However, the patch also adds BPF_PROG_LOAD to the obj_priv_map
test, and BPF_MAP_TYPE_ARRAY to several other tests. 

Could the commit message be updated to reflect all the changes being made,
or should these undocumented changes be removed?

> diff --git a/tools/testing/selftests/bpf/prog_tests/token.c b/tools/testing/selftests/bpf/prog_tests/token.c
> index f2f5d36ae00a4..25a76e09a3c96 100644
> --- a/tools/testing/selftests/bpf/prog_tests/token.c
> +++ b/tools/testing/selftests/bpf/prog_tests/token.c
> @@ -1168,8 +1168,13 @@ void serial_test_token(void)
>  	}
>  	if (test__start_subtest("obj_priv_map")) {
>  		struct bpffs_opts opts = {
> -			.cmds = bit(BPF_MAP_CREATE),
> +			/* BPF_PROG_LOAD is needed for bpf_object__probe_loading */
> +			.cmds = bit(BPF_MAP_CREATE) | bit(BPF_PROG_LOAD),
>  			.maps = bit(BPF_MAP_TYPE_QUEUE),
> +			/* Allow SOCKET_FILTER and TRACEPOINT for bpf_object__probe_loading */
> +			.progs = bit(BPF_PROG_TYPE_SOCKET_FILTER) |
> +					 bit(BPF_PROG_TYPE_TRACEPOINT),
> +			.attachs = ~0ULL,
>  		};

[Severity: Medium]
By adding BPF_PROG_LOAD, SOCKET_FILTER, and TRACEPOINT here, does this
defeat the purpose of the obj_priv_map test? 

This test appears specifically designed to verify that a token restricted to
only BPF_MAP_CREATE can successfully authorize map-only operations.

If bpf_object__probe_loading() causes the entire object load to abort with
-EPERM when a confined token is used, isn't this a functional bug in libbpf
rather than a test issue? 

Should libbpf be fixed to handle probe -EPERM gracefully (or skip probes for
objects without programs) instead of expanding the token privileges in the
tests?

[ ... ]

> @@ -1213,9 +1232,13 @@ void serial_test_token(void)
>  	if (test__start_subtest("obj_priv_btf_success")) {
>  		struct bpffs_opts opts = {
>  			/* allow BTF loading */
> -			.cmds = bit(BPF_BTF_LOAD) | bit(BPF_MAP_CREATE) | bit(BPF_PROG_LOAD),
> -			.maps = bit(BPF_MAP_TYPE_STRUCT_OPS),
> -			.progs = bit(BPF_PROG_TYPE_STRUCT_OPS),
> +			.cmds = bit(BPF_BTF_LOAD) | bit(BPF_MAP_CREATE) |
> +					bit(BPF_PROG_LOAD),
> +			.maps = bit(BPF_MAP_TYPE_STRUCT_OPS) | bit(BPF_MAP_TYPE_ARRAY),
> +			/* Allow SOCKET_FILTER and TRACEPOINT for bpf_object__probe_loading */
> +			.progs = bit(BPF_PROG_TYPE_STRUCT_OPS) |
> +					 bit(BPF_PROG_TYPE_SOCKET_FILTER) |
> +					 bit(BPF_PROG_TYPE_TRACEPOINT),
>  			.attachs = ~0ULL,
>  		};

[Severity: Medium]
Is there a specific reason BPF_MAP_TYPE_ARRAY is being added to the
maps allowance here? It seems unrelated to the probe loading fix described
in the commit message.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529031604.888137-1-chenyuan_fl@163.com?part=1