Re: [RFC bpf-next v1 3/8] bpf, x86: no_caller_saved_registers for bpf_get_smp_processor_id()

Andrii Nakryiko
2024-07-02T00:41:59Z

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
>