Re: [PATCH bpf-next v2 05/13] bpf: keep the range across a sign extension that cannot change it

[bot+bpf-ci]

> bpf: keep the range across a sign extension that cannot change it
>
> coerce_reg_to_size_sx() decides whether a sign extension is lossless by
> comparing the bits above the field in smin and smax:
>
>         top_smax_value = ((u64)reg_smax(reg) >> num_bits) << num_bits;
>         top_smin_value = ((u64)reg_smin(reg) >> num_bits) << num_bits;
>         if (top_smax_value != top_smin_value)
>                 goto out;
>
> Equal high bits do imply the truncation is lossless, but the converse does
> not hold. Whenever the range straddles zero the high bits necessarily
> differ -- smin sign-extends to all ones, smax to all zeroes -- even when
> every value in the range fits the field and (sN)v == v throughout. The
> second gate, "both of s64_max/s64_min positive or negative", rejects the
> same shape again for the same reason.
>
> So a register holding an errno-or-zero value, [-4095, 0], comes out of r0
> = (s32)r0 as the full [S32_MIN, S32_MAX] even though the instruction is a
> no-op on it. The no_sext test at the call site does not help: it is an
> unsigned check, so it only covers non-negative values that fit.
>
> Test the range against the field directly and return early when it fits.
> Sign extension is then the identity, so nothing needs updating -- which
> also preserves var_off, where the existing path would have replaced known
> bits with a coarse tnum_range().
>
> This only tightens: the early return fires exactly where the value is
> provably unchanged, and the cases the current tests do accept still take
> the same path and produce the same bounds.

Does the last sentence accurately describe the change? The early return
is placed before the top_s*_value and sign gates, so it intercepts every
fitting range, including those the existing gates accept.

For example, with 'w0 &= 0x3c; r0 = (s8)r0', the register holds [0, 60]
with var_off = (0; 0x3c). The old path passed both gates (equal top bits,
both non-negative) and set var_off = tnum_range(0, 60) = TNUM(0, 0x3f),
losing the known low bits. The early return now keeps (0; 0x3c).

This is the var_off preservation the message describes two sentences
earlier, so "produce the same bounds" seems inaccurate. Would "produce
bounds no wider than before" be more precise, given that the accepted
cases are where the behaviour actually changes?

>
> Signed-off-by: Vineet Gupta <vineet.gupta@xxxxxxxxx>

This looks like a bug fix for sign extension handling in the verifier.
Should this include:

  Fixes: 1f9a1ea821ff ("bpf: Support new sign-extension load insns")

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 58e788f53ae5..eb093194e2a3 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5754,6 +5754,7 @@ static void set_sext64_default_val(struct bpf_reg_state *reg, int size)
>  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;
> +	s64 field_smin, field_smax;
>  	u64 top_smax_value, top_smin_value;
>  	u64 num_bits = size * 8;
>
> @@ -5773,6 +5774,27 @@ static void coerce_reg_to_size_sx(struct bpf_reg_state *reg, int size)
>  		return;
>  	}
>
> +	if (size == 1) {
> +		field_smin = S8_MIN;
> +		field_smax = S8_MAX;
> +	} else if (size == 2) {
> +		field_smin = S16_MIN;
> +		field_smax = S16_MAX;
> +	} else {
> +		/* size == 4 */
> +		field_smin = S32_MIN;
> +		field_smax = S32_MAX;
> +	}
> +
> +	/*
> +	 * The range already fits the field, so (sN)v == v for every value the
> +	 * register can hold and the sign extension changes nothing. The tests
> +	 * below cannot reach this case once smin is negative: a negative smin
> +	 * and a non-negative smax never share their high bits.
> +	 */

Is the comment's condition accurate? It says the tests below cannot
reach this case once smin is negative, but the justification only covers
a negative smin together with a non-negative smax.

A fitting range that is entirely negative is reached and accepted by
the tests below. For reg_smin = -63, reg_smax = -1 with size == 1:

  top_smax_value = ((u64)-1 >> 8) << 8 = 0xffffffffffffff00
  top_smin_value = ((u64)-63 >> 8) << 8 = 0xffffffffffffff00

The first gate passes (equal top bits), and the second gate also accepts
since (s64_max >= 0) == (s64_min >= 0) holds for both negative. The same
is true for entirely non-negative fitting cases like [0, 60] with size == 1.

So the early return intercepts cases the tests below handle, which is
the var_off-preserving improvement the commit message describes. Should
the condition be "once the range straddles zero" rather than "once smin
is negative"?

> +	if (reg_smin(reg) >= field_smin && reg_smax(reg) <= field_smax)
> +		return;
> +
>  	top_smax_value = ((u64)reg_smax(reg) >> num_bits) << num_bits;
>  	top_smin_value = ((u64)reg_smin(reg) >> num_bits) << num_bits;


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34506184282