Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points - Toke Høiland-Jørgensen

From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.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>,
	KP Singh <kpsingh@chromium.org>,
	netdev@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points
Date: Thu, 16 Jul 2020 12:50:05 +0200	[thread overview]
Message-ID: <87mu3zentu.fsf@toke.dk> (raw)
In-Reply-To: <20200715204406.vt64vgvzsbr6kolm@ast-mbp.dhcp.thefacebook.com>

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Wed, Jul 15, 2020 at 03:09:02PM +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 ||
>> +		    !btf_id) {
>> +			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;
>> +		}
>> +
>> +	} else if (btf_id) {
>> +		err = -EINVAL;
>> +		goto out_put_prog;
>> +	} else {
>> +		btf_id = prog->aux->attach_btf_id;
>> +		tgt_prog = prog->aux->linked_prog;
>> +		if (tgt_prog)
>> +			bpf_prog_inc(tgt_prog); /* we call bpf_prog_put() on link release */
>
> so the first prog_load cmd will beholding the first target prog?
> This is complete non starter.
> You didn't mention such decision anywhere.
> The first ext prog will attach to the first dispatcher xdp prog,
> then that ext prog will multi attach to second dispatcher xdp prog and
> the first dispatcher prog will live in the kernel forever.

Huh, yeah, you're right that's no good. Missing that was a think-o on my
part, sorry about that :/

> That's not what we discussed back in April.

No, you mentioned turning aux->linked_prog into a list. However once I
started looking at it I figured it was better to actually have all this
(the trampoline and ref) as part of the bpf_link structure, since
logically they're related.

But as you pointed out, the original reference sticks. So either that
needs to be removed, or I need to go back to the 'aux->linked_progs as a
list' idea. Any preference?

>> +	}
>> +	err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
>> +				      &fmodel, &addr, NULL, NULL);
>
> This is a second check for btf id match?
> What's the point? The first one was done at load time.
> When tgt_prog_fd/tgt_btf_id are zero there is no need to recheck.

It's not strictly needed if tgt_prog/btf_id is not set, but it doesn't
hurt either; and it was convenient to reuse it to resolve the func addr
for the trampoline + it means everything goes through the same code path.

> I really hope I'm misreading these patches, because they look very raw.

I don't think you are. I'll admit to them being a bit raw, but this was
as far as I got and since I'll be away for three weeks I figured it was
better to post them in case anyone else was interested in playing with
it.

So if anyone wants to pick these patches up while I'm gone, feel free;
otherwise, I'll get back to it after my vacation :)

-Toke