Re: [PATCH bpf-next] bpf: Fix unwarranted warning on speculative path

[Luis Gerhorst <luis.gerhorst@fau.de>]

Eduard Zingerman <eddyz87@gmail.com> writes:

> On Wed, 2025-06-25 at 23:43 +0200, Paul Chaignon wrote:
>
> [...]
>
>> > So, suppose there is a program:
>> > 
>> >      15: (18) r1 = 0x2020200005642020
>> >      17: (7b) *(u64 *)(r10 -264) = r1
>> > 
>> > Insn processing sequence would look like (starting from 15):
>> > - prev_insn_idx <- 15
>> > - do_check_insn()
>> >   - env->insn_idx <- 17
>> > - prev_insn_idx <- 17
>> > - do_check_insn():
>> >   - nospec_result <- true
>> >   - env->insn_idx <- 18
>> > - state->speculative && cur_aux(env)->nospec_result == true:
>> >   - WARN_ON_ONCE(18 != 17 + 1) // no warning
>> > 
>> > What do I miss?
>> 
>> In the if condition, "cur_aux(env)" points to the aux data of the next
>> instruction (#17 here) because we incremented "insn_idx" in
>> do_check_insn(). In my fix, "insn" points to the previous instruction
>> because we retrieved it before calling do_check_insn().
>> 
>> Therefore, the processing sequence would look like:
>> - prev_insn_idx <- 15
>> - do_check_insn()
>>   - env->insn_idx <- 17
>> - state->speculative && cur_aux(env)->nospec_result == true:
>>   - WARN_ON_ONCE(17 != 15 + 1) // warning
>
> I'm sorry, I'm a bit slow today (well, maybe not only today).
> Isn't it a slightly different bug in the original check?
> Suppose current insn is ST/STX that do_check_insn() marked as
> nospec_result. I think the intent was to stop branch processing right
> at that point, as nospec instruction would be inserted after this
> store => no need to speculate further.
> In other words, cur_aux(env)->nospec_result pointing to instruction
> after ST/STX was not anticipated. (Luis, wdyt?)

That's a very good point, nospec_result should only stop the path
analysis after the insn that has it set was analyzed. Otherwise, a
nospec required before the insn may not be added.

In reply to this you find a RFC fix and test that shows a nospec might
be missing. If this makes sense I will send a polished version.

The tests fail without the fix [1] (the offset no longer matches because
the nospec before the stack-write is missing, which is the main point),
but succeed otherwise [2].

I manually verified the fix resolves the warning in the reproducer, but
I can add a test for the polished version.

[1] https://github.com/kernel-patches/bpf/actions/runs/15901586938/job/44846011518?pr=9199#step:5:11308
[2] https://github.com/kernel-patches/bpf/pull/9198