From: Kui-Feng Lee <kuifeng@fb.com>
To: "andrii.nakryiko@gmail.com" <andrii.nakryiko@gmail.com>
Cc: "daniel@iogearbox.net" <daniel@iogearbox.net>,
Kernel Team <Kernel-team@fb.com>,
"ast@kernel.org" <ast@kernel.org>,
"andrii@kernel.org" <andrii@kernel.org>,
"bpf@vger.kernel.org" <bpf@vger.kernel.org>
Subject: Re: [PATCH bpf-next v5 2/5] bpf, x86: Create bpf_tramp_run_ctx on the caller thread's stack
Date: Wed, 13 Apr 2022 20:14:08 +0000 [thread overview]
Message-ID: <f0ca703b8ab72c21d28dc4538a60a89bc6a7f26d.camel@fb.com> (raw)
In-Reply-To: <CAEf4BzZiEAysLb41XNzvZXdHqr4ikgw8ggTbvd8KpsWvqtS5zg@mail.gmail.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);
> >
>
> [...]
next prev parent reply other threads:[~2022-04-13 20:14 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-12 16:55 [PATCH bpf-next v5 0/5] Attach a cookie to a tracing program Kui-Feng Lee
2022-04-12 16:55 ` [PATCH bpf-next v5 1/5] bpf, x86: Generate trampolines from bpf_tramp_links Kui-Feng Lee
2022-04-13 2:43 ` Andrii Nakryiko
2022-04-13 17:53 ` Kui-Feng Lee
2022-04-13 19:08 ` Andrii Nakryiko
2022-04-12 16:55 ` [PATCH bpf-next v5 2/5] bpf, x86: Create bpf_tramp_run_ctx on the caller thread's stack Kui-Feng Lee
2022-04-13 2:55 ` Andrii Nakryiko
2022-04-13 18:06 ` Kui-Feng Lee
2022-04-13 20:14 ` Kui-Feng Lee [this message]
2022-04-12 16:55 ` [PATCH bpf-next v5 3/5] bpf, x86: Attach a cookie to fentry/fexit/fmod_ret Kui-Feng Lee
2022-04-13 3:03 ` Andrii Nakryiko
2022-04-13 18:14 ` Kui-Feng Lee
2022-04-12 16:55 ` [PATCH bpf-next v5 4/5] lib/bpf: Assign cookies to links in libbpf Kui-Feng Lee
2022-04-13 3:14 ` Andrii Nakryiko
2022-04-12 16:55 ` [PATCH bpf-next v5 5/5] selftest/bpf: The test cses of BPF cookie for fentry/fexit/fmod_ret Kui-Feng Lee
2022-04-13 3:17 ` Andrii Nakryiko
2022-04-13 18:30 ` Kui-Feng Lee
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f0ca703b8ab72c21d28dc4538a60a89bc6a7f26d.camel@fb.com \
--to=kuifeng@fb.com \
--cc=Kernel-team@fb.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).