Message-ID: <CAEf4BzajBMf9btVJLfOYNdEbBHgs1m5o=D5mDcmTV4gPYTf9-w@mail.gmail.com>
Date:   Fri, 18 Sep 2020 11:47:56 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>,
        KP Singh <kpsingh@...omium.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v6 05/10] bpf: support attaching freplace
 programs to multiple attach points

On Thu, Sep 17, 2020 at 1:21 PM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@...hat.com>
>
> This enables support for attaching freplace programs to multiple attach
> points. It does this by amending the UAPI for bpf_link_Create with a target
> btf ID that can be used to supply the new attachment point along with the
> target program fd. The target must be compatible with the target that was
> supplied at program load time.
>
> The implementation reuses the checks that were factored out of
> check_attach_btf_id() to ensure compatibility between the BTF types of the
> old and new attachment. If these match, a new bpf_tracing_link will be
> created for the new attach target, allowing multiple attachments to
> co-exist simultaneously.
>
> The code could theoretically support multiple-attach of other types of
> tracing programs as well, but since I don't have a use case for any of
> those, there is no API support for doing so.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
> ---

You patch set breaks at least bpf_iter tests:

$ sudo ./test_progs -t bpf_iter
...
#4 bpf_iter:FAIL
Summary: 0/19 PASSED, 0 SKIPPED, 6 FAILED

Please check and fix.

>  include/linux/bpf.h            |    2 +
>  include/uapi/linux/bpf.h       |    9 +++-
>  kernel/bpf/syscall.c           |  101 ++++++++++++++++++++++++++++++++++------
>  kernel/bpf/verifier.c          |    9 ++++
>  tools/include/uapi/linux/bpf.h |    9 +++-
>  5 files changed, 110 insertions(+), 20 deletions(-)
>

[...]

> -static int bpf_tracing_prog_attach(struct bpf_prog *prog)
> +static int bpf_tracing_prog_attach(struct bpf_prog *prog,
> +                                  int tgt_prog_fd,
> +                                  u32 btf_id)
>  {
>         struct bpf_link_primer link_primer;
>         struct bpf_prog *tgt_prog = NULL;
> +       struct bpf_trampoline *tr = NULL;
>         struct bpf_tracing_link *link;
> -       struct bpf_trampoline *tr;
> +       struct btf_func_model fmodel;
> +       u64 key = 0;
> +       long addr;
>         int err;
>
>         switch (prog->type) {
> @@ -2589,6 +2595,28 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)

bpf_tracing_prog_attach logic looks correct to me now, thanks.

>                 goto out_put_prog;
>         }
>

[...]

> @@ -3934,6 +3986,16 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *
>         return -EINVAL;
>  }
>
> +static int freplace_bpf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)

Any reason to have this separate from tracing_bpf_link_attach? I'd
merge them and do a simple switch inside, based on prog->type. It
would also be easier to follow the flow if this expected_attach_type
check was first and returned -EINVAL immediately at the top.


> +{
> +       if (attr->link_create.attach_type == prog->expected_attach_type)
> +               return bpf_tracing_prog_attach(prog,
> +                                              attr->link_create.target_fd,
> +                                              attr->link_create.target_btf_id);
> +       return -EINVAL;
> +

nit: unnecessary empty line?

> +}
> +
>  #define BPF_LINK_CREATE_LAST_FIELD link_create.iter_info_len
>  static int link_create(union bpf_attr *attr)
>  {
> @@ -3944,18 +4006,25 @@ static int link_create(union bpf_attr *attr)
>         if (CHECK_ATTR(BPF_LINK_CREATE))
>                 return -EINVAL;
>
> -       ptype = attach_type_to_prog_type(attr->link_create.attach_type);
> -       if (ptype == BPF_PROG_TYPE_UNSPEC)
> -               return -EINVAL;
> -
> -       prog = bpf_prog_get_type(attr->link_create.prog_fd, ptype);
> +       prog = bpf_prog_get(attr->link_create.prog_fd);
>         if (IS_ERR(prog))
>                 return PTR_ERR(prog);
>
>         ret = bpf_prog_attach_check_attach_type(prog,
>                                                 attr->link_create.attach_type);
>         if (ret)
> -               goto err_out;
> +               goto out;
> +
> +       if (prog->type == BPF_PROG_TYPE_EXT) {
> +               ret = freplace_bpf_link_attach(attr, prog);
> +               goto out;
> +       }
> +
> +       ptype = attach_type_to_prog_type(attr->link_create.attach_type);
> +       if (ptype == BPF_PROG_TYPE_UNSPEC) {
> +               ret = -EINVAL;
> +               goto out;
> +       }

you seem to be missing a check that prog->type matches ptype,
previously implicitly performed by bpf_prog_get_type(), no?

>
>         switch (ptype) {
>         case BPF_PROG_TYPE_CGROUP_SKB:
> @@ -3983,7 +4052,7 @@ static int link_create(union bpf_attr *attr)
>                 ret = -EINVAL;
>         }
>
> -err_out:
> +out:
>         if (ret < 0)
>                 bpf_prog_put(prog);
>         return ret;

[...]

Powered by blists - more mailing lists