Re: [PATCH bpf-next v2 3/4] bpf, x86: Support BPF cookie for fentry/fexit/fmod_ret.

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

On Tue, 2022-03-22 at 14:06 -0700, Andrii Nakryiko wrote:
> On Tue, Mar 22, 2022 at 9:08 AM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > 
> > On Mon, 2022-03-21 at 16:18 -0700, Andrii Nakryiko wrote:
> > > On Tue, Mar 15, 2022 at 5:44 PM Kui-Feng Lee <kuifeng@fb.com>
> > > wrote:
> > > > 
> > > > Add a bpf_cookie field to attach a cookie to an instance of
> > > > struct
> > > > bpf_link.  The cookie of a bpf_link will be installed when
> > > > calling
> > > > the
> > > > associated program to make it available to the program.
> > > > 
> > > > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > > > ---
> > > >  arch/x86/net/bpf_jit_comp.c    |  4 ++--
> > > >  include/linux/bpf.h            |  1 +
> > > >  include/uapi/linux/bpf.h       |  1 +
> > > >  kernel/bpf/syscall.c           | 11 +++++++----
> > > >  kernel/trace/bpf_trace.c       | 17 +++++++++++++++++
> > > >  tools/include/uapi/linux/bpf.h |  1 +
> > > >  tools/lib/bpf/bpf.c            | 14 ++++++++++++++
> > > >  tools/lib/bpf/bpf.h            |  1 +
> > > >  tools/lib/bpf/libbpf.map       |  1 +
> > > >  9 files changed, 45 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/net/bpf_jit_comp.c
> > > > b/arch/x86/net/bpf_jit_comp.c
> > > > index 29775a475513..5fab8530e909 100644
> > > > --- a/arch/x86/net/bpf_jit_comp.c
> > > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > > @@ -1753,8 +1753,8 @@ static int invoke_bpf_prog(const struct
> > > > btf_func_model *m, u8 **pprog,
> > > > 
> > > >         EMIT1(0x52);             /* push rdx */
> > > > 
> > > > -       /* mov rdi, 0 */
> > > > -       emit_mov_imm64(&prog, BPF_REG_1, 0, 0);
> > > > +       /* mov rdi, cookie */
> > > > +       emit_mov_imm64(&prog, BPF_REG_1, (long) l->cookie >>
> > > > 32,
> > > > (u32) (long) l->cookie);
> > > 
> > > why __u64 to long casting? I don't think you need to cast
> > > anything at
> > > all, but if you want to make that more explicit than just casting
> > > to
> > > (u32) should be fine, no?
> > > 
> > > > 
> > > >         /* Prepare struct bpf_trace_run_ctx.
> > > >          * sub rsp, sizeof(struct bpf_trace_run_ctx)
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index d20a23953696..9469f9264b4f 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -1040,6 +1040,7 @@ struct bpf_link {
> > > >         struct bpf_prog *prog;
> > > >         struct work_struct work;
> > > >         struct hlist_node tramp_hlist;
> > > > +       u64 cookie;
> > > 
> > > I was a bit hesitant about adding tramp_hlist into generic struct
> > > bpf_link, but now with also cookie there I'm even more convinced
> > > that
> > > it's not the right thing to do... Some BPF links won't have
> > > cookie,
> > > some (like multi-kprobe) will have lots of them.
> > > 
> > > Should we create struct bpf_tramp_link {} which will have
> > > tramp_hlist
> > > and cookie? As for tramp_hlist, we can probably also keep it back
> > > in
> > > bpf_prog_aux and just fetch it through link->prog->aux-
> > > >tramp_hlist
> > > in
> > > trampoline code. This might reduce amount of code churn in patch
> > > 1.
> > 
> > Do you mean a struct likes like?
> > 
> > struct bpf_tramp_link {
> >   struct bpf_link link;
> >   struct hlist_node tramp_hlist;
> >   u64 cookie;
> > };
> 
> something like this, yes. Keep in mind that we already use struct
> bpf_tracing_link which is used for all trampoline-based programs,
> except for struct_ops. So we can either somehow make struct_ops just
> result struct bpf_tracing_link (cc Martin for ideas, he was thinking
> about doing proper bpf_link support for struct_ops anyways), or we'll
> need this kind of struct inheritance to reuse the same layout between
> struct_ops and struct bpf_tracing_link.

I just introduced bpf_tramp_link. But, moved cookie to
bpf_tracing_link.  And, struct_ops creates bpf_tramp_link instead of
bpf_link.