diff mbox series

[bpf] riscv, bpf: Fix kfunc parameters incompatibility between bpf and riscv abi

Message ID 20240324103306.2202954-1-pulehui@huaweicloud.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf] riscv, bpf: Fix kfunc parameters incompatibility between bpf and riscv abi | expand

Checks

Context Check Description
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-24 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-VM_Test-23 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-VM_Test-22 fail Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-32 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-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-40 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-VM_Test-39 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-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-PR fail PR summary
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 8 maintainers not CCed: aou@eecs.berkeley.edu ndesaulniers@google.com yonghong.song@linux.dev morbo@google.com nathan@kernel.org paul.walmsley@sifive.com justinstitt@google.com llvm@lists.linux.dev
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 22 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Pu Lehui March 24, 2024, 10:33 a.m. UTC 
From: Pu Lehui <pulehui@huawei.com>

We encountered a failing case when running selftest in no_alu32 mode:

The failure case is `kfunc_call/kfunc_call_test4` and its source code is
like bellow:
```
long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
int kfunc_call_test4(struct __sk_buff *skb)
{
	...
	tmp = bpf_kfunc_call_test4(-3, -30, -200, -1000);
	...
}
```

And its corresponding asm code is:
```
0: r1 = -3
1: r2 = -30
2: r3 = 0xffffff38 # opcode: 18 03 00 00 38 ff ff ff 00 00 00 00 00 00 00 00
4: r4 = -1000
5: call bpf_kfunc_call_test4
```

insn 2 is parsed to ld_imm64 insn to emit 0x00000000ffffff38 imm, and
converted to int type and then send to bpf_kfunc_call_test4. But since
it is zero-extended in the bpf calling convention, riscv jit will
directly treat it as an unsigned 32-bit int value, and then fails with
the message "actual 4294966063 != expected -1234".

The reason is the incompatibility between bpf and riscv abi, that is,
bpf will do zero-extension on uint, but riscv64 requires sign-extension
on int or uint. We can solve this problem by sign extending the 32-bit
parameters in kfunc.

The issue is related to [0], and thanks to Yonghong and Alexei.

Link: https://github.com/llvm/llvm-project/pull/84874 [0]
Fixes: d40c3847b485 ("riscv, bpf: Add kfunc support for RV64")
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 arch/riscv/net/bpf_jit_comp64.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Puranjay Mohan March 24, 2024, 6:26 p.m. UTC | #1 
Pu Lehui <pulehui@huaweicloud.com> writes:

> From: Pu Lehui <pulehui@huawei.com>
>
> We encountered a failing case when running selftest in no_alu32 mode:
>
> The failure case is `kfunc_call/kfunc_call_test4` and its source code is
> like bellow:
> ```
> long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
> int kfunc_call_test4(struct __sk_buff *skb)
> {
> 	...
> 	tmp = bpf_kfunc_call_test4(-3, -30, -200, -1000);
> 	...
> }
> ```
>
> And its corresponding asm code is:
> ```
> 0: r1 = -3
> 1: r2 = -30
> 2: r3 = 0xffffff38 # opcode: 18 03 00 00 38 ff ff ff 00 00 00 00 00 00 00 00
> 4: r4 = -1000
> 5: call bpf_kfunc_call_test4
> ```
>
> insn 2 is parsed to ld_imm64 insn to emit 0x00000000ffffff38 imm, and
> converted to int type and then send to bpf_kfunc_call_test4. But since
> it is zero-extended in the bpf calling convention, riscv jit will
> directly treat it as an unsigned 32-bit int value, and then fails with
> the message "actual 4294966063 != expected -1234".
>
> The reason is the incompatibility between bpf and riscv abi, that is,
> bpf will do zero-extension on uint, but riscv64 requires sign-extension
> on int or uint. We can solve this problem by sign extending the 32-bit
> parameters in kfunc.
>
> The issue is related to [0], and thanks to Yonghong and Alexei.
>
> Link: https://github.com/llvm/llvm-project/pull/84874 [0]
> Fixes: d40c3847b485 ("riscv, bpf: Add kfunc support for RV64")
> Signed-off-by: Pu Lehui <pulehui@huawei.com>
> ---
>  arch/riscv/net/bpf_jit_comp64.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index 869e4282a2c4..e3fc39370f7d 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
> @@ -1454,6 +1454,22 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
>  		if (ret < 0)
>  			return ret;
>  
> +		if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> +			const struct btf_func_model *fm;
> +			int idx;
> +
> +			fm = bpf_jit_find_kfunc_model(ctx->prog, insn);
> +			if (!fm)
> +				return -EINVAL;
> +
> +			for (idx = 0; idx < fm->nr_args; idx++) {
> +				u8 reg = bpf_to_rv_reg(BPF_REG_1 + idx, ctx);
> +
> +				if (fm->arg_size[idx] == sizeof(int))
> +					emit_sextw(reg, reg, ctx);
> +			}
> +		}
> +
>  		ret = emit_call(addr, fixed_addr, ctx);
>  		if (ret)
>  			return ret;
> -- 
> 2.34.1

