netdev - Re: [PATCH bpf-next v7 04/10] bpf: move prog->aux->linked_prog and trampoline into bpf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Sep 2020 16:05:59 -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 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

> +       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;
>  }
>
>