diff mbox series

LoongArch: BPF: Don't override subprog's return value

Message ID 20250325141046.38347-1-hengqi.chen@gmail.com (mailing list archive)
State New
Delegated to: BPF
Headers show
Series LoongArch: BPF: Don't override subprog's return value | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-12 success Logs for aarch64-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-11 success Logs for aarch64-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for s390x-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-20 success Logs for s390x-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-17 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-17 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-50 success Logs for x86_64-llvm-18 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-51 success Logs for x86_64-llvm-18 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-gcc / veristat-kernel / x86_64-gcc veristat_kernel
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-gcc / veristat-meta / x86_64-gcc veristat_meta
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-18 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-49 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-48 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-47 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc

Commit Message

Hengqi Chen March 25, 2025, 2:10 p.m. UTC 
The verifier test `calls: div by 0 in subprog` triggers a panic at the
ld.bu instruction. The ld.bu insn is trying to load byte from memory
address returned by the subprog. The subprog actually set the correct
address at the a5 register (dedicated register for BPF return values).
But at commit 73c359d1d356 ("LoongArch: BPF: Sign-extend return values")
we also sign extended a5 to the a0 register (return value in LoongArch).
For function call insn, we later propagate the a0 register back to a5
register. This is right for native calls but wrong for bpf2bpf calls
which expect zero-extended return value in a5 register. So only move a0
to a5 for native calls (i.e. non-BPF_PSEUDO_CALL).

Fixes: 73c359d1d356 ("LoongArch: BPF: Sign-extend return values")
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
 arch/loongarch/net/bpf_jit.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Tiezhu Yang March 25, 2025, 4:07 p.m. UTC | #1 
On 3/25/25 22:10, Hengqi Chen wrote:
> The verifier test `calls: div by 0 in subprog` triggers a panic at the
> ld.bu instruction. The ld.bu insn is trying to load byte from memory
> address returned by the subprog. The subprog actually set the correct
> address at the a5 register (dedicated register for BPF return values).
> But at commit 73c359d1d356 ("LoongArch: BPF: Sign-extend return values")
> we also sign extended a5 to the a0 register (return value in LoongArch).
> For function call insn, we later propagate the a0 register back to a5
> register. This is right for native calls but wrong for bpf2bpf calls
> which expect zero-extended return value in a5 register. So only move a0
> to a5 for native calls (i.e. non-BPF_PSEUDO_CALL).
> 
> Fixes: 73c359d1d356 ("LoongArch: BPF: Sign-extend return values")
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
>   arch/loongarch/net/bpf_jit.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index a06bf89fed67..73ff1a657aa5 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
> @@ -907,7 +907,9 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
>   
>   		move_addr(ctx, t1, func_addr);
>   		emit_insn(ctx, jirl, LOONGARCH_GPR_RA, t1, 0);
> -		move_reg(ctx, regmap[BPF_REG_0], LOONGARCH_GPR_A0);
> +
> +		if (insn->src_reg != BPF_PSEUDO_CALL)
> +			move_reg(ctx, regmap[BPF_REG_0], LOONGARCH_GPR_A0);
>   		break;
>   
>   	/* tail call */

In my opinion, it is better to give a test case and show the test
result without and with this change.

The test case should be put in the commit message at least and then
added into tools/testing/selftests/bpf or lib/test_bpf.c if not exist.

