1. Patch 1 and Patch 2 are dependent patches to resolve the BPF check error in 32-bit ARM. 2. Patch 3 supports bpf fkunc in 32-bit ARM. 3. Patch 4 is used to add test cases to cover some parameter scenarios states by AAPCS.
The following is the test_progs result in the 32-bit ARM environment:
# uname -a Linux qemuarm32 6.1.0-rc3+ #2 SMP Thu Nov 3 15:31:29 CST 2022 armv7l armv7l armv7l GNU/Linux # echo 1 > /proc/sys/net/core/bpf_jit_enable # ./test_progs -t kfunc_call #1/1 kfunc_call/kfunc_syscall_test_fail:OK #1/2 kfunc_call/kfunc_syscall_test_null_fail:OK #1/3 kfunc_call/kfunc_call_test_get_mem_fail_rdonly:OK #1/4 kfunc_call/kfunc_call_test_get_mem_fail_use_after_free:OK #1/5 kfunc_call/kfunc_call_test_get_mem_fail_oob:OK #1/6 kfunc_call/kfunc_call_test_get_mem_fail_not_const:OK #1/7 kfunc_call/kfunc_call_test_mem_acquire_fail:OK #1/8 kfunc_call/kfunc_call_test1:OK #1/9 kfunc_call/kfunc_call_test2:OK #1/10 kfunc_call/kfunc_call_test4:OK #1/11 kfunc_call/kfunc_call_test_ref_btf_id:OK #1/12 kfunc_call/kfunc_call_test_get_mem:OK #1/13 kfunc_call/kfunc_syscall_test:OK #1/14 kfunc_call/kfunc_syscall_test_null:OK #1/17 kfunc_call/destructive:OK
Yang Jihong (4): bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension bpf: Remove size check for sk in bpf_skb_is_valid_access for 32-bit architecture bpf: Add kernel function call support in 32-bit ARM bpf:selftests: Add kfunc_call test for mixing 32-bit and 64-bit parameters
arch/arm/net/bpf_jit_32.c | 130 ++++++++++++++++++ kernel/bpf/verifier.c | 3 + net/bpf/test_run.c | 6 + net/core/filter.c | 2 - .../selftests/bpf/prog_tests/kfunc_call.c | 1 + .../selftests/bpf/progs/kfunc_call_test.c | 23 ++++ 6 files changed, 163 insertions(+), 2 deletions(-)
For ARM32 architecture, if data width of kfunc return value is 32 bits, need to do explicit zero extension for high 32-bit, insn_def_regno should return dst_reg for BPF_JMP type of BPF_PSEUDO_KFUNC_CALL. Otherwise, opt_subreg_zext_lo32_rnd_hi32 returns -EFAULT, resulting in BPF failure.
Signed-off-by: Yang Jihong yangjihong1@huawei.com --- kernel/bpf/verifier.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 7f0a9f6cb889..bac37757ffca 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2404,6 +2404,9 @@ static int insn_def_regno(const struct bpf_insn *insn) { switch (BPF_CLASS(insn->code)) { case BPF_JMP: + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) + return insn->dst_reg; + fallthrough; case BPF_JMP32: case BPF_ST: return -1;
The error code -EACCES is returned when bpf prog is tested in 32-bit environment, This is because bpf_object__relocate modifies the instruction to change memory size to 4 bytes, as shown in the following messages:
libbpf: prog 'kfunc_call_test1': relo #2: matching candidate #0 <byte_off> [18342] struct __sk_buff.sk (0:30:0 @ offset 168) libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) off 168 -> 168 libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) mem_sz 8 -> 4
As a result, the bpf_skb_is_valid_access check fails. For 32-bit architecture, unnecessary checks need to be deleted.
Signed-off-by: Yang Jihong yangjihong1@huawei.com --- net/core/filter.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c index bb0136e7a8e4..eab7ce89740c 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -8269,8 +8269,6 @@ static bool bpf_skb_is_valid_access(int off, int size, enum bpf_access_type type return false; break; case offsetof(struct __sk_buff, sk): - if (type == BPF_WRITE || size != sizeof(__u64)) - return false; info->reg_type = PTR_TO_SOCK_COMMON_OR_NULL; break; case offsetof(struct __sk_buff, tstamp_type):
On Thu, Nov 03, 2022 at 05:21:16PM +0800, Yang Jihong wrote:
The error code -EACCES is returned when bpf prog is tested in 32-bit environment, This is because bpf_object__relocate modifies the instruction to change memory size to 4 bytes, as shown in the following messages:
libbpf: prog 'kfunc_call_test1': relo #2: matching candidate #0 <byte_off> [18342] struct __sk_buff.sk (0:30:0 @ offset 168) libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) off 168 -> 168 libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) mem_sz 8 -> 4
As a result, the bpf_skb_is_valid_access check fails. For 32-bit architecture, unnecessary checks need to be deleted.
Isn't the purpose of this check to ensure that the entire pointer is written, and BPF can't write half of it?
case offsetof(struct __sk_buff, sk):
if (type == BPF_WRITE || size != sizeof(__u64))return false;
Wouldn't "(size != sizeof(struct bpf_sock *) && size != sizeof(__u64))" be more appropriate here, so 32-bit can only write the 32-bit pointer or the full 64-bit value, and 64-bit can only write the 64-bit pointer? Or is there a reason not to? bpf folk?
On Thu, Nov 3, 2022 at 4:23 AM Russell King (Oracle) linux@armlinux.org.uk wrote:
On Thu, Nov 03, 2022 at 05:21:16PM +0800, Yang Jihong wrote:
The error code -EACCES is returned when bpf prog is tested in 32-bit environment, This is because bpf_object__relocate modifies the instruction to change memory size to 4 bytes, as shown in the following messages:
libbpf: prog 'kfunc_call_test1': relo #2: matching candidate #0 <byte_off> [18342] struct __sk_buff.sk (0:30:0 @ offset 168) libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) off 168 -> 168 libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) mem_sz 8 -> 4
As a result, the bpf_skb_is_valid_access check fails. For 32-bit architecture, unnecessary checks need to be deleted.
Isn't the purpose of this check to ensure that the entire pointer is written, and BPF can't write half of it?
case offsetof(struct __sk_buff, sk):
if (type == BPF_WRITE || size != sizeof(__u64))return false;Wouldn't "(size != sizeof(struct bpf_sock *) && size != sizeof(__u64))" be more appropriate here, so 32-bit can only write the 32-bit pointer or the full 64-bit value, and 64-bit can only write the 64-bit pointer? Or is there a reason not to? bpf folk?
You're correct. The patch is completely wrong. The bug is elsewhere.
On Thu, Nov 3, 2022 at 11:15 AM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
On Thu, Nov 3, 2022 at 4:23 AM Russell King (Oracle) linux@armlinux.org.uk wrote:
On Thu, Nov 03, 2022 at 05:21:16PM +0800, Yang Jihong wrote:
The error code -EACCES is returned when bpf prog is tested in 32-bit environment, This is because bpf_object__relocate modifies the instruction to change memory size to 4 bytes, as shown in the following messages:
libbpf: prog 'kfunc_call_test1': relo #2: matching candidate #0 <byte_off> [18342] struct __sk_buff.sk (0:30:0 @ offset 168) libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) off 168 -> 168 libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) mem_sz 8 -> 4
As a result, the bpf_skb_is_valid_access check fails. For 32-bit architecture, unnecessary checks need to be deleted.
Isn't the purpose of this check to ensure that the entire pointer is written, and BPF can't write half of it?
case offsetof(struct __sk_buff, sk):
if (type == BPF_WRITE || size != sizeof(__u64))return false;Wouldn't "(size != sizeof(struct bpf_sock *) && size != sizeof(__u64))" be more appropriate here, so 32-bit can only write the 32-bit pointer or the full 64-bit value, and 64-bit can only write the 64-bit pointer? Or is there a reason not to? bpf folk?
You're correct. The patch is completely wrong. The bug is elsewhere.
So I looked at this a bit (and replied to the old version of this patch). What happens in the kernel is that we expect 64-bit load but rewrite it to 32-bit load on 32-bit architectures (because we just use sizeof(struct sk_buff, sk) which is 4 bytes on 32-bit arch.
The problem here is that libbpf adjusts such pointer accesses from 8-byte read to 4-byte reads for preserve_access_index (because libbpf sees that pointer is really 4 byte long), which is what we actually want in the general case. Here the assumption was made before CO-RE that __sk_buff is a stable (and fake) UAPI and the correct BPF program will access sk as a 64-bit pointer because BPF-side pointers always appear as 64-bit.
But from a correctness standpoint I think it should be fine to enable both 32- and 64-bit loads for such pointers in __sk_buff for 32-bit host arch. This will work well with CO-RE and will be correctly rewritten to 32-bit or 64-bit accesses, depending on host architecture.
We should still reject 32-bit load on 64-bit host arch, though.
On Fri, Nov 4, 2022 at 3:43 PM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:
On Thu, Nov 3, 2022 at 11:15 AM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
On Thu, Nov 3, 2022 at 4:23 AM Russell King (Oracle) linux@armlinux.org.uk wrote:
On Thu, Nov 03, 2022 at 05:21:16PM +0800, Yang Jihong wrote:
The error code -EACCES is returned when bpf prog is tested in 32-bit environment, This is because bpf_object__relocate modifies the instruction to change memory size to 4 bytes, as shown in the following messages:
libbpf: prog 'kfunc_call_test1': relo #2: matching candidate #0 <byte_off> [18342] struct __sk_buff.sk (0:30:0 @ offset 168) libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) off 168 -> 168 libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) mem_sz 8 -> 4
As a result, the bpf_skb_is_valid_access check fails. For 32-bit architecture, unnecessary checks need to be deleted.
Isn't the purpose of this check to ensure that the entire pointer is written, and BPF can't write half of it?
case offsetof(struct __sk_buff, sk):
if (type == BPF_WRITE || size != sizeof(__u64))return false;Wouldn't "(size != sizeof(struct bpf_sock *) && size != sizeof(__u64))" be more appropriate here, so 32-bit can only write the 32-bit pointer or the full 64-bit value, and 64-bit can only write the 64-bit pointer? Or is there a reason not to? bpf folk?
You're correct. The patch is completely wrong. The bug is elsewhere.
So I looked at this a bit (and replied to the old version of this patch). What happens in the kernel is that we expect 64-bit load but rewrite it to 32-bit load on 32-bit architectures (because we just use sizeof(struct sk_buff, sk) which is 4 bytes on 32-bit arch.
The problem here is that libbpf adjusts such pointer accesses from 8-byte read to 4-byte reads for preserve_access_index (because libbpf sees that pointer is really 4 byte long), which is what we actually want in the general case. Here the assumption was made before CO-RE that __sk_buff is a stable (and fake) UAPI and the correct BPF program will access sk as a 64-bit pointer because BPF-side pointers always appear as 64-bit.
But from a correctness standpoint I think it should be fine to enable both 32- and 64-bit loads for such pointers in __sk_buff for 32-bit host arch. This will work well with CO-RE and will be correctly rewritten to 32-bit or 64-bit accesses, depending on host architecture.
We should still reject 32-bit load on 64-bit host arch, though.
Replied in the other thread as well :) The CO_RE screws up access here. Since it's a load of a pointer the verifier has to see it as a 8-byte load. When CO-RE converts it to 4 byte the verifier won't recognize it as a pointer load anymore. We cannot easily convert 64-bit BPF ISA into 32-bit. It's a massive amount of work.
Hello,
On 2022/11/5 7:37, Alexei Starovoitov wrote:
On Fri, Nov 4, 2022 at 3:43 PM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:
On Thu, Nov 3, 2022 at 11:15 AM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
On Thu, Nov 3, 2022 at 4:23 AM Russell King (Oracle) linux@armlinux.org.uk wrote:
On Thu, Nov 03, 2022 at 05:21:16PM +0800, Yang Jihong wrote:
The error code -EACCES is returned when bpf prog is tested in 32-bit environment, This is because bpf_object__relocate modifies the instruction to change memory size to 4 bytes, as shown in the following messages:
libbpf: prog 'kfunc_call_test1': relo #2: matching candidate #0 <byte_off> [18342] struct __sk_buff.sk (0:30:0 @ offset 168) libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) off 168 -> 168 libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) mem_sz 8 -> 4
As a result, the bpf_skb_is_valid_access check fails. For 32-bit architecture, unnecessary checks need to be deleted.
Isn't the purpose of this check to ensure that the entire pointer is written, and BPF can't write half of it?
case offsetof(struct __sk_buff, sk):
if (type == BPF_WRITE || size != sizeof(__u64))return false;Wouldn't "(size != sizeof(struct bpf_sock *) && size != sizeof(__u64))" be more appropriate here, so 32-bit can only write the 32-bit pointer or the full 64-bit value, and 64-bit can only write the 64-bit pointer? Or is there a reason not to? bpf folk?
You're correct. The patch is completely wrong. The bug is elsewhere.
So I looked at this a bit (and replied to the old version of this patch). What happens in the kernel is that we expect 64-bit load but rewrite it to 32-bit load on 32-bit architectures (because we just use sizeof(struct sk_buff, sk) which is 4 bytes on 32-bit arch.
The problem here is that libbpf adjusts such pointer accesses from 8-byte read to 4-byte reads for preserve_access_index (because libbpf sees that pointer is really 4 byte long), which is what we actually want in the general case. Here the assumption was made before CO-RE that __sk_buff is a stable (and fake) UAPI and the correct BPF program will access sk as a 64-bit pointer because BPF-side pointers always appear as 64-bit.
But from a correctness standpoint I think it should be fine to enable both 32- and 64-bit loads for such pointers in __sk_buff for 32-bit host arch. This will work well with CO-RE and will be correctly rewritten to 32-bit or 64-bit accesses, depending on host architecture.
We should still reject 32-bit load on 64-bit host arch, though.
Replied in the other thread as well :) The CO_RE screws up access here. Since it's a load of a pointer the verifier has to see it as a 8-byte load. When CO-RE converts it to 4 byte the verifier won't recognize it as a pointer load anymore. We cannot easily convert 64-bit BPF ISA into 32-bit. It's a massive amount of work. .
From the above discussion, there are two different solutions: 1. Modify bpf_skb_is_valid_access to ensure that 32-bit can only load the 32-bit pointer or the full 64-bit value, and 64-bit can only load the 64-bit pointer 2. Modify libbpf, CO_RE skips adjust load's mem size and retains the 8-byte load. The two fixes will be added in the next version. Please review the solution to be selected.
Thanks, Yang
Hello,
On 2022/11/3 19:23, Russell King (Oracle) wrote:
On Thu, Nov 03, 2022 at 05:21:16PM +0800, Yang Jihong wrote:
The error code -EACCES is returned when bpf prog is tested in 32-bit environment, This is because bpf_object__relocate modifies the instruction to change memory size to 4 bytes, as shown in the following messages:
libbpf: prog 'kfunc_call_test1': relo #2: matching candidate #0 <byte_off> [18342] struct __sk_buff.sk (0:30:0 @ offset 168) libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) off 168 -> 168 libbpf: prog 'kfunc_call_test1': relo #2: patched insn #1 (LDX/ST/STX) mem_sz 8 -> 4
As a result, the bpf_skb_is_valid_access check fails. For 32-bit architecture, unnecessary checks need to be deleted.
Isn't the purpose of this check to ensure that the entire pointer is written, and BPF can't write half of it?
case offsetof(struct __sk_buff, sk):
if (type == BPF_WRITE || size != sizeof(__u64))return false;Wouldn't "(size != sizeof(struct bpf_sock *) && size != sizeof(__u64))" be more appropriate here, so 32-bit can only write the 32-bit pointer or the full 64-bit value, and 64-bit can only write the 64-bit pointer? Or is there a reason not to? bpf folk?
Thanks for the detailed proposals, will fix it in next version.
Thanks, Yang
This patch adds kernel function call support to the 32-bit ARM bpf jit.
Signed-off-by: Yang Jihong yangjihong1@huawei.com --- arch/arm/net/bpf_jit_32.c | 130 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+)
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c index 6a1c9fca5260..51428c82bec6 100644 --- a/arch/arm/net/bpf_jit_32.c +++ b/arch/arm/net/bpf_jit_32.c @@ -1337,6 +1337,118 @@ static void build_epilogue(struct jit_ctx *ctx) #endif }
+/* + * Input parameters of function in 32-bit ARM architecture: + * The first four word-sized parameters passed to a function will be + * transferred in registers R0-R3. Sub-word sized arguments, for example, + * char, will still use a whole register. + * Arguments larger than a word will be passed in multiple registers. + * If more arguments are passed, the fifth and subsequent words will be passed + * on the stack. + * + * The first for args of a function will be considered for + * putting into the 32bit register R1, R2, R3 and R4. + * + * Two 32bit registers are used to pass a 64bit arg. + * + * For example, + * void foo(u32 a, u32 b, u32 c, u32 d, u32 e): + * u32 a: R0 + * u32 b: R1 + * u32 c: R2 + * u32 d: R3 + * u32 e: stack + * + * void foo(u64 a, u32 b, u32 c, u32 d): + * u64 a: R0 (lo32) R1 (hi32) + * u32 b: R2 + * u32 c: R3 + * u32 d: stack + * + * void foo(u32 a, u64 b, u32 c, u32 d): + * u32 a: R0 + * u64 b: R2 (lo32) R3 (hi32) + * u32 c: stack + * u32 d: stack + * + * void foo(u32 a, u32 b, u64 c, u32 d): + * u32 a: R0 + * u32 b: R1 + * u64 c: R2 (lo32) R3 (hi32) + * u32 d: stack + * + * The return value will be stored in the R0 (and R1 for 64bit value). + * + * For example, + * u32 foo(u32 a, u32 b, u32 c): + * return value: R0 + * + * u64 foo(u32 a, u32 b, u32 c): + * return value: R0 (lo32) R1 (hi32) + */ +static int emit_kfunc_call(const struct bpf_insn *insn, struct jit_ctx *ctx, const u32 func) +{ + int i; + const struct btf_func_model *fm; + const s8 *tmp = bpf2a32[TMP_REG_1]; + const u8 arg_regs[] = { ARM_R0, ARM_R1, ARM_R2, ARM_R3 }; + int nr_arg_regs = ARRAY_SIZE(arg_regs); + int arg_regs_idx = 0, stack_off = 0; + + fm = bpf_jit_find_kfunc_model(ctx->prog, insn); + if (!fm) + return -EINVAL; + + for (i = 0; i < fm->nr_args; i++) { + if (fm->arg_size[i] > sizeof(u32)) { + if (arg_regs_idx + 1 < nr_arg_regs) { + /* + * AAPCS states: + * A double-word sized type is passed in two + * consecutive registers (e.g., r0 and r1, or + * r2 and r3). The content of the registers is + * as if the value had been loaded from memory + * representation with a single LDM instruction. + */ + if (arg_regs_idx & 1) + arg_regs_idx++; + + emit(ARM_LDRD_I(arg_regs[arg_regs_idx], ARM_FP, + EBPF_SCRATCH_TO_ARM_FP( + bpf2a32[BPF_REG_1 + i][1])), ctx); + + arg_regs_idx += 2; + } else { + stack_off = ALIGN(stack_off, STACK_ALIGNMENT); + + emit(ARM_LDRD_I(tmp[1], ARM_FP, + EBPF_SCRATCH_TO_ARM_FP( + bpf2a32[BPF_REG_1 + i][1])), ctx); + emit(ARM_STRD_I(tmp[1], ARM_SP, stack_off), ctx); + + stack_off += 8; + } + } else { + if (arg_regs_idx + 1 < nr_arg_regs) { + emit_a32_mov_r(arg_regs[arg_regs_idx++], + bpf2a32[BPF_REG_1 + i][1], ctx); + } else { + emit(ARM_LDR_I(tmp[1], ARM_FP, + EBPF_SCRATCH_TO_ARM_FP( + bpf2a32[BPF_REG_1 + i][1])), ctx); + emit(ARM_STR_I(tmp[1], ARM_SP, stack_off), ctx); + + stack_off += 4; + } + } + } + + emit_a32_mov_i(tmp[1], func, ctx); + emit_blx_r(tmp[1], ctx); + + return 0; +} + /* * Convert an eBPF instruction to native instruction, i.e * JITs an eBPF instruction. @@ -1603,6 +1715,10 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx) case BPF_LDX | BPF_MEM | BPF_H: case BPF_LDX | BPF_MEM | BPF_B: case BPF_LDX | BPF_MEM | BPF_DW: + case BPF_LDX | BPF_PROBE_MEM | BPF_W: + case BPF_LDX | BPF_PROBE_MEM | BPF_H: + case BPF_LDX | BPF_PROBE_MEM | BPF_B: + case BPF_LDX | BPF_PROBE_MEM | BPF_DW: rn = arm_bpf_get_reg32(src_lo, tmp2[1], ctx); emit_ldx_r(dst, rn, off, ctx, BPF_SIZE(code)); break; @@ -1785,6 +1901,16 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx) const s8 *r5 = bpf2a32[BPF_REG_5]; const u32 func = (u32)__bpf_call_base + (u32)imm;
+ if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) { + int err; + + err = emit_kfunc_call(insn, ctx, func); + + if (err) + return err; + break; + } + emit_a32_mov_r64(true, r0, r1, ctx); emit_a32_mov_r64(true, r1, r2, ctx); emit_push_r64(r5, ctx); @@ -2022,3 +2148,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) return prog; }
+bool bpf_jit_supports_kfunc_call(void) +{ + return true; +}
On Thu, Nov 03, 2022 at 05:21:17PM +0800, Yang Jihong wrote:
This patch adds kernel function call support to the 32-bit ARM bpf jit.
Signed-off-by: Yang Jihong yangjihong1@huawei.com
arch/arm/net/bpf_jit_32.c | 130 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+)
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c index 6a1c9fca5260..51428c82bec6 100644 --- a/arch/arm/net/bpf_jit_32.c +++ b/arch/arm/net/bpf_jit_32.c @@ -1337,6 +1337,118 @@ static void build_epilogue(struct jit_ctx *ctx) #endif } +/*
- Input parameters of function in 32-bit ARM architecture:
- The first four word-sized parameters passed to a function will be
- transferred in registers R0-R3. Sub-word sized arguments, for example,
- char, will still use a whole register.
- Arguments larger than a word will be passed in multiple registers.
- If more arguments are passed, the fifth and subsequent words will be passed
- on the stack.
- The first for args of a function will be considered for
- putting into the 32bit register R1, R2, R3 and R4.
- Two 32bit registers are used to pass a 64bit arg.
- For example,
- void foo(u32 a, u32 b, u32 c, u32 d, u32 e):
u32 a: R0
u32 b: R1
u32 c: R2
u32 d: R3
u32 e: stack
- void foo(u64 a, u32 b, u32 c, u32 d):
u64 a: R0 (lo32) R1 (hi32)
u32 b: R2
u32 c: R3
u32 d: stack
- void foo(u32 a, u64 b, u32 c, u32 d):
u32 a: R0
u64 b: R2 (lo32) R3 (hi32)
u32 c: stack
u32 d: stack
This code supports both EABI and OABI, but the above is EABI-only. Either we need to decide not to support OABI, or we need to add code for both. That can probably be done by making:
- for (i = 0; i < fm->nr_args; i++) {
if (fm->arg_size[i] > sizeof(u32)) {if (arg_regs_idx + 1 < nr_arg_regs) {/** AAPCS states:* A double-word sized type is passed in two* consecutive registers (e.g., r0 and r1, or* r2 and r3). The content of the registers is* as if the value had been loaded from memory* representation with a single LDM instruction.*/if (arg_regs_idx & 1)arg_regs_idx++;
... this conditional on IS_ENABLED(CONFIG_AEABI).
emit(ARM_LDRD_I(arg_regs[arg_regs_idx], ARM_FP,EBPF_SCRATCH_TO_ARM_FP(bpf2a32[BPF_REG_1 + i][1])), ctx);
You probably want to re-use the internals of arm_bpf_get_reg64() to load the register.
arg_regs_idx += 2;} else {stack_off = ALIGN(stack_off, STACK_ALIGNMENT);emit(ARM_LDRD_I(tmp[1], ARM_FP,EBPF_SCRATCH_TO_ARM_FP(bpf2a32[BPF_REG_1 + i][1])), ctx);
Same here.
emit(ARM_STRD_I(tmp[1], ARM_SP, stack_off), ctx);
and the internals of arm_bpf_put_reg64() here. Not all Arm CPUs that this code runs on supports ldrd and strd.
Hello,
On 2022/11/3 19:35, Russell King (Oracle) wrote:
On Thu, Nov 03, 2022 at 05:21:17PM +0800, Yang Jihong wrote:
This patch adds kernel function call support to the 32-bit ARM bpf jit.
Signed-off-by: Yang Jihong yangjihong1@huawei.com
arch/arm/net/bpf_jit_32.c | 130 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+)
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c index 6a1c9fca5260..51428c82bec6 100644 --- a/arch/arm/net/bpf_jit_32.c +++ b/arch/arm/net/bpf_jit_32.c @@ -1337,6 +1337,118 @@ static void build_epilogue(struct jit_ctx *ctx) #endif } +/*
- Input parameters of function in 32-bit ARM architecture:
- The first four word-sized parameters passed to a function will be
- transferred in registers R0-R3. Sub-word sized arguments, for example,
- char, will still use a whole register.
- Arguments larger than a word will be passed in multiple registers.
- If more arguments are passed, the fifth and subsequent words will be passed
- on the stack.
- The first for args of a function will be considered for
- putting into the 32bit register R1, R2, R3 and R4.
- Two 32bit registers are used to pass a 64bit arg.
- For example,
- void foo(u32 a, u32 b, u32 c, u32 d, u32 e):
u32 a: R0
u32 b: R1
u32 c: R2
u32 d: R3
u32 e: stack
- void foo(u64 a, u32 b, u32 c, u32 d):
u64 a: R0 (lo32) R1 (hi32)
u32 b: R2
u32 c: R3
u32 d: stack
- void foo(u32 a, u64 b, u32 c, u32 d):
u32 a: R0
u64 b: R2 (lo32) R3 (hi32)
u32 c: stack
u32 d: stackThis code supports both EABI and OABI, but the above is EABI-only. Either we need to decide not to support OABI, or we need to add code for both. That can probably be done by making:
Yes, the OABI situation was not considered here before, Because I don't have OABI ARM machine, I can't actually verify it, only EABI is supported. In the next version, will check whether CONFIG_AEABI is enabled.
- for (i = 0; i < fm->nr_args; i++) {
if (fm->arg_size[i] > sizeof(u32)) {if (arg_regs_idx + 1 < nr_arg_regs) {/** AAPCS states:* A double-word sized type is passed in two* consecutive registers (e.g., r0 and r1, or* r2 and r3). The content of the registers is* as if the value had been loaded from memory* representation with a single LDM instruction.*/if (arg_regs_idx & 1)arg_regs_idx++;... this conditional on IS_ENABLED(CONFIG_AEABI).
emit(ARM_LDRD_I(arg_regs[arg_regs_idx], ARM_FP,EBPF_SCRATCH_TO_ARM_FP(bpf2a32[BPF_REG_1 + i][1])), ctx);You probably want to re-use the internals of arm_bpf_get_reg64() to load the register.
OK, will re-use arm_bpf_get_reg64 in next version.
arg_regs_idx += 2;} else {stack_off = ALIGN(stack_off, STACK_ALIGNMENT);emit(ARM_LDRD_I(tmp[1], ARM_FP,EBPF_SCRATCH_TO_ARM_FP(bpf2a32[BPF_REG_1 + i][1])), ctx);Same here.
OK, will re-use arm_bpf_get_reg64 in next version.
emit(ARM_STRD_I(tmp[1], ARM_SP, stack_off), ctx);and the internals of arm_bpf_put_reg64() here. Not all Arm CPUs that this code runs on supports ldrd and strd.
Yes, the ARM CPUs that do not support LDRD and STRD have not been considered, will fix in next version.
Thanks, Yang
for function foo(u32 a, u64 b, u32 c) in 32-bit ARM: a is in r0, b is in r2-r3, c is stored on the stack. Because the AAPCS states: "A double-word sized type is passed in two consecutive registers (e.g., r0 and r1, or r2 and r3). The content of the registers is as if the value had been loaded from memory representation with a single LDM instruction." Supplement the test cases in this case.
Signed-off-by: Yang Jihong yangjihong1@huawei.com --- net/bpf/test_run.c | 6 +++++ .../selftests/bpf/prog_tests/kfunc_call.c | 1 + .../selftests/bpf/progs/kfunc_call_test.c | 23 +++++++++++++++++++ 3 files changed, 30 insertions(+)
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index 13d578ce2a09..bdfb3081e1ce 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -551,6 +551,11 @@ struct sock * noinline bpf_kfunc_call_test3(struct sock *sk) return sk; }
+u64 noinline bpf_kfunc_call_test4(struct sock *sk, u64 a, u64 b, u32 c, u32 d) +{ + return a + b + c + d; +} + struct prog_test_member1 { int a; }; @@ -739,6 +744,7 @@ BTF_SET8_START(test_sk_check_kfunc_ids) BTF_ID_FLAGS(func, bpf_kfunc_call_test1) BTF_ID_FLAGS(func, bpf_kfunc_call_test2) BTF_ID_FLAGS(func, bpf_kfunc_call_test3) +BTF_ID_FLAGS(func, bpf_kfunc_call_test4) BTF_ID_FLAGS(func, bpf_kfunc_call_test_acquire, KF_ACQUIRE | KF_RET_NULL) BTF_ID_FLAGS(func, bpf_kfunc_call_memb_acquire, KF_ACQUIRE | KF_RET_NULL) BTF_ID_FLAGS(func, bpf_kfunc_call_test_release, KF_RELEASE) diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c index 5af1ee8f0e6e..13a105bb05ed 100644 --- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c @@ -72,6 +72,7 @@ static struct kfunc_test_params kfunc_tests[] = { /* success cases */ TC_TEST(kfunc_call_test1, 12), TC_TEST(kfunc_call_test2, 3), + TC_TEST(kfunc_call_test4, 16), TC_TEST(kfunc_call_test_ref_btf_id, 0), TC_TEST(kfunc_call_test_get_mem, 42), SYSCALL_TEST(kfunc_syscall_test, 0), diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c index f636e50be259..7cccb014d26e 100644 --- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c +++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c @@ -6,6 +6,8 @@ extern int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym; extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b, __u32 c, __u64 d) __ksym; +extern __u64 bpf_kfunc_call_test4(struct sock *sk, __u64 a, __u64 b, + __u32 c, __u32 d) __ksym;
extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym; extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym; @@ -17,6 +19,27 @@ extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym; extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym; extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
+SEC("tc") +int kfunc_call_test4(struct __sk_buff *skb) +{ + struct bpf_sock *sk = skb->sk; + __u64 a = 1ULL << 32; + __u32 ret; + + if (!sk) + return -1; + + sk = bpf_sk_fullsock(sk); + if (!sk) + return -1; + + a = bpf_kfunc_call_test4((struct sock *)sk, a | 2, a | 3, 4, 5); + ret = a >> 32; /* ret should be 2 */ + ret += (__u32)a; /* ret should be 16 */ + + return ret; +} + SEC("tc") int kfunc_call_test2(struct __sk_buff *skb) {
linux-kselftest-mirror@lists.linaro.org
-
Alexei Starovoitov -
Andrii Nakryiko -
Russell King (Oracle) -
Yang Jihong