Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes:
> 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.
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
