On Mon, Mar 2, 2020 at 2:12 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote: > > Andrii Nakryiko <andriin@xxxxxx> 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 :) > > 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 >
