Re: [PATCH bpf-next v1 07/14] bpf, loongarch: Fix stack arguments for indirect trampolines

Re: [PATCH bpf-next v1 07/14] bpf, loongarch: Fix stack arguments for indirect trampolines

[Tiezhu Yang]

Cc: loongarch@lists.linux.dev
Cc: Hengqi Chen <hengqi.chen@gmail.com>

On 2026/8/22 上午7:35, Kumar Kartikeya Dwivedi wrote:
> 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.
> 
> Fixes: c9ebe2016de9 ("LoongArch: BPF: Support up to 12 function arguments for trampoline")
> Cc: Tiezhu Yang <yangtiezhu@loongson.cn>
> Cc: Huacai Chen <chenhuacai@kernel.org>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>   arch/loongarch/net/bpf_jit.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index 29c281bef28e..d193293a0fd2 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
> @@ -1662,17 +1662,18 @@ 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)
>   {
> +	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 */
> +			/* Skip the saved T0 and FP slots for a 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;
> @@ -1995,7 +1996,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 */

According to the suggestion from bot+bpf-ci, adding the following
comment before "int stack_args_off = is_struct_ops ? 0 : 16;" and
removing the pre-existing comment inside the else branch would be
much better:

/*
  * 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.
  */

This clarifies both frame layouts without causing any ambiguity
inside the loop.

Other than that, the code looks good to me:

Acked-by: Tiezhu Yang <yangtiezhu@loongson.cn>

By the way, I wrote a selftest for this patch:

   selftests/bpf: Add test for indirect struct_ops trampoline
 
https://lore.kernel.org/bpf/20260828042715.16546-1-yangtiezhu@loongson.cn/

Without this patch:

   $ sudo ./test_progs -t struct_ops_trampoline
   ...
   run_struct_ops_trampoline:FAIL:check_stack_passed_arg9 unexpected 
check_stack_passed_arg9: actual 69570864 != expected 9999
   #471     struct_ops_trampoline:FAIL
   Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED

With this patch:

   $ sudo ./test_progs -t struct_ops_trampoline
   #471     struct_ops_trampoline:OK
   Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED

Tested-by: Tiezhu Yang <yangtiezhu@loongson.cn>

Thanks,
Tiezhu

Re: [PATCH bpf-next v1 07/14] bpf, loongarch: Fix stack arguments for indirect trampolines

[Kumar Kartikeya Dwivedi]

On Fri Aug 28, 2026 at 6:33 AM CEST, Tiezhu Yang wrote:
> Cc: loongarch@lists.linux.dev
> Cc: Hengqi Chen <hengqi.chen@gmail.com>
>
> On 2026/8/22 上午7:35, Kumar Kartikeya Dwivedi wrote:
>> 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.
>>
>> Fixes: c9ebe2016de9 ("LoongArch: BPF: Support up to 12 function arguments for trampoline")
>> Cc: Tiezhu Yang <yangtiezhu@loongson.cn>
>> Cc: Huacai Chen <chenhuacai@kernel.org>
>> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>> ---
>>   arch/loongarch/net/bpf_jit.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
>> index 29c281bef28e..d193293a0fd2 100644
>> --- a/arch/loongarch/net/bpf_jit.c
>> +++ b/arch/loongarch/net/bpf_jit.c
>> @@ -1662,17 +1662,18 @@ 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)
>>   {
>> +	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 */
>> +			/* Skip the saved T0 and FP slots for a 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;
>> @@ -1995,7 +1996,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 */
>
> According to the suggestion from bot+bpf-ci, adding the following
> comment before "int stack_args_off = is_struct_ops ? 0 : 16;" and
> removing the pre-existing comment inside the else branch would be
> much better:
>
> /*
>   * 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.
>   */
>
> This clarifies both frame layouts without causing any ambiguity
> inside the loop.
>
> Other than that, the code looks good to me:
>
> Acked-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>
> By the way, I wrote a selftest for this patch:
>
>    selftests/bpf: Add test for indirect struct_ops trampoline
>
> https://lore.kernel.org/bpf/20260828042715.16546-1-yangtiezhu@loongson.cn/
>
> Without this patch:
>
>    $ sudo ./test_progs -t struct_ops_trampoline
>    ...
>    run_struct_ops_trampoline:FAIL:check_stack_passed_arg9 unexpected
> check_stack_passed_arg9: actual 69570864 != expected 9999
>    #471     struct_ops_trampoline:FAIL
>    Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
>
> With this patch:
>
>    $ sudo ./test_progs -t struct_ops_trampoline
>    #471     struct_ops_trampoline:OK
>    Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
>
> Tested-by: Tiezhu Yang <yangtiezhu@loongson.cn>

Thanks a lot for the test. Please also take a look at the argument rebase
patches once you folks have more cycles. Will definitely need your help and
reviews since they are mostly vibe coded.

>
> Thanks,
> Tiezhu

Re: [PATCH bpf-next v1 07/14] bpf, loongarch: Fix stack arguments for indirect trampolines

[Tiezhu Yang]

On 2026/8/28 下午12:55, Kumar Kartikeya Dwivedi wrote:
> On Fri Aug 28, 2026 at 6:33 AM CEST, Tiezhu Yang wrote:

...

>> With this patch:
>>
>>     $ sudo ./test_progs -t struct_ops_trampoline
>>     #471     struct_ops_trampoline:OK
>>     Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
>>
>> Tested-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> 
> Thanks a lot for the test. Please also take a look at the argument rebase
> patches once you folks have more cycles. Will definitely need your help and
> reviews since they are mostly vibe coded.

Hi Kumar,

I have sent a standalone prerequisite fix for the pre-existing
issue:

LoongArch: BPF: Fix off-by-one error for insn_is_cast_user
https://lore.kernel.org/loongarch/20260828075100.24776-1-yangtiezhu@loongson.cn/

IMO, when you respin, please rebase your patches 07, 08, and 09
on top of this fix for your v2 series.

Additionally, please send the v2 as a standalone series to the
loongarch mailing list loongarch@lists.linux.dev and the reviewer
Hengqi Chen <hengqi.chen@gmail.com> too, as the LoongArch BPF JIT
patches are typically routed through the loongarch-next tree.

https://lore.kernel.org/loongarch/CAAhV-H55YcyXRewC+0YzTHXb1jab0GkAV13EF9nFbTx=UkX4ww@mail.gmail.com/

Thanks,
Tiezhu

Re: [PATCH bpf-next v1 07/14] bpf, loongarch: Fix stack arguments for indirect trampolines

[Kumar Kartikeya Dwivedi]

On Fri Aug 28, 2026 at 10:19 AM CEST, Tiezhu Yang wrote:
> On 2026/8/28 下午12:55, Kumar Kartikeya Dwivedi wrote:
>> On Fri Aug 28, 2026 at 6:33 AM CEST, Tiezhu Yang wrote:
>
> ...
>
>>> With this patch:
>>>
>>>     $ sudo ./test_progs -t struct_ops_trampoline
>>>     #471     struct_ops_trampoline:OK
>>>     Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
>>>
>>> Tested-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>>
>> Thanks a lot for the test. Please also take a look at the argument rebase
>> patches once you folks have more cycles. Will definitely need your help and
>> reviews since they are mostly vibe coded.
>
> Hi Kumar,
>
> I have sent a standalone prerequisite fix for the pre-existing
> issue:
>
> LoongArch: BPF: Fix off-by-one error for insn_is_cast_user
> https://lore.kernel.org/loongarch/20260828075100.24776-1-yangtiezhu@loongson.cn/
>
> IMO, when you respin, please rebase your patches 07, 08, and 09
> on top of this fix for your v2 series.
>
> Additionally, please send the v2 as a standalone series to the
> loongarch mailing list loongarch@lists.linux.dev and the reviewer
> Hengqi Chen <hengqi.chen@gmail.com> too, as the LoongArch BPF JIT
> patches are typically routed through the loongarch-next tree.
>
> https://lore.kernel.org/loongarch/CAAhV-H55YcyXRewC+0YzTHXb1jab0GkAV13EF9nFbTx=UkX4ww@mail.gmail.com/
>

Ok, I will send v2 without loongarch changes and then split out and send them
separately later on (or let me know if you'd like to commandeer them, I'm happy
to do it either way).

Thanks