Re: [PATCH bpf-next 1/2] bpf: Report maximum combined stack depth

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incorrect max stack depth statistics for programs using adaptive private stacks
- [Low] Misleading max stack depth reported on verification failure
--

commit 26f9ea7ef0d72a21cea912b2cbf8938c9d23389b
Author: Paul Chaignon <paul.chaignon@gmail.com>

bpf: Report maximum combined stack depth

This patch modifies the BPF verifier to report the maximum combined stack
depth in its logs alongside the per-subprogram stack depths. It introduces
a new max_stack_depth field to track the worst-case stack usage during
callgraph analysis.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5045,6 +5045,8 @@ static int check_max_stack_depth_subprog(struct bpf_verifier_env *env, int idx,
>  		}
>  	} else {
>  		depth += subprog_depth;
> +		if (depth > env->max_stack_depth)
> +			env->max_stack_depth = depth;
>  		if (depth > MAX_BPF_STACK) {
>  			total = 0;
>  			for (tmp = idx; tmp >= 0; tmp = dinfo[tmp].caller)

[ ... ]

> @@ -5185,6 +5187,8 @@ static int check_max_stack_depth(struct bpf_verifier_env *env)
>  	if (priv_stack_mode == PRIV_STACK_UNKNOWN)
>  		priv_stack_mode = bpf_enable_priv_stack(env->prog);
>  
> +	env->max_stack_depth = env->subprog_info[0].stack_depth;
> +
>  	/* All async_cb subprogs use normal kernel stack. If a particular
>  	 * subprog appears in both main prog and async_cb subtree, that

If a BPF program uses an adaptive private stack, the else block in
check_max_stack_depth_subprog() isn't executed. Since env->max_stack_depth
is unconditionally initialized here to the main subprogram's stack depth,
could this result in incorrect reporting for private stacks?

For example, if the main program uses 16 bytes and calls a subprogram
using 500 bytes on a private stack, could this cause the verifier to
incorrectly log a maximum stack depth of 16 (e.g., stack depth 16+500
max 16)?

[ ... ]

> @@ -18289,7 +18293,7 @@ static void print_verification_stats(struct bpf_verifier_env *env)
>  		verbose(env, "stack depth %d", env->subprog_info[0].stack_depth);
>  		for (i = 1; i < subprog_cnt; i++)
>  			verbose(env, "+%d", env->subprog_info[i].stack_depth);
> -		verbose(env, "\n");
> +		verbose(env, " max %d\n", env->max_stack_depth);
>  		verbose(env, "insns processed %d", env->subprog_info[0].insn_processed);
>  		for (i = 1; i < subprog_cnt; i++)
>  			if (bpf_subprog_is_global(env, i))

If verification fails early in the process (for instance, during
do_check_main()), check_max_stack_depth() might be bypassed entirely.
Because env->max_stack_depth is zero-initialized by default, would this
result in misleading log output like stack depth 32+256 max 0 when the
stats are printed for the failed run?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/05f82d1180b68e856bb0cc03a5cd86305f5b7669.1778604369.git.paul.chaignon@gmail.com?part=1