Re: [PATCH bpf-next v6 05/10] bpf: support attaching freplace programs to multiple attach points

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

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

Huh, did notice something was broken, but they didn't when I reverted
the patch either, so I put it down to just the tests being broken. I'll
take another look :)

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

I created a different one function it had to be called at a different
place; don't mind combining them, though.

>> +{
>> +       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?

Ah yes, good catch! I played around with different versions of this, and
guess I forgot to put that check back in for this one...

-Toke