n-acd: runtime eBPF support detection

[jvaclav-rh]

Currently, eBPF support is toggled by a compile-time flag based on whether we expect the system to support it or not.

This PR makes it so that n-acd always attempts to use eBPF, and gracefully handles failure if it isn't supported (e.g. due to lacking capabilities).

This change is desirable for programs that would like to use n-acd with eBPF as often as possible, but expect that eBPF setup may fail (and are OK with falling back).

[dvdhrm]

Hi, thanks for the suggestion! Can you explain who would use it? Distros know their kernel versions, so they can easily require a kernel with eBPF. Who would build n-acd without knowing their kernel version?

[jvaclav-rh]

Can you explain who would use it? Distros know their kernel versions, so they can easily require a kernel with eBPF. Who would build n-acd without knowing their kernel version?

It is not so much about whether the kernel supports eBPF but also about if the application has capabilities to use it at runtime. For example it would be very useful for NetworkManager if it is e.g. running in a container.

edit: I also just noticed an older pull request (#9) that had some discussion related to this.

[jvaclav-rh]

@dvdhrm Is there anything I can explain or amend to help move this forward?

[bengal]

Can you explain who would use it? Distros know their kernel versions, so they can easily require a kernel with eBPF. Who would build n-acd without knowing their kernel version?

It is not so much about whether the kernel supports eBPF but also about if the application has capabilities to use it at runtime. For example it would be very useful for NetworkManager if it is e.g. running in a container.

edit: I also just noticed an older pull request (#9) that had some discussion related to this.

See also: #9 (comment) . If n-acd runs as root in a network namespace where the current unprivileged user is mapped as root, eBPF doesn't work unless /proc/sys/kernel/unprivileged_bpf_disabled is changed to 0. It would be better to automatically fall back to non-BPF operations in such cases.

[bengal]

After removing the option, README.md must be updated.

[bengal]

This probably should be changed as suggested in #9 (comment)

[jvaclav-rh]

I am not sure I understand the original comment -- it talks about that if we manage to successfully create the BPF map, all of the IPs would be instantly added. But if we move this ... != 1 condition up, and return early, wouldn't that mean the IP is silently dropped, not added to the RBtree and thus not included in the later created bpf map? Would that be the desired behavior?

[bengal]

I think the suggestion was more to remove this check and let n_acd_bpf_map_add() return success when the map is negative. In this way, we keep tracking the probe->acd->n_bpf_map value correctly; then, if the map can be created later, it will be re-initialized with the right size in n_acd_ensure_bpf_map_space() . @dvdhrm ?

[dvdhrm]

Yes

[jvaclav-rh]

@dvdhrm I am going to try and continue working on this, but before doing anything I wanted to ask which approach makes more sense to you. If we support switching to eBPF at runtime, we have two options:

• Once we start using eBPF, we assume it's always going to be available. If it's not used, we can start using it later. If we encounter an error with eBPF after it's already set up, ACD fails.
• We fully support "silently" switching between BPF<->eBPF, if setting up eBPF fails we fall back to BPF. This would be more complex, but more robust.

What do you think is better/makes more sense?

[dvdhrm]

I would prefer to create the map once, and only ever re-create it if it already exists. Hence, we never end up using ebpf later on, if it failed initially.

However, I still think it is wise to keep any counters up to date, even if we do not end up using them. It is much less confusing to see correct counters in debug-dumps, than random values. And it makes it a lot less likely to trip if we use those counters for other purposes later on.

[jvaclav-rh]

Hi, I incorporated the comments left here into the code. It should now behave appropriately; it seems to work quite nicely with this demo program:

#include "n-acd.h"
#include <poll.h>
#include <assert.h>
#include <stdio.h>
#include <netinet/if_ether.h>

int main(int argc, char **argv) {
    NAcdProbeConfig *probe_config;
    NAcdConfig *config;
    NAcdProbe *probe;
    NAcd *acd;
    int r;
    
    r = n_acd_config_new(&config);
    assert(!r);

    n_acd_config_set_ifindex(config, 1);
    n_acd_config_set_transport(config, N_ACD_TRANSPORT_ETHERNET);
    n_acd_config_set_mac(config, (uint8_t[]){ 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe }, ETH_ALEN);

    r = n_acd_new(&acd, config);
    assert(!r);

    r = n_acd_probe_config_new(&probe_config);
    assert(!r);

    n_acd_probe_config_set_ip(probe_config, (struct in_addr){ htobe32((192 << 24) | (168 << 16) | (1 << 0)) });
    n_acd_probe_config_set_timeout(probe_config, 100);

    r = n_acd_probe(acd, &probe, probe_config);
    assert(!r);

    printf("using ebpf: %s\n", n_acd_has_bpf(acd) ? "yes" : "no");

    n_acd_probe_config_free(probe_config);
    n_acd_probe_free(probe);
    n_acd_unref(acd);
    return 0;
}

$ cd build
$ gcc demo.c -I../src -L./src -lnacd -o demo
$ sudo sysctl -w kernel.unprivileged_bpf_disabled=1
kernel.unprivileged_bpf_disabled = 1

$ sudo LD_LIBRARY_PATH=src ./demo
using ebpf: yes

$ podman run --privileged --rm -w /build -v $PWD:/build:z -e LD_LIBRARY_PATH=src fedora:42 ./demo
using ebpf: no

[dvdhrm]

Looks good! Thanks a lot! A minor comment inline for @bengal to decide. Otherwise, good to go!