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
> + 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).
> return 0;
> }
>
>
