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 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.
This is fine because it is done in the verifier before the bpf_prog is
visible to the rest of the kernel, but saying the mutex protects all
accesses would be misleading, I think.

I guess I could change it to "protects access to tgt_* pointers after
prog becomes visible" or somesuch...

>> +       struct bpf_prog *tgt_prog;
>> +       struct bpf_trampoline *tgt_trampoline;
>>         bool verifier_zext; /* Zero extensions has been inserted by verifier. */
>>         bool offload_requested;
>>         bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
>
> [...]
>
>>  static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
>> @@ -11418,8 +11417,8 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>>  static int check_attach_btf_id(struct bpf_verifier_env *env)
>>  {
>>         struct bpf_prog *prog = env->prog;
>> -       struct bpf_prog *tgt_prog = prog->aux->linked_prog;
>>         u32 btf_id = prog->aux->attach_btf_id;
>> +       struct bpf_prog *tgt_prog = prog->aux->tgt_prog;
>>         struct btf_func_model fmodel;
>>         struct bpf_trampoline *tr;
>>         const struct btf_type *t;
>> @@ -11483,7 +11482,9 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
>>         if (!tr)
>>                 return -ENOMEM;
>>
>> -       prog->aux->trampoline = tr;
>> +       mutex_lock(&prog->aux->tgt_mutex);
>> +       prog->aux->tgt_trampoline = tr;
>> +       mutex_unlock(&prog->aux->tgt_mutex);
>
> I think here you don't need to lock mutex, because
> check_attach_btf_id() is called during verification before bpf_prog
> itself is visible to user-space, so there is no way to have concurrent
> access to it. If that wasn't the case, you'd need to take mutex lock
> before you assign tgt_prog local variable from prog->aux->tgt_prog
> above (and plus you'd need extra null checks and stuff).

Yeah, I did realise that (see above), but put it in because it doesn't
hurt, and it makes the comment above (about protecting writing) actually
be true :)

But changing the wording to include 'after it becomes visible' would
also fix this, so I'll remove the locking here...

-Toke