Re: [PATCH bpf-next v8 05/11] bpf: support attaching freplace programs to multiple attach points

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andriin@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	Jiri Olsa <jolsa@redhat.com>,
	Eelco Chaudron <echaudro@redhat.com>,
	KP Singh <kpsingh@chromium.org>,
	netdev@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next v8 05/11] bpf: support attaching freplace programs to multiple attach points
Date: Wed, 23 Sep 2020 18:04:54 -0700	[thread overview]
Message-ID: <20200924010454.anythmtl53l3d3xv@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <160079991915.8301.7262542368795622735.stgit@toke.dk>

On Tue, Sep 22, 2020 at 08:38:39PM +0200, Toke Høiland-Jørgensen wrote:
> +
> +	if (tgt_prog_fd) {
> +		/* For now we only allow new targets for BPF_PROG_TYPE_EXT */
> +		if (prog->type != BPF_PROG_TYPE_EXT) {
> +			err = -EINVAL;
> +			goto out_put_prog;
> +		}
> +
> +		tgt_prog = bpf_prog_get(tgt_prog_fd);
> +		if (IS_ERR(tgt_prog)) {
> +			err = PTR_ERR(tgt_prog);
> +			tgt_prog = NULL;
> +			goto out_put_prog;
> +		}
> +
> +		key = ((u64)tgt_prog->aux->id) << 32 | btf_id;

key = bpf_trampoline_compute_key(tgt_prog, btf_id);
would be handy here.

> +	}
> +
>  	link = kzalloc(sizeof(*link), GFP_USER);
>  	if (!link) {
>  		err = -ENOMEM;
> @@ -2594,12 +2622,28 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
>  
>  	mutex_lock(&prog->aux->tgt_mutex);
>  
> -	if (!prog->aux->tgt_trampoline) {
> +	if (!prog->aux->tgt_trampoline && !tgt_prog) {
>  		err = -ENOENT;
>  		goto out_unlock;
>  	}

Could you add a comment explaining all cases, since it's hard to follow right
now and few month later no one will remember.
As far as I understood:
prog->aux->dest_trampoline != NULL -> the program was just loaded and not attached to anything.
prog->aux->dest_trampoline == NULL -> the program was loaded and raw_tp_open-ed.
tgt_prog != NULL only when sepcifying tgt_prog_fd + target_btf_id in link_create api.
tgt_prog == NULL when this function is called from raw_tp_open.

Only the case of both NULL is invalid.

> -	tr = prog->aux->tgt_trampoline;
> -	tgt_prog = prog->aux->tgt_prog;
> +
> +	if (!prog->aux->tgt_trampoline ||
> +	    (key && key != prog->aux->tgt_trampoline->key)) {
> +
> +		err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
> +					      &fmodel, &addr, NULL, NULL);
> +		if (err)
> +			goto out_unlock;
> +
> +		tr = bpf_trampoline_get(key, (void *)addr, &fmodel);
> +		if (!tr) {
> +			err = -ENOMEM;
> +			goto out_unlock;
> +		}
> +	} else {

This 'else' is the case when a prog was loaded and _not_ raw_tp_open-ed
and the user is doing link_create with tgt_prog_fd + target_btf_id
into exactly the same place as attach_btf_id during load?
So this is the alternative api to raw_tp_open, right?
Please explain this in commit log and in comments.
It's not some minor detail.

> +		tr = prog->aux->tgt_trampoline;
> +		tgt_prog = prog->aux->tgt_prog;
> +	}
>  
>  	err = bpf_link_prime(&link->link, &link_primer);
>  	if (err)
> @@ -2614,16 +2658,24 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
>  
>  	link->tgt_prog = tgt_prog;
>  	link->trampoline = tr;
> -
> -	prog->aux->tgt_prog = NULL;
> -	prog->aux->tgt_trampoline = NULL;
> +	if (tr == prog->aux->tgt_trampoline) {
> +		/* if we got a new ref from syscall, drop existing one from prog */
> +		if (tgt_prog_fd)
> +			bpf_prog_put(prog->aux->tgt_prog);
> +		prog->aux->tgt_trampoline = NULL;
> +		prog->aux->tgt_prog = NULL;
> +	}

What happens when the user did prog load with attach_btf_id into one tgt_prog
but then link_create into a different tgt_prog?
bpf_check_attach_target + bpf_trampoline_get will allocate new trampoline (potentially)
and tr != prog->aux->dest_trampoline,
so we won't trigger the above code.
prog->aux->dest_prog/dest_tramoline will still point to some prog.
Later raw_tp_open will succeed and prog will be attached to two places.
I would probably make it unconditional that both raw_tp_open
and link_create clear dest_prog/dest_trampoline, but can be convinced otherwise.
What use case do you have in mind to allow that?
Anyway it needs to be documented and tests written.

> +		if ((prog->aux->tgt_prog_type &&
> +		     prog->aux->tgt_prog_type != tgt_prog->type) ||
> +		    (prog->aux->tgt_attach_type &&
> +		     prog->aux->tgt_attach_type != tgt_prog->expected_attach_type))
> +			return -EINVAL;

May be call them saved_tgt_prog_type and saved_tgt_attach_type ?
Since that's another variant of 'target' meaning. Here the prog type survives
the target prog, since it will be still valid even when the first prog it was
attached to will be unloaded.