On Tue, Sep 22, 2020 at 3:17 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>
> 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.
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?
> 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.
>
Of course you don't need to take lock while you are constructing
bpf_prog... It's like taking a lock inside a constructor in C++ before
the outside world can ever access object's fields. No harm, but also
pointless.
> 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 :)
>
See above about locking in a constructor analogy.
But as is the code is split-brained: it accesses prog->aux->tgt_prog
outside of mutex and prog->aux->tgt_trampoline inside the mutex. So
when reading this the natural question is why it's not one way of
doing things. Reading a field without a lock held is (in general) just
as wrong as updating that field without lock.
> But changing the wording to include 'after it becomes visible' would
> also fix this, so I'll remove the locking here...
I'd leave a comment that check_attach_btf_id is only called during
BPF_PROG_LOAD before bpf_prog is visible to user-space, so there is no
possibility of concurrently accessing its fields yet. That would make
it clear why we don't need a lock.
>
> -Toke
>
