Eduard Zingerman <eddyz87@xxxxxxxxx> 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
