On Tue, May 10, 2022 at 9:44 AM Kui-Feng Lee <kuifeng@xxxxxx> wrote:
>
> On Mon, 2022-05-09 at 11:58 -0700, Andrii Nakryiko wrote:
> > On Sat, May 7, 2022 at 8:21 PM Kui-Feng Lee <kuifeng@xxxxxx> wrote:
> > >
> > > Pass a cookie along with BPF_LINK_CREATE requests.
> > >
> > > Add a bpf_cookie field to struct bpf_tracing_link to attach a
> > > cookie.
> > > The cookie of a bpf_tracing_link is available by calling
> > > bpf_get_attach_cookie when running the BPF program of the attached
> > > link.
> > >
> > > The value of a cookie will be set at bpf_tramp_run_ctx by the
> > > trampoline of the link.
> > >
> > > Signed-off-by: Kui-Feng Lee <kuifeng@xxxxxx>
> > > ---
> > > arch/x86/net/bpf_jit_comp.c | 12 ++++++++++--
> > > include/linux/bpf.h | 1 +
> > > include/uapi/linux/bpf.h | 9 +++++++++
> > > kernel/bpf/bpf_lsm.c | 17 +++++++++++++++++
> > > kernel/bpf/syscall.c | 12 ++++++++----
> > > kernel/bpf/trampoline.c | 7 +++++--
> > > kernel/trace/bpf_trace.c | 17 +++++++++++++++++
> > > tools/include/uapi/linux/bpf.h | 9 +++++++++
> > > 8 files changed, 76 insertions(+), 8 deletions(-)
> > >
> >
> > LGTM with a suggestion for some follow up clean up.
> >
> > Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> >
> > > diff --git a/arch/x86/net/bpf_jit_comp.c
> > > b/arch/x86/net/bpf_jit_comp.c
> > > index bf4576a6938c..52a5eba2d5e8 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -1764,13 +1764,21 @@ static int invoke_bpf_prog(const struct
> > > btf_func_model *m, u8 **pprog,
> > > struct bpf_tramp_link *l, int
> > > stack_size,
> > > bool save_ret)
> > > {
> > > + u64 cookie = 0;
> > > 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);
> > > + if (l->link.type == BPF_LINK_TYPE_TRACING) {
> >
> > It would probably be nicer to put cookie field into struct
> > bpf_tramp_link instead so that the JIT compiler doesn't have to do
> > this special handling. It also makes sense that struct bpf_trampoline
> > *trampoline is moved into struct bpf_tramp_link itself (given
> > trampoline is always there for bpf_tramp_link).
>
> It will increase the size of bpf_tramp_link a little bit, but they are
> not used by bpf_struct_ops.
>
It feels like the right tradeoff to keep architecture-specific
trampoline code oblivious to these details. Some day structs_ops might
support cookies as well. And either way 8 bytes for struct_ops link
isn't a big deal.
> >
> > > + struct bpf_tracing_link *tr_link =
> > > + container_of(l, struct bpf_tracing_link,
> > > link);
> > > +
> > > + cookie = tr_link->cookie;
> > > + }
> > > +
> > > + /* mov rdi, cookie */
> > > + emit_mov_imm64(&prog, BPF_REG_1, (long) cookie >> 32, (u32)
> > > (long) cookie);
> > >
> > > /* Prepare struct bpf_tramp_run_ctx.
> > > *
> >
> > [...]
>
