On Thu, Sep 17, 2020 at 1:21 PM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>
> From: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
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;
[...]