Thanks for doing this, it fixes the issue I was seeing with arena_htab
selftest after enabling arena on RISCV.

Tested-by: Puranjay Mohan <puranjay12@gmail.com>
Reviewed-by: Puranjay Mohan <puranjay12@gmail.com>
Alexei Starovoitov March 24, 2024, 6:40 p.m. UTC | #2 
On Sun, Mar 24, 2024 at 3:32 AM Pu Lehui <pulehui@huaweicloud.com> wrote:
>
> From: Pu Lehui <pulehui@huawei.com>
>
> We encountered a failing case when running selftest in no_alu32 mode:
>
> The failure case is `kfunc_call/kfunc_call_test4` and its source code is
> like bellow:
> ```
> long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
> int kfunc_call_test4(struct __sk_buff *skb)
> {
>         ...
>         tmp = bpf_kfunc_call_test4(-3, -30, -200, -1000);
>         ...
> }
> ```
>
> And its corresponding asm code is:
> ```
> 0: r1 = -3
> 1: r2 = -30
> 2: r3 = 0xffffff38 # opcode: 18 03 00 00 38 ff ff ff 00 00 00 00 00 00 00 00
> 4: r4 = -1000
> 5: call bpf_kfunc_call_test4
> ```
>
> insn 2 is parsed to ld_imm64 insn to emit 0x00000000ffffff38 imm, and
> converted to int type and then send to bpf_kfunc_call_test4. But since
> it is zero-extended in the bpf calling convention, riscv jit will
> directly treat it as an unsigned 32-bit int value, and then fails with
> the message "actual 4294966063 != expected -1234".
>
> The reason is the incompatibility between bpf and riscv abi, that is,
> bpf will do zero-extension on uint, but riscv64 requires sign-extension
> on int or uint. We can solve this problem by sign extending the 32-bit
> parameters in kfunc.
>
> The issue is related to [0], and thanks to Yonghong and Alexei.
>
> Link: https://github.com/llvm/llvm-project/pull/84874 [0]
> Fixes: d40c3847b485 ("riscv, bpf: Add kfunc support for RV64")
> Signed-off-by: Pu Lehui <pulehui@huawei.com>
> ---
>  arch/riscv/net/bpf_jit_comp64.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index 869e4282a2c4..e3fc39370f7d 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c
> @@ -1454,6 +1454,22 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
>                 if (ret < 0)
>                         return ret;
>
> +               if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> +                       const struct btf_func_model *fm;
> +                       int idx;
> +
> +                       fm = bpf_jit_find_kfunc_model(ctx->prog, insn);
> +                       if (!fm)
> +                               return -EINVAL;
> +
> +                       for (idx = 0; idx < fm->nr_args; idx++) {
> +                               u8 reg = bpf_to_rv_reg(BPF_REG_1 + idx, ctx);
> +
> +                               if (fm->arg_size[idx] == sizeof(int))
> +                                       emit_sextw(reg, reg, ctx);
> +                       }
> +               }
> +

