On Fri, 4 Sept 2026 at 13:33, <[email protected]> wrote:
>
> > diff --git a/arch/x86/net/bpf_timed_may_goto.S
> > b/arch/x86/net/bpf_timed_may_goto.S
> > index 54c690cae190..6e9d9e7c2b1d 100644
> > --- a/arch/x86/net/bpf_timed_may_goto.S
> > +++ b/arch/x86/net/bpf_timed_may_goto.S
> > @@ -11,12 +11,6 @@
> > SYM_FUNC_START(arch_bpf_timed_may_goto)
> > ANNOTATE_NOENDBR
> >
> > - /*
> > - * r10 passes us stack depth, load the pointer to count and timestamp
> > - * into r10 by adding it to BPF frame pointer.
> > - */
> > - leaq (%rbp, %r10, 1), %r10
> > -
>
> A subsystem pattern flags this as potentially concerning: This commit
> changes the x86 trampoline to the new 'pointer in BPF_REG_AX' ABI, but the
> producer side of that ABI was changed one commit earlier, in d8319a04dafc
> ("bpf: Fix timed may_goto stack pointer for private stacks"), which
> rewrote kernel/bpf/fixups.c to emit:
>
> insn_buf[4] = BPF_MOV64_REG(BPF_REG_AX, BPF_REG_FP);
> insn_buf[5] = BPF_ALU64_IMM(BPF_ADD, BPF_REG_AX, stack_off_cnt);
>
> At commit d8319a04dafc (i.e. HEAD~1), does x86 still execute
> 'leaq (%rbp, %r10, 1), %r10' on a register that already holds a resolved
> pointer, producing r10 = rbp + (frame_ptr + stack_off_cnt) -- roughly
> 2*rbp, a non-canonical address?
>
> bpf_check_timed_may_goto() then reads and writes p->timestamp / p->count
> through it (kernel/bpf/core.c:3410). Would any BPF program containing
> may_goto oops on x86_64 at that commit?
>
> The same window exists for arm64, riscv, powerpc64 and s390, whose
> trampolines are only converted in the four later commits d35642770d98,
> 7053db7d3c0d, 6e03227c5677 and 20356163d51e. Is the tree bisectable across
> the series?
>
> The two halves carry different Fixes: tags (this one 2fb761823ead, the
> fixups.c one e723608bf428), which invites a partial stable backport that
> reintroduces exactly this wild-pointer write. Would squashing the fixups.c
> ABI change with the arch trampoline updates, or ordering all arch updates
> before the generic change, address the bisectability concern?
>
> > /* Setup frame. */
> > pushq %rbp
> > movq %rsp, %rbp
>
I tried to follow the same pattern used when the patches were
introduced, but this is a valid concern.
Should I just squash all the patches to one then?
> The x86-only __xlated expectations for the timed may_goto expansion in
> tools/testing/selftests/bpf/progs/verifier_bpf_fastcall.c were never
> updated for the new 7-insn -> 8-insn sequence. The test expects:
>
> __xlated("7: if r12 == 0x0 goto pc+6")
> __xlated("8: r12 -= 1")
> __xlated("9: if r12 != 0x0 goto pc+2")
> __xlated("10: r12 = -24")
> __xlated("11: call unknown")
> __xlated("12: *(u64 *)(r10 -24) = r12")
>
I've fixed this in the selftests patch.
> Does this test fail on x86_64 for the whole series? Every line from index
> 7 onward is now wrong: the first branch becomes 'goto pc+7' (insn->off + 6
> instead of insn->off + 5), the second becomes 'goto pc+3' instead of
> 'pc+2', 'r12 = -24' is replaced by the pair 'r12 = r10' / 'r12 += -24',
> and the trailing indices shift by one ('call unknown' at 12, the store at
> 13, then 14 and 15).
>
> git log 28bd77eb835b..8dc5c7d5340d shows the only selftest commit in the
> series is 8dc5c7d5340d ("selftests/bpf: Test timed may_goto with private
> stacks"), which adds new coverage but does not touch
> verifier_bpf_fastcall.c. Note the original x86 JIT enablement commit
> 2fb761823ead -- the commit this patch lists in its Fixes: tag -- adjusted
> this very test for the same reason, so the update belongs with the ABI
> change.
>
> > commit 28bd77eb835bc1a95190315e93afbc6bb186b9ff
> > Author: Siddharth Chintamaneni <[email protected]>
> >
> > bpf, x86: Use resolved pointer for timed may_goto
> >
> > The timed may_goto fixup now passes the resolved counter pointer through
> > BPF_REG_AX instead of a stack offset.
> >
> > Use the pointer directly rather than adding it to RBP. This preserves
> > the
> > private-stack address selected by the JIT through R9.
> >
> > Fixes: 2fb761823ead ("bpf, x86: Add x86 JIT support for timed may_goto")
> ^^^^^^^^^^^
>
> The existing Fixes: tag points to commit 2fb761823ead, which is not an
> ancestor of this fix and is on a different branch (bpf-next/res_spin_lock).
>
I'll fix this in the next iteration
> The bug was actually introduced by commit 47ab38f3f4d9 in this branch's
> history, which added the incorrect code 'leaq (%rbp, %r10, 1), %r10' that
> overwrites r10 instead of preserving it.
>
> Should this use:
>
> Fixes: 47ab38f3f4d9 ("adding ci files")
>
>
> ---
> 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/33914557238
