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

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

[Tiezhu Yang]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

LoongArch passes arguments beyond a0-a7 at the caller stack pointer. The
trampoline store_args() helper always reads those arguments at FP + 16,
which is correct for an fentry trampoline: its prologue leaves FP 16 bytes
below the stack pointer at trampoline entry after accounting for the saved
parent and traced-function frames.

A struct_ops indirect trampoline is entered through a function pointer and
only saves its own RA and FP before setting FP to the entry stack pointer.
Its stack arguments therefore start at FP, not FP + 16. As a result, every
stack-passed struct_ops argument is currently read two slots late.

Select the source offset based on whether the trampoline is indirect. This
also prepares the stack-passed arena argument path to consume the actual
pointer slot.

With this patch, the BPF selftest "./test_progs -t struct_ops_multi_args"
passed on LoongArch.

Fixes: c9ebe2016de9 ("LoongArch: BPF: Support up to 12 function arguments for trampoline")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
Here is the original link:
https://lore.kernel.org/bpf/20260821233516.3426127-8-memxor@gmail.com/

This version is based on the series:
https://lore.kernel.org/loongarch/20261016081006.26069-1-yangtiezhu@loongson.cn/

 arch/loongarch/net/bpf_jit.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
index 1a7c8effc37b..03c59956c0b7 100644
--- a/arch/loongarch/net/bpf_jit.c
+++ b/arch/loongarch/net/bpf_jit.c
@@ -1725,17 +1725,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)
 {
+
+	/*
+	 * 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;
 	int i;
 
 	for (i = 0; i < nr_arg_slots; i++) {
 		if (i < LOONGARCH_MAX_REG_ARGS)
 			emit_insn(ctx, std, LOONGARCH_GPR_A0 + i, LOONGARCH_GPR_FP, -args_off);
 		else {
-			/* Skip slots for T0 and FP of traced function */
 			emit_insn(ctx, ldd, LOONGARCH_GPR_T1, LOONGARCH_GPR_FP,
-				  16 + (i - LOONGARCH_MAX_REG_ARGS) * 8);
+				  stack_args_off + (i - LOONGARCH_MAX_REG_ARGS) * 8);
 			emit_insn(ctx, std, LOONGARCH_GPR_T1, LOONGARCH_GPR_FP, -args_off);
 		}
 		args_off -= 8;
@@ -2068,7 +2074,7 @@ static int __arch_prepare_bpf_trampoline(struct jit_ctx *ctx, struct bpf_tramp_i
 	func_meta = nr_arg_slots;
 	emit_store_stack_imm64(ctx, LOONGARCH_GPR_T1, -func_meta_off, func_meta);
 
-	store_args(ctx, nr_arg_slots, args_off);
+	store_args(ctx, nr_arg_slots, args_off, is_struct_ops);
 
 	if (bpf_fsession_cnt(tnodes)) {
 		/* clear all session cookies' value */
-- 
2.42.0

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

[bot+bpf-ci]

[-- 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