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@xxxxxx> 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@xxxxxx>
> > ---
> > 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?
>
> > +
> > + 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?
You are right. I had added this check for doing the following line
only for some cases, and didn't aware the check is no more useful after
changing the way of doing it.
> > + 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);
> >
>
> [...]