The btf_func_model usage looks good.
Glad that no new flags were necessary, since both int and uint
need to be sign extend the existing arg_size was enough.

Since we're at it. Do we need to do zero extension of return value ?
There is
__bpf_kfunc int bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
but the selftest with it is too simple:
        return bpf_kfunc_call_test2((struct sock *)sk, 1, 2);

Could you extend this selftest with a return of large int/uint
with 31th bit set to force sign extension in native
kernel risc-v code ?
I suspect the bpf side will be confused.
Which would mean that risc-v JIT in addition to:
        if (insn->src_reg != BPF_PSEUDO_CALL)
            emit_mv(bpf_to_rv_reg(BPF_REG_0, ctx), RV_REG_A0, ctx);

need to conditionally do:
 if (fm->ret_size == sizeof(int))
   emit_zextw(bpf_to_rv_reg(BPF_REG_0, ctx),
              bpf_to_rv_reg(BPF_REG_0, ctx), ctx);
?
Pu Lehui March 25, 2024, 3:27 p.m. UTC | #3 
On 2024/3/25 2:40, Alexei Starovoitov wrote:
> On Sun, Mar 24, 2024 at 3:32 AM Pu Lehui <pulehui@huaweicloud.com> wrote:
[SNIP]
>>
>> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
>> index 869e4282a2c4..e3fc39370f7d 100644
>> --- a/arch/riscv/net/bpf_jit_comp64.c
>> +++ b/arch/riscv/net/bpf_jit_comp64.c
>> @@ -1454,6 +1454,22 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
>>                  if (ret < 0)
>>                          return ret;
>>
>> +               if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
>> +                       const struct btf_func_model *fm;
>> +                       int idx;
>> +
>> +                       fm = bpf_jit_find_kfunc_model(ctx->prog, insn);
>> +                       if (!fm)
>> +                               return -EINVAL;
>> +
>> +                       for (idx = 0; idx < fm->nr_args; idx++) {
>> +                               u8 reg = bpf_to_rv_reg(BPF_REG_1 + idx, ctx);
>> +
>> +                               if (fm->arg_size[idx] == sizeof(int))
>> +                                       emit_sextw(reg, reg, ctx);
>> +                       }
>> +               }
>> +
> 
> The btf_func_model usage looks good.
> Glad that no new flags were necessary, since both int and uint
> need to be sign extend the existing arg_size was enough.
> 
> Since we're at it. Do we need to do zero extension of return value ?
> There is
> __bpf_kfunc int bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
> but the selftest with it is too simple:
>          return bpf_kfunc_call_test2((struct sock *)sk, 1, 2); >
> Could you extend this selftest with a return of large int/uint
> with 31th bit set to force sign extension in native

Sorry for late. riscv64 will sign-extend int/uint return values. I 
thought this would be a good test, so I tried the following:
```
u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym; <-- here change int to u32
int kfunc_call_test2(struct __sk_buff *skb)
{
	long tmp;
	
	tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
	return (tmp >> 32) + tmp;
}
```
As expected, if the return value is sign-extended, the bpf program will 
return 0xfffffff1. If the return value is zero-extended, the bpf program 
will return 0xfffffff2. But in fact, riscv returns 0xfffffff2. Upon 
further discovery, it seems clang will compensate for unsigned return 
values. Curious!
for example:
```
u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym;
int kfunc_call_test2(struct __sk_buff *skb)
{
	long tmp;
	
	tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
	bpf_printk("tmp: 0x%lx", tmp);
	return (tmp >> 32) + tmp;
}
```
and the bytecode will be:
```
  0:       18 01 00 00 00 00 00 f0 00 00 00 00 00 00 00 00 r1 = 
0xf0000000 ll
  2:       b7 02 00 00 02 00 00 00 r2 = 0x2
  3:       85 10 00 00 ff ff ff ff call -0x1
  4:       bf 06 00 00 00 00 00 00 r6 = r0
  5:       bf 63 00 00 00 00 00 00 r3 = r6
  6:       67 03 00 00 20 00 00 00 r3 <<= 0x20 <-- zero extension
  7:       77 03 00 00 20 00 00 00 r3 >>= 0x20
  8:       18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x0 ll
10:       b7 02 00 00 0b 00 00 00 r2 = 0xb
11:       85 00 00 00 06 00 00 00 call 0x6
12:       bf 60 00 00 00 00 00 00 r0 = r6
13:       95 00 00 00 00 00 00 00 exit
```

