Re: [PATCH v1 2/2] LoongArch: BPF: Use beqz and bnez instructions for trampoline

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3200 bytes --]

> LoongArch: BPF: Use beqz and bnez instructions for trampoline

TL;DR: Commit message only: the claimed branch-overflow risk looks impossible,
since trampolines fit in one page and the old code behaved identically, so the
Fixes: and Cc: stable tags may not be justified.

> LoongArch: BPF: Use beqz and bnez instructions for trampoline
>
> Currently, the implementation for BPF trampoline branches does not match
> the existing technical comments. The code uses larch_insn_gen_beq, where
> the comment indicates beqz should be used.
>
> Moreover, the beq instruction belongs to the reg2i16 format which limits
> the jump offset to 16 bits (+/-128KB). After using the beqz instruction
> which belongs to the reg1i21 format, the jump range will be extended to
> 21 bits (+/-4MB). This mitigates potential branch overflow risks in the
> complex BPF trampolines.
>
> While at it, also apply the same optimization to the bne instruction by
> replacing it with bnez and adding a corresponding comment.
>
> Fixes: f9b6b41f0cf3 ("LoongArch: BPF: Add basic bpf trampoline support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>

The commit message says the new instructions "mitigate potential branch
overflow risks in the complex BPF trampolines", and the patch carries a
Fixes: tag and Cc: stable.  Can a beq/bne inside a LoongArch BPF trampoline
actually go out of range?

bpf_trampoline_update() in kernel/bpf/trampoline.c rejects any trampoline
larger than one page:

    size = arch_bpf_trampoline_size(&tr->func.model, tr->flags, tnodes,
                                    tr->func.addr);
    ...
    if (size > PAGE_SIZE) {
        err = -E2BIG;
        goto out;
    }

On LoongArch PAGE_SIZE is at most 64KB (the 64KB_2LEVEL and 64KB_3LEVEL
configs), while the beq/bne reg2i16 range is +/-128KB.  So no branch inside
a trampoline can get near that limit.

bpf_struct_ops_prepare_trampoline() puts a single node in
tnodes[BPF_TRAMP_FENTRY] and has no fmod_ret branches, so its only
conditional branch skips one call sequence.

Even if the range were exceeded, larch_insn_gen_beq() and
larch_insn_gen_bne() return INSN_BREAK, and validate_code() in
arch_prepare_bpf_trampoline() would then fail the trampoline with -EINVAL.
The old code could not silently emit a wrong branch.

The old and new instructions also test the same condition:

    larch_insn_gen_beq(LOONGARCH_GPR_A0, LOONGARCH_GPR_ZERO, offset)
        -> beq $zero, $a0
    larch_insn_gen_beqz(LOONGARCH_GPR_A0, offset)
        -> beqz $a0

and likewise bne $zero, $t1 versus bnez $t1, each replacing the same single
nop slot.  So the patch appears to change no behavior and only makes the
code match the existing "update branch with beqz" comment.

Could the changelog drop the overflow-risk wording, or describe the change
as a cleanup, and could the Fixes: and Cc: stable tags be reconsidered given
that stable-kernel-rules requires a fix for a real bug?


---
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/41407991756