On Thu, 2024-03-28 at 23:01 -0400, Harishankar Vishwanathan wrote:
[...]
> @@ -13387,18 +13389,19 @@ static void scalar32_min_max_or(struct bpf_reg_state *dst_reg,
> */
> dst_reg->u32_min_value = max(dst_reg->u32_min_value, umin_val);
> dst_reg->u32_max_value = var32_off.value | var32_off.mask;
> - if (dst_reg->s32_min_value < 0 || smin_val < 0) {
> + if (dst_reg->s32_min_value > 0 && smin_val > 0 &&
Hello,
Could you please elaborate a bit, why do you use "> 0" not ">= 0" here?
It seems that having one of the min values as 0 shouldn't be an issue,
but maybe I miss something.
> + (s32)dst_reg->u32_min_value <= (s32)dst_reg->u32_max_value) {
> + /* ORing two positives gives a positive, so safe to cast
> + * u32 result into s32 when u32 doesn't cross sign boundary.
> + */
> + dst_reg->s32_min_value = dst_reg->u32_min_value;
> + dst_reg->s32_max_value = dst_reg->u32_max_value;
> + } else {
> /* Lose signed bounds when ORing negative numbers,
> * ain't nobody got time for that.
> */
> dst_reg->s32_min_value = S32_MIN;
> dst_reg->s32_max_value = S32_MAX;
> - } else {
> - /* ORing two positives gives a positive, so safe to
> - * cast result into s64.
> - */
> - dst_reg->s32_min_value = dst_reg->u32_min_value;
> - dst_reg->s32_max_value = dst_reg->u32_max_value;
> }
> }
[...]
> @@ -13453,10 +13457,10 @@ static void scalar32_min_max_xor(struct bpf_reg_state *dst_reg,
> /* We get both minimum and maximum from the var32_off. */
> dst_reg->u32_min_value = var32_off.value;
> dst_reg->u32_max_value = var32_off.value | var32_off.mask;
> -
> - if (dst_reg->s32_min_value >= 0 && smin_val >= 0) {
> - /* XORing two positive sign numbers gives a positive,
> - * so safe to cast u32 result into s32.
> + if (dst_reg->s32_min_value > 0 && smin_val > 0 &&
Same question here.
> + (s32)dst_reg->u32_min_value <= (s32)dst_reg->u32_max_value) {
> + /* XORing two positives gives a positive, so safe to cast
> + * u32 result into s32 when u32 doesn't cross sign boundary.
> */
> dst_reg->s32_min_value = dst_reg->u32_min_value;
> dst_reg->s32_max_value = dst_reg->u32_max_value;
[...]
