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

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 4/18/26 6:16 PM, Kumar Kartikeya Dwivedi wrote:
> Add a mode where log_level 16 can be used to receive warnings and have
> an associated log buffer. Add a warn() macro that emits messages to log
> buffer without any restrictions, aggregate the warnings emitted, and
> then use it to decide whether we reset the log or not.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>   Documentation/bpf/kfuncs.rst |  4 +++-
>   include/linux/bpf_verifier.h | 11 +++++++++--
>   kernel/bpf/log.c             |  2 +-
>   kernel/bpf/verifier.c        | 21 +++++++++++++++++++--
>   4 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> index 75e6c078e0e7..47d60011d8eb 100644
> --- a/Documentation/bpf/kfuncs.rst
> +++ b/Documentation/bpf/kfuncs.rst
> @@ -353,7 +353,9 @@ marked with KF_DEPRECATED should also have any relevant information
>   captured in its kernel doc. Such information typically includes the
>   kfunc's expected remaining lifespan, a recommendation for new
>   functionality that can replace it if any is available, and possibly a
> -rationale for why it is being removed.
> +rationale for why it is being removed. When verifier warning logging is
> +requested, the verifier will emit a warning when a BPF program uses a
> +deprecated kfunc.
>   
>   Note that while on some occasions, a KF_DEPRECATED kfunc may continue to be
>   supported and have its KF_DEPRECATED flag removed, it is likely to be far more
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index b148f816f25b..515afcb83ec7 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -689,15 +689,21 @@ struct bpf_verifier_log {
>   #define BPF_LOG_LEVEL2	2
>   #define BPF_LOG_STATS	4
>   #define BPF_LOG_FIXED	8
> +#define BPF_LOG_LEVEL_WARN	16
>   #define BPF_LOG_LEVEL	(BPF_LOG_LEVEL1 | BPF_LOG_LEVEL2)
> -#define BPF_LOG_MASK	(BPF_LOG_LEVEL | BPF_LOG_STATS | BPF_LOG_FIXED)
> +#define BPF_LOG_MASK	(BPF_LOG_LEVEL | BPF_LOG_STATS | BPF_LOG_FIXED | BPF_LOG_LEVEL_WARN)
>   #define BPF_LOG_KERNEL	(BPF_LOG_MASK + 1) /* kernel internal flag */
>   #define BPF_LOG_MIN_ALIGNMENT 8U
>   #define BPF_LOG_ALIGNMENT 40U
>   
>   static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
>   {
> -	return log && log->level;
> +	return log && (log->level & ~BPF_LOG_LEVEL_WARN);
> +}
> +
> +static inline bool bpf_verifier_warn_needed(const struct bpf_verifier_log *log)
> +{
> +	return log && (log->level & BPF_LOG_LEVEL_WARN);
>   }
>   

It looks like there is a little inconsistency in how we handle warnings 
vs stats and other log type, all other log types go through generic 
verbose function and have level check inlined in verifier, while 
warnings have a separate warn() func.
I think warning should either follow the existing design similar to 
other types or we should consistently refactor all log types.

>   #define BPF_MAX_SUBPROGS 256
> @@ -848,6 +854,7 @@ struct bpf_verifier_env {
>   	bool bypass_spec_v4;
>   	bool seen_direct_write;
>   	bool seen_exception;
> +	bool warnings;

nit: bool has_warnings is a bit more verbose, but I think better 
describes this variable.

>   	struct bpf_insn_aux_data *insn_aux_data; /* array of per-insn state */
>   	const struct bpf_line_info *prev_linfo;
>   	struct bpf_verifier_log log;
> diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c
> index 011e4ec25acd..2cf79fac8d43 100644
> --- a/kernel/bpf/log.c
> +++ b/kernel/bpf/log.c
> @@ -154,7 +154,7 @@ void bpf_vlog_reset(struct bpf_verifier_log *log, u64 new_pos)
>   	if (WARN_ON_ONCE(new_pos > log->end_pos))
>   		return;
>   
> -	if (!bpf_verifier_log_needed(log) || log->level == BPF_LOG_KERNEL)
> +	if (!log || log->level == 0 || log->level == BPF_LOG_KERNEL)
>   		return;
>   
>   	/* if position to which we reset is beyond current log window,
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 69d75515ed3f..8de2a4e5f5de 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -47,6 +47,7 @@ static const struct bpf_verifier_ops * const bpf_verifier_ops[] = {
>   enum bpf_features {
>   	BPF_FEAT_RDONLY_CAST_TO_VOID = 0,
>   	BPF_FEAT_STREAMS	     = 1,
> +	BPF_FEAT_VERIFIER_WARNINGS   = 2,

What is the usage scenario for feature detection for verifier warnings?
My gut feeling is that if you don't have warnings feature, you don't 
have warnings, and bitmask should just work if you pass warnings bit 
it's noop.

>   	__MAX_BPF_FEAT,
>   };
>   
> @@ -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;
> +}
> +
>   static void verbose_invalid_scalar(struct bpf_verifier_env *env,
>   				   struct bpf_reg_state *reg,
>   				   struct bpf_retval_range range, const char *ctx,
> @@ -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)))

Is it possible that env->warnings == true, but 
bpf_verifier_warn_needed() == false? As far as I see you just check if 
`!env->warnings` which better correspond to the comment.

>   		bpf_vlog_reset(&env->log, head->log_pos);
>   	if (insn_idx)
>   		*insn_idx = head->insn_idx;
> @@ -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;