another example:
```
u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym;
int kfunc_call_test2(struct __sk_buff *skb)
{
	long tmp;
	
	tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
	return (tmp >> 20) + tmp; <-- change from 32 to 20
}
```
and the bytecode will be:
```
  0:       18 01 00 00 00 00 00 f0 00 00 00 00 00 00 00 00 r1 = 
0xf0000000 ll
  2:       b7 02 00 00 02 00 00 00 r2 = 0x2
  3:       85 10 00 00 ff ff ff ff call -0x1
  4:       18 02 00 00 00 00 f0 ff 00 00 00 00 00 00 00 00 r2 = 
0xfff00000 ll <-- 32-bit truncation
  6:       bf 01 00 00 00 00 00 00 r1 = r0
  7:       5f 21 00 00 00 00 00 00 r1 &= r2
  8:       77 01 00 00 14 00 00 00 r1 >>= 0x14
  9:       0f 01 00 00 00 00 00 00 r1 += r0
10:       bf 10 00 00 00 00 00 00 r0 = r1
11:       95 00 00 00 00 00 00 00 exit
```

It is difficult to construct this test case.

> kernel risc-v code ?
> I suspect the bpf side will be confused.
> Which would mean that risc-v JIT in addition to:
>          if (insn->src_reg != BPF_PSEUDO_CALL)
>              emit_mv(bpf_to_rv_reg(BPF_REG_0, ctx), RV_REG_A0, ctx);
> 
> need to conditionally do:
>   if (fm->ret_size == sizeof(int))
>     emit_zextw(bpf_to_rv_reg(BPF_REG_0, ctx),
>                bpf_to_rv_reg(BPF_REG_0, ctx), ctx);
> ?

