Re: [PATCH RFC bpf-next 4/7] bpf: Improve 64bits bounds refinement from u32 bounds

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Medium] The test framework simulator `prog_tests/reg_bounds.c` introduces unconditional early returns, preventing simultaneous refinement of boundaries and causing divergence from the verifier.
--

commit 12b1f0ea2a161fbaa54af860eb0602fc32004fbd
Author: Paul Chaignon <paul.chaignon@gmail.com>

bpf: Improve 64bits bounds refinement from u32 bounds

This commit improves the refinement of 64-bit bounds from u32 bounds in the
BPF verifier, tightening the umin64/umax64 and smin64/smax64 values when
their lower 32 bits fall outside the known u32 range. It also updates the
selftests bounding simulator to match this logic.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1d5d2b7e5140b..bcf8a38a4d720 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -2273,17 +2273,51 @@ static void deduce_bounds_64_from_32(struct bpf_reg_state *reg)
[ ... ]
> +	umin_32lower = reg->umin_value & 0xffffffffULL;
> +	if (umin_32lower < reg->u32_min_value || umin_32lower > reg->u32_max_value) {
> +		new_umin = (reg->umin_value & ~0xffffffffULL) | reg->u32_min_value;
[ ... ]
> +		reg->umin_value = new_umin;
> +	}
> +	umax_32lower = reg->umax_value & 0xffffffffULL;
> +	if (umax_32lower < reg->u32_min_value || umax_32lower > reg->u32_max_value) {
> +		new_umax = (reg->umax_value & ~0xffffffffULL) | reg->u32_max_value;
[ ... ]
> +		reg->umax_value = new_umax;
> +	}

[ ... ]

> diff --git a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
> index c0b3a357a0f4c..d38550fc16240 100644
> --- a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
> +++ b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
[ ... ]
> @@ -518,8 +518,8 @@ static struct range range_refine(enum num_t x_t, struct range x, enum num_t y_t,
>  			 */
>  			if (xmin_swap < x.a)
>  				xmin_swap += 0x100000000;
> -			if (xmin_swap == x.b)
> -				return range(x_t, x.b, x.b);
> +			if (xmin_swap != x.a)
> +				return range(x_t, xmin_swap, x.b);
>  		} else if (xmax_lower32 < y.a || xmax_lower32 > y.b) {
>  			/* Same for the umax64, but we want to *decrease*
>  			 * umax64 to the *maximum* value that matches the u32
> @@ -528,8 +528,8 @@ static struct range range_refine(enum num_t x_t, struct range x, enum num_t y_t,
>  			xmax_swap = swap_low32(x.b, y.b);
>  			if (xmax_swap > x.b)
>  				xmax_swap -= 0x100000000;
> -			if (xmax_swap == x.a)
> -				return range(x_t, x.a, x.a);
> +			if (xmax_swap != x.b)
> +				return range(x_t, x.a, xmax_swap);
>  		}
>  	}

Since the actual verifier logic in deduce_bounds_64_from_32() evaluates and
updates umin and umax sequentially, should the simulation logic here allow
both bounds to be refined simultaneously?

By using an "else if" and immediately returning when the lower bound is
refined, won't the simulator skip upper bound refinement entirely and produce
looser expected bounds than the verifier's actual output?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1776344897.git.paul.chaignon@gmail.com?part=4