Re: [PATCH bpf-next v3 1/4] bpf: Add support for verifier warning messages

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Unused static function `warn()` breaks the build and bisection.
- [High] Disabling log reset completely after a warning leads to log buffer explosion and overwrites the warning when combined with `BPF_LOG_LEVEL1`.
--

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

bpf: Add support for verifier warning messages

This commit adds a mode where log_level 16 can be used to receive warnings
and have an associated log buffer. It introduces a warn function and
aggregates warnings to decide whether to reset the log.

> Add a warn() macro that emits messages to log
> buffer without any restrictions, aggregate the warnings emitted, and

This isn't a bug, but the commit message states it adds a warn() macro
when a static function is actually added.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 69d75515ed3f7..8de2a4e5f5de2 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -282,6 +283,20 @@ __printf(2, 3) static void verbose(void *private_data, const char *fmt, ...)
>  	va_end(args);
>  }
>  
> +__printf(2, 3) static void warn(void *private_data, const char *fmt, ...)
> +{
> +	struct bpf_verifier_env *env = private_data;
> +	va_list args;
> +
> +	if (!bpf_verifier_warn_needed(&env->log))
> +		return;
> +
> +	va_start(args, fmt);
> +	bpf_verifier_vlog(&env->log, fmt, args);
> +	va_end(args);
> +	env->warnings = true;
> +}

Is this static function called anywhere in this patch?

Unused static functions trigger a -Wunused-function compiler warning, which
can break builds when CONFIG_WERROR is enabled.

[ ... ]

> @@ -1683,7 +1698,8 @@ static int pop_stack(struct bpf_verifier_env *env, int *prev_insn_idx,
>  		if (err)
>  			return err;
>  	}
> -	if (pop_log)
> +	/* Preserve warning output across branch explorations. */
> +	if (pop_log && !(env->warnings && bpf_verifier_warn_needed(&env->log)))
>  		bpf_vlog_reset(&env->log, head->log_pos);
>  	if (insn_idx)
>  		*insn_idx = head->insn_idx;

Can this completely disable log resets once a warning is emitted?

If a user requests both standard logging and warning logging, both verbose()
and warn() will actively write to the log buffer.

If log pruning is permanently disabled after the first warning, will the
verifier dump the state of every successfully verified instruction into the
log for all subsequent branch explorations?

Because the log buffer acts as a rotating buffer by default, could this
flood of output wrap around and overwrite the warning that was meant to be
preserved?

Would it be better to track the log position of the warning and reset to
the maximum of head->log_pos and the warning position?

[ ... ]

> @@ -18803,7 +18819,8 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
>  
>  	ret = do_check(env);
>  out:
> -	if (!ret && pop_log)
> +	if (!ret && pop_log &&
> +	    !(env->warnings && bpf_verifier_warn_needed(&env->log)))
>  		bpf_vlog_reset(&env->log, 0);
>  	free_states(env);
>  	return ret;

Could the same log buffer wrapping issue happen here when log resets are
disabled after a warning?

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