Agree on zero-extending int/uint return values ​​when returning from 
kfunc to bpf ctx. I will add it in next version. Thanks.
Alexei Starovoitov March 25, 2024, 6:34 p.m. UTC | #4 
On Mon, Mar 25, 2024 at 8:28 AM Pu Lehui <pulehui@huaweicloud.com> wrote:
>
>
>
> On 2024/3/25 2:40, Alexei Starovoitov wrote:
> > On Sun, Mar 24, 2024 at 3:32 AM Pu Lehui <pulehui@huaweicloud.com> wrote:
> [SNIP]
> >>
> >> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> >> index 869e4282a2c4..e3fc39370f7d 100644
> >> --- a/arch/riscv/net/bpf_jit_comp64.c
> >> +++ b/arch/riscv/net/bpf_jit_comp64.c
> >> @@ -1454,6 +1454,22 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
> >>                  if (ret < 0)
> >>                          return ret;
> >>
> >> +               if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> >> +                       const struct btf_func_model *fm;
> >> +                       int idx;
> >> +
> >> +                       fm = bpf_jit_find_kfunc_model(ctx->prog, insn);
> >> +                       if (!fm)
> >> +                               return -EINVAL;
> >> +
> >> +                       for (idx = 0; idx < fm->nr_args; idx++) {
> >> +                               u8 reg = bpf_to_rv_reg(BPF_REG_1 + idx, ctx);
> >> +
> >> +                               if (fm->arg_size[idx] == sizeof(int))
> >> +                                       emit_sextw(reg, reg, ctx);
> >> +                       }
> >> +               }
> >> +
> >
> > The btf_func_model usage looks good.
> > Glad that no new flags were necessary, since both int and uint
> > need to be sign extend the existing arg_size was enough.
> >
> > Since we're at it. Do we need to do zero extension of return value ?
> > There is
> > __bpf_kfunc int bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
> > but the selftest with it is too simple:
> >          return bpf_kfunc_call_test2((struct sock *)sk, 1, 2); >
> > Could you extend this selftest with a return of large int/uint
> > with 31th bit set to force sign extension in native
>
> Sorry for late. riscv64 will sign-extend int/uint return values. I
> thought this would be a good test, so I tried the following:
> ```
> u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym; <-- here change int to u32
> int kfunc_call_test2(struct __sk_buff *skb)
> {
>         long tmp;
>
>         tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
>         return (tmp >> 32) + tmp;
> }
> ```
> As expected, if the return value is sign-extended, the bpf program will
> return 0xfffffff1. If the return value is zero-extended, the bpf program
> will return 0xfffffff2. But in fact, riscv returns 0xfffffff2. Upon
> further discovery, it seems clang will compensate for unsigned return
> values. Curious!
> for example:
> ```
> u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym;
> int kfunc_call_test2(struct __sk_buff *skb)
> {
>         long tmp;
>
>         tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
>         bpf_printk("tmp: 0x%lx", tmp);
>         return (tmp >> 32) + tmp;
> }
> ```
> and the bytecode will be:
> ```
>   0:       18 01 00 00 00 00 00 f0 00 00 00 00 00 00 00 00 r1 =
> 0xf0000000 ll
>   2:       b7 02 00 00 02 00 00 00 r2 = 0x2
>   3:       85 10 00 00 ff ff ff ff call -0x1
>   4:       bf 06 00 00 00 00 00 00 r6 = r0
>   5:       bf 63 00 00 00 00 00 00 r3 = r6
>   6:       67 03 00 00 20 00 00 00 r3 <<= 0x20 <-- zero extension
>   7:       77 03 00 00 20 00 00 00 r3 >>= 0x20
>   8:       18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x0 ll
> 10:       b7 02 00 00 0b 00 00 00 r2 = 0xb
> 11:       85 00 00 00 06 00 00 00 call 0x6
> 12:       bf 60 00 00 00 00 00 00 r0 = r6
> 13:       95 00 00 00 00 00 00 00 exit
> ```
>
> another example:
> ```
> u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym;
> int kfunc_call_test2(struct __sk_buff *skb)
> {
>         long tmp;
>
>         tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
>         return (tmp >> 20) + tmp; <-- change from 32 to 20
> }
> ```
> and the bytecode will be:
> ```
>   0:       18 01 00 00 00 00 00 f0 00 00 00 00 00 00 00 00 r1 =
> 0xf0000000 ll
>   2:       b7 02 00 00 02 00 00 00 r2 = 0x2
>   3:       85 10 00 00 ff ff ff ff call -0x1
>   4:       18 02 00 00 00 00 f0 ff 00 00 00 00 00 00 00 00 r2 =
> 0xfff00000 ll <-- 32-bit truncation
>   6:       bf 01 00 00 00 00 00 00 r1 = r0
>   7:       5f 21 00 00 00 00 00 00 r1 &= r2
>   8:       77 01 00 00 14 00 00 00 r1 >>= 0x14
>   9:       0f 01 00 00 00 00 00 00 r1 += r0
> 10:       bf 10 00 00 00 00 00 00 r0 = r1
> 11:       95 00 00 00 00 00 00 00 exit
> ```
>
> It is difficult to construct this test case.

Yeah.
I also tried a bunch of experiments with llvm and gcc-bpf.
Both compilers emit zero extension when u32 is being used as u64.

