Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes:
> On Sat, Sep 19, 2020 at 4:50 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>>
>> From: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
>>
>> 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@xxxxxx>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
>> ---
>> 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
