Subject: Re: [PATCH RESEND bpf-next v3 4/9] bpf: support attaching freplace programs to multiple attach points

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Mon, 14 Sep 2020 18:08:43 +0200

Andrii Nakryiko <[email-masked]> writes:

> On Fri, Sep 11, 2020 at 3:01 AM Toke Høiland-Jørgensen <[email-masked]> wrote:
>>
>> From: Toke Høiland-Jørgensen <[email-masked]>
>>
>> This enables support for attaching freplace programs to multiple attach
>> points. It does this by amending UAPI for bpf_raw_tracepoint_open with a
>> target prog fd and btf ID pair that can be used to supply the new
>> attachment point. 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, the bpf_tracing_prog_attach() function will reject new targets for
>> anything other than PROG_TYPE_EXT programs.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <[email-masked]>
>> ---
>
> It feels like using a semi-constructed bpf_tracing_link inside
> prog->aux->tgt_link is just an unnecessary complication, after reading
> this and previous patches. Seems more straightforward and simpler to
> store tgt_attach_type/tgt_prog_type (permanently) and
> tgt_prog/tgt_trampoline (until first attachment) in prog->aux and then
> properly create bpf_link on attach.

I updated v4 with your comments, but kept the link in prog->aux; the
reason being that having a container for the two pointers makes it
possible to atomically swap it out with xchg() as you suggested
previously. Could you please take a look at v4? If you still think it's
better to just keep two separate pointers (and add a lock) in prog->aux,
I can change it to that. But I'd rather avoid the lock if possible...

-Toke