netdev - Re: [PATCHv4 bpf-next 1/5] bpf: Allow trampoline re-attach for tracing and lsm programs

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:   Tue, 13 Apr 2021 15:03:27 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <jolsa@...nel.org>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        kernel test robot <lkp@...el.com>,
        Julia Lawall <julia.lawall@...6.fr>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Julia Lawall <julia.lawall@...ia.fr>
Subject: Re: [PATCHv4 bpf-next 1/5] bpf: Allow trampoline re-attach for
 tracing and lsm programs

On Mon, Apr 12, 2021 at 9:28 AM Jiri Olsa <jolsa@...nel.org> wrote:
>
> Currently we don't allow re-attaching of trampolines. Once
> it's detached, it can't be re-attach even when the program
> is still loaded.
>
> Adding the possibility to re-attach the loaded tracing and
> lsm programs.
>
> Fixing missing unlock with proper cleanup goto jump reported
> by Julia.
>
> Reported-by: kernel test robot <lkp@...el.com>
> Reported-by: Julia Lawall <julia.lawall@...6.fr>
> Acked-by: Toke Høiland-Jørgensen <toke@...hat.com>
> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
>  kernel/bpf/syscall.c    | 23 +++++++++++++++++------
>  kernel/bpf/trampoline.c |  2 +-
>  2 files changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 6428634da57e..f02c6a871b4f 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2645,14 +2645,25 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
>          *   target_btf_id using the link_create API.
>          *
>          * - if tgt_prog == NULL when this function was called using the old
> -         *   raw_tracepoint_open API, and we need a target from prog->aux
> -         *
> -         * The combination of no saved target in prog->aux, and no target
> -         * specified on load is illegal, and we reject that here.
> +        *   raw_tracepoint_open API, and we need a target from prog->aux
> +        *
> +        * - if prog->aux->dst_trampoline and tgt_prog is NULL, the program
> +        *   was detached and is going for re-attachment.
>          */
>         if (!prog->aux->dst_trampoline && !tgt_prog) {
> -               err = -ENOENT;
> -               goto out_unlock;
> +               /*
> +                * Allow re-attach for TRACING and LSM programs. If it's
> +                * currently linked, bpf_trampoline_link_prog will fail.
> +                * EXT programs need to specify tgt_prog_fd, so they
> +                * re-attach in separate code path.
> +                */
> +               if (prog->type != BPF_PROG_TYPE_TRACING &&
> +                   prog->type != BPF_PROG_TYPE_LSM) {
> +                       err = -EINVAL;
> +                       goto out_unlock;
> +               }
> +               btf_id = prog->aux->attach_btf_id;
> +               key = bpf_trampoline_compute_key(NULL, prog->aux->attach_btf, btf_id);
>         }
>
>         if (!prog->aux->dst_trampoline ||
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 1f3a4be4b175..48b8b9916aa2 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -437,7 +437,7 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog, struct bpf_trampoline *tr)
>                 tr->extension_prog = NULL;
>                 goto out;
>         }
> -       hlist_del(&prog->aux->tramp_hlist);
> +       hlist_del_init(&prog->aux->tramp_hlist);

there is another hlist_del few lines above in error handling path of
bpf_trampoline_link_prog(), it should probably be also updated to
hlist_del_init(), no?

>         tr->progs_cnt[kind]--;
>         err = bpf_trampoline_update(tr);
>  out:
> --
> 2.30.2
>