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@xxxxxx> 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@xxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
> [...]
>
> > -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!
