Re: [PATCH bpf-next v2 01/17] bpf: Add verifier diagnostics report helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Code and commit message mismatch regarding scratch storage implementation.
- [Low] Unused static functions break the build under -Werror.
--

commit e4ecd576e6d1cc63d9f51109086546f1b4b2955b
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Add verifier diagnostics report helpers

This commit adds a small diagnostics renderer for verifier reports and wires
it into the BPF build. It provides helpers to emit a common text structure
including a failure header and reusable report sections.

> Wrap report prose at 100 columns so Reason and Suggestion text stays readable
> without changing source or instruction gutters. Keep reusable formatting
> scratch storage in the environment-owned diagnostics object.

[Severity: Medium]
The commit message states that formatting scratch storage is kept in the
environment-owned diagnostics object. However, looking at the patch,
bpf_diag_vprint_indented() uses kvasprintf() to dynamically allocate memory
instead of using an environment-owned scratch buffer.

It looks like struct bpf_diag_scratch is introduced in a later patch in this
series ("bpf: Add source and instruction diagnostic context"). Could the
commit message be updated to match the current patch's implementation?

> diff --git a/kernel/bpf/diagnostics.c b/kernel/bpf/diagnostics.c
> new file mode 100644
> index 0000000000000..a18fd5aa395d6
> --- /dev/null
> +++ b/kernel/bpf/diagnostics.c
[ ... ]
> +static void bpf_diag_report_section(struct bpf_verifier_env *env,
> +				    const char *title)
> +{
> +	if (!bpf_diag_enabled(env))
> +		return;
> +
> +	bpf_diag_log(env, "\n%s:\n", title);
> +}
> +
> +static void bpf_diag_report_reason(struct bpf_verifier_env *env,
> +				   const char *fmt, ...)
> +{
> +	va_list args;
> +
> +	if (!bpf_diag_enabled(env))
> +		return;
> +
> +	bpf_diag_report_section(env, "Reason");
> +
> +	va_start(args, fmt);
> +	bpf_diag_vprint_indented(env, fmt, args);
> +	va_end(args);
> +}
> +
> +static void bpf_diag_report_suggestion(struct bpf_verifier_env *env,
> +				       const char *fmt, ...)
> +{

[Severity: Low]
Are bpf_diag_report_reason(), bpf_diag_report_suggestion(), and
bpf_diag_report_section() used anywhere in this specific patch?

Since they are defined as static but not called by any exported functions in
diagnostics.c in this commit, does this trigger a unused-function compiler
warning that breaks the build when CONFIG_WERROR is enabled?

While they are heavily utilized starting in the subsequent patches (e.g.,
"bpf: Report Verifier Internal errors"), would it be better to introduce them
in the patch where they are first used to ensure every individual patch in the
series compiles and preserves bisectability?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619205934.1312876-1-memxor@gmail.com?part=1