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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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.

> +}
> +
> +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.

>  int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
>                             union bpf_attr __user *uattr)
>  {
>         const struct bpf_struct_ops *st_ops = &bpf_bpf_dummy_ops;
>         const struct btf_type *func_proto;
>         struct bpf_dummy_ops_test_args *args;
> -       struct bpf_tramp_progs *tprogs;
> +       struct bpf_tramp_links *tlinks;
> +       struct bpf_tramp_link *link = NULL;
>         void *image = NULL;
>         unsigned int op_idx;
>         int prog_ret;

[...]