Re: [PATCH v2 bpf-next 1/6] bpf: Refactor reg_bounds_sanity_check

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

Paul Chaignon <paul.chaignon@gmail.com> writes:

> From: Harishankar Vishwanathan <harishankar.vishwanathan@gmail.com>
>
> This commit refactors reg_bounds_sanity_check to factor out the logic
> that performs the sanity check from the logic that does the reporting.
>
> Signed-off-by: Harishankar Vishwanathan <harishankar.vishwanathan@gmail.com>
> Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
> ---
>  kernel/bpf/verifier.c | 50 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 01c18f4268de..b638ab841c10 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2802,40 +2802,60 @@ static void reg_bounds_sync(struct bpf_reg_state *reg)
>  	__update_reg_bounds(reg);
>  }
>  
> -static int reg_bounds_sanity_check(struct bpf_verifier_env *env,
> -				   struct bpf_reg_state *reg, const char *ctx)
> +static bool range_bounds_violation(struct bpf_reg_state *reg)
>  {
> -	const char *msg;
> -
> -	if (reg->umin_value > reg->umax_value ||
> -	    reg->smin_value > reg->smax_value ||
> -	    reg->u32_min_value > reg->u32_max_value ||
> -	    reg->s32_min_value > reg->s32_max_value) {
> -		    msg = "range bounds violation";
> -		    goto out;
> -	}
> +	return (reg->umin_value > reg->umax_value || reg->smin_value > reg->smax_value ||
> +		reg->u32_min_value > reg->u32_max_value ||
> +		reg->s32_min_value > reg->s32_max_value);
> +}
>  
> +static bool const_tnum_out_of_sync_with_range_bounds(struct bpf_reg_state *reg)
> +{
>  	if (tnum_is_const(reg->var_off)) {
>  		u64 uval = reg->var_off.value;
>  		s64 sval = (s64)uval;
>  
>  		if (reg->umin_value != uval || reg->umax_value != uval ||
>  		    reg->smin_value != sval || reg->smax_value != sval) {
> -			msg = "const tnum out of sync with range bounds";
> -			goto out;
> +			return true;
nit: maybe it's going to look simpler if you rewrite it with early return?
static bool const_tnum_out_of_sync_with_range_bounds(struct bpf_reg_state *reg)
{
	u64 uval = reg->var_off.value;
	s64 sval = (s64)uval;

  	if (!tnum_is_const(reg->var_off))
                return false;

        return reg->umin_value != uval || reg->umax_value != uval ||
               reg->smin_value != sval || reg->smax_value != sval;
}

same principle can be applied to
const_subreg_tnum_out_of_sync_with_range_bounds(), which looks like a
very long function name, will something like
subreg_tnum_range_mismatch() capture the idea?
>  
> +static bool const_subreg_tnum_out_of_sync_with_range_bounds(struct bpf_reg_state *reg)
> +{
>  	if (tnum_subreg_is_const(reg->var_off)) {
>  		u32 uval32 = tnum_subreg(reg->var_off).value;
>  		s32 sval32 = (s32)uval32;
>  
>  		if (reg->u32_min_value != uval32 || reg->u32_max_value != uval32 ||
>  		    reg->s32_min_value != sval32 || reg->s32_max_value != sval32) {
> -			msg = "const subreg tnum out of sync with range bounds";
> -			goto out;
> +			return true;
>  		}
>  	}
> +	return false;
> +}
> +
> +static int reg_bounds_sanity_check(struct bpf_verifier_env *env,
> +				   struct bpf_reg_state *reg, const char *ctx)
> +{
> +	const char *msg;
> +
> +	if (range_bounds_violation(reg)) {
> +		msg = "range bounds violation";
> +		goto out;
> +	}
> +
> +	if (const_tnum_out_of_sync_with_range_bounds(reg)) {
> +		msg = "const tnum out of sync with range bounds";
> +		goto out;
> +	}
> +
> +	if (const_subreg_tnum_out_of_sync_with_range_bounds(reg)) {
> +		msg = "const subreg tnum out of sync with range bounds";
> +		goto out;
> +	}
Other than those few nits, the change looks good.
>  
>  	return 0;
>  out:
> -- 
> 2.43.0