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;
> }
[...]