> > kernel risc-v code ?
> > I suspect the bpf side will be confused.
> > Which would mean that risc-v JIT in addition to:
> >          if (insn->src_reg != BPF_PSEUDO_CALL)
> >              emit_mv(bpf_to_rv_reg(BPF_REG_0, ctx), RV_REG_A0, ctx);
> >
> > need to conditionally do:
> >   if (fm->ret_size == sizeof(int))
> >     emit_zextw(bpf_to_rv_reg(BPF_REG_0, ctx),
> >                bpf_to_rv_reg(BPF_REG_0, ctx), ctx);
> > ?
>
> Agree on zero-extending int/uint return values when returning from
> kfunc to bpf ctx. I will add it in next version. Thanks.

Looking at existing compilers behavior it's probably unnecessary.
I think this patch is fine as-is.
I'll apply it shortly.
Pu Lehui March 26, 2024, 1:32 a.m. UTC | #5 
On 2024/3/26 2:34, Alexei Starovoitov wrote:
> On Mon, Mar 25, 2024 at 8:28 AM Pu Lehui <pulehui@huaweicloud.com> wrote:
>>
>>
>>
>> On 2024/3/25 2:40, Alexei Starovoitov wrote:
>>> On Sun, Mar 24, 2024 at 3:32 AM Pu Lehui <pulehui@huaweicloud.com> wrote:
>> [SNIP]
>>>>
>>>> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
>>>> index 869e4282a2c4..e3fc39370f7d 100644
>>>> --- a/arch/riscv/net/bpf_jit_comp64.c
>>>> +++ b/arch/riscv/net/bpf_jit_comp64.c
>>>> @@ -1454,6 +1454,22 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
>>>>                   if (ret < 0)
>>>>                           return ret;
>>>>
>>>> +               if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
>>>> +                       const struct btf_func_model *fm;
>>>> +                       int idx;
>>>> +
>>>> +                       fm = bpf_jit_find_kfunc_model(ctx->prog, insn);
>>>> +                       if (!fm)
>>>> +                               return -EINVAL;
>>>> +
>>>> +                       for (idx = 0; idx < fm->nr_args; idx++) {
>>>> +                               u8 reg = bpf_to_rv_reg(BPF_REG_1 + idx, ctx);
>>>> +
>>>> +                               if (fm->arg_size[idx] == sizeof(int))
>>>> +                                       emit_sextw(reg, reg, ctx);
>>>> +                       }
>>>> +               }
>>>> +
>>>
>>> The btf_func_model usage looks good.
>>> Glad that no new flags were necessary, since both int and uint
>>> need to be sign extend the existing arg_size was enough.
>>>
>>> Since we're at it. Do we need to do zero extension of return value ?
>>> There is
>>> __bpf_kfunc int bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
>>> but the selftest with it is too simple:
>>>           return bpf_kfunc_call_test2((struct sock *)sk, 1, 2); >
>>> Could you extend this selftest with a return of large int/uint
>>> with 31th bit set to force sign extension in native
>>
>> Sorry for late. riscv64 will sign-extend int/uint return values. I
>> thought this would be a good test, so I tried the following:
>> ```
>> u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym; <-- here change int to u32
>> int kfunc_call_test2(struct __sk_buff *skb)
>> {
>>          long tmp;
>>
>>          tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
>>          return (tmp >> 32) + tmp;
>> }
>> ```
>> As expected, if the return value is sign-extended, the bpf program will
>> return 0xfffffff1. If the return value is zero-extended, the bpf program
>> will return 0xfffffff2. But in fact, riscv returns 0xfffffff2. Upon
>> further discovery, it seems clang will compensate for unsigned return
>> values. Curious!
>> for example:
>> ```
>> u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym;
>> int kfunc_call_test2(struct __sk_buff *skb)
>> {
>>          long tmp;
>>
>>          tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
>>          bpf_printk("tmp: 0x%lx", tmp);
>>          return (tmp >> 32) + tmp;
>> }
>> ```
>> and the bytecode will be:
>> ```
>>    0:       18 01 00 00 00 00 00 f0 00 00 00 00 00 00 00 00 r1 =
>> 0xf0000000 ll
>>    2:       b7 02 00 00 02 00 00 00 r2 = 0x2
>>    3:       85 10 00 00 ff ff ff ff call -0x1
>>    4:       bf 06 00 00 00 00 00 00 r6 = r0
>>    5:       bf 63 00 00 00 00 00 00 r3 = r6
>>    6:       67 03 00 00 20 00 00 00 r3 <<= 0x20 <-- zero extension
>>    7:       77 03 00 00 20 00 00 00 r3 >>= 0x20
>>    8:       18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x0 ll
>> 10:       b7 02 00 00 0b 00 00 00 r2 = 0xb
>> 11:       85 00 00 00 06 00 00 00 call 0x6
>> 12:       bf 60 00 00 00 00 00 00 r0 = r6
>> 13:       95 00 00 00 00 00 00 00 exit
>> ```
>>
>> another example:
>> ```
>> u32 bpf_kfunc_call_test2(u32 a, u32 b) __ksym;
>> int kfunc_call_test2(struct __sk_buff *skb)
>> {
>>          long tmp;
>>
>>          tmp = bpf_kfunc_call_test2(0xfffffff0, 2);
>>          return (tmp >> 20) + tmp; <-- change from 32 to 20
>> }
>> ```
>> and the bytecode will be:
>> ```
>>    0:       18 01 00 00 00 00 00 f0 00 00 00 00 00 00 00 00 r1 =
>> 0xf0000000 ll
>>    2:       b7 02 00 00 02 00 00 00 r2 = 0x2
>>    3:       85 10 00 00 ff ff ff ff call -0x1
>>    4:       18 02 00 00 00 00 f0 ff 00 00 00 00 00 00 00 00 r2 =
>> 0xfff00000 ll <-- 32-bit truncation
>>    6:       bf 01 00 00 00 00 00 00 r1 = r0
>>    7:       5f 21 00 00 00 00 00 00 r1 &= r2
>>    8:       77 01 00 00 14 00 00 00 r1 >>= 0x14
>>    9:       0f 01 00 00 00 00 00 00 r1 += r0
>> 10:       bf 10 00 00 00 00 00 00 r0 = r1
>> 11:       95 00 00 00 00 00 00 00 exit
>> ```
>>
>> It is difficult to construct this test case.
> 
> Yeah.
> I also tried a bunch of experiments with llvm and gcc-bpf.
> Both compilers emit zero extension when u32 is being used as u64.
> 
>>> kernel risc-v code ?
>>> I suspect the bpf side will be confused.
>>> Which would mean that risc-v JIT in addition to:
>>>           if (insn->src_reg != BPF_PSEUDO_CALL)
>>>               emit_mv(bpf_to_rv_reg(BPF_REG_0, ctx), RV_REG_A0, ctx);
>>>
>>> need to conditionally do:
>>>    if (fm->ret_size == sizeof(int))
>>>      emit_zextw(bpf_to_rv_reg(BPF_REG_0, ctx),
>>>                 bpf_to_rv_reg(BPF_REG_0, ctx), ctx);
>>> ?
>>
>> Agree on zero-extending int/uint return values when returning from
>> kfunc to bpf ctx. I will add it in next version. Thanks.
> 
> Looking at existing compilers behavior it's probably unnecessary.
> I think this patch is fine as-is.
> I'll apply it shortly.

Alright, feel free to apply it. Thanks
diff mbox series

Patch

diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index 869e4282a2c4..e3fc39370f7d 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -1454,6 +1454,22 @@  int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 		if (ret < 0)
 			return ret;
 
+		if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
+			const struct btf_func_model *fm;
+			int idx;
+
+			fm = bpf_jit_find_kfunc_model(ctx->prog, insn);
+			if (!fm)
+				return -EINVAL;
+
+			for (idx = 0; idx < fm->nr_args; idx++) {
+				u8 reg = bpf_to_rv_reg(BPF_REG_1 + idx, ctx);
+
+				if (fm->arg_size[idx] == sizeof(int))
+					emit_sextw(reg, reg, ctx);
+			}
+		}
+
 		ret = emit_call(addr, fixed_addr, ctx);
 		if (ret)
 			return ret;