Thanks,
Tiezhu
Tiezhu Yang March 25, 2025, 4:13 p.m. UTC | #2 
On 3/26/25 00:07, Tiezhu Yang wrote:
> On 3/25/25 22:10, Hengqi Chen wrote:
>> The verifier test `calls: div by 0 in subprog` triggers a panic at the
>> ld.bu instruction. The ld.bu insn is trying to load byte from memory
>> address returned by the subprog. The subprog actually set the correct
>> address at the a5 register (dedicated register for BPF return values).
>> But at commit 73c359d1d356 ("LoongArch: BPF: Sign-extend return values")
>> we also sign extended a5 to the a0 register (return value in LoongArch).
>> For function call insn, we later propagate the a0 register back to a5
>> register. This is right for native calls but wrong for bpf2bpf calls
>> which expect zero-extended return value in a5 register. So only move a0
>> to a5 for native calls (i.e. non-BPF_PSEUDO_CALL).
>>
>> Fixes: 73c359d1d356 ("LoongArch: BPF: Sign-extend return values")
>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>> ---
>>   arch/loongarch/net/bpf_jit.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
>> index a06bf89fed67..73ff1a657aa5 100644
>> --- a/arch/loongarch/net/bpf_jit.c
>> +++ b/arch/loongarch/net/bpf_jit.c
>> @@ -907,7 +907,9 @@ static int build_insn(const struct bpf_insn *insn, 
>> struct jit_ctx *ctx, bool ext
>>           move_addr(ctx, t1, func_addr);
>>           emit_insn(ctx, jirl, LOONGARCH_GPR_RA, t1, 0);
>> -        move_reg(ctx, regmap[BPF_REG_0], LOONGARCH_GPR_A0);
>> +
>> +        if (insn->src_reg != BPF_PSEUDO_CALL)
>> +            move_reg(ctx, regmap[BPF_REG_0], LOONGARCH_GPR_A0);
>>           break;
>>       /* tail call */
> 
> In my opinion, it is better to give a test case and show the test
> result without and with this change.
> 
> The test case should be put in the commit message at least and then
> added into tools/testing/selftests/bpf or lib/test_bpf.c if not exist.

Oh, it seems that there is a test case `calls: div by 0 in subprog`, so
it will be great to add a test case in lib/test_bpf.c to test bpf jit if
possible.

Thanks,
Tiezhu
Hengqi Chen March 27, 2025, 2:32 p.m. UTC | #3 
Hi Tiezhu,

On Wed, Mar 26, 2025 at 12:13 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
> On 3/26/25 00:07, Tiezhu Yang wrote:
> > On 3/25/25 22:10, Hengqi Chen wrote:
> >> The verifier test `calls: div by 0 in subprog` triggers a panic at the
> >> ld.bu instruction. The ld.bu insn is trying to load byte from memory
> >> address returned by the subprog. The subprog actually set the correct
> >> address at the a5 register (dedicated register for BPF return values).
> >> But at commit 73c359d1d356 ("LoongArch: BPF: Sign-extend return values")
> >> we also sign extended a5 to the a0 register (return value in LoongArch).
> >> For function call insn, we later propagate the a0 register back to a5
> >> register. This is right for native calls but wrong for bpf2bpf calls
> >> which expect zero-extended return value in a5 register. So only move a0
> >> to a5 for native calls (i.e. non-BPF_PSEUDO_CALL).
> >>
> >> Fixes: 73c359d1d356 ("LoongArch: BPF: Sign-extend return values")
> >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> >> ---
> >>   arch/loongarch/net/bpf_jit.c | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> >> index a06bf89fed67..73ff1a657aa5 100644
> >> --- a/arch/loongarch/net/bpf_jit.c
> >> +++ b/arch/loongarch/net/bpf_jit.c
> >> @@ -907,7 +907,9 @@ static int build_insn(const struct bpf_insn *insn,
> >> struct jit_ctx *ctx, bool ext
> >>           move_addr(ctx, t1, func_addr);
> >>           emit_insn(ctx, jirl, LOONGARCH_GPR_RA, t1, 0);
> >> -        move_reg(ctx, regmap[BPF_REG_0], LOONGARCH_GPR_A0);
> >> +
> >> +        if (insn->src_reg != BPF_PSEUDO_CALL)
> >> +            move_reg(ctx, regmap[BPF_REG_0], LOONGARCH_GPR_A0);
> >>           break;
> >>       /* tail call */
> >
> > In my opinion, it is better to give a test case and show the test
> > result without and with this change.
> >
> > The test case should be put in the commit message at least and then
> > added into tools/testing/selftests/bpf or lib/test_bpf.c if not exist.
>
> Oh, it seems that there is a test case `calls: div by 0 in subprog`, so
> it will be great to add a test case in lib/test_bpf.c to test bpf jit if
> possible.
>

Don't think this is necessary, the verifier test is well maintained and
part of the BPF CI.

Actually, I am not familiar with the test_bpf.ko :)

> Thanks,
> Tiezhu
>
diff mbox series

Patch

diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
index a06bf89fed67..73ff1a657aa5 100644
--- a/arch/loongarch/net/bpf_jit.c
+++ b/arch/loongarch/net/bpf_jit.c
@@ -907,7 +907,9 @@  static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
 
 		move_addr(ctx, t1, func_addr);
 		emit_insn(ctx, jirl, LOONGARCH_GPR_RA, t1, 0);
-		move_reg(ctx, regmap[BPF_REG_0], LOONGARCH_GPR_A0);
+
+		if (insn->src_reg != BPF_PSEUDO_CALL)
+			move_reg(ctx, regmap[BPF_REG_0], LOONGARCH_GPR_A0);
 		break;
 
 	/* tail call */