Re: [PATCH bpf-next v4 7/7] bpf/verifier: improve code after range computation recent changes.

[Eduard Zingerman <eddyz87@gmail.com>]

[...]


> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index b6344cead2e2..a6fd10b119ba 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13695,33 +13695,19 @@ static void scalar_min_max_arsh(struct bpf_reg_state *dst_reg,
>  	__update_reg_bounds(dst_reg);
>  }
>  
> -static bool is_const_reg_and_valid(const struct bpf_reg_state *reg, bool alu32,
> -				   bool *valid)
> -{
> -	s64 smin_val = reg->smin_value;
> -	s64 smax_val = reg->smax_value;
> -	u64 umin_val = reg->umin_value;
> -	u64 umax_val = reg->umax_value;
> -	s32 s32_min_val = reg->s32_min_value;
> -	s32 s32_max_val = reg->s32_max_value;
> -	u32 u32_min_val = reg->u32_min_value;
> -	u32 u32_max_val = reg->u32_max_value;
> -	bool is_const = alu32 ? tnum_subreg_is_const(reg->var_off) :
> -				tnum_is_const(reg->var_off);
> -
> +static bool is_valid_const_reg(const struct bpf_reg_state *reg, bool alu32)
> +{
>  	if (alu32) {
> -		if ((is_const &&
> -		     (s32_min_val != s32_max_val || u32_min_val != u32_max_val)) ||
> -		      s32_min_val > s32_max_val || u32_min_val > u32_max_val)
> -			*valid = false;

This check first originated in the following commit from 2018:

6f16101e6a8b ("bpf: mark dst unknown on inconsistent {s, u}bounds adjustments")

Back then it was added to handle the following program:

  0: (b7) r0 = 0
  1: (d5) if r0 s<= 0x0 goto pc+0          <---- note pc+0 here
   R0=inv0 R1=ctx(id=0,off=0,imm=0) R10=fp0
  2: (1f) r0 -= r1
   R0=inv0 R1=ctx(id=0,off=0,imm=0) R10=fp0
  verifier internal error: known but bad sbounds

Apparently, verifier visited both conditional branches for this program
deducing impossible bounds for the 'false' branch.
Nowadays is_scalar_branch_taken() should handle such situations w/o issues.
Still, I'm not sure if we want to remove this safety check.

[...]