On Mon, Mar 2, 2020 at 2:12 AM Toke Høiland-Jørgensen [off-list ref] wrote:

Andrii Nakryiko [off-list ref] writes:

quoted

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 :)

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.

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.

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?

I'll try to propose patches for what this could look like; I think it
could co-exist with this bpf_link abstraction, though, so no need to
hold up this series...

Yeah, either way, this is important and is desired behavior not just
for freplace cases.

-Toke