Message-ID: <20200717020507.jpxxe4dbc2watsfh@ast-mbp.dhcp.thefacebook.com>
Date:   Thu, 16 Jul 2020 19:05:07 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.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

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

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

bpf_tracing_prog_attach() with tgt_prog_fd/btf_id will alloc new bpf_tracing_link
and will add it to a link list.

Just a rough idea. I wonder what Andrii thinks.

> 
> >> +	}
> >> +	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.

Doing the same work twice is a sign that this function needs to split
into more than 3 helpers, so the work is not repeated.

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

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.

Powered by blists - more mailing lists