Message-ID: <CAEf4BzYdBy0xOVBb3RSVqtrc9+XL459LjT9hNGfmTy=QYDQ+AQ@mail.gmail.com>
Date:   Tue, 22 Sep 2020 09:45:09 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>,
        KP Singh <kpsingh@...omium.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v7 04/10] bpf: move prog->aux->linked_prog and
 trampoline into bpf_link on attach

On Tue, Sep 22, 2020 at 3:17 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@...il.com> writes:
>
> > On Sat, Sep 19, 2020 at 4:50 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
> >>
> >> From: Toke Høiland-Jørgensen <toke@...hat.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@...com>
> >> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.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?

> 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
>

Powered by blists - more mailing lists