Re: [PATCH bpf-next v7 6/6] selftests/bpf: Test subprogram instruction statistics

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The expected verifier logs for `stats_async_schedule`, `stats_async_outer`, and `stats_async_nested_schedule` mandate exactly 7 instructions and `stack 0`. However, because these are standard C functions (lacking `__naked`) that use inline assembly with `__clobber_common` (which clobbers all caller-saved registers `r0-r5`), the compiler must allocate callee-saved registers (e.g., `r6`, `r7`) for their input operands. This forces the emission of prologue and epilogue stack spills, guaranteeing a non-zero stack depth and a higher instruction count. The rigid `__msg` assertions will fail at runtime.
--

--- Patch [6]: [PATCH bpf-next v7 6/6] selftests/bpf: Test subprogram instruction statistics ---
commit af1dc26c02b580a6a5ab3fdfc1f191d5895df8f8
Author: Kumar Kartikeya Dwivedi <[email protected]>

selftests/bpf: Test subprogram instruction statistics

Add small verifier programs with deterministic instruction streams to
exercise per-subprogram self and inclusive instruction accounting. Use
assembly for normal call chains and straight-line callback bodies containing
only moves, calls, and returns or exits.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_subprog_insn_stats.c b/tools/testing/selftests/bpf/progs/verifier_subprog_insn_stats.c
> new file mode 100644
> index 0000000000000..cbf83ee9f8f2f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/verifier_subprog_insn_stats.c

[ ... ]

> +__noinline __used
> +static __u64 stats_async_schedule(struct bpf_map *map, __u32 *key,
> +				  struct timer_value *value, void *ctx)
> +{
> +	asm volatile (
> +		"r1 = %[timer];"
> +		"r2 = %[stats_async_leaf];"
> +		"call %[bpf_timer_set_callback];"
> +		:
> +		: [timer] "r" (value),
> +		  __imm_ptr(stats_async_leaf),
> +		  __imm(bpf_timer_set_callback)
> +		: __clobber_common
> +	);
> +	return 0;
> +}
> +
> +SEC("?raw_tp")
> +__success __log_level(4)
> +/*
> + * self: 9 + 7 + 2 = 18
> + * totals: leaf 2, scheduler 7, main root 18
> + */
> +__msg("subprog 0 (stats_async_direct) main insns_self 9 insns_total 18 stack 0")
> +__msg("subprog {{[0-9]+}} (stats_async_schedule) static insns_self 7 "
> +      "insns_total 7 stack 0")

[Severity: Medium]
Since stats_async_schedule() is a standard C function (lacking the __naked
attribute) and the inline assembly uses __clobber_common (clobbering r0-r5),
won't the compiler be forced to allocate callee-saved registers (like r6 or r7)
for the input operands?

If the compiler emits prologue and epilogue stack spills for these registers,
it would result in a non-zero stack depth and a higher instruction count,
causing the expected "stack 0" and "insns_self 7" verifier log assertions to
fail at runtime.

[ ... ]

> +__noinline __used
> +static int stats_async_outer(void *map, __u32 *key, struct bpf_timer *timer)
> +{
> +	asm volatile (
> +		"r1 = %[timer];"
> +		"r2 = %[stats_async_nested_leaf];"
> +		"call %[bpf_timer_set_callback];"
> +		:
> +		: [timer] "r" (timer),
> +		  __imm_ptr(stats_async_nested_leaf),
> +		  __imm(bpf_timer_set_callback)
> +		: __clobber_common
> +	);
> +	return 0;
> +}
> +
> +__noinline __used
> +static __u64 stats_async_nested_schedule(struct bpf_map *map, __u32 *key,
> +					 struct timer_value *value, void *ctx)
> +{
> +	asm volatile (
> +		"r1 = %[timer];"
> +		"r2 = %[stats_async_outer];"
> +		"call %[bpf_timer_set_callback];"
> +		:
> +		: [timer] "r" (value),
> +		  __imm_ptr(stats_async_outer),
> +		  __imm(bpf_timer_set_callback)
> +		: __clobber_common
> +	);
> +	return 0;
> +}
> +
> +SEC("?raw_tp")
> +__success __log_level(4)
> +/*
> + * self: 9 + 7 + 7 + 2 = 25
> + * totals: leaf 2, outer 7, scheduler 7, main root 25
> + */
> +__msg("subprog 0 (stats_async_nested) main insns_self 9 insns_total 25 stack 0")
> +__msg("subprog {{[0-9]+}} (stats_async_nested_schedule) static insns_self 7 "
> +      "insns_total 7 stack 0")
> +__msg("subprog {{[0-9]+}} (stats_async_outer) static insns_self 7 "
> +      "insns_total 7 stack 0")

[Severity: Medium]
Will these assertions fail for the same reason?

Like stats_async_schedule(), both stats_async_outer() and 
stats_async_nested_schedule() lack __naked and use __clobber_common, which 
will likely cause compiler-emitted stack spills and exceed the expected 
"stack 0" and "insns_self 7" values.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6