Re: [PATCH bpf-next v7 3/5] bpf, x86: Attach a cookie to fentry/fexit/fmod_ret/lsm.

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

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@fb.com> 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@fb.com>
> > ---
> >  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@kernel.org>
> 
> > 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.

> 
> > +               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.
> >          *
> 
> [...]