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

[Vineet Gupta <vineet.gupta@linux.dev>]

On 9/10/26 11:22 PM, bot+bpf-ci@kernel.org wrote:
>> 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?

Yes this is the right phrasing.


>> Signed-off-by: Vineet Gupta <vineet.gupta@linux.dev>
> 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")

This is a limitation not really a regression - I doubt if this would be 
backported.
but up to the maintainers.

>> 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"?

Yes this needs fixing.

Thx,
-Vineet

>> +	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