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