* [PATCH bpf-next v3 0/4] bpf: Support kernel function call in 32-bit ARM
@ 2022-11-26 9:45 Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension Yang Jihong
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Yang Jihong @ 2022-11-26 9:45 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, illusionist.neo, linux, davem,
edumazet, kuba, pabeni, mykolal, shuah, benjamin.tissoires,
memxor, colin.i.king, asavkov, delyank, bpf, linux-arm-kernel,
linux-kernel, netdev, linux-kselftest
Cc: yangjihong1
1. Patch1 is dependent patch to fix zext extension error in 32-bit ARM.
2. Patch2 supports bpf fkunc in 32-bit ARM for EABI.
3. Patch3 is used to add test cases to cover some parameter scenarios
states by AAPCS.
4. Patch4 fix a comment error.
The following is the test_progs result in the 32-bit ARM environment:
# uname -m
armv7l
# 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_test5:OK
#1/12 kfunc_call/kfunc_call_test6:OK
#1/13 kfunc_call/kfunc_call_test_ref_btf_id:OK
#1/14 kfunc_call/kfunc_call_test_get_mem:OK
#1/15 kfunc_call/kfunc_syscall_test:OK
#1/16 kfunc_call/kfunc_syscall_test_null:OK
#1/19 kfunc_call/destructive:OK
---
Changes since v2:
- Remove patches to adjust sk size check for CO_RE in 32-bit arch.
- Add check of kfunc's return value in insn_def_regno.
- Adjust is_reg64 for insn_def_regno.
- The check of CONFIG_AEABI is moved from emit_kfunc_call to
bpf_jit_supports_kfunc_call.
- Fix a comment error in fixup_kfunc_call.
Yang Jihong (4):
bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext
extension
bpf: Add kernel function call support in 32-bit ARM for EABI
bpf:selftests: Add kfunc_call test for mixing 32-bit and 64-bit
parameters
bpf: Fix comment error in fixup_kfunc_call function
arch/arm/net/bpf_jit_32.c | 137 ++++++++++++++++++
kernel/bpf/verifier.c | 46 +++++-
net/bpf/test_run.c | 18 +++
.../selftests/bpf/prog_tests/kfunc_call.c | 3 +
.../selftests/bpf/progs/kfunc_call_test.c | 52 +++++++
5 files changed, 252 insertions(+), 4 deletions(-)
--
2.30.GIT
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension
2022-11-26 9:45 [PATCH bpf-next v3 0/4] bpf: Support kernel function call in 32-bit ARM Yang Jihong
@ 2022-11-26 9:45 ` Yang Jihong
2022-11-28 1:57 ` Alexei Starovoitov
2022-11-26 9:45 ` [PATCH bpf-next v3 2/4] bpf: Add kernel function call support in 32-bit ARM for EABI Yang Jihong
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Yang Jihong @ 2022-11-26 9:45 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, illusionist.neo, linux, davem,
edumazet, kuba, pabeni, mykolal, shuah, benjamin.tissoires,
memxor, colin.i.king, asavkov, delyank, bpf, linux-arm-kernel,
linux-kernel, netdev, linux-kselftest
Cc: yangjihong1
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 | 44 ++++++++++++++++++++++++++++++++++++++++---
1 file changed, 41 insertions(+), 3 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 264b3dc714cc..193ea927aa69 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1927,6 +1927,21 @@ find_kfunc_desc(const struct bpf_prog *prog, u32 func_id, u16 offset)
sizeof(tab->descs[0]), kfunc_desc_cmp_by_id_off);
}
+static int kfunc_desc_cmp_by_imm(const void *a, const void *b);
+
+static const struct bpf_kfunc_desc *
+find_kfunc_desc_by_imm(const struct bpf_prog *prog, s32 imm)
+{
+ struct bpf_kfunc_desc desc = {
+ .imm = imm,
+ };
+ struct bpf_kfunc_desc_tab *tab;
+
+ tab = prog->aux->kfunc_tab;
+ return bsearch(&desc, tab->descs, tab->nr_descs,
+ sizeof(tab->descs[0]), kfunc_desc_cmp_by_imm);
+}
+
static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env *env,
s16 offset)
{
@@ -2342,6 +2357,13 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
*/
if (insn->src_reg == BPF_PSEUDO_CALL)
return false;
+
+ /* Kfunc call will reach here because of insn_has_def32,
+ * conservatively return TRUE.
+ */
+ if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
+ return true;
+
/* Helper call will reach here because of arg type
* check, conservatively return TRUE.
*/
@@ -2405,10 +2427,26 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
}
/* Return the regno defined by the insn, or -1. */
-static int insn_def_regno(const struct bpf_insn *insn)
+static int insn_def_regno(struct bpf_verifier_env *env, const struct bpf_insn *insn)
{
switch (BPF_CLASS(insn->code)) {
case BPF_JMP:
+ if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
+ const struct bpf_kfunc_desc *desc;
+
+ /* The value of desc cannot be NULL */
+ desc = find_kfunc_desc_by_imm(env->prog, insn->imm);
+
+ /* A kfunc can return void.
+ * The btf type of the kfunc's return value needs
+ * to be checked against "void" first
+ */
+ if (desc->func_model.ret_size == 0)
+ return -1;
+ else
+ return insn->dst_reg;
+ }
+ fallthrough;
case BPF_JMP32:
case BPF_ST:
return -1;
@@ -2430,7 +2468,7 @@ static int insn_def_regno(const struct bpf_insn *insn)
/* Return TRUE if INSN has defined any 32-bit value explicitly. */
static bool insn_has_def32(struct bpf_verifier_env *env, struct bpf_insn *insn)
{
- int dst_reg = insn_def_regno(insn);
+ int dst_reg = insn_def_regno(env, insn);
if (dst_reg == -1)
return false;
@@ -13335,7 +13373,7 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
int load_reg;
insn = insns[adj_idx];
- load_reg = insn_def_regno(&insn);
+ load_reg = insn_def_regno(env, &insn);
if (!aux[adj_idx].zext_dst) {
u8 code, class;
u32 imm_rnd;
--
2.30.GIT
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH bpf-next v3 2/4] bpf: Add kernel function call support in 32-bit ARM for EABI
2022-11-26 9:45 [PATCH bpf-next v3 0/4] bpf: Support kernel function call in 32-bit ARM Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension Yang Jihong
@ 2022-11-26 9:45 ` Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 3/4] bpf:selftests: Add kfunc_call test for mixing 32-bit and 64-bit parameters Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 4/4] bpf: Fix comment error in fixup_kfunc_call function Yang Jihong
3 siblings, 0 replies; 12+ messages in thread
From: Yang Jihong @ 2022-11-26 9:45 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, illusionist.neo, linux, davem,
edumazet, kuba, pabeni, mykolal, shuah, benjamin.tissoires,
memxor, colin.i.king, asavkov, delyank, bpf, linux-arm-kernel,
linux-kernel, netdev, linux-kselftest
Cc: yangjihong1
This patch adds kernel function call support to 32-bit ARM bpf jit for
EABI.
Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
arch/arm/net/bpf_jit_32.c | 137 ++++++++++++++++++++++++++++++++++++++
1 file changed, 137 insertions(+)
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index 6a1c9fca5260..ae3a36d909f4 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -1337,6 +1337,125 @@ 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
+ *
+ * void foo(u64 a, u64 b):
+ * u64 a: R0 (lo32) R1 (hi32)
+ * u64 b: R2 (lo32) R3 (hi32)
+ *
+ * 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)
+ *
+ * The above is for AEABI only, OABI does not support this function.
+ */
+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;
+ const s8 *rd;
+ s8 rt;
+
+ 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)) {
+ rd = arm_bpf_get_reg64(bpf2a32[BPF_REG_1 + i], tmp, ctx);
+
+ 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_MOV_R(arg_regs[arg_regs_idx++], rd[1]), ctx);
+ emit(ARM_MOV_R(arg_regs[arg_regs_idx++], rd[0]), ctx);
+ } else {
+ stack_off = ALIGN(stack_off, STACK_ALIGNMENT);
+
+ if (__LINUX_ARM_ARCH__ >= 6 ||
+ ctx->cpu_architecture >= CPU_ARCH_ARMv5TE) {
+ emit(ARM_STRD_I(rd[1], ARM_SP, stack_off), ctx);
+ } else {
+ emit(ARM_STR_I(rd[1], ARM_SP, stack_off), ctx);
+ emit(ARM_STR_I(rd[0], ARM_SP, stack_off), ctx);
+ }
+
+ stack_off += 8;
+ }
+ } else {
+ rt = arm_bpf_get_reg32(bpf2a32[BPF_REG_1 + i][1], tmp[1], ctx);
+
+ if (arg_regs_idx < nr_arg_regs) {
+ emit(ARM_MOV_R(arg_regs[arg_regs_idx++], rt), ctx);
+ } else {
+ emit(ARM_STR_I(rt, 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 +1722,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 +1908,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 +2155,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
return prog;
}
+bool bpf_jit_supports_kfunc_call(void)
+{
+ return IS_ENABLED(CONFIG_AEABI);
+}
--
2.30.GIT
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH bpf-next v3 3/4] bpf:selftests: Add kfunc_call test for mixing 32-bit and 64-bit parameters
2022-11-26 9:45 [PATCH bpf-next v3 0/4] bpf: Support kernel function call in 32-bit ARM Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 2/4] bpf: Add kernel function call support in 32-bit ARM for EABI Yang Jihong
@ 2022-11-26 9:45 ` Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 4/4] bpf: Fix comment error in fixup_kfunc_call function Yang Jihong
3 siblings, 0 replies; 12+ messages in thread
From: Yang Jihong @ 2022-11-26 9:45 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, illusionist.neo, linux, davem,
edumazet, kuba, pabeni, mykolal, shuah, benjamin.tissoires,
memxor, colin.i.king, asavkov, delyank, bpf, linux-arm-kernel,
linux-kernel, netdev, linux-kselftest
Cc: yangjihong1
32-bit ARM has four registers to save function parameters,
add test cases to cover additional scenarios.
Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
net/bpf/test_run.c | 18 +++++++
.../selftests/bpf/prog_tests/kfunc_call.c | 3 ++
.../selftests/bpf/progs/kfunc_call_test.c | 52 +++++++++++++++++++
3 files changed, 73 insertions(+)
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index fcb3e6c5e03c..5e8895027f0d 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -551,6 +551,21 @@ 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;
+}
+
+u64 noinline bpf_kfunc_call_test5(u64 a, u64 b)
+{
+ return a + b;
+}
+
+u64 noinline bpf_kfunc_call_test6(u32 a, u32 b, u32 c, u32 d, u32 e)
+{
+ return a + b + c + d + e;
+}
+
struct prog_test_member1 {
int a;
};
@@ -739,6 +754,9 @@ 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_test5)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test6)
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..6a6822e99071 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
@@ -72,6 +72,9 @@ 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_test5, 7),
+ TC_TEST(kfunc_call_test6, 15),
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..0385ce2d4c6e 100644
--- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
@@ -6,6 +6,11 @@
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 __u64 bpf_kfunc_call_test5(__u64 a, __u64 b) __ksym;
+extern __u64 bpf_kfunc_call_test6(__u32 a, __u32 b, __u32 c, __u32 d,
+ __u32 e) __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 +22,53 @@ 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_test6(struct __sk_buff *skb)
+{
+ __u64 a = 1ULL << 32;
+ __u32 ret;
+
+ a = bpf_kfunc_call_test6(1, 2, 3, 4, 5);
+ ret = a >> 32; /* ret should be 0 */
+ ret += (__u32)a; /* ret should be 15 */
+
+ return ret;
+}
+
+SEC("tc")
+int kfunc_call_test5(struct __sk_buff *skb)
+{
+ __u64 a = 1ULL << 32;
+ __u32 ret;
+
+ a = bpf_kfunc_call_test5(a | 2, a | 3);
+ ret = a >> 32; /* ret should be 2 */
+ ret += (__u32)a; /* ret should be 7 */
+
+ return ret;
+}
+
+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)
{
--
2.30.GIT
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH bpf-next v3 4/4] bpf: Fix comment error in fixup_kfunc_call function
2022-11-26 9:45 [PATCH bpf-next v3 0/4] bpf: Support kernel function call in 32-bit ARM Yang Jihong
` (2 preceding siblings ...)
2022-11-26 9:45 ` [PATCH bpf-next v3 3/4] bpf:selftests: Add kfunc_call test for mixing 32-bit and 64-bit parameters Yang Jihong
@ 2022-11-26 9:45 ` Yang Jihong
3 siblings, 0 replies; 12+ messages in thread
From: Yang Jihong @ 2022-11-26 9:45 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, illusionist.neo, linux, davem,
edumazet, kuba, pabeni, mykolal, shuah, benjamin.tissoires,
memxor, colin.i.king, asavkov, delyank, bpf, linux-arm-kernel,
linux-kernel, netdev, linux-kselftest
Cc: yangjihong1
insn->imm for kfunc is the relative address of __bpf_call_base,
instead of __bpf_base_call, Fix the comment error.
Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
---
kernel/bpf/verifier.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 193ea927aa69..eb58fea645ca 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -13927,7 +13927,7 @@ static int fixup_kfunc_call(struct bpf_verifier_env *env,
}
/* insn->imm has the btf func_id. Replace it with
- * an address (relative to __bpf_base_call).
+ * an address (relative to __bpf_call_base).
*/
desc = find_kfunc_desc(env->prog, insn->imm, insn->off);
if (!desc) {
--
2.30.GIT
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension
2022-11-26 9:45 ` [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension Yang Jihong
@ 2022-11-28 1:57 ` Alexei Starovoitov
2022-11-28 12:40 ` Yang Jihong
0 siblings, 1 reply; 12+ messages in thread
From: Alexei Starovoitov @ 2022-11-28 1:57 UTC (permalink / raw)
To: Yang Jihong
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, illusionist.neo, linux, davem,
edumazet, kuba, pabeni, mykolal, shuah, benjamin.tissoires,
memxor, colin.i.king, asavkov, delyank, bpf, linux-arm-kernel,
linux-kernel, netdev, linux-kselftest
On Sat, Nov 26, 2022 at 05:45:27PM +0800, Yang Jihong wrote:
> 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 | 44 ++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 264b3dc714cc..193ea927aa69 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1927,6 +1927,21 @@ find_kfunc_desc(const struct bpf_prog *prog, u32 func_id, u16 offset)
> sizeof(tab->descs[0]), kfunc_desc_cmp_by_id_off);
> }
>
> +static int kfunc_desc_cmp_by_imm(const void *a, const void *b);
> +
> +static const struct bpf_kfunc_desc *
> +find_kfunc_desc_by_imm(const struct bpf_prog *prog, s32 imm)
> +{
> + struct bpf_kfunc_desc desc = {
> + .imm = imm,
> + };
> + struct bpf_kfunc_desc_tab *tab;
> +
> + tab = prog->aux->kfunc_tab;
> + return bsearch(&desc, tab->descs, tab->nr_descs,
> + sizeof(tab->descs[0]), kfunc_desc_cmp_by_imm);
> +}
> +
> static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env *env,
> s16 offset)
> {
> @@ -2342,6 +2357,13 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
> */
> if (insn->src_reg == BPF_PSEUDO_CALL)
> return false;
> +
> + /* Kfunc call will reach here because of insn_has_def32,
> + * conservatively return TRUE.
> + */
> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
> + return true;
> +
> /* Helper call will reach here because of arg type
> * check, conservatively return TRUE.
> */
> @@ -2405,10 +2427,26 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
> }
>
> /* Return the regno defined by the insn, or -1. */
> -static int insn_def_regno(const struct bpf_insn *insn)
> +static int insn_def_regno(struct bpf_verifier_env *env, const struct bpf_insn *insn)
> {
> switch (BPF_CLASS(insn->code)) {
> case BPF_JMP:
> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> + const struct bpf_kfunc_desc *desc;
> +
> + /* The value of desc cannot be NULL */
> + desc = find_kfunc_desc_by_imm(env->prog, insn->imm);
> +
> + /* A kfunc can return void.
> + * The btf type of the kfunc's return value needs
> + * to be checked against "void" first
> + */
> + if (desc->func_model.ret_size == 0)
> + return -1;
> + else
> + return insn->dst_reg;
> + }
> + fallthrough;
I cannot make any sense of this patch.
insn->dst_reg above is 0.
The kfunc call doesn't define a register from insn_def_regno() pov.
Are you hacking insn_def_regno() to return 0 so that
if (WARN_ON(load_reg == -1)) {
verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");
return -EFAULT;
}
in opt_subreg_zext_lo32_rnd_hi32() doesn't trigger ?
But this verifier message should have been a hint that you need
to analyze why zext_dst is set on this kfunc call.
Maybe it shouldn't ?
Did you analyze the logic of mark_btf_func_reg_size() ?
Before producing any patches please understand the logic fully.
Your commit log
"insn_def_regno should
return dst_reg for BPF_JMP type of BPF_PSEUDO_KFUNC_CALL."
Makes no sense to me, since dst_reg is unused in JMP insn.
There is no concept of a src or dst register in a JMP insn.
32-bit x86 supports calling kfuncs. See emit_kfunc_call().
And we don't have this "verifier bug. zext_dst is set" issue there, right?
But what you're saying in the commit log:
"if data width of kfunc return value is 32 bits"
should have been applicable to x86-32 as well.
So please start with a test that demonstrates the issue on x86-32 and
then we can discuss the way to fix it.
The patch 2 sort-of makes sense.
For patch 3 pls add new test funcs to bpf_testmod.
We will move all of them from net/bpf/test_run.c to bpf_testmod eventually.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension
2022-11-28 1:57 ` Alexei Starovoitov
@ 2022-11-28 12:40 ` Yang Jihong
2022-11-28 16:41 ` Alexei Starovoitov
0 siblings, 1 reply; 12+ messages in thread
From: Yang Jihong @ 2022-11-28 12:40 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, illusionist.neo, linux, davem,
edumazet, kuba, pabeni, mykolal, shuah, benjamin.tissoires,
memxor, colin.i.king, asavkov, delyank, bpf, linux-arm-kernel,
linux-kernel, netdev, linux-kselftest
On 2022/11/28 9:57, Alexei Starovoitov wrote:
> On Sat, Nov 26, 2022 at 05:45:27PM +0800, Yang Jihong wrote:
>> 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 | 44 ++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 264b3dc714cc..193ea927aa69 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -1927,6 +1927,21 @@ find_kfunc_desc(const struct bpf_prog *prog, u32 func_id, u16 offset)
>> sizeof(tab->descs[0]), kfunc_desc_cmp_by_id_off);
>> }
>>
>> +static int kfunc_desc_cmp_by_imm(const void *a, const void *b);
>> +
>> +static const struct bpf_kfunc_desc *
>> +find_kfunc_desc_by_imm(const struct bpf_prog *prog, s32 imm)
>> +{
>> + struct bpf_kfunc_desc desc = {
>> + .imm = imm,
>> + };
>> + struct bpf_kfunc_desc_tab *tab;
>> +
>> + tab = prog->aux->kfunc_tab;
>> + return bsearch(&desc, tab->descs, tab->nr_descs,
>> + sizeof(tab->descs[0]), kfunc_desc_cmp_by_imm);
>> +}
>> +
>> static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env *env,
>> s16 offset)
>> {
>> @@ -2342,6 +2357,13 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
>> */
>> if (insn->src_reg == BPF_PSEUDO_CALL)
>> return false;
>> +
>> + /* Kfunc call will reach here because of insn_has_def32,
>> + * conservatively return TRUE.
>> + */
>> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
>> + return true;
>> +
>> /* Helper call will reach here because of arg type
>> * check, conservatively return TRUE.
>> */
>> @@ -2405,10 +2427,26 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
>> }
>>
>> /* Return the regno defined by the insn, or -1. */
>> -static int insn_def_regno(const struct bpf_insn *insn)
>> +static int insn_def_regno(struct bpf_verifier_env *env, const struct bpf_insn *insn)
>> {
>> switch (BPF_CLASS(insn->code)) {
>> case BPF_JMP:
>> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
>> + const struct bpf_kfunc_desc *desc;
>> +
>> + /* The value of desc cannot be NULL */
>> + desc = find_kfunc_desc_by_imm(env->prog, insn->imm);
>> +
>> + /* A kfunc can return void.
>> + * The btf type of the kfunc's return value needs
>> + * to be checked against "void" first
>> + */
>> + if (desc->func_model.ret_size == 0)
>> + return -1;
>> + else
>> + return insn->dst_reg;
>> + }
>> + fallthrough;
>
> I cannot make any sense of this patch.
> insn->dst_reg above is 0.
> The kfunc call doesn't define a register from insn_def_regno() pov.
>
> Are you hacking insn_def_regno() to return 0 so that
> if (WARN_ON(load_reg == -1)) {
> verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");
> return -EFAULT;
> }
> in opt_subreg_zext_lo32_rnd_hi32() doesn't trigger ?
>
> But this verifier message should have been a hint that you need
> to analyze why zext_dst is set on this kfunc call.
> Maybe it shouldn't ?
> Did you analyze the logic of mark_btf_func_reg_size() ?
make r0 zext is not caused by mark_btf_func_reg_size.
This problem occurs when running the kfunc_call_test_ref_btf_id test
case in the 32-bit ARM environment.
The bpf prog is as follows:
int kfunc_call_test_ref_btf_id(struct __sk_buff *skb)
{
struct prog_test_ref_kfunc *pt;
unsigned long s = 0;
int ret = 0;
pt = bpf_kfunc_call_test_acquire(&s);
if (pt) {
// here, do_check clears the upper 32bits of r0 through:
// check_alu_op
// ->check_reg_arg
// ->mark_insn_zext
if (pt->a != 42 || pt->b != 108)
ret = -1;
bpf_kfunc_call_test_release(pt);
}
return ret;
}
>
> Before producing any patches please understand the logic fully.
> Your commit log
> "insn_def_regno should
> return dst_reg for BPF_JMP type of BPF_PSEUDO_KFUNC_CALL."
>
> Makes no sense to me, since dst_reg is unused in JMP insn.
> There is no concept of a src or dst register in a JMP insn.
>
> 32-bit x86 supports calling kfuncs. See emit_kfunc_call().
> And we don't have this "verifier bug. zext_dst is set" issue there, right?
> But what you're saying in the commit log:
> "if data width of kfunc return value is 32 bits"
> should have been applicable to x86-32 as well.
> So please start with a test that demonstrates the issue on x86-32 and
> then we can discuss the way to fix it.
>
> The patch 2 sort-of makes sense.
>
> For patch 3 pls add new test funcs to bpf_testmod.
> We will move all of them from net/bpf/test_run.c to bpf_testmod eventually.
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension
2022-11-28 12:40 ` Yang Jihong
@ 2022-11-28 16:41 ` Alexei Starovoitov
2022-12-03 2:58 ` Yang Jihong
0 siblings, 1 reply; 12+ messages in thread
From: Alexei Starovoitov @ 2022-11-28 16:41 UTC (permalink / raw)
To: Yang Jihong
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Shubham Bansal,
Russell King, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Mykola Lysenko, Shuah Khan, Benjamin Tissoires,
Kumar Kartikeya Dwivedi, colin.i.king, Artem Savkov,
Delyan Kratunov, bpf, linux-arm-kernel, LKML,
Network Development, open list:KERNEL SELFTEST FRAMEWORK
On Mon, Nov 28, 2022 at 4:40 AM Yang Jihong <yangjihong1@huawei.com> wrote:
>
>
>
> On 2022/11/28 9:57, Alexei Starovoitov wrote:
> > On Sat, Nov 26, 2022 at 05:45:27PM +0800, Yang Jihong wrote:
> >> 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 | 44 ++++++++++++++++++++++++++++++++++++++++---
> >> 1 file changed, 41 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index 264b3dc714cc..193ea927aa69 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -1927,6 +1927,21 @@ find_kfunc_desc(const struct bpf_prog *prog, u32 func_id, u16 offset)
> >> sizeof(tab->descs[0]), kfunc_desc_cmp_by_id_off);
> >> }
> >>
> >> +static int kfunc_desc_cmp_by_imm(const void *a, const void *b);
> >> +
> >> +static const struct bpf_kfunc_desc *
> >> +find_kfunc_desc_by_imm(const struct bpf_prog *prog, s32 imm)
> >> +{
> >> + struct bpf_kfunc_desc desc = {
> >> + .imm = imm,
> >> + };
> >> + struct bpf_kfunc_desc_tab *tab;
> >> +
> >> + tab = prog->aux->kfunc_tab;
> >> + return bsearch(&desc, tab->descs, tab->nr_descs,
> >> + sizeof(tab->descs[0]), kfunc_desc_cmp_by_imm);
> >> +}
> >> +
> >> static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env *env,
> >> s16 offset)
> >> {
> >> @@ -2342,6 +2357,13 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
> >> */
> >> if (insn->src_reg == BPF_PSEUDO_CALL)
> >> return false;
> >> +
> >> + /* Kfunc call will reach here because of insn_has_def32,
> >> + * conservatively return TRUE.
> >> + */
> >> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
> >> + return true;
> >> +
> >> /* Helper call will reach here because of arg type
> >> * check, conservatively return TRUE.
> >> */
> >> @@ -2405,10 +2427,26 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
> >> }
> >>
> >> /* Return the regno defined by the insn, or -1. */
> >> -static int insn_def_regno(const struct bpf_insn *insn)
> >> +static int insn_def_regno(struct bpf_verifier_env *env, const struct bpf_insn *insn)
> >> {
> >> switch (BPF_CLASS(insn->code)) {
> >> case BPF_JMP:
> >> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> >> + const struct bpf_kfunc_desc *desc;
> >> +
> >> + /* The value of desc cannot be NULL */
> >> + desc = find_kfunc_desc_by_imm(env->prog, insn->imm);
> >> +
> >> + /* A kfunc can return void.
> >> + * The btf type of the kfunc's return value needs
> >> + * to be checked against "void" first
> >> + */
> >> + if (desc->func_model.ret_size == 0)
> >> + return -1;
> >> + else
> >> + return insn->dst_reg;
> >> + }
> >> + fallthrough;
> >
> > I cannot make any sense of this patch.
> > insn->dst_reg above is 0.
> > The kfunc call doesn't define a register from insn_def_regno() pov.
> >
> > Are you hacking insn_def_regno() to return 0 so that
> > if (WARN_ON(load_reg == -1)) {
> > verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");
> > return -EFAULT;
> > }
> > in opt_subreg_zext_lo32_rnd_hi32() doesn't trigger ?
> >
> > But this verifier message should have been a hint that you need
> > to analyze why zext_dst is set on this kfunc call.
> > Maybe it shouldn't ?
> > Did you analyze the logic of mark_btf_func_reg_size() ?
> make r0 zext is not caused by mark_btf_func_reg_size.
>
> This problem occurs when running the kfunc_call_test_ref_btf_id test
> case in the 32-bit ARM environment.
Why is it not failing on x86-32 ?
> The bpf prog is as follows:
> int kfunc_call_test_ref_btf_id(struct __sk_buff *skb)
> {
> struct prog_test_ref_kfunc *pt;
> unsigned long s = 0;
> int ret = 0;
>
> pt = bpf_kfunc_call_test_acquire(&s);
> if (pt) {
> // here, do_check clears the upper 32bits of r0 through:
> // check_alu_op
> // ->check_reg_arg
> // ->mark_insn_zext
> if (pt->a != 42 || pt->b != 108)
> ret = -1;
> bpf_kfunc_call_test_release(pt);
> }
> return ret;
> }
>
> >
> > Before producing any patches please understand the logic fully.
> > Your commit log
> > "insn_def_regno should
> > return dst_reg for BPF_JMP type of BPF_PSEUDO_KFUNC_CALL."
> >
> > Makes no sense to me, since dst_reg is unused in JMP insn.
> > There is no concept of a src or dst register in a JMP insn.
> >
> > 32-bit x86 supports calling kfuncs. See emit_kfunc_call().
> > And we don't have this "verifier bug. zext_dst is set" issue there, right?
> > But what you're saying in the commit log:
> > "if data width of kfunc return value is 32 bits"
> > should have been applicable to x86-32 as well.
> > So please start with a test that demonstrates the issue on x86-32 and
> > then we can discuss the way to fix it.
> >
> > The patch 2 sort-of makes sense.
> >
> > For patch 3 pls add new test funcs to bpf_testmod.
> > We will move all of them from net/bpf/test_run.c to bpf_testmod eventually.
> > .
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension
2022-11-28 16:41 ` Alexei Starovoitov
@ 2022-12-03 2:58 ` Yang Jihong
2022-12-03 16:40 ` Alexei Starovoitov
0 siblings, 1 reply; 12+ messages in thread
From: Yang Jihong @ 2022-12-03 2:58 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Shubham Bansal,
Russell King, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Mykola Lysenko, Shuah Khan, Benjamin Tissoires,
Kumar Kartikeya Dwivedi, colin.i.king, Artem Savkov,
Delyan Kratunov, bpf, linux-arm-kernel, LKML,
Network Development, open list:KERNEL SELFTEST FRAMEWORK
On 2022/11/29 0:41, Alexei Starovoitov wrote:
> On Mon, Nov 28, 2022 at 4:40 AM Yang Jihong <yangjihong1@huawei.com> wrote:
>>
>>
>>
>> On 2022/11/28 9:57, Alexei Starovoitov wrote:
>>> On Sat, Nov 26, 2022 at 05:45:27PM +0800, Yang Jihong wrote:
>>>> 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 | 44 ++++++++++++++++++++++++++++++++++++++++---
>>>> 1 file changed, 41 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>> index 264b3dc714cc..193ea927aa69 100644
>>>> --- a/kernel/bpf/verifier.c
>>>> +++ b/kernel/bpf/verifier.c
>>>> @@ -1927,6 +1927,21 @@ find_kfunc_desc(const struct bpf_prog *prog, u32 func_id, u16 offset)
>>>> sizeof(tab->descs[0]), kfunc_desc_cmp_by_id_off);
>>>> }
>>>>
>>>> +static int kfunc_desc_cmp_by_imm(const void *a, const void *b);
>>>> +
>>>> +static const struct bpf_kfunc_desc *
>>>> +find_kfunc_desc_by_imm(const struct bpf_prog *prog, s32 imm)
>>>> +{
>>>> + struct bpf_kfunc_desc desc = {
>>>> + .imm = imm,
>>>> + };
>>>> + struct bpf_kfunc_desc_tab *tab;
>>>> +
>>>> + tab = prog->aux->kfunc_tab;
>>>> + return bsearch(&desc, tab->descs, tab->nr_descs,
>>>> + sizeof(tab->descs[0]), kfunc_desc_cmp_by_imm);
>>>> +}
>>>> +
>>>> static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env *env,
>>>> s16 offset)
>>>> {
>>>> @@ -2342,6 +2357,13 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>>> */
>>>> if (insn->src_reg == BPF_PSEUDO_CALL)
>>>> return false;
>>>> +
>>>> + /* Kfunc call will reach here because of insn_has_def32,
>>>> + * conservatively return TRUE.
>>>> + */
>>>> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
>>>> + return true;
>>>> +
>>>> /* Helper call will reach here because of arg type
>>>> * check, conservatively return TRUE.
>>>> */
>>>> @@ -2405,10 +2427,26 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>>> }
>>>>
>>>> /* Return the regno defined by the insn, or -1. */
>>>> -static int insn_def_regno(const struct bpf_insn *insn)
>>>> +static int insn_def_regno(struct bpf_verifier_env *env, const struct bpf_insn *insn)
>>>> {
>>>> switch (BPF_CLASS(insn->code)) {
>>>> case BPF_JMP:
>>>> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
>>>> + const struct bpf_kfunc_desc *desc;
>>>> +
>>>> + /* The value of desc cannot be NULL */
>>>> + desc = find_kfunc_desc_by_imm(env->prog, insn->imm);
>>>> +
>>>> + /* A kfunc can return void.
>>>> + * The btf type of the kfunc's return value needs
>>>> + * to be checked against "void" first
>>>> + */
>>>> + if (desc->func_model.ret_size == 0)
>>>> + return -1;
>>>> + else
>>>> + return insn->dst_reg;
>>>> + }
>>>> + fallthrough;
>>>
>>> I cannot make any sense of this patch.
>>> insn->dst_reg above is 0.
>>> The kfunc call doesn't define a register from insn_def_regno() pov.
>>>
>>> Are you hacking insn_def_regno() to return 0 so that
>>> if (WARN_ON(load_reg == -1)) {
>>> verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");
>>> return -EFAULT;
>>> }
>>> in opt_subreg_zext_lo32_rnd_hi32() doesn't trigger ?
>>>
>>> But this verifier message should have been a hint that you need
>>> to analyze why zext_dst is set on this kfunc call.
>>> Maybe it shouldn't ?
>>> Did you analyze the logic of mark_btf_func_reg_size() ?
>> make r0 zext is not caused by mark_btf_func_reg_size.
>>
>> This problem occurs when running the kfunc_call_test_ref_btf_id test
>> case in the 32-bit ARM environment.
>
> Why is it not failing on x86-32 ?
Use the latest mainline kernel code to test on the x86_32 machine. The
test also fails:
# ./test_progs -t kfunc_call/kfunc_call_test_ref_btf_id
Failed to load bpf_testmod.ko into the kernel: -8
WARNING! Selftests relying on bpf_testmod.ko will be skipped.
libbpf: prog 'kfunc_call_test_ref_btf_id': BPF program load failed:
Bad address
libbpf: prog 'kfunc_call_test_ref_btf_id': -- BEGIN PROG LOAD LOG --
processed 25 insns (limit 1000000) max_states_per_insn 0 total_states
2 peak_states 2 mark_read 1
-- END PROG LOAD LOG --
libbpf: prog 'kfunc_call_test_ref_btf_id': failed to load: -14
libbpf: failed to load object 'kfunc_call_test'
libbpf: failed to load BPF skeleton 'kfunc_call_test': -14
verify_success:FAIL:skel unexpected error: -14
Therefore, this problem also exists on x86_32:
"verifier bug. zext_dst is set, but no reg is defined"
>
>> The bpf prog is as follows:
>> int kfunc_call_test_ref_btf_id(struct __sk_buff *skb)
>> {
>> struct prog_test_ref_kfunc *pt;
>> unsigned long s = 0;
>> int ret = 0;
>>
>> pt = bpf_kfunc_call_test_acquire(&s);
>> if (pt) {
>> // here, do_check clears the upper 32bits of r0 through:
>> // check_alu_op
>> // ->check_reg_arg
>> // ->mark_insn_zext
>> if (pt->a != 42 || pt->b != 108)
>> ret = -1;
>> bpf_kfunc_call_test_release(pt);
>> }
>> return ret;
>> }
>>
>>>
>>> Before producing any patches please understand the logic fully.
>>> Your commit log
>>> "insn_def_regno should
>>> return dst_reg for BPF_JMP type of BPF_PSEUDO_KFUNC_CALL."
>>>
>>> Makes no sense to me, since dst_reg is unused in JMP insn.
>>> There is no concept of a src or dst register in a JMP insn.
>>>
>>> 32-bit x86 supports calling kfuncs. See emit_kfunc_call().
>>> And we don't have this "verifier bug. zext_dst is set" issue there, right?
>>> But what you're saying in the commit log:
>>> "if data width of kfunc return value is 32 bits"
>>> should have been applicable to x86-32 as well.
>>> So please start with a test that demonstrates the issue on x86-32 and
>>> then we can discuss the way to fix it.
>>>
>>> The patch 2 sort-of makes sense.
>>>
>>> For patch 3 pls add new test funcs to bpf_testmod.
>>> We will move all of them from net/bpf/test_run.c to bpf_testmod eventually.
>>> .
>>>
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension
2022-12-03 2:58 ` Yang Jihong
@ 2022-12-03 16:40 ` Alexei Starovoitov
2022-12-05 1:19 ` Yang Jihong
0 siblings, 1 reply; 12+ messages in thread
From: Alexei Starovoitov @ 2022-12-03 16:40 UTC (permalink / raw)
To: Yang Jihong
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Shubham Bansal,
Russell King, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Mykola Lysenko, Shuah Khan, Benjamin Tissoires,
Kumar Kartikeya Dwivedi, colin.i.king, Artem Savkov,
Delyan Kratunov, bpf, linux-arm-kernel, LKML,
Network Development, open list:KERNEL SELFTEST FRAMEWORK
On Fri, Dec 2, 2022 at 6:58 PM Yang Jihong <yangjihong1@huawei.com> wrote:
>
>
>
> On 2022/11/29 0:41, Alexei Starovoitov wrote:
> > On Mon, Nov 28, 2022 at 4:40 AM Yang Jihong <yangjihong1@huawei.com> wrote:
> >>
> >>
> >>
> >> On 2022/11/28 9:57, Alexei Starovoitov wrote:
> >>> On Sat, Nov 26, 2022 at 05:45:27PM +0800, Yang Jihong wrote:
> >>>> 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 | 44 ++++++++++++++++++++++++++++++++++++++++---
> >>>> 1 file changed, 41 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >>>> index 264b3dc714cc..193ea927aa69 100644
> >>>> --- a/kernel/bpf/verifier.c
> >>>> +++ b/kernel/bpf/verifier.c
> >>>> @@ -1927,6 +1927,21 @@ find_kfunc_desc(const struct bpf_prog *prog, u32 func_id, u16 offset)
> >>>> sizeof(tab->descs[0]), kfunc_desc_cmp_by_id_off);
> >>>> }
> >>>>
> >>>> +static int kfunc_desc_cmp_by_imm(const void *a, const void *b);
> >>>> +
> >>>> +static const struct bpf_kfunc_desc *
> >>>> +find_kfunc_desc_by_imm(const struct bpf_prog *prog, s32 imm)
> >>>> +{
> >>>> + struct bpf_kfunc_desc desc = {
> >>>> + .imm = imm,
> >>>> + };
> >>>> + struct bpf_kfunc_desc_tab *tab;
> >>>> +
> >>>> + tab = prog->aux->kfunc_tab;
> >>>> + return bsearch(&desc, tab->descs, tab->nr_descs,
> >>>> + sizeof(tab->descs[0]), kfunc_desc_cmp_by_imm);
> >>>> +}
> >>>> +
> >>>> static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env *env,
> >>>> s16 offset)
> >>>> {
> >>>> @@ -2342,6 +2357,13 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
> >>>> */
> >>>> if (insn->src_reg == BPF_PSEUDO_CALL)
> >>>> return false;
> >>>> +
> >>>> + /* Kfunc call will reach here because of insn_has_def32,
> >>>> + * conservatively return TRUE.
> >>>> + */
> >>>> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
> >>>> + return true;
> >>>> +
> >>>> /* Helper call will reach here because of arg type
> >>>> * check, conservatively return TRUE.
> >>>> */
> >>>> @@ -2405,10 +2427,26 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
> >>>> }
> >>>>
> >>>> /* Return the regno defined by the insn, or -1. */
> >>>> -static int insn_def_regno(const struct bpf_insn *insn)
> >>>> +static int insn_def_regno(struct bpf_verifier_env *env, const struct bpf_insn *insn)
> >>>> {
> >>>> switch (BPF_CLASS(insn->code)) {
> >>>> case BPF_JMP:
> >>>> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> >>>> + const struct bpf_kfunc_desc *desc;
> >>>> +
> >>>> + /* The value of desc cannot be NULL */
> >>>> + desc = find_kfunc_desc_by_imm(env->prog, insn->imm);
> >>>> +
> >>>> + /* A kfunc can return void.
> >>>> + * The btf type of the kfunc's return value needs
> >>>> + * to be checked against "void" first
> >>>> + */
> >>>> + if (desc->func_model.ret_size == 0)
> >>>> + return -1;
> >>>> + else
> >>>> + return insn->dst_reg;
> >>>> + }
> >>>> + fallthrough;
> >>>
> >>> I cannot make any sense of this patch.
> >>> insn->dst_reg above is 0.
> >>> The kfunc call doesn't define a register from insn_def_regno() pov.
> >>>
> >>> Are you hacking insn_def_regno() to return 0 so that
> >>> if (WARN_ON(load_reg == -1)) {
> >>> verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");
> >>> return -EFAULT;
> >>> }
> >>> in opt_subreg_zext_lo32_rnd_hi32() doesn't trigger ?
> >>>
> >>> But this verifier message should have been a hint that you need
> >>> to analyze why zext_dst is set on this kfunc call.
> >>> Maybe it shouldn't ?
> >>> Did you analyze the logic of mark_btf_func_reg_size() ?
> >> make r0 zext is not caused by mark_btf_func_reg_size.
> >>
> >> This problem occurs when running the kfunc_call_test_ref_btf_id test
> >> case in the 32-bit ARM environment.
> >
> > Why is it not failing on x86-32 ?
> Use the latest mainline kernel code to test on the x86_32 machine. The
> test also fails:
>
> # ./test_progs -t kfunc_call/kfunc_call_test_ref_btf_id
> Failed to load bpf_testmod.ko into the kernel: -8
> WARNING! Selftests relying on bpf_testmod.ko will be skipped.
> libbpf: prog 'kfunc_call_test_ref_btf_id': BPF program load failed:
> Bad address
> libbpf: prog 'kfunc_call_test_ref_btf_id': -- BEGIN PROG LOAD LOG --
> processed 25 insns (limit 1000000) max_states_per_insn 0 total_states
> 2 peak_states 2 mark_read 1
> -- END PROG LOAD LOG --
> libbpf: prog 'kfunc_call_test_ref_btf_id': failed to load: -14
> libbpf: failed to load object 'kfunc_call_test'
> libbpf: failed to load BPF skeleton 'kfunc_call_test': -14
> verify_success:FAIL:skel unexpected error: -14
>
> Therefore, this problem also exists on x86_32:
> "verifier bug. zext_dst is set, but no reg is defined"
The kernel returns -14 == EFAULT.
That's a completely different issue.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension
2022-12-03 16:40 ` Alexei Starovoitov
@ 2022-12-05 1:19 ` Yang Jihong
2022-12-07 8:49 ` Yang Jihong
0 siblings, 1 reply; 12+ messages in thread
From: Yang Jihong @ 2022-12-05 1:19 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Shubham Bansal,
Russell King, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Mykola Lysenko, Shuah Khan, Benjamin Tissoires,
Kumar Kartikeya Dwivedi, colin.i.king, Artem Savkov,
Delyan Kratunov, bpf, linux-arm-kernel, LKML,
Network Development, open list:KERNEL SELFTEST FRAMEWORK
On 2022/12/4 0:40, Alexei Starovoitov wrote:
> On Fri, Dec 2, 2022 at 6:58 PM Yang Jihong <yangjihong1@huawei.com> wrote:
>>
>>
>>
>> On 2022/11/29 0:41, Alexei Starovoitov wrote:
>>> On Mon, Nov 28, 2022 at 4:40 AM Yang Jihong <yangjihong1@huawei.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2022/11/28 9:57, Alexei Starovoitov wrote:
>>>>> On Sat, Nov 26, 2022 at 05:45:27PM +0800, Yang Jihong wrote:
>>>>>> 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 | 44 ++++++++++++++++++++++++++++++++++++++++---
>>>>>> 1 file changed, 41 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>>>> index 264b3dc714cc..193ea927aa69 100644
>>>>>> --- a/kernel/bpf/verifier.c
>>>>>> +++ b/kernel/bpf/verifier.c
>>>>>> @@ -1927,6 +1927,21 @@ find_kfunc_desc(const struct bpf_prog *prog, u32 func_id, u16 offset)
>>>>>> sizeof(tab->descs[0]), kfunc_desc_cmp_by_id_off);
>>>>>> }
>>>>>>
>>>>>> +static int kfunc_desc_cmp_by_imm(const void *a, const void *b);
>>>>>> +
>>>>>> +static const struct bpf_kfunc_desc *
>>>>>> +find_kfunc_desc_by_imm(const struct bpf_prog *prog, s32 imm)
>>>>>> +{
>>>>>> + struct bpf_kfunc_desc desc = {
>>>>>> + .imm = imm,
>>>>>> + };
>>>>>> + struct bpf_kfunc_desc_tab *tab;
>>>>>> +
>>>>>> + tab = prog->aux->kfunc_tab;
>>>>>> + return bsearch(&desc, tab->descs, tab->nr_descs,
>>>>>> + sizeof(tab->descs[0]), kfunc_desc_cmp_by_imm);
>>>>>> +}
>>>>>> +
>>>>>> static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env *env,
>>>>>> s16 offset)
>>>>>> {
>>>>>> @@ -2342,6 +2357,13 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>>>>> */
>>>>>> if (insn->src_reg == BPF_PSEUDO_CALL)
>>>>>> return false;
>>>>>> +
>>>>>> + /* Kfunc call will reach here because of insn_has_def32,
>>>>>> + * conservatively return TRUE.
>>>>>> + */
>>>>>> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
>>>>>> + return true;
>>>>>> +
>>>>>> /* Helper call will reach here because of arg type
>>>>>> * check, conservatively return TRUE.
>>>>>> */
>>>>>> @@ -2405,10 +2427,26 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>>>>> }
>>>>>>
>>>>>> /* Return the regno defined by the insn, or -1. */
>>>>>> -static int insn_def_regno(const struct bpf_insn *insn)
>>>>>> +static int insn_def_regno(struct bpf_verifier_env *env, const struct bpf_insn *insn)
>>>>>> {
>>>>>> switch (BPF_CLASS(insn->code)) {
>>>>>> case BPF_JMP:
>>>>>> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
>>>>>> + const struct bpf_kfunc_desc *desc;
>>>>>> +
>>>>>> + /* The value of desc cannot be NULL */
>>>>>> + desc = find_kfunc_desc_by_imm(env->prog, insn->imm);
>>>>>> +
>>>>>> + /* A kfunc can return void.
>>>>>> + * The btf type of the kfunc's return value needs
>>>>>> + * to be checked against "void" first
>>>>>> + */
>>>>>> + if (desc->func_model.ret_size == 0)
>>>>>> + return -1;
>>>>>> + else
>>>>>> + return insn->dst_reg;
>>>>>> + }
>>>>>> + fallthrough;
>>>>>
>>>>> I cannot make any sense of this patch.
>>>>> insn->dst_reg above is 0.
>>>>> The kfunc call doesn't define a register from insn_def_regno() pov.
>>>>>
>>>>> Are you hacking insn_def_regno() to return 0 so that
>>>>> if (WARN_ON(load_reg == -1)) {
>>>>> verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");
>>>>> return -EFAULT;
>>>>> }
>>>>> in opt_subreg_zext_lo32_rnd_hi32() doesn't trigger ?
>>>>>
>>>>> But this verifier message should have been a hint that you need
>>>>> to analyze why zext_dst is set on this kfunc call.
>>>>> Maybe it shouldn't ?
>>>>> Did you analyze the logic of mark_btf_func_reg_size() ?
>>>> make r0 zext is not caused by mark_btf_func_reg_size.
>>>>
>>>> This problem occurs when running the kfunc_call_test_ref_btf_id test
>>>> case in the 32-bit ARM environment.
>>>
>>> Why is it not failing on x86-32 ?
>> Use the latest mainline kernel code to test on the x86_32 machine. The
>> test also fails:
>>
>> # ./test_progs -t kfunc_call/kfunc_call_test_ref_btf_id
>> Failed to load bpf_testmod.ko into the kernel: -8
>> WARNING! Selftests relying on bpf_testmod.ko will be skipped.
>> libbpf: prog 'kfunc_call_test_ref_btf_id': BPF program load failed:
>> Bad address
>> libbpf: prog 'kfunc_call_test_ref_btf_id': -- BEGIN PROG LOAD LOG --
>> processed 25 insns (limit 1000000) max_states_per_insn 0 total_states
>> 2 peak_states 2 mark_read 1
>> -- END PROG LOAD LOG --
>> libbpf: prog 'kfunc_call_test_ref_btf_id': failed to load: -14
>> libbpf: failed to load object 'kfunc_call_test'
>> libbpf: failed to load BPF skeleton 'kfunc_call_test': -14
>> verify_success:FAIL:skel unexpected error: -14
>>
>> Therefore, this problem also exists on x86_32:
>> "verifier bug. zext_dst is set, but no reg is defined"
>
> The kernel returns -14 == EFAULT.
> That's a completely different issue.
It's the same problem. The opt_subreg_zext_lo32_rnd_hi32 function fails
to check here and returns -EFAULT
opt_subreg_zext_lo32_rnd_hi32 {
...
if (WARN_ON(load_reg == -1)) {
verbose(env, "verifier bug. zext_dst is set, but no reg is
defined\n");
return -EFAULT;
}
...
}
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension
2022-12-05 1:19 ` Yang Jihong
@ 2022-12-07 8:49 ` Yang Jihong
0 siblings, 0 replies; 12+ messages in thread
From: Yang Jihong @ 2022-12-07 8:49 UTC (permalink / raw)
To: Alexei Starovoitov, Yonghong Song, bjorn
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Shubham Bansal,
Russell King, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Mykola Lysenko, Shuah Khan, Benjamin Tissoires,
Kumar Kartikeya Dwivedi, colin.i.king, Artem Savkov,
Delyan Kratunov, bpf, linux-arm-kernel, LKML,
Network Development, open list:KERNEL SELFTEST FRAMEWORK
Hello,
On 2022/12/5 9:19, Yang Jihong wrote:
>
>
> On 2022/12/4 0:40, Alexei Starovoitov wrote:
>> On Fri, Dec 2, 2022 at 6:58 PM Yang Jihong <yangjihong1@huawei.com>
>> wrote:
>>>
>>>
>>>
>>> On 2022/11/29 0:41, Alexei Starovoitov wrote:
>>>> On Mon, Nov 28, 2022 at 4:40 AM Yang Jihong <yangjihong1@huawei.com>
>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2022/11/28 9:57, Alexei Starovoitov wrote:
>>>>>> On Sat, Nov 26, 2022 at 05:45:27PM +0800, Yang Jihong wrote:
>>>>>>> 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 | 44
>>>>>>> ++++++++++++++++++++++++++++++++++++++++---
>>>>>>> 1 file changed, 41 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>>>>> index 264b3dc714cc..193ea927aa69 100644
>>>>>>> --- a/kernel/bpf/verifier.c
>>>>>>> +++ b/kernel/bpf/verifier.c
>>>>>>> @@ -1927,6 +1927,21 @@ find_kfunc_desc(const struct bpf_prog
>>>>>>> *prog, u32 func_id, u16 offset)
>>>>>>> sizeof(tab->descs[0]),
>>>>>>> kfunc_desc_cmp_by_id_off);
>>>>>>> }
>>>>>>>
>>>>>>> +static int kfunc_desc_cmp_by_imm(const void *a, const void *b);
>>>>>>> +
>>>>>>> +static const struct bpf_kfunc_desc *
>>>>>>> +find_kfunc_desc_by_imm(const struct bpf_prog *prog, s32 imm)
>>>>>>> +{
>>>>>>> + struct bpf_kfunc_desc desc = {
>>>>>>> + .imm = imm,
>>>>>>> + };
>>>>>>> + struct bpf_kfunc_desc_tab *tab;
>>>>>>> +
>>>>>>> + tab = prog->aux->kfunc_tab;
>>>>>>> + return bsearch(&desc, tab->descs, tab->nr_descs,
>>>>>>> + sizeof(tab->descs[0]), kfunc_desc_cmp_by_imm);
>>>>>>> +}
>>>>>>> +
>>>>>>> static struct btf *__find_kfunc_desc_btf(struct
>>>>>>> bpf_verifier_env *env,
>>>>>>> s16 offset)
>>>>>>> {
>>>>>>> @@ -2342,6 +2357,13 @@ static bool is_reg64(struct
>>>>>>> bpf_verifier_env *env, struct bpf_insn *insn,
>>>>>>> */
>>>>>>> if (insn->src_reg == BPF_PSEUDO_CALL)
>>>>>>> return false;
>>>>>>> +
>>>>>>> + /* Kfunc call will reach here because of
>>>>>>> insn_has_def32,
>>>>>>> + * conservatively return TRUE.
>>>>>>> + */
>>>>>>> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
>>>>>>> + return true;
>>>>>>> +
>>>>>>> /* Helper call will reach here because of
>>>>>>> arg type
>>>>>>> * check, conservatively return TRUE.
>>>>>>> */
>>>>>>> @@ -2405,10 +2427,26 @@ static bool is_reg64(struct
>>>>>>> bpf_verifier_env *env, struct bpf_insn *insn,
>>>>>>> }
>>>>>>>
>>>>>>> /* Return the regno defined by the insn, or -1. */
>>>>>>> -static int insn_def_regno(const struct bpf_insn *insn)
>>>>>>> +static int insn_def_regno(struct bpf_verifier_env *env, const
>>>>>>> struct bpf_insn *insn)
>>>>>>> {
>>>>>>> switch (BPF_CLASS(insn->code)) {
>>>>>>> case BPF_JMP:
>>>>>>> + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
>>>>>>> + const struct bpf_kfunc_desc *desc;
>>>>>>> +
>>>>>>> + /* The value of desc cannot be NULL */
>>>>>>> + desc = find_kfunc_desc_by_imm(env->prog,
>>>>>>> insn->imm);
>>>>>>> +
>>>>>>> + /* A kfunc can return void.
>>>>>>> + * The btf type of the kfunc's return value
>>>>>>> needs
>>>>>>> + * to be checked against "void" first
>>>>>>> + */
>>>>>>> + if (desc->func_model.ret_size == 0)
>>>>>>> + return -1;
>>>>>>> + else
>>>>>>> + return insn->dst_reg;
>>>>>>> + }
>>>>>>> + fallthrough;
>>>>>>
>>>>>> I cannot make any sense of this patch.
>>>>>> insn->dst_reg above is 0.
>>>>>> The kfunc call doesn't define a register from insn_def_regno() pov.
>>>>>>
>>>>>> Are you hacking insn_def_regno() to return 0 so that
>>>>>> if (WARN_ON(load_reg == -1)) {
>>>>>> verbose(env, "verifier bug. zext_dst is set, but no reg is
>>>>>> defined\n");
>>>>>> return -EFAULT;
>>>>>> }
>>>>>> in opt_subreg_zext_lo32_rnd_hi32() doesn't trigger ?
>>>>>>
>>>>>> But this verifier message should have been a hint that you need
>>>>>> to analyze why zext_dst is set on this kfunc call.
>>>>>> Maybe it shouldn't ?
>>>>>> Did you analyze the logic of mark_btf_func_reg_size() ?
>>>>> make r0 zext is not caused by mark_btf_func_reg_size.
>>>>>
>>>>> This problem occurs when running the kfunc_call_test_ref_btf_id test
>>>>> case in the 32-bit ARM environment.
>>>>
>>>> Why is it not failing on x86-32 ?
>>> Use the latest mainline kernel code to test on the x86_32 machine. The
>>> test also fails:
>>>
>>> # ./test_progs -t kfunc_call/kfunc_call_test_ref_btf_id
>>> Failed to load bpf_testmod.ko into the kernel: -8
>>> WARNING! Selftests relying on bpf_testmod.ko will be skipped.
>>> libbpf: prog 'kfunc_call_test_ref_btf_id': BPF program load failed:
>>> Bad address
>>> libbpf: prog 'kfunc_call_test_ref_btf_id': -- BEGIN PROG LOAD LOG --
>>> processed 25 insns (limit 1000000) max_states_per_insn 0
>>> total_states
>>> 2 peak_states 2 mark_read 1
>>> -- END PROG LOAD LOG --
>>> libbpf: prog 'kfunc_call_test_ref_btf_id': failed to load: -14
>>> libbpf: failed to load object 'kfunc_call_test'
>>> libbpf: failed to load BPF skeleton 'kfunc_call_test': -14
>>> verify_success:FAIL:skel unexpected error: -14
>>>
>>> Therefore, this problem also exists on x86_32:
>>> "verifier bug. zext_dst is set, but no reg is defined"
>>
>> The kernel returns -14 == EFAULT.
>> That's a completely different issue.
> It's the same problem. The opt_subreg_zext_lo32_rnd_hi32 function fails
> to check here and returns -EFAULT
>
> opt_subreg_zext_lo32_rnd_hi32 {
> ...
> if (WARN_ON(load_reg == -1)) {
> verbose(env, "verifier bug. zext_dst is set, but no reg is
> defined\n");
> return -EFAULT;
> }
> ...
> }
>> .
I see that there are emails from the community talking about the same
problem, and come up with a solution:
https://lore.kernel.org/bpf/20221202103620.1915679-1-bjorn@kernel.org/T/
will remove this patch based on that patch.
Thanks,
Yang
>>
>
> .
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-12-07 8:51 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-26 9:45 [PATCH bpf-next v3 0/4] bpf: Support kernel function call in 32-bit ARM Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 1/4] bpf: Adapt 32-bit return value kfunc for 32-bit ARM when zext extension Yang Jihong
2022-11-28 1:57 ` Alexei Starovoitov
2022-11-28 12:40 ` Yang Jihong
2022-11-28 16:41 ` Alexei Starovoitov
2022-12-03 2:58 ` Yang Jihong
2022-12-03 16:40 ` Alexei Starovoitov
2022-12-05 1:19 ` Yang Jihong
2022-12-07 8:49 ` Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 2/4] bpf: Add kernel function call support in 32-bit ARM for EABI Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 3/4] bpf:selftests: Add kfunc_call test for mixing 32-bit and 64-bit parameters Yang Jihong
2022-11-26 9:45 ` [PATCH bpf-next v3 4/4] bpf: Fix comment error in fixup_kfunc_call function Yang Jihong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).