Re: [PATCH bpf-next v7 1/5] bpf, x86: Generate trampolines from bpf_tramp_links

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

On Mon, 2022-05-09 at 11:54 -0700, Andrii Nakryiko wrote:
> On Sat, May 7, 2022 at 8:21 PM Kui-Feng Lee <kuifeng@fb.com> wrote:
> > 
> > Replace struct bpf_tramp_progs with struct bpf_tramp_links to
> > collect
> > struct bpf_tramp_link(s) for a trampoline.  struct bpf_tramp_link
> > extends bpf_link to act as a linked list node.
> > 
> > arch_prepare_bpf_trampoline() accepts a struct bpf_tramp_links to
> > collects all bpf_tramp_link(s) that a trampoline should call.
> > 
> > Change BPF trampoline and bpf_struct_ops to pass bpf_tramp_links
> > instead of bpf_tramp_progs.
> > 
> > Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
> > ---
> >  arch/x86/net/bpf_jit_comp.c    | 36 +++++++++--------
> >  include/linux/bpf.h            | 36 +++++++++++------
> >  include/linux/bpf_types.h      |  1 +
> >  include/uapi/linux/bpf.h       |  1 +
> >  kernel/bpf/bpf_struct_ops.c    | 69 ++++++++++++++++++++++--------
> > --
> >  kernel/bpf/syscall.c           | 23 ++++-------
> >  kernel/bpf/trampoline.c        | 73 +++++++++++++++++++-----------
> > ----
> >  net/bpf/bpf_dummy_struct_ops.c | 36 ++++++++++++++---
> >  tools/bpf/bpftool/link.c       |  1 +
> >  tools/include/uapi/linux/bpf.h |  1 +
> >  10 files changed, 174 insertions(+), 103 deletions(-)
> > 
> 
> Two things that can be done as a follow up, otherwise LGTM:
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> [...]
> 
> > -int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_progs
> > *tprogs,
> > -                                     struct bpf_prog *prog,
> > +static void bpf_struct_ops_link_release(struct bpf_link *link)
> > +{
> > +}
> > +
> > +static void bpf_struct_ops_link_dealloc(struct bpf_link *link)
> > +{
> > +       kfree(link);
> 
> This works by accident because struct bpf_link is at the top of
> struct
> bpf_tramp_link. But to do this properly you'd need container_of() to
> get struct bpf_tramp_link and then free that. I don't think it needs
> a
> respin just for this, but please send a follow-up fix.
> 

Fixed!

> > +}
> > +
> > +static const struct bpf_link_ops bpf_struct_ops_link_lops = {
> > +       .release = bpf_struct_ops_link_release,
> > +       .dealloc = bpf_struct_ops_link_dealloc,
> > +};
> > +
> 
> [...]
> 
> > diff --git a/net/bpf/bpf_dummy_struct_ops.c
> > b/net/bpf/bpf_dummy_struct_ops.c
> > index d0e54e30658a..41552d6f1d23 100644
> > --- a/net/bpf/bpf_dummy_struct_ops.c
> > +++ b/net/bpf/bpf_dummy_struct_ops.c
> > @@ -72,13 +72,28 @@ static int dummy_ops_call_op(void *image,
> > struct bpf_dummy_ops_test_args *args)
> >                     args->args[3], args->args[4]);
> >  }
> > 
> > +static void bpf_struct_ops_link_release(struct bpf_link *link)
> > +{
> > +}
> > +
> > +static void bpf_struct_ops_link_dealloc(struct bpf_link *link)
> > +{
> > +       kfree(link);
> > +}
> > +
> > +static const struct bpf_link_ops bpf_struct_ops_link_lops = {
> > +       .release = bpf_struct_ops_link_release,
> > +       .dealloc = bpf_struct_ops_link_dealloc,
> > +};
> > +
> 
> You already defined this ops struct and release/dealloc
> implementation
> in kernel/bpf/bpf_struct_ops.c, we need to reuse it here. Just make
> the bpf_struct_ops.c's non-static and declare it in
> include/linux/bpf.h. Again, don't think we need a respin just for
> this, it's mostly code hygiene.

Fixed!