Re: [PATCH v2] LoongArch: BPF: Fix stack arguments for indirect trampolines

[bot+bpf-ci@kernel.org]

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

> LoongArch: BPF: Fix stack arguments for indirect trampolines

TL;DR: Nits only: the new store_args() comment in the LoongArch trampoline
names the wrong slots (T0/FP of traced function rather than parent RA/FP
skipped by FP + 16), and there's a stray blank line.

> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index e78eb582c300..37c51488fc8f 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
> @@ -1674,17 +1674,23 @@ int bpf_arch_text_invalidate(void *dst, size_t len)
>  	return ret;
>  }
>  
> -static void store_args(struct jit_ctx *ctx, int nr_arg_slots, int args_off)
> +static void store_args(struct jit_ctx *ctx, int nr_arg_slots, int args_off, bool is_struct_ops)
>  {
> +

This isn't a bug, but is the blank line right after the opening brace of
store_args() intended?

> +	/*
> +	 * Regular fentry trampolines need to skip the 16-byte saved T0 and FP
> +	 * slots of the traced function. Indirect struct_ops trampolines place
> +	 * incoming stack arguments directly at FP.
> +	 */
> +	int stack_args_off = is_struct_ops ? 0 : 16;

The new comment says the fentry path skips "the 16-byte saved T0 and FP
slots of the traced function".  Are those the slots that FP + 16 steps
over?

In __arch_prepare_bpf_trampoline() the fentry prologue is:

    /* RA and FP for parent function */
    emit_insn(ctx, addid, LOONGARCH_GPR_SP, LOONGARCH_GPR_SP, -16);
    emit_insn(ctx, std, LOONGARCH_GPR_RA, LOONGARCH_GPR_SP, 8);
    emit_insn(ctx, std, LOONGARCH_GPR_FP, LOONGARCH_GPR_SP, 0);
    emit_insn(ctx, addid, LOONGARCH_GPR_FP, LOONGARCH_GPR_SP, 16);

    /* RA and FP for traced function */
    emit_insn(ctx, addid, LOONGARCH_GPR_SP, LOONGARCH_GPR_SP, -stack_size);
    emit_insn(ctx, std, LOONGARCH_GPR_T0, LOONGARCH_GPR_SP, stack_size - 8);
    emit_insn(ctx, std, LOONGARCH_GPR_FP, LOONGARCH_GPR_SP, stack_size - 16);
    emit_insn(ctx, addid, LOONGARCH_GPR_FP, LOONGARCH_GPR_SP, stack_size);

If E is the SP at trampoline entry, the final FP is E - 16, so the bytes
skipped by FP + 16, [FP + 0, FP + 16), are the RA and entry FP stored by
the first block, which the code labels "RA and FP for parent function".
The frame layout diagram at the top of the function also places them at
FP + 8 "[ RA to parent func ]" and FP + 0 "[ FP of parent func ]".

The T0 and FP of the traced function are stored by the second block at
FP - 8 and FP - 16, below FP, so a positive offset from FP never reaches
them.

Could the comment name the right slots, for example "skip the 16-byte
RA/FP frame record saved for the parent function"?  The old one-line
comment ("Skip slots for T0 and FP of traced function") used the same
wording, but this patch rewrites and expands it.  The commit message's
"after accounting for the saved parent and traced-function frames" is
closer to what the code does.

[ ... ]


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