Tail call program removed from PROG_ARRAY

[yasir-ss]

Describe the bug
When I write to a ring buffer in a BPF program and read from it in user space, I observe that all tail programs are removed. The tail program no longer shows up in bpftool prog. bpftool map dump name jump_table (jump_table is the array containing the tail program) says it Found 0 elements; it originally had 1 element, the tail program.

To Reproduce

$ go version
go version go1.17 linux/amd64
$ uname -a
Linux yasir-virtualbox 5.13.0-27-generic #29-Ubuntu SMP Wed Jan 12 17:36:47 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
$ cat /etc/issue
Ubuntu 21.10 \n \l

This BPF program consists of main_prog calling a tail program. The tail program writes 0 to a ring buffer.

#include <linux/bpf.h>
#include <bpf/bpf_helpers.h>

struct bpf_map_def SEC("maps") flow_ring_buf = {
    .type = BPF_MAP_TYPE_RINGBUF,
    .max_entries = 1<<12
};

struct bpf_map_def SEC("maps") jump_table = {
   .type = BPF_MAP_TYPE_PROG_ARRAY,
   .key_size = sizeof(__u32),
   .value_size = sizeof(__u32),
   .max_entries = 1,
};

SEC("xdp")
int main_prog(struct xdp_md *ctx) {
    bpf_tail_call(ctx, &jump_table, 0);

    bpf_printk("Tail call failed");

    return XDP_PASS;
}

SEC("xdp_2")
int tail_prog(struct xdp_md *ctx) {
    __u32 num = 0;

    bpf_ringbuf_output(&flow_ring_buf, &num, sizeof(__u32), 0);

    return XDP_PASS;
}

char _license[] SEC("license") = "GPL";

This Go program loads the programs and map and reads from the ring buffer:

package main

import "C"
import (
    "errors"
    "github.com/cilium/ebpf"
    "github.com/cilium/ebpf/ringbuf"
    "github.com/vishvananda/netlink"
    "log"
)

type bpfObjects struct {
    MainProg        *ebpf.Program `ebpf:"main_prog"`
    TailProg        *ebpf.Program `ebpf:"tail_prog"`
    JumpTable       *ebpf.Map     `ebpf:"jump_table"`
    FlowRingBuf     *ebpf.Map     `ebpf:"flow_ring_buf"`
}

func main() {
    var objects bpfObjects

    spec, err := ebpf.LoadCollectionSpec("test.o")

    if err != nil {
        log.Fatalln("ebpf.LoadCollectionSpec", err)
    }

    if err := spec.LoadAndAssign(&objects, nil); err != nil {
        log.Fatalln("ebpf.LoadAndAssign", err)
    }

    // Update the jump table with the tail prog
    if err = objects.JumpTable.Update(uint32(0), uint32(objects.TailProg.FD()), ebpf.UpdateAny); err != nil {
        log.Fatalln("Update prog_array", err)
    }

    link, err := netlink.LinkByName("enp0s8")

    if err != nil {
        log.Fatalln("netlink.LinkByName", err)
    }

    // Load the program onto the interface
    if err = netlink.LinkSetXdpFdWithFlags(link, objects.MainProg.FD(), 0x2); err != nil {
        log.Fatalln("netlink.LinkSetXdpFdWithFlags:", err)
    }

    // A
    // Problem doesn't happen if you comment out code below and replace with select {}
    reader, err := ringbuf.NewReader(objects.FlowRingBuf)

    for {
        _, err := reader.Read()

        if err != nil {
            if errors.Is(err, ringbuf.ErrClosed) {
                log.Println("Received signal, exiting..")
                return
            }
            log.Printf("reading from reader: %s", err)
            continue
        }
    }
}

Compile the C program using clang -O2 -g -Wall -target bpf -c test.o -o test.o

Pass traffic on the interface. The tail program is removed after a random amount of time.

If I don't read from the ring buffer in userspace, the tail program is not removed. If I remove the call to bpf_ringbuf_output() in tail_prog, the tail program is not removed.

Expected behavior
The tail program should remain loaded when writing into the ring buffer.

[mythi]

Pass traffic on the interface. The tail program is removed after a random amount of time.

I'll see if I can reproduce it here. Is the "random amount of time" related to how many packets are sent? I have a setup with ping and the prog attached to lo...

If I don't read from the ring buffer in userspace, the tail program is not removed.

have you tried with a perf buffer?

[yasir-ss]

I'll see if I can reproduce it here. Is the "random amount of time" related to how many packets are sent? I have a setup with ping and the prog attached to lo...

It does not. I'm using k6 to test this, and I observe that this sometimes happens immediately or after under a minute.

have you tried with a perf buffer?

No

[ti-mo]

Hi @yasir-ss. Sorry, but what you're describing wrt. removing the call to bpf_ringbuf_output() doesn't make much sense. As long as you keep your program running, tail_prog will remain loaded in the kernel, and will remain in jump_table at slot 0.

