Re: [PATCH v0 2/3] bpf: verifier: Simplify register sign extension with tnum_scast

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2025-01-30 at 13:23 +0200, Dimitar Kanaliev wrote:

[...]

>  static void coerce_reg_to_size_sx(struct bpf_reg_state *reg, int size)
>  {
> -	s64 init_s64_max, init_s64_min, s64_max, s64_min, u64_cval;
> -	u64 top_smax_value, top_smin_value;
> -	u64 num_bits = size * 8;
> +	u64 s = size * 8 - 1;
> +	u64 sign_mask = 1ULL << s;
> +	s64 smin_value, smax_value;
> +	u64 umax_value;
>  
> -	if (tnum_is_const(reg->var_off)) {
> -		u64_cval = reg->var_off.value;
> -		if (size == 1)
> -			reg->var_off = tnum_const((s8)u64_cval);
> -		else if (size == 2)
> -			reg->var_off = tnum_const((s16)u64_cval);
> -		else
> -			/* size == 4 */
> -			reg->var_off = tnum_const((s32)u64_cval);
> -
> -		u64_cval = reg->var_off.value;
> -		reg->smax_value = reg->smin_value = u64_cval;
> -		reg->umax_value = reg->umin_value = u64_cval;
> -		reg->s32_max_value = reg->s32_min_value = u64_cval;
> -		reg->u32_max_value = reg->u32_min_value = u64_cval;
> +	if (size >= 8)
>  		return;
> -	}
>  
> -	top_smax_value = ((u64)reg->smax_value >> num_bits) << num_bits;
> -	top_smin_value = ((u64)reg->smin_value >> num_bits) << num_bits;
> +	reg->var_off = tnum_scast(reg->var_off, size);
>  
> -	if (top_smax_value != top_smin_value)
> -		goto out;
> -
> -	/* find the s64_min and s64_min after sign extension */
> -	if (size == 1) {
> -		init_s64_max = (s8)reg->smax_value;
> -		init_s64_min = (s8)reg->smin_value;
> -	} else if (size == 2) {
> -		init_s64_max = (s16)reg->smax_value;
> -		init_s64_min = (s16)reg->smin_value;
> +	if (reg->var_off.mask & sign_mask) {
> +		smin_value = -(1LL << s);
> +		smax_value = (1LL << s) - 1;
>  	} else {
> -		init_s64_max = (s32)reg->smax_value;
> -		init_s64_min = (s32)reg->smin_value;
> +		smin_value = (s64)(reg->var_off.value);
> +		smax_value = (s64)(reg->var_off.value | reg->var_off.mask);

Note the following code in __update_reg64_bounds():

static void __update_reg64_bounds(struct bpf_reg_state *reg)
{
	/* min signed is max(sign bit) | min(other bits) */
	reg->smin_value = max_t(s64, reg->smin_value,
				reg->var_off.value | (reg->var_off.mask & S64_MIN));
	/* max signed is min(sign bit) | max(other bits) */
	reg->smax_value = min_t(s64, reg->smax_value,
				reg->var_off.value | (reg->var_off.mask & S64_MAX));
	reg->umin_value = max(reg->umin_value, reg->var_off.value);
	reg->umax_value = min(reg->umax_value,
			      reg->var_off.value | reg->var_off.mask);
}

Is it possible to set {u,s}min/{u,s}max to {U,S}64_MIN/{U,S}64_MAX and rely on
__update_reg64_bounds() for this computation?

>  	}
>  
> -	s64_max = max(init_s64_max, init_s64_min);
> -	s64_min = min(init_s64_max, init_s64_min);
> +	reg->smin_value = smin_value;
> +	reg->smax_value = smax_value;
>  
> -	/* both of s64_max/s64_min positive or negative */
> -	if ((s64_max >= 0) == (s64_min >= 0)) {
> -		reg->s32_min_value = reg->smin_value = s64_min;
> -		reg->s32_max_value = reg->smax_value = s64_max;
> -		reg->u32_min_value = reg->umin_value = s64_min;
> -		reg->u32_max_value = reg->umax_value = s64_max;
> -		reg->var_off = tnum_range(s64_min, s64_max);
> -		return;
> -	}
> +	reg->umin_value = reg->var_off.value;
> +	umax_value = reg->var_off.value | reg->var_off.mask;
> +	reg->umax_value = umax_value;
>  
> -out:
> -	set_sext64_default_val(reg, size);

After this commit the functions set_sext64_default_val() and
set_sext32_default_val() are never called.

> +	reg->s32_min_value = (s32)smin_value;
> +	reg->s32_max_value = (s32)smax_value;
> +	reg->u32_min_value = (u32)reg->umin_value;
> +	reg->u32_max_value = (u32)umax_value;
>  }

[...]