Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220716230344.239749011@linutronix.de> <20220716230954.898341815@linutronix.de>
In-Reply-To: <20220716230954.898341815@linutronix.de>
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date:   Mon, 18 Jul 2022 22:30:01 -0700
Message-ID: <CAADnVQJb6vY5j+RAW9NYA1gcPXsocytFTL1xvNx+eyWojy3deg@mail.gmail.com>
Subject: Re: [patch 37/38] x86/bpf: Emit call depth accounting if required
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        Andrew Cooper <Andrew.Cooper3@citrix.com>,
        Pawan Gupta <pawan.kumar.gupta@linux.intel.com>,
        Johannes Wikner <kwikner@ethz.ch>,
        Alyssa Milburn <alyssa.milburn@linux.intel.com>,
        Jann Horn <jannh@google.com>, "H.J. Lu" <hjl.tools@gmail.com>,
        Joao Moreira <joao.moreira@intel.com>,
        Joseph Nuzman <joseph.nuzman@intel.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Jul 16, 2022 at 4:18 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Ensure that calls in BPF jitted programs are emitting call depth accounting
> when enabled to keep the call/return balanced. The return thunk jump is
> already injected due to the earlier retbleed mitigations.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  arch/x86/include/asm/alternative.h |    6 +++++
>  arch/x86/kernel/callthunks.c       |   19 ++++++++++++++++
>  arch/x86/net/bpf_jit_comp.c        |   43 ++++++++++++++++++++++++-------------
>  3 files changed, 53 insertions(+), 15 deletions(-)
>
> --- a/arch/x86/include/asm/alternative.h
> +++ b/arch/x86/include/asm/alternative.h
> @@ -95,6 +95,7 @@ extern void callthunks_patch_module_call
>  extern void callthunks_module_free(struct module *mod);
>  extern void *callthunks_translate_call_dest(void *dest);
>  extern bool is_callthunk(void *addr);
> +extern int x86_call_depth_emit_accounting(u8 **pprog, void *func);
>  #else
>  static __always_inline void callthunks_patch_builtin_calls(void) {}
>  static __always_inline void
> @@ -109,6 +110,11 @@ static __always_inline bool is_callthunk
>  {
>         return false;
>  }
> +static __always_inline int x86_call_depth_emit_accounting(u8 **pprog,
> +                                                         void *func)
> +{
> +       return 0;
> +}
>  #endif
>
>  #ifdef CONFIG_SMP
> --- a/arch/x86/kernel/callthunks.c
> +++ b/arch/x86/kernel/callthunks.c
> @@ -706,6 +706,25 @@ int callthunk_get_kallsym(unsigned int s
>         return ret;
>  }
>
> +#ifdef CONFIG_BPF_JIT
> +int x86_call_depth_emit_accounting(u8 **pprog, void *func)
> +{
> +       unsigned int tmpl_size = callthunk_desc.template_size;
> +       void *tmpl = callthunk_desc.template;
> +
> +       if (!thunks_initialized)
> +               return 0;
> +
> +       /* Is function call target a thunk? */
> +       if (is_callthunk(func))
> +               return 0;
> +
> +       memcpy(*pprog, tmpl, tmpl_size);
> +       *pprog += tmpl_size;
> +       return tmpl_size;
> +}
> +#endif
> +
>  #ifdef CONFIG_MODULES
>  void noinline callthunks_patch_module_calls(struct callthunk_sites *cs,
>                                             struct module *mod)
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -340,6 +340,12 @@ static int emit_call(u8 **pprog, void *f
>         return emit_patch(pprog, func, ip, 0xE8);
>  }
>
> +static int emit_rsb_call(u8 **pprog, void *func, void *ip)
> +{
> +       x86_call_depth_emit_accounting(pprog, func);
> +       return emit_patch(pprog, func, ip, 0xE8);
> +}
> +
>  static int emit_jump(u8 **pprog, void *func, void *ip)
>  {
>         return emit_patch(pprog, func, ip, 0xE9);
> @@ -1431,19 +1437,26 @@ st:                     if (is_imm8(insn->off))
>                         break;
>
>                         /* call */
> -               case BPF_JMP | BPF_CALL:
> +               case BPF_JMP | BPF_CALL: {
> +                       int offs;
> +
>                         func = (u8 *) __bpf_call_base + imm32;
>                         if (tail_call_reachable) {
>                                 /* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
>                                 EMIT3_off32(0x48, 0x8B, 0x85,
>                                             -round_up(bpf_prog->aux->stack_depth, 8) - 8);
> -                               if (!imm32 || emit_call(&prog, func, image + addrs[i - 1] + 7))
> +                               if (!imm32)
>                                         return -EINVAL;
> +                               offs = 7 + x86_call_depth_emit_accounting(&prog, func);

It's a bit hard to read all the macro magic in patches 28-30,
but I suspect the asm inside
callthunk_desc.template
that will be emitted here before the call
will do
some math on %rax
movq %rax, PER_CPU_VAR(__x86_call_depth).

Only %rax register is scratched by the callthunk_desc, right?
If so, it's ok for all cases except this one.
See the comment few lines above
after if (tail_call_reachable)
and commit ebf7d1f508a7 ("bpf, x64: rework pro/epilogue and tailcall
handling in JIT")
We use %rax to keep the tail_call count.
The callthunk_desc would need to preserve %rax.
I guess extra push %rax/pop %rax would do it.

>                         } else {
> -                               if (!imm32 || emit_call(&prog, func, image + addrs[i - 1]))
> +                               if (!imm32)
>                                         return -EINVAL;
> +                               offs = x86_call_depth_emit_accounting(&prog, func);
>                         }
> +                       if (emit_call(&prog, func, image + addrs[i - 1] + offs))
> +                               return -EINVAL;
>                         break;
> +               }