| Message ID | 20240629094733.3863850-4-eddyz87@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | no_caller_saved_registers attribute for helper calls | expand |
On Sat, Jun 29, 2024 at 2:48 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On x86 verifier replaces call to bpf_get_smp_processor_id() with a > sequence of instructions that modify only R0. > This satisfies attribute no_caller_saved_registers contract. > Allow rewrite of no_caller_saved_registers patterns for > bpf_get_smp_processor_id() in order to use this function > as a canary for no_caller_saved_registers tests. > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > --- > kernel/bpf/helpers.c | 1 + > kernel/bpf/verifier.c | 11 +++++++++-- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c > index 229396172026..1df01f454590 100644 > --- a/kernel/bpf/helpers.c > +++ b/kernel/bpf/helpers.c > @@ -158,6 +158,7 @@ const struct bpf_func_proto bpf_get_smp_processor_id_proto = { > .func = bpf_get_smp_processor_id, > .gpl_only = false, > .ret_type = RET_INTEGER, > + .nocsr = true, I'm wondering if we should call this flag in such a way that it's clear that this is more of an request, while the actual nocsr cleanup and stuff is done only if BPF verifier/BPF JIT support that for specific architecture/config/etc? > }; > > BPF_CALL_0(bpf_get_numa_node_id) > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 1340d3e60d30..d68ed70186c8 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -16030,7 +16030,14 @@ static u8 get_helper_reg_mask(const struct bpf_func_proto *fn) > */ > static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) > { > - return false; > + switch (imm) { > +#ifdef CONFIG_X86_64 > + case BPF_FUNC_get_smp_processor_id: > + return env->prog->jit_requested && bpf_jit_supports_percpu_insn(); > +#endif please see bpf_jit_inlines_helper_call(), arm64 and risc-v inline it in JIT, so we need to validate they don't assume any of R1-R5 register to be a scratch register > + default: > + return false; > + } > } > > /* If 'insn' is a call that follows no_caller_saved_registers contract > @@ -20666,7 +20673,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env) > #ifdef CONFIG_X86_64 > /* Implement bpf_get_smp_processor_id() inline. */ > if (insn->imm == BPF_FUNC_get_smp_processor_id && > - prog->jit_requested && bpf_jit_supports_percpu_insn()) { > + verifier_inlines_helper_call(env, insn->imm)) { > /* BPF_FUNC_get_smp_processor_id inlining is an > * optimization, so if pcpu_hot.cpu_number is ever > * changed in some incompatible and hard to support > -- > 2.45.2 >
On Mon, 2024-07-01 at 17:41 -0700, Andrii Nakryiko wrote: [...] > > @@ -158,6 +158,7 @@ const struct bpf_func_proto bpf_get_smp_processor_id_proto = { > > .func = bpf_get_smp_processor_id, > > .gpl_only = false, > > .ret_type = RET_INTEGER, > > + .nocsr = true, > > I'm wondering if we should call this flag in such a way that it's > clear that this is more of an request, while the actual nocsr cleanup > and stuff is done only if BPF verifier/BPF JIT support that for > specific architecture/config/etc? Can change to .allow_nocsr. On the other hand, can remove this flag completely and rely on call_csr_mask(). [...] > > @@ -16030,7 +16030,14 @@ static u8 get_helper_reg_mask(const struct bpf_func_proto *fn) > > */ > > static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) > > { > > - return false; > > + switch (imm) { > > +#ifdef CONFIG_X86_64 > > + case BPF_FUNC_get_smp_processor_id: > > + return env->prog->jit_requested && bpf_jit_supports_percpu_insn(); > > +#endif > > please see bpf_jit_inlines_helper_call(), arm64 and risc-v inline it > in JIT, so we need to validate they don't assume any of R1-R5 register > to be a scratch register At the moment I return false for this archs. Or do you suggest these to be added in the current patch-set? [...]
On Tue, Jul 2, 2024 at 1:44 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Mon, 2024-07-01 at 17:41 -0700, Andrii Nakryiko wrote: > > [...] > > > > @@ -158,6 +158,7 @@ const struct bpf_func_proto bpf_get_smp_processor_id_proto = { > > > .func = bpf_get_smp_processor_id, > > > .gpl_only = false, > > > .ret_type = RET_INTEGER, > > > + .nocsr = true, > > > > I'm wondering if we should call this flag in such a way that it's > > clear that this is more of an request, while the actual nocsr cleanup > > and stuff is done only if BPF verifier/BPF JIT support that for > > specific architecture/config/etc? > > Can change to .allow_nocsr. On the other hand, can remove this flag > completely and rely on call_csr_mask(). I like the declaration that helper is eligible to be close to helper definition, so I'd definitely keep it, but yeah "allow_nocsr" seems betterto me > > [...] > > > > @@ -16030,7 +16030,14 @@ static u8 get_helper_reg_mask(const struct bpf_func_proto *fn) > > > */ > > > static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) > > > { > > > - return false; > > > + switch (imm) { > > > +#ifdef CONFIG_X86_64 > > > + case BPF_FUNC_get_smp_processor_id: > > > + return env->prog->jit_requested && bpf_jit_supports_percpu_insn(); > > > +#endif > > > > please see bpf_jit_inlines_helper_call(), arm64 and risc-v inline it > > in JIT, so we need to validate they don't assume any of R1-R5 register > > to be a scratch register > > At the moment I return false for this archs. > Or do you suggest these to be added in the current patch-set? I'd add them from the get go. CC Puranjay to double-check? > > [...]
On Tue, 2024-07-02 at 14:11 -0700, Andrii Nakryiko wrote: [...] > > > > +#ifdef CONFIG_X86_64 > > > > + case BPF_FUNC_get_smp_processor_id: > > > > + return env->prog->jit_requested && bpf_jit_supports_percpu_insn(); > > > > +#endif > > > > > > please see bpf_jit_inlines_helper_call(), arm64 and risc-v inline it > > > in JIT, so we need to validate they don't assume any of R1-R5 register > > > to be a scratch register > > > > At the moment I return false for this archs. > > Or do you suggest these to be added in the current patch-set? > > I'd add them from the get go. CC Puranjay to double-check? I'll give it a try.
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Tue, Jul 2, 2024 at 1:44 PM Eduard Zingerman <eddyz87@gmail.com> wrote: >> >> On Mon, 2024-07-01 at 17:41 -0700, Andrii Nakryiko wrote: >> >> [...] >> >> > > @@ -158,6 +158,7 @@ const struct bpf_func_proto bpf_get_smp_processor_id_proto = { >> > > .func = bpf_get_smp_processor_id, >> > > .gpl_only = false, >> > > .ret_type = RET_INTEGER, >> > > + .nocsr = true, >> > >> > I'm wondering if we should call this flag in such a way that it's >> > clear that this is more of an request, while the actual nocsr cleanup >> > and stuff is done only if BPF verifier/BPF JIT support that for >> > specific architecture/config/etc? >> >> Can change to .allow_nocsr. On the other hand, can remove this flag >> completely and rely on call_csr_mask(). > > I like the declaration that helper is eligible to be close to helper > definition, so I'd definitely keep it, but yeah "allow_nocsr" seems > betterto me > >> >> [...] >> >> > > @@ -16030,7 +16030,14 @@ static u8 get_helper_reg_mask(const struct bpf_func_proto *fn) >> > > */ >> > > static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) >> > > { >> > > - return false; >> > > + switch (imm) { >> > > +#ifdef CONFIG_X86_64 >> > > + case BPF_FUNC_get_smp_processor_id: >> > > + return env->prog->jit_requested && bpf_jit_supports_percpu_insn(); >> > > +#endif >> > >> > please see bpf_jit_inlines_helper_call(), arm64 and risc-v inline it >> > in JIT, so we need to validate they don't assume any of R1-R5 register >> > to be a scratch register They don't assume any register to be scratch (except R0) so we can enable this on arm64 and riscv. >> >> At the moment I return false for this archs. Yes, verifier_inlines_helper_call() should keep returning false for arm64 and risc-v. >> Or do you suggest these to be added in the current patch-set? The correct way to do this would be to change call_csr_mask() to have: verifier_inlines_helper_call(env, insn->imm) || bpf_jit_inlines_helper_call(insn->imm) > I'd add them from the get go. CC Puranjay to double-check? Thanks, Puranjay
On Wed, 2024-07-03 at 11:27 +0000, Puranjay Mohan wrote: [...] > > > > > @@ -16030,7 +16030,14 @@ static u8 get_helper_reg_mask(const struct bpf_func_proto *fn) > > > > > */ > > > > > static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) > > > > > { > > > > > - return false; > > > > > + switch (imm) { > > > > > +#ifdef CONFIG_X86_64 > > > > > + case BPF_FUNC_get_smp_processor_id: > > > > > + return env->prog->jit_requested && bpf_jit_supports_percpu_insn(); > > > > > +#endif > > > > > > > > please see bpf_jit_inlines_helper_call(), arm64 and risc-v inline it > > > > in JIT, so we need to validate they don't assume any of R1-R5 register > > > > to be a scratch register > > They don't assume any register to be scratch (except R0) so we can > enable this on arm64 and riscv. Puranjay, just out of curiosity and tangential to this patch-set, I see that get_smp_processor_id is implemented as follows in riscv jit: emit_ld(bpf_to_rv_reg(BPF_REG_0, ctx), offsetof(struct thread_info, cpu), RV_REG_TP, ctx); Where bpf_to_rv_reg() refers to regmap, which in turn has the following line: static const int regmap[] = { [BPF_REG_0] = RV_REG_A5, ... } At the same time, [1] says: > 18.2 RVG Calling Convention > ... > Values are returned from functions in integer registers a0 and a1 and > floating-point registers fa0 and fa1. [1] https://riscv.org/wp-content/uploads/2015/01/riscv-calling.pdf So, I would expect r0 to be mapped to a0, do you happen to know why is it a5? [...]
Eduard Zingerman <eddyz87@gmail.com> writes: > On Wed, 2024-07-03 at 11:27 +0000, Puranjay Mohan wrote: > > [...] > >> > > > > @@ -16030,7 +16030,14 @@ static u8 get_helper_reg_mask(const struct bpf_func_proto *fn) >> > > > > */ >> > > > > static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) >> > > > > { >> > > > > - return false; >> > > > > + switch (imm) { >> > > > > +#ifdef CONFIG_X86_64 >> > > > > + case BPF_FUNC_get_smp_processor_id: >> > > > > + return env->prog->jit_requested && bpf_jit_supports_percpu_insn(); >> > > > > +#endif >> > > > >> > > > please see bpf_jit_inlines_helper_call(), arm64 and risc-v inline it >> > > > in JIT, so we need to validate they don't assume any of R1-R5 register >> > > > to be a scratch register >> >> They don't assume any register to be scratch (except R0) so we can >> enable this on arm64 and riscv. > > Puranjay, just out of curiosity and tangential to this patch-set, > I see that get_smp_processor_id is implemented as follows in riscv jit: > > emit_ld(bpf_to_rv_reg(BPF_REG_0, ctx), offsetof(struct thread_info, cpu), > RV_REG_TP, ctx); > > Where bpf_to_rv_reg() refers to regmap, which in turn has the following line: > > static const int regmap[] = { > [BPF_REG_0] = RV_REG_A5, > ... > } > > At the same time, [1] says: > >> 18.2 RVG Calling Convention >> ... >> Values are returned from functions in integer registers a0 and a1 and >> floating-point registers fa0 and fa1. > > [1] https://riscv.org/wp-content/uploads/2015/01/riscv-calling.pdf > > So, I would expect r0 to be mapped to a0, do you happen to know why is it a5? I had the same question when I started working with the JITs. This is seen on both risc-v and arm64, where as you said on risc-v R0 should be mapped to A0 but is mapped to A5. Similarly, on ARM64, BPF_R0 should be mapped to ARM64_R0 but is mapped to ARM64_R7. Here is my understanding of this: The reason for this quirk is the usage of BPF register R0 as defined by BPF Registers and calling convention [1] [1] says: ``` * R0: return value from function calls, and exit value for BPF programs * R1 - R5: arguments for function calls ``` On arm64 and risc-v the first argument and the return value are passed/returned in the same register, A0 on risc-v and R0 on arm64. In BPF, the first argument to a function is passed in R1 and not in R0. So when we map these registers to riscv or arm64 calling convention, we have to map BPF_R1 to A0 on risc-v and to R0 on ARM64. This is to make argument passing easy. Therefore BPF_R0 is mapped to A5 on risc-v and ARM64_R7 on arm64. And when we JIT the 'BPF_JMP | BPF_CALL' we add a mov instruction at the end to move A0 to A5 on risc-v and R0 to R7 on arm64. But when inlining the call we can directly put the result in A5 or R7. Thanks, Puranjay [1] https://kernel.googlesource.com/pub/scm/linux/kernel/git/bpf/bpf-next/+/refs/heads/master/Documentation/bpf/standardization/abi.rst
On Thu, 2024-07-04 at 11:19 +0000, Puranjay Mohan wrote: [...] > And when we JIT the 'BPF_JMP | BPF_CALL' we add a mov instruction at the > end to move A0 to A5 on risc-v and R0 to R7 on arm64. > > But when inlining the call we can directly put the result in A5 or R7. Oh, I see that additional move, thank you for explaining! Best regards, Eduard
On Wed, 2024-07-03 at 11:27 +0000, Puranjay Mohan wrote: [...] > The correct way to do this would be to change call_csr_mask() to have: > > verifier_inlines_helper_call(env, insn->imm) || bpf_jit_inlines_helper_call(insn->imm) Hi Puranjay, I've added bpf_jit_inlines_helper_call() logic in RFC v2 [1]. If you have a riscv setup at hand, would it be possible to ask you to run test case 'verifier_nocsr/canary_arm64_riscv64' on it? I verified that it works for arm64 in [2,3] but it would be nice to have it checked on riscv, which is currently not a part of the CI. Thanks, Eduard [1] https://lore.kernel.org/bpf/20240704102402.1644916-1-eddyz87@gmail.com/ [2] https://github.com/kernel-patches/bpf/actions/runs/9792217835/job/27037905408?pr=7274 [3] https://productionresultssa19.blob.core.windows.net/actions-results/6fb742ca-e78f-420e-9f1c-66e1e23365ef/workflow-job-run-0caa83a1-3221-5ca7-0e7d-7eb42ae68938/logs/job/job-logs.txt?rsct=text%2Fplain&se=2024-07-04T10%3A32%3A00Z&sig=gb0wAZ%2FPMOfZwCCtJyTqAogIyKZ%2F3lCZCfPUpMdN8rE%3D&ske=2024-07-04T18%3A36%3A08Z&skoid=ca7593d4-ee42-46cd-af88-8b886a2f84eb&sks=b&skt=2024-07-04T06%3A36%3A08Z&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skv=2023-11-03&sp=r&spr=https&sr=b&st=2024-07-04T10%3A21%3A55Z&sv=2023-11-03
Eduard Zingerman <eddyz87@gmail.com> writes: > On Wed, 2024-07-03 at 11:27 +0000, Puranjay Mohan wrote: > > [...] > >> The correct way to do this would be to change call_csr_mask() to have: >> >> verifier_inlines_helper_call(env, insn->imm) || bpf_jit_inlines_helper_call(insn->imm) > > Hi Puranjay, > > I've added bpf_jit_inlines_helper_call() logic in RFC v2 [1]. > If you have a riscv setup at hand, would it be possible to ask you to > run test case 'verifier_nocsr/canary_arm64_riscv64' on it? > I verified that it works for arm64 in [2,3] but it would be nice to > have it checked on riscv, which is currently not a part of the CI. Hi Eduard, I have qemu setup for risc-v. I will test this and let you know the results. Thanks, Puranjay
On Thu, 2024-07-04 at 17:24 +0000, Puranjay Mohan wrote: [...] > I have qemu setup for risc-v. I will test this and let you know the > results. Great, thank you!
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 229396172026..1df01f454590 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -158,6 +158,7 @@ const struct bpf_func_proto bpf_get_smp_processor_id_proto = { .func = bpf_get_smp_processor_id, .gpl_only = false, .ret_type = RET_INTEGER, + .nocsr = true, }; BPF_CALL_0(bpf_get_numa_node_id) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 1340d3e60d30..d68ed70186c8 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -16030,7 +16030,14 @@ static u8 get_helper_reg_mask(const struct bpf_func_proto *fn) */ static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) { - return false; + switch (imm) { +#ifdef CONFIG_X86_64 + case BPF_FUNC_get_smp_processor_id: + return env->prog->jit_requested && bpf_jit_supports_percpu_insn(); +#endif + default: + return false; + } } /* If 'insn' is a call that follows no_caller_saved_registers contract @@ -20666,7 +20673,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env) #ifdef CONFIG_X86_64 /* Implement bpf_get_smp_processor_id() inline. */ if (insn->imm == BPF_FUNC_get_smp_processor_id && - prog->jit_requested && bpf_jit_supports_percpu_insn()) { + verifier_inlines_helper_call(env, insn->imm)) { /* BPF_FUNC_get_smp_processor_id inlining is an * optimization, so if pcpu_hot.cpu_number is ever * changed in some incompatible and hard to support
On x86 verifier replaces call to bpf_get_smp_processor_id() with a sequence of instructions that modify only R0. This satisfies attribute no_caller_saved_registers contract. Allow rewrite of no_caller_saved_registers patterns for bpf_get_smp_processor_id() in order to use this function as a canary for no_caller_saved_registers tests. Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> --- kernel/bpf/helpers.c | 1 + kernel/bpf/verifier.c | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-)