Message-ID: <87imemct2d.fsf@toke.dk>
Date:   Fri, 17 Jul 2020 12:52:10 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.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>,
        KP Singh <kpsingh@...omium.org>, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

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

> On Thu, Jul 16, 2020 at 12:50:05PM +0200, Toke Høiland-Jørgensen wrote:
>> Alexei Starovoitov <alexei.starovoitov@...il.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?
>
> Good question. Back then I was thinking about converting linked_prog into link
> list, since standalone single linked_prog is quite odd, because attaching ext
> prog to multiple tgt progs should have equivalent properties across all
> attachments.
> Back then bpf_link wasn't quite developed.
> Now I feel moving into bpf_tracing_link is better.
> I guess a link list of bpf_tracing_link-s from 'struct bpf_prog' might work.
> At prog load time we can do bpf_link_init() only (without doing bpf_link_prime)
> and keep this pre-populated bpf_link with target bpf prog and trampoline
> in a link list accessed from 'struct bpf_prog'.
> Then bpf_tracing_prog_attach() without extra tgt_prog_fd/btf_id would complete
> that bpf_tracing_link by calling bpf_link_prime() and bpf_link_settle()
> without allocating new one.
> Something like:
> struct bpf_tracing_link {
>         struct bpf_link link;  /* ext prog pointer is hidding in there */
>         enum bpf_attach_type attach_type;
>         struct bpf_trampoline *tr;
>         struct bpf_prog *tgt_prog; /* old aux->linked_prog */
> };
>
> ext prog -> aux -> link list of above bpf_tracing_link-s

Yeah, I thought along these lines as well (was thinking a new struct
referenced from bpf_tracing_link, but sure, why not just stick the whole
thing into aux?).

> It's a circular reference, obviously.
> Need to think through the complications and locking.

Yup, will do so when I get back to this. One other implication of this
change: If we make the linked_prog completely dynamic you can no longer
do:

link_fd = bpf_raw_tracepoint_open(prog);
close(link_fd);
link_fd = bpf_raw_tracepoint_open(prog):

since after that close(), the original linked_prog will be gone. Unless
we always leave at least one linked_prog alive? But then we can't
guarantee that it's the target that was supplied on program load if it
was reattached. Is that acceptable?

>> 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.
>
> Since it was v2 I figured you want it to land and it's ready.
> Next time please mention the state of patches.
> It's absolutely fine to post raw patches. It's fine to post stuff
> that doesn't compile. But please explain the state in commit logs or cover.

Right, sorry that was not clear; will make sure to spell it out next
time.

-Toke

Powered by blists - more mailing lists