Re: [PATCH bpf-next v1 2/2] selftests/bpf: Handle unaligned access failure in fexit_bpf2bpf

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/bpf: fexit_bpf2bpf skips tests instead of aligning subprog offset
- [Medium] selftests/bpf: get_unaligned_access_enabled() masks EACCES verifier errors
--

commit 7711595f0d56f0638a8acca1420d5b1c4401c310
Author: Tiezhu Yang <yangtiezhu@loongson.cn>

selftests/bpf: Handle unaligned access failure in fexit_bpf2bpf

This commit adds a get_unaligned_access_enabled() helper and uses it in
fexit_bpf2bpf to skip subtests when bpf_prog_test_load() fails with -EACCES
and CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is not enabled.

> diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> index d2f2dcc0f2e11..1c0e93dcb3ce5 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c
[ ... ]
> @@ -76,6 +77,11 @@ static void test_fexit_bpf2bpf_common(const char *obj_file,
>  
>  	err = bpf_prog_test_load(target_obj_file, BPF_PROG_TYPE_UNSPEC,
>  			    &tgt_obj, &tgt_fd);
> +	if (err < 0 && errno == EACCES && !get_unaligned_access_enabled()) {
> +		test__skip();
> +		return;
> +	}

[Severity: Medium]
Should the packet offset alignment in test_pkt_write_access_subprog() and
new_test_pkt_write_access_subprog() be constrained instead of skipping these
subtests in test_fexit_bpf2bpf_common() and test_fmod_ret_freplace()?

The five subtests (target_yes_callees, func_replace, func_replace_multi,
fmod_ret_freplace, and func_replace_global_func) test BPF-to-BPF fexit,
freplace, and fmod_ret rather than unaligned packet access.

In tools/testing/selftests/bpf/progs/test_pkt_access.c, the caller
test_pkt_access() passes a 2-byte-aligned packet offset (void *)tcp - data.
However, because test_pkt_write_access_subprog() is a global function, the
verifier checks it standalone with an arbitrary scalar off bounded only by
off <= 54:

tools/testing/selftests/bpf/progs/test_pkt_access.c:test_pkt_write_access_subprog() {
	...
	if (off > sizeof(struct ethhdr) + sizeof(struct ipv6hdr))
		return -1;

	tcp = data + off;
	if (tcp + 1 > data_end)
		return -1;
	/* make modification to the packet data */
	tcp->check++;
	...
}

And the replacement program in
tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c has the same pattern in
new_test_pkt_write_access_subprog():

tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c:new_test_pkt_write_access_subprog() {
	...
	if (off > sizeof(struct ethhdr) + sizeof(struct ipv6hdr))
		return -1;

	tcp = data + off;
	if (tcp + 1 > data_end)
		return -1;

	/* make modifications to the packet data */
	tcp->check++;
	...
}

Skipping these tests in fexit_bpf2bpf drops fexit and freplace coverage on
strict-alignment architectures while leaving the other selftests that load
test_pkt_access.bpf.o (pkt_access, dynptr, kfree_skb, and prog_run_opts)
failing with the same -EACCES verifier error.

Would constraining the alignment of off (or performing a 1-byte modification)
in test_pkt_write_access_subprog() and new_test_pkt_write_access_subprog()
allow all of these tests to run on strict-alignment systems?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/unpriv_helpers.c b/tools/testing/selftests/bpf/unpriv_helpers.c
> index c8dd5d8485841..54ce342b3fe41 100644
> --- a/tools/testing/selftests/bpf/unpriv_helpers.c
> +++ b/tools/testing/selftests/bpf/unpriv_helpers.c
> @@ -161,3 +161,8 @@ bool get_kasan_multi_shot_enabled(void)
>  {
>  	return cmdline_contains("kasan_multi_shot");
>  }
> +
> +bool get_unaligned_access_enabled(void)
> +{
> +	return config_contains("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y") == 1;
> +}

[Severity: Medium]
Can get_unaligned_access_enabled() cause test_fexit_bpf2bpf_common() and
test_fmod_ret_freplace() to mask genuine -EACCES verifier regressions as
skips?

In tools/testing/selftests/bpf/unpriv_helpers.c, open_config() returns NULL
and config_contains() returns -1 when neither /boot/config-$(uname -r) nor
/proc/config.gz is available in the test environment. Because
get_unaligned_access_enabled() checks for equality with 1, a -1 error return
is treated the same as CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS being disabled,
even on architectures where it is enabled.

In addition, EACCES is the generic error code returned by the BPF verifier for
most verification failures, and test_fexit_bpf2bpf_common() also loads target
objects that do not perform unaligned packet accesses
(./test_pkt_md_access.bpf.o, ./connect4_prog.bpf.o, and
./test_global_func7.bpf.o). Whenever get_unaligned_access_enabled() returns
false, wouldn't any verifier regression returning -EACCES in those programs be
silently reported as skipped instead of failing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102081230.7063-1-yangtiezhu@loongson.cn?part=2