Re: [PATCH bpf-next v4 18/20] selftests/bpf: Add an end-to-end .bpf_cleanup exception test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: endianness bug in raw BPF instruction emission
- [Low] bpf: BPF Comment Style Violation
--

commit 322ad0f798ee8015c8f51c953009198fc70b2b9b
Author: Yonghong Song <yonghong.song@linux.dev>

selftests/bpf: Add an end-to-end .bpf_cleanup exception test

This commit adds an end-to-end test for exception cleanup using .bpf_cleanup
records. It defines inline assembly constructs mimicking compiler output to
evaluate how the kernel handles and optionally rejects various landing pad
combinations and malformed paths.

> diff --git a/tools/testing/selftests/bpf/prog_tests/exceptions_cleanup.c b/tools/testing/selftests/bpf/prog_tests/exceptions_cleanup.c
> new file mode 100644
> index 0000000000000..c932da7cbec16
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/exceptions_cleanup.c
> @@ -0,0 +1,121 @@
[ ... ]
> +/* A table with more records than the program has instructions: no valid one
> + * can look like that, and it is refused before the kernel allocates for it.
> + */

[Severity: Low]
Does this code adhere to the BPF comment style guidelines?

The BPF subsystem requires multi-line comments to have the opening '/*'
on its own line, with the comment text starting on the next line. In
test_cleanup_info_cnt(), the text starts on the same line as the opening.

> +static void test_cleanup_info_cnt(void)
> +{
[ ... ]
> +	/* Turned away on the count, rather than on whatever the records past
> +	 * the one below happen to hold.
> +	 */

[Severity: Low]
This isn't a bug, but there's another multi-line comment in
test_cleanup_info_cnt() starting on the same line as the opening '/*'.

> +	ASSERT_HAS_SUBSTR(log, "cleanup info has 1048576 records for 2 instructions",
> +			  "log");
> +}
> +
> +void test_exceptions_cleanup(void)
> +{
[ ... ]
> +	/* foo3 returns 2 ^ 1 == 3, so foo2 throws from its own second region;
> +	 * foo3's frame is long gone, so its pad must not run.
> +	 */

[Severity: Low]
This isn't a bug, but this multi-line comment in test_exceptions_cleanup()
also starts the text on the same line as the opening '/*'.

> +	if (test__start_subtest("throw_from_foo2"))
> +		run(skel, 2, THROW_COOKIE, PADS_FOO2_THREW);
> +
> +	exceptions_cleanup__destroy(skel);
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/exceptions_cleanup.c b/tools/testing/selftests/bpf/progs/exceptions_cleanup.c
> new file mode 100644
> index 0000000000000..a3a8c14e0db28
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/exceptions_cleanup.c
> @@ -0,0 +1,157 @@
[ ... ]
> +/* Called from foo1, which owns nothing and has no cleanup record: a call an
> + * exception may leave with no landing pad to run on the way. The throw is
> + * never taken -- @never is a global, so the verifier cannot prune it -- and
> + * the bit says the ordinary return path ran.
> + */

[Severity: Low]
This isn't a bug, but this multi-line comment before bump() in
exceptions_cleanup.c also violates the BPF comment style requirement.

> +static __used __naked __noinline void bump(void)
> +{
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/exceptions_cleanup_fail.c b/tools/testing/selftests/bpf/progs/exceptions_cleanup_fail.c
> new file mode 100644
> index 0000000000000..db6ac7d8bd6de
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/exceptions_cleanup_fail.c
> @@ -0,0 +1,662 @@
[ ... ]
> +/* 1. A subprogram that may unwind, also used as a helper callback:
> + * bpf_loop()'s own kernel frame would end the walk before it found a
> + * boundary.
> + */

[Severity: Low]
This isn't a bug, but this multi-line comment before throwing_cb() in
exceptions_cleanup_fail.c starts text on the same line as the opening '/*'.

> +static int throwing_cb(__u32 idx, void *ctx)
> +{
[ ... ]
> +	/* gotox r2, as a raw insn: the mnemonic only reached the LLVM
> +	 * assembler in llvm 22, and BPF_RAW_INSN() needs <linux/bpf.h>, which
> +	 * vmlinux.h rules out.
> +	 */
> +	".8byte 0x20d;"

[Severity: Medium]
Will this raw instruction emission work correctly on big-endian architectures?

The inline assembly in gotox_in_pad() emits a 64-bit integer literal. On a
little-endian target, 0x20d places the correct opcode bytes at the start
(offset 0). However, on a big-endian target, the 64-bit value is emitted with
leading zeros, placing 0x00 in the code byte. This corrupts the instruction
stream and will cause the BPF verifier to reject it.

> +"l0_%=:"
> +	"call bpf_unwind_resume;"
> +	"exit;"
[ ... ]
> +	/* r0 = *(u32 *)skb[0]. Spelled as a raw insn because BPF_LD_ABS()
> +	 * needs <linux/filter.h>, which this file cannot have -- vmlinux.h
> +	 * already defines the uapi enums.
> +	 */
> +	".8byte 0x20;"

[Severity: Medium]
Similarly here in ld_abs_in_pad(), does emitting 0x20 as an 8-byte literal
corrupt the instruction stream on big-endian architectures?

On big-endian targets, this will evaluate with 0x00 at the code byte instead
of BPF_LD_ABS (0x20), causing malformed instructions to be passed to the
verifier.

> +	"call bpf_unwind_resume;"
> +	"exit;"
> +	CLEANUP_REC("1b", "2b", "3b")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921210033.1715000-1-yonghong.song@linux.dev?part=18