Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> writes: > On Fri, Apr 03, 2020 at 10:38:38AM +0200, Toke Høiland-Jørgensen wrote: >> Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> 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. I do agree with this, actually, and mostly brought it up as a point of discussion to see if we could come up with something better. And I think this: > 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. will work for me, so great! :) -Toke
