netdev - Re: bpf: ability to attach freplace to multiple parents

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 07 Apr 2020 11:20:39 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.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

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

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

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