BPF objects' lifecycles are tied to the process that created them, except when they are attached to another kernel object (like you're doing here with XDP) or pinned to bpffs. The thing that's biting you here is that PROG_ARRAYs get cleared (not removed) when their last userspace reference disappears, or when its last bpffs pin is removed. Since XDP references main_prog, which references jump_table, the tail call map is not deallocated, so you can still read it using bpftool after your program exits.

Edit: @yasir-ss, as Lorenz suggested below, you need the following:

func (o *bpfObjects) Close() {
  o.MainProg.Close()
  o.TailProg.Close()
  o.JumpTable.Close()
  o.FlowRingBuf.Close()
}

Then, after LoadAndAssign(), call defer objects.Close().

See bpf2go in examples/ to automate all this.

---

If you want a tail call setup to persist throughout userspace program restarts, you can use e.g. objects.JumpTable.Pin("/sys/fs/bpf/jump_table"), but keep in mind this introduces state you will have to deal with. See CollectionOptions.Maps.PinPath and the pinning flag in map definitions.

On that note, see the following snippet to automatically populate your tail call map:

ebpf/testdata/btf_map_init.c

Lines 11 to 22 in 42f2a98

	// Tail call map (program array) initialized with program pointers.
	struct {
	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
	__uint(key_size, sizeof(uint32_t));
	__uint(max_entries, 2);
	__array(values, int ());
	} prog_array_init __section(".maps") = {
	.values = {
	// Skip index 0 to exercise empty array slots.
	[1] = &tail_1,
	},
	};

2 more things:

• bpf_map_def is deprecated, use BTF-style maps as seen in the snippet above
• Prefer bpf-to-bpf function calls instead of tail calls if you don't need to run on pre-4.16 and your program is not too large to require per-function verification. This will simplify your program significantly.

[yasir-ss]

If I remove jump_table from bpfObjects:

type bpfObjects struct {
    MainProg        *ebpf.Program `ebpf:"main_prog"`
    TailProg        *ebpf.Program `ebpf:"tail_prog"`
    FlowRingBuf     *ebpf.Map     `ebpf:"flow_ring_buf"`
}

LoadAndAssign returns this error: ProgramArray jump_table must be assigned to prevent missed tail calls. This is happening despite initializing jump_table in the BPF program.

[yasir-ss]

An even simpler solution would be to prevent your Go code from garbage collecting your JumpTable reference by adding a defer objects.Close().

The problem still happens when I add defer objects.TailProg.Close() after calling LoadAssign(). I'm not even updating jump_table in user space anymore.

[ti-mo]

Your userspace reference to TailProg closing won't cut off the program, it's losing your JumpTable reference at runtime that causes the prog array to be cleared. You want to defer objects.JumpTable.Close(), this will keep a reference to JumpTable alive until main() exits.

Ideally, you write a function bpfObject.Close() that calls Close() on all its members and you defer objects.Close() after LoadAndAssign.

[sivaaraali]

Wondering why the prog array is being cleared when there is still a reference from the tail call. Why is the reference from the tail call not considered a valid reference for lifetime management? I would have expected the tail call programs to be alive as long as the main program is alive.

[ti-mo]

The main program can invoke a tail call, but tail calls can also call into themselves. Keep in mind that, to the verifier, these are all just normal programs with references to a prog array. A reference to a prog array only counts towards the refcount of the map itself, not towards any of the programs it contains.

If prog array references would count as strong references to all progs, we could easily end up with a self-referential web of progs/maps that would implicitly outlive user space, needing explicit manual cleanup. I think this was a decision made in the kernel early on to keep the implementation simple, without risking resource leaks.

[dylandreimerink]

I played around a bit with the example. First creating a dump with strace, which ends like this:

