Re: [PATCH bpf-next v3 19/20] selftests/bpf: Cover the exception cleanup shapes the chain does not reach

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] bpf: multi-line comments violate the modern BPF subsystem style
--

commit 31d63d33bfdfb4e044e29aa2e2717a6f909808c9
Author: Yonghong Song <yonghong.song@linux.dev>

selftests/bpf: Cover the exception cleanup shapes the chain does not reach

This commit adds tests to cover exception cleanup scenarios that the call
chain doesn't reach. It introduces new shapes like dead continuations, shared
callees, and throwing subprograms in BPF selftests.

> diff --git a/tools/testing/selftests/bpf/prog_tests/exceptions_cleanup.c b/tools/testing/selftests/bpf/prog_tests/exceptions_cleanup.c
> --- a/tools/testing/selftests/bpf/prog_tests/exceptions_cleanup.c
> +++ b/tools/testing/selftests/bpf/prog_tests/exceptions_cleanup.c
[ ... ]
> +	/* A pad in the main program's own frame, not in a subprogram. */
> +	if (test__start_subtest("main_program_pad"))
> +		run_shape(skel, skel->progs.entry_main_pad, 101, THROW_COOKIE,
> +			  RAN_MAIN_PAD);
> +
> +	/* The same call site either way: the subprogram's throw unwinds into
> +	 * this frame and runs its pad, an extension's stops at its own boundary.
> +	 */

[Severity: Low]
This isn't a bug, but does this comment in test_shapes() violate the modern
BPF subsystem multi-line comment style? The opening /* is preferred to be
on its own line.

> +	if (test__start_subtest("freplace_subprog_throws"))
> +		run_shape(skel, skel->progs.entry_freplace, 7, THROW_COOKIE,
> +			  RAN_FREPLACE);
> +	if (test__start_subtest("freplace_extension_throws"))
> +		test_freplace(skel);
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/exceptions_cleanup_shapes.c b/tools/testing/selftests/bpf/progs/exceptions_cleanup_shapes.c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/exceptions_cleanup_shapes.c
[ ... ]
> +__u64 magic = 0x5eed;
> +__u64 pads_ran = 0;
> +__u64 pad_runs = 0;
> +
> +/* 1. Everything a cleanup table leaves dead: a throw's continuation, the tail
> + * after a resume with an ld_imm64 and a branch in it, and the block only that
> + * continuation reaches.
> + */

[Severity: Low]
This isn't a bug, but does this multi-line comment above sweep_frame() follow
the preferred BPF subsystem format? The opening /* should typically reside
on its own line.

> +static __used __naked __noinline __u64 sweep_frame(void)
> +{
[ ... ]
> +SEC("syscall")
> +int entry_sweep(void *ctx)
> +{
> +	return sweep_frame();
> +}
> +
> +/* 2. A callee called from both a covered and an uncovered site: the pad
> + * belongs to the call site, not the callee. Either site can throw, and the RCU
> + * lock between them is what an unwind out of the uncovered one leaks.
> + */

[Severity: Low]
This isn't a bug, but similarly, does this comment above shared_callee() need
the opening /* placed on its own line to match the modern BPF comment style?

> +static __used __noinline __u64 shared_callee(__u64 x)
> +{
> +	if (x > 100)

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