Re: [PATCH RFC bpf-next 5/6] bpf: report register diff summary for hot callchains

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Misuse of the `list_for_each` macro in `bpf_sample_state_diffs()` causes out-of-bounds reads and an eventual kernel panic.
- [Medium] Early returns in `states_equal()` and `func_states_equal()` fail to populate the `bpf_state_diff` output parameter, misleadingly attributing differences to register R0.

Pre-existing issues:
- [High] The `printed_subs` bitmap in `print_hotspots()` is undersized, which can lead to a stack buffer overflow.
--

commit d10e1bb2745385ee84a9743a4009ae6e95842d64
Author: Eduard Zingerman <eddyz87@gmail.com>

bpf: report register diff summary for hot callchains

This commit modifies the verifier to report a summary of register differences
for hot callchains ending at iter_next, may_goto, or callback-calling
instructions. It does this by taking the most frequently varying register
or stack locations from explored states.

> diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c
> index 8773381360095..17b48d9f0c105 100644
> --- a/kernel/bpf/states.c
> +++ b/kernel/bpf/states.c
[ ... ]
> @@ -970,7 +1004,8 @@ static void reset_idmap_scratch(struct bpf_verifier_env *env)
>  static bool states_equal(struct bpf_verifier_env *env,
>  			 struct bpf_verifier_state *old,
>  			 struct bpf_verifier_state *cur,
> -			 enum exact_level exact)
> +			 enum exact_level exact,
> +			 struct bpf_state_diff *diff)
>  {
>  	u32 insn_idx;
>  	int i;

[Severity: Medium]
Since diff is initialized to zero by the caller, will early returns in
states_equal() and func_states_equal() (such as mismatches in in_sleepable,
callback_depth, or refsafe()) fail to update the diff parameter?

Would this result in falsely attributing state mismatches to register R0
and producing misleading profiling statistics?

[ ... ]
> @@ -1588,3 +1627,94 @@ int bpf_is_state_visited(struct bpf_verifier_env *env, int insn_idx)
[ ... ]
> +int bpf_sample_state_diffs(struct bpf_verifier_env *env,
> +			   struct bpf_callchain *cc,
> +			   struct bpf_state_diff *top_diffs,
> +			   int *nr_diffs)
> +{
[ ... ]
> +	list_for_each(pos_i, head) {
> +		sl_i = container_of(pos_i, struct bpf_verifier_state_list, node);
> +		if (!callchain_matches_state(cc, &sl_i->state))
> +			continue;
> +		list_for_each(pos_j, pos_i) {
> +			struct bpf_state_diff diff = {};
> +
> +			sl_j = container_of(pos_j, struct bpf_verifier_state_list, node);
> +			if (!callchain_matches_state(cc, &sl_j->state))

[Severity: High]
Does this nested loop traversal result in processing the dummy list head?

Because list_for_each() iterates until the pointer wraps around to its
start point (in this case, pos_i), the traversal will inevitably evaluate
the dummy head (&env->explored_states[hash_idx]).

When container_of() computes sl_j from this dummy head, won't the
subsequent callchain_matches_state() dereference an invalid pointer and
cause an out-of-bounds read?

> +				continue;
> +			if (states_equal(env, &sl_i->state, &sl_j->state, NOT_EXACT, &diff))
> +				continue;
[ ... ]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8eb8f1ce17c76..7a84eeae395fa 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -17340,16 +17369,16 @@ static void disasm_subprog(struct bpf_verifier_env *env, struct bpf_subprog_info
>   * Print several most visited simulated stack traces,
>   * and a disasembly of related subprograms.
>   */
> -static void print_hotspots(struct bpf_verifier_env *env)
> +static int print_hotspots(struct bpf_verifier_env *env)
>  {
>  	DECLARE_BITMAP(printed_subs, BPF_MAX_SUBPROGS) = {};

[Severity: High]
This is a pre-existing issue, but could the printed_subs bitmap be too small
here?

The subprog_info array in the verifier environment contains BPF_MAX_SUBPROGS
+ 2 elements to account for fake and exception subprograms.

If a recorded hot callchain includes an exception subprogram, will the offset
sub - env->subprog_info evaluate to an index greater than or equal to
BPF_MAX_SUBPROGS, causing a stack buffer overflow when __set_bit() is
called?

>  	struct bpf_callchain_entry *top[3] = {};
>  	struct bpf_callchain_entry *entry;
>  	struct bpf_subprog_info *sub;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-better-1m-reporting-v1-0-51e4f2c59780@gmail.com?part=5