Message-ID: <87imjms8cm.fsf@toke.dk>
Date:   Mon, 02 Mar 2020 23:24:57 +0100
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 0/3] Introduce pinnable bpf_link kernel abstraction

Andrii Nakryiko <andrii.nakryiko@...il.com> writes:

> On Mon, Mar 2, 2020 at 2:12 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>>
>> Andrii Nakryiko <andriin@...com> writes:
>>
>> > This patch series adds bpf_link abstraction, analogous to libbpf's already
>> > existing bpf_link abstraction. This formalizes and makes more uniform existing
>> > bpf_link-like BPF program link (attachment) types (raw tracepoint and tracing
>> > links), which are FD-based objects that are automatically detached when last
>> > file reference is closed. These types of BPF program links are switched to
>> > using bpf_link framework.
>> >
>> > FD-based bpf_link approach provides great safety guarantees, by ensuring there
>> > is not going to be an abandoned BPF program attached, if user process suddenly
>> > exits or forgets to clean up after itself. This is especially important in
>> > production environment and is what all the recent new BPF link types followed.
>> >
>> > One of the previously existing  inconveniences of FD-based approach, though,
>> > was the scenario in which user process wants to install BPF link and exit, but
>> > let attached BPF program run. Now, with bpf_link abstraction in place, it's
>> > easy to support pinning links in BPF FS, which is done as part of the same
>> > patch #1. This allows FD-based BPF program links to survive exit of a user
>> > process and original file descriptor being closed, by creating an file entry
>> > in BPF FS. This provides great safety by default, with simple way to opt out
>> > for cases where it's needed.
>>
>> While being able to pin the fds returned by bpf_raw_tracepoint_open()
>> certainly helps, I still feel like this is the wrong abstraction for
>> freplace(): When I'm building a program using freplace to put in new
>> functions (say, an XDP multi-prog dispatcher :)), I really want the
>> 'new' functions (i.e., the freplace'd bpf_progs) to share their lifetime
>> with the calling BPF program. I.e., I want to be able to do something
>> like:
>
> freplace programs will take refcount on a BPF program they are
> replacing, so in that sense they do share lifetime, except dependency
> is opposite to what you describe: rootlet/dispatcher program can't go
> away as long it has at least one freplace program attached. It
> (dispatcher) might get detached, though, but freplace, technically,
> will still be attached to now-detached dispatcher (so won't be
> invoked, yet still attached). I hope that makes sense :)

Yes, I realise that; I just think it's the wrong way 'round :)

>> prog_fd = sys_bpf(BPF_PROG_LOAD, ...); // dispatcher
>> func_fd = sys_bpf(BPF_PROG_LOAD, ...); // replacement func
>> err = sys_bpf(BPF_PROG_REPLACE_FUNC, prog_fd, btf_id, func_fd); // does *not* return an fd
>>
>> That last call should make the ref-counting be in the prog_fd -> func_fd
>> direction, so that when prog_fd is released, it will do
>> bpf_prog_put(func_fd). There could be an additional call like
>> sys_bpf(BPF_PROG_REPLACE_FUNC_DETACH, prog_fd, btf_id) for explicit
>> detach as well, of course.
>
> Taking this additional refcount will create a dependency loop (see
> above), so that's why it wasn't done, I think.

Right, that's why I want the new operation to 'flip' the direction of
the refcnt inside the kernel, so there would no longer be a reference
from the replacement to the dispatcher, only the other way (so no
loops).

> With FD-based bpf_link, though, you'll be able to "transfer ownership"
> from application that installed freplace program in the first place,
> to the program that eventually will unload/replace dispatcher BPF
> program. You do that by pinning freplace program in BPFFS location,
> that's known to this libxdp library, and when you need to detach and
> unload XDP dispatcher and overriden XDP programs, the "admin process"
> which manages XDP dispatcher, will be able to just go and unpin and
> detach everything, if necessary.

Yes, so let's assume we want to do it this way (with replacement
programs pinned in a known location in bpffs). First off, that will
introduce a hard dependency on bpffs for XDP; so now your networking
program also needs access to mounting filesystems, or you need to rely
on the system (including your container runtime) to set this up right
for you.

Assuming that has been solved, the steps to replace an existing set of
programs with a new one would be something like:

0. We assume that we already have /sys/fs/bpf/xdp/eth0/mainprog,
   /sys/fs/bpf/xdp/eth0/prog1, /sys/fs/bpf/xdp/eth0/prog2, and that
   mainprog is attached to eth0
   
1. Create new dispatcher, attach programs, pin replacement progs
   somewhere (/sys/fs/bpf/xdp/eth0.new/{mainprog,prog1,prog2,prog3}?)

2. Attach new mainprog to eth0, replacing the old one

3. `mv /sys/fs/bpf/xdp/eth0 /sys/fs/bpf/xdp/eth0.old`

4. `mv /sys/fs/bpf/xdp/eth0.new /sys/fs/bpf/xdp/eth0`

5. `rm -r /sys/fs/bpf/xdp/eth0.old`


Or you could switch steps 2 and 3. Either way, there is no way to do
steps 2..4 as one atomic operation; so you can end up in an inconsistent
state.

Oh, and what happens if the netdevice moves namespaces while it has an
XDP program attached? Then you can end up with the bpffs where you
pinned the programs originally not being available at all anymore.

Contrast this to the case where the replacement programs are just
referenced from the main prog: none of the above will be necessary, and
replacement will just be one atomic operation on the interface.

>> With such an API, lifecycle management for an XDP program keeps being
>> obvious: There's an fd for the root program attached to the interface,
>> and that's it. When that is released the whole thing disappears. Whereas
>> with the bpf_raw_tracepoint_open() API, the userspace program suddenly
>> has to make sure all the component function FDs are pinned, which seems
>> cumbersome and error-prone...
>
> I thought that's what libxdp is supposed to do (among other things).
> So for user applications it will be all hidden inside the library API,
> no?

Sure, but that also means that even if we somehow manage to write a
library without any bugs, it will still be called from arbitrary
applications that may crash between two operations and leave things in
an inconsistent state.

So I guess what I'm saying is that while it most likely is *possible* to
build the multi-prog facility using bpf_raw_tracepoint_open() + pinning,
I believe that the result will be brittle, and that we would be better
served with a different kernel primitive.

Do you see what I mean? Or am I missing something obvious here?

-Toke

Powered by blists - more mailing lists