Re: [PATCH bpf-next v7 04/10] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

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

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

> On Tue, Sep 22, 2020 at 3:17 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Sat, Sep 19, 2020 at 4:50 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>
>> >> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> >>
>> >> In preparation for allowing multiple attachments of freplace programs, move
>> >> the references to the target program and trampoline into the
>> >> bpf_tracing_link structure when that is created. To do this atomically,
>> >> introduce a new mutex in prog->aux to protect writing to the two pointers
>> >> to target prog and trampoline, and rename the members to make it clear that
>> >> they are related.
>> >>
>> >> With this change, it is no longer possible to attach the same tracing
>> >> program multiple times (detaching in-between), since the reference from the
>> >> tracing program to the target disappears on the first attach. However,
>> >> since the next patch will let the caller supply an attach target, that will
>> >> also make it possible to attach to the same place multiple times.
>> >>
>> >> Acked-by: Andrii Nakryiko <andriin@fb.com>
>> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> >> ---
>> >>  include/linux/bpf.h     |   15 +++++++++-----
>> >>  kernel/bpf/btf.c        |    6 +++---
>> >>  kernel/bpf/core.c       |    9 ++++++---
>> >>  kernel/bpf/syscall.c    |   49 +++++++++++++++++++++++++++++++++++++++--------
>> >>  kernel/bpf/trampoline.c |   12 ++++--------
>> >>  kernel/bpf/verifier.c   |    9 +++++----
>> >>  6 files changed, 68 insertions(+), 32 deletions(-)
>> >>
>> >
>> > [...]
>> >
>> >> @@ -741,7 +743,9 @@ struct bpf_prog_aux {
>> >>         u32 max_rdonly_access;
>> >>         u32 max_rdwr_access;
>> >>         const struct bpf_ctx_arg_aux *ctx_arg_info;
>> >> -       struct bpf_prog *linked_prog;
>> >> +       struct mutex tgt_mutex; /* protects writing of tgt_* pointers below */
>> >
>> > nit: not just writing, "accessing" would be a better word
>>
>> Except it's not, really: the values are read without taking the mutex.
>
> Huh? So you are taking a mutex in bpf_tracing_prog_attach before
> reading prog->aux->tgt_prog and prog->aux->tgt_trampoline just for
> fun?.. Why don't you read those pointers outside of mutex and let's
> have discussion about race conditions?

No, of course not. What I meant was that not everywhere that accesses
the field takes the lock; it's not just check_attach_btf_id(), it's also
resolve_prog_type() which is called from several places.

Of course, as you point out this is all technically part of the
'constructor', as it's all inside the verifier. But you have to keep
quite a lot of mental state to realise that, so I thought it would be
confusing to have a comment that says the mutex protects all accesses to
the field, and then have a bunch of places access the field without
holding the mutex.

However, just adding the "*after* prog becomes visible" bit to the
comment helps with that, so I'm not arguing for keeping it, just
explaining why I did it the way I did initially :)

-Toke