Re: [PATCH RFC bpf-next 4/6] bpf: report hot simulated callchains when 1M instructions limit is met

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing Memcg Accounting for callchain profiles
- [High] Stack buffer overflow when updating printed_subs bitmap
- [Medium] Uninitialized stack memory copied to heap in update_callchain_profile
- [Low] Log buffer truncation due to subprogram disassemblies preceding callchain summary
- [Low] Compiler warning on 32-bit architectures due to incorrect format specifier for ptrdiff_t
--

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

bpf: report hot simulated callchains when 1M instructions limit is met
    
This patch introduces a mechanism to report the hottest simulated callchains
when the BPF verifier hits the 1M instruction budget. It collects and counts
callchains during verification, printing a summary and disassembly of the
relevant subprograms to help debugging.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2b3584712ad28..8eb8f1ce17c76 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -17247,6 +17247,152 @@ static int check_indirect_jump(struct bpf_verifier_env *env, struct bpf_insn *in
>  	return INSN_IDX_UPDATED;
>  }
>  
> +static void compute_callchain(struct bpf_verifier_state *st, struct bpf_callchain *cc)
> +{
> +	int i;
> +
> +	cc->curframe = st->curframe;
> +	for (i = 0; i <= st->curframe; i++)
> +		cc->insn_idx[i] = bpf_frame_insn_idx(st, i);
> +}
> +
> +static int update_callchain_profile(struct bpf_verifier_env *env, struct bpf_verifier_state *st)
> +{
> +	struct bpf_callchain_entry *entry;
> +	struct bpf_callchain cc;
> +	u32 hash;
> +
> +	compute_callchain(st, &cc);
> +	hash = jhash2(cc.insn_idx, cc.curframe + 1, cc.curframe + 1);
> +	hash_for_each_possible(env->callchain_htab, entry, node, hash) {
> +		if (entry->cc.curframe == cc.curframe &&
> +		    !memcmp(entry->cc.insn_idx, cc.insn_idx, (cc.curframe + 1) * sizeof(u32))) {
> +			entry->count++;
> +			return 0;
> +		}
> +	}
> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);

[Severity: High]
Does this code allow an unprivileged user (or one inside a container) to
bypass memory limits?

Since the verifier allocates these callchain profiles dynamically during
verification (up to the 1M instruction budget), missing the __GFP_ACCOUNT
flag could cause large amounts of unaccounted kernel memory to be consumed,
potentially leading to an OOM condition.

> +	if (!entry)
> +		return -ENOMEM;
> +	entry->cc = cc;

[Severity: Medium]
Will this copy uninitialized stack memory to the heap?

In compute_callchain(), only cc.curframe and the active elements of the
cc.insn_idx array are initialized. The remainder of the array on the stack
remains uninitialized. Assigning the whole struct here copies those
uninitialized bytes, which will trigger KMSAN warnings.

> +	entry->count = 1;
> +	hash_add(env->callchain_htab, &entry->node, hash);
> +	return 0;
> +}

[ ... ]

> +static void print_callchain_entry(struct bpf_verifier_env *env,
> +				  struct bpf_callchain_entry *entry, int idx)
> +{
> +	struct bpf_callchain *cc = &entry->cc;
> +	const struct bpf_line_info *linfo;
> +	struct bpf_subprog_info *sub;
> +	int i, insn_idx;
> +
> +	verbose(env, "#%d most visited simulated stacktrace (visited %llu times):\n",
> +		idx, entry->count);
> +	for (i = 0; i <= cc->curframe; i++) {
> +		insn_idx = cc->insn_idx[i];
> +		sub = bpf_find_containing_subprog(env, insn_idx);
> +		linfo = bpf_find_linfo(env->prog, insn_idx);
> +		if (sub->name)
> +			verbose(env, "  %s/%d", sub->name, insn_idx);
> +		else
> +			verbose(env, "  subprog#%ld/%d", sub - env->subprog_info, insn_idx);

[Severity: Low]
Does this generate a compiler warning on 32-bit architectures?

The expression sub - env->subprog_info yields a ptrdiff_t. On 32-bit systems,
ptrdiff_t is typically int, so using %ld causes a -Wformat warning. Could this
be changed to %td?

> +		if (linfo)
> +			verbose(env, " (%s:%u)",
> +				btf_name_by_offset(env->prog->aux->btf, linfo->file_name_off),
> +				BPF_LINE_INFO_LINE_NUM(linfo->line_col));
> +		verbose(env, "\n");
> +	}
> +}
> +
> +static void disasm_subprog(struct bpf_verifier_env *env, struct bpf_subprog_info *sub)
> +{
> +	u32 i, end = (sub + 1)->start;
> +
> +	if (sub->name)
> +		verbose(env, "%s():\n", sub->name);
> +	else
> +		verbose(env, "subprog#%ld:\n", sub - env->subprog_info);

[Severity: Low]
Similar to the previous comment, does this need %td instead of %ld for the
ptrdiff_t type?

> +	env->prev_linfo = NULL;
> +	for (i = sub->start; i < end; i++) {
> +		verbose_linfo(env, i, "  ; ");
> +		verbose(env, "  %d: ", i);
> +		bpf_verbose_insn(env, &env->prog->insnsi[i]);
> +		if (bpf_is_ldimm64(&env->prog->insnsi[i]))
> +			i++;
> +	}
> +}
> +
> +/*
> + * Print several most visited simulated stack traces,
> + * and a disasembly of related subprograms.
> + */
> +static void print_hotspots(struct bpf_verifier_env *env)
> +{
> +	DECLARE_BITMAP(printed_subs, BPF_MAX_SUBPROGS) = {};

[Severity: High]
Does this code overflow the stack array when processing fake or exception
subprograms?

The verifier allocates extra slots for fake and exception subprograms, meaning
env->subprog_cnt can reach BPF_MAX_SUBPROGS + 2. Sizing the bitmap strictly
for BPF_MAX_SUBPROGS could result in out-of-bounds stack writes in the loop
below.

> +	struct bpf_callchain_entry *top[3] = {};
> +	struct bpf_callchain_entry *entry;
> +	struct bpf_subprog_info *sub;
> +	int i, j, bkt, nr_top = 0;

[ ... ]

> +	for_each_set_bit(i, printed_subs, env->subprog_cnt) {
> +		disasm_subprog(env, &env->subprog_info[i]);
> +		verbose(env, "\n");
> +	}
> +
> +	/* Print the hot callchains */
> +	for (i = 0; i < nr_top; i++) {
> +		print_callchain_entry(env, top[i], i + 1);
> +		verbose(env, "\n");
> +	}
> +}

[Severity: Low]
Could printing the disassemblies first cause the callchain summary to be
truncated?

For non-trivial BPF programs, subprograms can contain thousands of
instructions. If these are printed before the hot callchains, the
user-provided log buffer might fill up completely, preventing the actual
callchain summary from being visible.

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