Re: [PATCH bpf] bpf: report verifier bugs as warnings

[Daniel Borkmann <daniel@iogearbox.net>]

On 04/02/2019 01:58 PM, Paul Chaignon wrote:
> Three checks for verifier bugs were introduced in commit f4d7e40 ("bpf:
> introduce function calls (verification)").  The bugs were reported as
> incorrect programs instead of kernel warnings as the present patch
> implements.
> 
> Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>

Thanks for the patch, I think these WARN_ONCE() are a bit out of context though,
meaning it lacks additional information about the program in kernel log once we
actually manage to trigger it which we'd otherwise would potentially have had with
the verbose() log. And from a program debugging pov, it makes it harder after this
patch when verification log would suggest that all is fine. Looks like we already
have a few WARN_ONCE() in verifier, they should probably be converted to verbose()
as well to be consistent. If we really want to have a kernel warn, then lets add a
helper macro verbose_and_warn(...) which will trigger a one-time warning, but keeps
the verbose log intact as well.

> ---
>  kernel/bpf/verifier.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 87221fda1321..12499e72b0d5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1145,9 +1145,9 @@ static int mark_reg_read(struct bpf_verifier_env *env,
>  		if (writes && state->live & REG_LIVE_WRITTEN)
>  			break;
>  		if (parent->live & REG_LIVE_DONE) {
> -			verbose(env, "verifier BUG type %s var_off %lld off %d\n",
> -				reg_type_str[parent->type],
> -				parent->var_off.value, parent->off);
> +			WARN_ONCE(1, "verifier bug type %s var_off %lld off %d\n",
> +				  reg_type_str[parent->type],
> +				  parent->var_off.value, parent->off);
>  			return -EFAULT;
>  		}
>  		/* ... then we depend on parent's value */
> @@ -2888,15 +2888,15 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  	target_insn = *insn_idx + insn->imm;
>  	subprog = find_subprog(env, target_insn + 1);
>  	if (subprog < 0) {
> -		verbose(env, "verifier bug. No program starts at insn %d\n",
> -			target_insn + 1);
> +		WARN_ONCE(1, "verifier bug. No program starts at insn %d\n",
> +			  target_insn + 1);
>  		return -EFAULT;
>  	}
>  
>  	caller = state->frame[state->curframe];
>  	if (state->frame[state->curframe + 1]) {
> -		verbose(env, "verifier bug. Frame %d already allocated\n",
> -			state->curframe + 1);
> +		WARN_ONCE(1, "verifier bug. Frame %d already allocated\n",
> +			  state->curframe + 1);
>  		return -EFAULT;
>  	}
>  
>