Re: [PATCH bpf-next v5 2/5] bpf, x86: Create bpf_tramp_run_ctx on the caller thread's stack

[Kui-Feng Lee <kuifeng@fb.com>]

On Tue, 2022-04-12 at 19:55 -0700, Andrii Nakryiko wrote:
> On Tue, Apr 12, 2022 at 9:56 AM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > 
> > BPF trampolines will create a bpf_tramp_run_ctx, a bpf_run_ctx, on
> > stacks and set/reset the current bpf_run_ctx before/after calling a
> > bpf_prog.
> > 
> > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > ---
> >  arch/x86/net/bpf_jit_comp.c | 55
> > +++++++++++++++++++++++++++++++++++++
> >  include/linux/bpf.h         | 17 +++++++++---
> >  kernel/bpf/syscall.c        |  7 +++--
> >  kernel/bpf/trampoline.c     | 20 +++++++++++---
> >  4 files changed, 89 insertions(+), 10 deletions(-)
> > 
> > diff --git a/arch/x86/net/bpf_jit_comp.c
> > b/arch/x86/net/bpf_jit_comp.c
> > index 4dcc0b1ac770..0f521be68f7b 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -1766,10 +1766,26 @@ static int invoke_bpf_prog(const struct
> > btf_func_model *m, u8 **pprog,
> >  {
> >         u8 *prog = *pprog;
> >         u8 *jmp_insn;
> > +       int ctx_cookie_off = offsetof(struct bpf_tramp_run_ctx,
> > bpf_cookie);
> >         struct bpf_prog *p = l->link.prog;
> > 
> > +       /* mov rdi, 0 */
> > +       emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> > +
> > +       /* Prepare struct bpf_tramp_run_ctx.
> > +        *
> > +        * bpf_tramp_run_ctx is already preserved by
> > +        * arch_prepare_bpf_trampoline().
> > +        *
> > +        * mov QWORD PTR [rsp + ctx_cookie_off], rdi
> > +        */
> > +       EMIT4(0x48, 0x89, 0x7C, 0x24); EMIT1(ctx_cookie_off);
> > +
> >         /* arg1: mov rdi, progs[i] */
> >         emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32, (u32)
> > (long) p);
> > +       /* arg2: mov rsi, rsp (struct bpf_run_ctx *) */
> > +       EMIT3(0x48, 0x89, 0xE6);
> > +
> >         if (emit_call(&prog,
> >                       p->aux->sleepable ?
> > __bpf_prog_enter_sleepable :
> >                       __bpf_prog_enter, prog))
> > @@ -1815,6 +1831,8 @@ static int invoke_bpf_prog(const struct
> > btf_func_model *m, u8 **pprog,
> >         emit_mov_imm64(&prog, BPF_REG_1, (long) p >> 32, (u32)
> > (long) p);
> >         /* arg2: mov rsi, rbx <- start time in nsec */
> >         emit_mov_reg(&prog, true, BPF_REG_2, BPF_REG_6);
> > +       /* arg3: mov rdx, rsp (struct bpf_run_ctx *) */
> > +       EMIT3(0x48, 0x89, 0xE2);
> >         if (emit_call(&prog,
> >                       p->aux->sleepable ? __bpf_prog_exit_sleepable
> > :
> >                       __bpf_prog_exit, prog))
> > @@ -2079,6 +2097,16 @@ int arch_prepare_bpf_trampoline(struct
> > bpf_tramp_image *im, void *image, void *i
> >                 }
> >         }
> > 
> > +       if (nr_args < 3 && (fentry->nr_links || fexit->nr_links ||
> > fmod_ret->nr_links))
> > +               EMIT1(0x52);    /* push rdx */
> 
> this nr_args < 3 condition is new, maybe leave a comment on why we
> need this? Also instead of repeating this whole (fentry->nr_links ||
> ... || ...) check, why not move if (nr_args < 3) inside the if right
> below?

I thought rdx is a nonvolatile (callee-saved) register.  Checking ABI
again, I was wrong.  I am removing this part.

> 
> > +
> > +       if (fentry->nr_links || fexit->nr_links || fmod_ret-
> > >nr_links) {
> 
> if (nr_args > 3) here?
> 
> > +               /* Prepare struct bpf_tramp_run_ctx.
> > +                * sub rsp, sizeof(struct bpf_tramp_run_ctx)
> > +                */
> > +               EMIT4(0x48, 0x83, 0xEC, sizeof(struct
> > bpf_tramp_run_ctx));
> > +       }
> > +
> >         if (fentry->nr_links)
> >                 if (invoke_bpf(m, &prog, fentry, regs_off,
> >                                flags & BPF_TRAMP_F_RET_FENTRY_RET))
> 
> [...]
> 
> >         if (fmod_ret->nr_links) {
> > @@ -2133,6 +2179,15 @@ int arch_prepare_bpf_trampoline(struct
> > bpf_tramp_image *im, void *image, void *i
> >                         goto cleanup;
> >                 }
> > 
> > +       /* pop struct bpf_tramp_run_ctx
> > +        * add rsp, sizeof(struct bpf_tramp_run_ctx)
> > +        */
> > +       if (fentry->nr_links || fexit->nr_links || fmod_ret-
> > >nr_links)
> 
> well, actually, can it ever be that this condition doesn't hold? That
> would mean we are generating empty trampoline for some reason, no? Do
> we do that? Checking bpf_trampoline_update() and
> bpf_struct_ops_prepare_trampoline() doesn't seem like we ever do
> this.
> So seems like all these checks can be dropped?
> 
> > +               EMIT4(0x48, 0x83, 0xC4, sizeof(struct
> > bpf_tramp_run_ctx));
> > +
> > +       if (nr_args < 3 && (fentry->nr_links || fexit->nr_links ||
> > fmod_ret->nr_links))
> > +               EMIT1(0x5A); /* pop rdx */
> 
> same, move it inside if above?
> 
> > +
> >         if (flags & BPF_TRAMP_F_RESTORE_REGS)
> >                 restore_regs(m, &prog, nr_args, regs_off);
> > 
> 
> [...]