netdev - Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach

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 for Android: free password hash cracker in your pocket

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

Message-ID: <87blavd31f.fsf@toke.dk>
Date:   Sat, 03 Apr 2021 13:24:12 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>
Cc:     netdev@...r.kernel.org, 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>
Subject: Re: [RFC PATCH bpf-next 1/4] bpf: Allow trampoline re-attach

Jiri Olsa <jolsa@...nel.org> writes:

> 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
> kernel program.

Hmm, yeah, didn't really consider this case when I added the original
disallow. But don't see why not, so (with one nit below):

Acked-by: Toke Høiland-Jørgensen <toke@...hat.com>

> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
>  kernel/bpf/syscall.c    | 25 +++++++++++++++++++------
>  kernel/bpf/trampoline.c |  2 +-
>  2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 9603de81811a..e14926b2e95a 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2645,14 +2645,27 @@ 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
> +	 *
> +	 * The combination of no saved target in prog->aux, and no target
> +	 * specified on is legal only for tracing programs re-attach, rest
> +	 * is illegal, and we reject that here.
>  	 */
>  	if (!prog->aux->dst_trampoline && !tgt_prog) {
> -		err = -ENOENT;
> -		goto out_unlock;
> +		/*
> +		 * Allow re-attach for tracing programs, if it's currently
> +		 * linked, bpf_trampoline_link_prog will fail.
> +		 */
> +		if (prog->type != BPF_PROG_TYPE_TRACING) {
> +			err = -ENOENT;
> +			goto out_unlock;
> +		}
> +		if (!prog->aux->attach_btf) {
> +			err = -EINVAL;
> +			goto out_unlock;
> +		}

I'm wondering about the two different return codes here. Under what
circumstances will aux->attach_btf be NULL, and why is that not an
ENOENT error? :)

-Toke