Message-ID: <20200407014455.u7x36kkfmxcllqa6@ast-mbp.dhcp.thefacebook.com>
Date:   Mon, 6 Apr 2020 18:44:55 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        "David S. Miller" <davem@...emloft.net>,
        Andrey Ignatov <rdna@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: bpf: ability to attach freplace to multiple parents

On Fri, Apr 03, 2020 at 10:38:38AM +0200, Toke Høiland-Jørgensen wrote:
> Alexei Starovoitov <alexei.starovoitov@...il.com> writes:
> 
> > It's a different link.
> > For fentry/fexit/freplace the link is pair:
> >   // target           ...         bpf_prog
> > (target_prog_fd_or_vmlinux, fentry_exit_replace_prog_fd).
> >
> > So for xdp case we will have:
> > root_link = (eth0_ifindex, dispatcher_prog_fd) // dispatcher prog attached to eth0
> > link1 = (dispatcher_prog_fd, xdp_firewall1_fd) // 1st extension prog attached to dispatcher
> > link2 = (dispatcher_prog_fd, xdp_firewall2_fd) // 2nd extension prog attached to dispatcher
> >
> > Now libxdp wants to update the dispatcher prog.
> > It generates new dispatcher prog with more placeholder entries or new policy:
> > new_dispatcher_prog_fd.
> > It's not attached anywhere.
> > Then libxdp calls new bpf_raw_tp_open() api I'm proposing above to create:
> > link3 = (new_dispatcher_prog_fd, xdp_firewall1_fd)
> > link4 = (new_dispatcher_prog_fd, xdp_firewall2_fd)
> > Now we have two firewalls attached to both old dispatcher prog and new dispatcher prog.
> > Both firewalls are executing via old dispatcher prog that is active.
> > Now libxdp calls:
> > bpf_link_udpate(root_link, dispatcher_prog_fd, new_dispatcher_prog_fd)
> > which atomically replaces old dispatcher prog with new dispatcher prog in eth0.
> > The traffic keeps flowing into both firewalls. No packets lost.
> > But now it goes through new dipsatcher prog.
> > libxdp can now:
> > close(dispatcher_prog_fd);
> > close(link1);
> > close(link2);
> > Closing (and destroying two links) will remove old dispatcher prog
> > from linked list in xdp_firewall1_prog->aux->linked_prog_list and from
> > xdp_firewall2_prog->aux->linked_prog_list.
> > Notice that there is no need to explicitly detach old dispatcher prog from eth0.
> > link_update() did it while replacing it with new dispatcher prog.
> 
> Yeah, this was the flow I had in mind already. However, what I meant was
> that *from the PoV of an application consuming the link fd*, this would
> lead to dangling links.
> 
> I.e., an application does:
> 
> app1_link_fd = libxdp_install_prog(prog1);
> 
> and stores link_fd somewhere (just holds on to it, or pins it
> somewhere).
> 
> Then later, another application does:
> 
> app2_link_fd = libxdp_install_prog(prog2);
> 
> but this has the side-effect of replacing the dispatcher, so
> app1_link_fd is now no longer valid.
> 
> This can be worked around, of course (e.g., just return the prog_fd and
> hide any link_fd details inside the library), but if the point of
> bpf_link is that the application could hold on to it and use it for
> subsequent replacements, that would be nice to have for consumers of the
> library as well, no?

link is a pair of (hook, prog). I don't think that single bpf-link (FD)
should represent (hook1, hook2, hook3, prog). It will be super confusing to the
user space when single FD magically turns into multi attach. If you really need
one object to represent multiple bpf_links where the same program is attached
to multiple location such abstraction needs to be done by user space library.
At the end it's libbpf job. I think it's fine for libbpf to have
'struct bpf_multi_link' where multiple 'struct bpf_link' can be aggregated.
>From task point of view they are all FDs and will get autoclosed and such.

There is also a way to update dispatch prog without introducing bpf_multi_link.
My understanding that you don't want libxdp to work as a daemon.
So app1 does libxdp_install_prog(prog1) and gets back
'struct bpf_link *' (which is FD internally).
App2 wants to refresh dispatcher prog.
It loads new prog. Finds bpf_link of app1 (ether in bpffs or via bpf_link idr).
Queries app1_prog_id->fd.
app1_link2_fd = bpf_raw_tp_open(app1_prog_fd, new_dispatch_prog, new_btf_id);
// now app1_prog is attached to two dispatcher progs

bpf_link_update(root_link, old_dispatcher_prog, new_dispatcher_prog);
// now traffic is going to app1 prog via new dispatcher

bpf_link_update_hook(app1_link1_fd, app1_link2_fd);
here I'm proposing a new operation that will close 2nd link and will update
hook of the first link with the hook of 2nd link if prog is the same.
Conceptually it's a similar operation to bpf_link_update() which replaces bpf
prog in the hook. bpf_link_update_hook() can replace the hook while keeping the
program the same.

Note it cannot be called earlier. app2 still need to attach app1 prog to
two dispatcher progs, replace dispatcher and only then switch the hook
in bpf_link internals. Otherwise app1 traffic will stop while new dispatcher
is not yet active.

Powered by blists - more mailing lists