futex(0x697cf0, FUTEX_WAIT_PRIVATE, 0, NULL) = 0
futex(0x697cf0, FUTEX_WAIT_PRIVATE, 0, NULL) = 0
futex(0x697cf0, FUTEX_WAIT_PRIVATE, 0, NULL) = -1 EAGAIN (Resource temporarily unavailable)
futex(0x697cf0, FUTEX_WAIT_PRIVATE, 0, NULL) = -1 EAGAIN (Resource temporarily unavailable)
futex(0xc000056950, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x6c5df8, FUTEX_WAKE_PRIVATE, 1)  = 1
futex(0x697cf0, FUTEX_WAIT_PRIVATE, 0, NULL) = -1 EAGAIN (Resource temporarily unavailable)
futex(0x6979c8, FUTEX_WAKE_PRIVATE, 1)  = 1
futex(0x697cf0, FUTEX_WAIT_PRIVATE, 0, NULL) = 0
futex(0xc000100150, FUTEX_WAKE_PRIVATE, 1) = 1
--- SIGURG {si_signo=SIGURG, si_code=SI_TKILL, si_pid=2244467, si_uid=0} ---
rt_sigreturn({mask=[]})                 = 139978841854774
--- SIGURG {si_signo=SIGURG, si_code=SI_TKILL, si_pid=2244467, si_uid=0} ---
rt_sigreturn({mask=[]})                 = 139978841854774
futex(0x6979f8, FUTEX_WAKE_PRIVATE, 1)  = 0
epoll_pwait(4, [], 128, 0, NULL, 0)     = 0
mmap(NULL, 262144, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f4f3437f000
futex(0xc000580150, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x6979f8, FUTEX_WAIT_PRIVATE, 0, {tv_sec=0, tv_nsec=100000}) = 0
close(11)                               = 0
close(10)                               = 0
close(9)                                = 0
close(8)                                = 0
close(3)                                = 0
futex(0xc000056950, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x697cf0, FUTEX_WAIT_PRIVATE, 0, NULL) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
--- SIGWINCH {si_signo=SIGWINCH, si_code=SI_KERNEL} ---
rt_sigreturn({mask=[]})                 = 202
futex(0x697cf0, FUTEX_WAIT_PRIVATE, 0, NULL) = 0
madvise(0xc000400000, 2097152, MADV_NOHUGEPAGE) = 0
madvise(0xc000536000, 8192, MADV_DONTNEED) = 0
futex(0xc000056950, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x697cf0, FUTEX_WAIT_PRIVATE, 0, NULL) = 0
futex(0x697cf0, FUTEX_WAIT_PRIVATE, 0, NULL) = 0

So it looks like we are indeed removing all references which causes the entry to become empty.

As far as I can tell, this is because objects get GC'ed after a while because it is not used anymore in the for loop, and apparently also not referenced anywhere.

Adding defer fmt.Println(objects) to just before the for loop fixes the issue, so does adding it after the for loop and breaking instead of returning.

[yasir-ss]

To verify this wasn't an issue with the kernel, I created the equivalent C program using libbpf 0.7, and I didn't see any issues:

#include <libbpf.h>
#include <bpf.h>
#include <stdio.h>
#include <net/if.h>

// Handles each insert into ring buffer
static int flow_buf_sample(void *ctx, void *data, size_t len) {
    return 0;
}

int main(int argc, char *argv[]) {
    struct bpf_object *obj;
    struct bpf_map *jump_table;
    struct bpf_map *flow_ring_buf;
    struct bpf_program *tail_prog;
    struct bpf_program *main_prog;

    int err;

    if ((obj = bpf_object__open_file("test.o", NULL)) == NULL) {
        fprintf(stderr, "Could not open ELF");
        return 1;
    }

    if ((err = bpf_object__load(obj)) < 0) {
        fprintf(stderr, "Could not load BPF");
        return 1;
    }

    if ((jump_table = bpf_object__find_map_by_name(obj, "jump_table")) == NULL) {
        fprintf(stderr, "Could not find jump_table map");
        return 1;
    }

    if ((flow_ring_buf = bpf_object__find_map_by_name(obj, "flow_ring_buf")) == NULL) {
        fprintf(stderr, "Could not find flow_ring_buf_map map");
        return 1;
    }

    if ((main_prog = bpf_object__find_program_by_name(obj, "main_prog")) == NULL) {
        fprintf(stderr, "Could not find main_prog");
        return 1;
    }

    if ((tail_prog = bpf_object__find_program_by_name(obj, "tail_prog")) == NULL) {
        fprintf(stderr, "Could not find tail_prog");
        return 1;
    }

    struct ring_buffer *ring_buffer = ring_buffer__new(bpf_map__fd(flow_ring_buf), flow_buf_sample, NULL, NULL);

    if (ring_buffer == NULL) {
        fprintf(stderr, "failed to create ring buffer\n");
        return 1;
    }

    int index0 = 0;
    int tail_prog_fd = bpf_program__fd(tail_prog);

    if ((err = bpf_map_update_elem(bpf_map__fd(jump_table), &index0, &tail_prog_fd, 0)) < 0) {
        fprintf(stderr, "failed update jump_table: %d", err);
    }

    int if_index = if_nametoindex(argv[1]);

    if (!if_index) {
        printf("get if_index from interface name failed\n");
        return 1;
    }

    if ((err = bpf_xdp_attach(if_index, bpf_program__fd(main_prog), 0x2, NULL)) != 0) {
        fprintf(stderr, "bpf_xdp_attach failed: %d", err);
        return 1;
    }

    while(1) {
        ring_buffer__poll(ring_buffer, -1);
    }

    return 0;
}

[ti-mo]

Please see my thread above (middle paragraph), it explains in detail how prog array lifecycle works and why a Go reference needs to be kept until the program exits.

The Go runtime/compiler indeed seems smart enough to determine that object is no longer accessed when the loop enters, so it is collected. Add runtime.GC() after creating the reader to reproduce.

As for the workaround you suggested; the break statement seems arbitrary. Since you're manually writing bpfObjecthere, you also need to write a Close() that cleans up all its resources and will take care of maintaining a reference until main exits.

Adding a defer objects.JumpTable.Close() after LoadAndAssign also works in this case.

[dylandreimerink]

Ah, the The thing that's biting you here is that PROG_ARRAYs get cleared (not removed) when their last userspace reference disappears, or when its last bpffs pin is removed. part, right. I thought you were talking about something we actively do, but its a kernel feature then. Didn't know that 👍

[yasir-ss]

Thanks, that makes sense. Yes, the break statement is arbitrary.

[ti-mo]

@yasir-ss I've documented this here: #579, hope this helps someone in the future. Thanks for the report!