merge: branch 'jv/import-nacd'

n-acd: detect eBPF capability at runtime

Closes #1049

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/2300

.gitlab-ci/run-test.sh

@@ -155,12 +155,7 @@ test_subtree() {
     do_clean
     pushd ./src/$d
 
-    ARGS=()
-    if [ "$d" = n-acd ]; then
-        ARGS+=('-Debpf=false')
-    fi
-
-    CC="$cc" CFLAGS="-Werror -Wall" meson build "${ARGS[@]}"
+    CC="$cc" CFLAGS="-Werror -Wall" meson build
     ninja -v -C build test
 
     popd

contrib/fedora/rpm/NetworkManager.spec

@@ -154,17 +154,6 @@
 %bcond_with ifcfg_migrate
 %endif
 
-%if 0%{?fedora}
-# Although eBPF would be available on Fedora's kernel, it seems
-# we often get SELinux denials (rh#1651654). But even aside them,
-# bpf(BPF_MAP_CREATE, ...) randomly fails with EPERM. That might
-# be related to `ulimit -l`. Anyway, this is not usable at the
-# moment.
-%global ebpf_enabled "no"
-%else
-%global ebpf_enabled "no"
-%endif
-
 # Fedora 33 enables LTO by default by setting CFLAGS="-flto -ffat-lto-objects".
 # However, we also require "-flto -flto-partition=none", so disable Fedora's
 # default and use our configure option --with-lto instead.
@@ -682,11 +671,6 @@ Preferably use nmcli instead.
 	-Dlibpsl=true \
 %else
 	-Dlibpsl=false \
-%endif
-%if %{ebpf_enabled} != "yes"
-	-Debpf=false \
-%else
-	-Debpf=true \
 %endif
 	-Dsession_tracking=systemd \
 	-Dsuspend_resume=systemd \

contrib/fedora/rpm/configure-for-system.sh

@@ -155,7 +155,6 @@ P_CRYPTO="${CRYPTO-}"
 P_DBUS_SYS_DIR="${DBUS_SYS_DIR-}"
 P_DHCP_DEFAULT="${DHCP_DEFAULT-}"
 P_DNS_RC_MANAGER_DEFAULT="${DNS_RC_MANAGER_DEFAULT-}"
-P_EBPF_ENABLED="${EBPF_ENABLED-no}"
 P_FIREWALLD_ZONE="${FIREWALLD_ZONE-}"
 P_IWD="${IWD-}"
 P_LOGGING_BACKEND_DEFAULT="${LOGGING_BACKEND_DEFAULT-}"
@@ -396,7 +395,6 @@ meson setup\
     -Dmodify_system=true \
     -Dconcheck=true \
     -Dlibpsl="$(bool_true "$P_FEDORA")" \
-    -Debpf="$(bool_true "$P_EBPF_ENABLED")" \
     -Dsession_tracking=systemd \
     -Dsuspend_resume=systemd \
     -Dsystemdsystemunitdir=/usr/lib/systemd/system \

contrib/scripts/nm-ci-run.sh

@@ -180,8 +180,6 @@ meson setup build \
     -D crypto=$_WITH_CRYPTO \
     -D docs=$_WITH_DOCS \
     \
-    -D ebpf=false \
-    \
     -D iwd=true \
     -D ofono=true \
     -D teamdctl=$_WITH_LIBTEAM \

data/NetworkManager.service.in

@@ -19,7 +19,7 @@ KillMode=process
 # With a huge number of interfaces, starting can take a long time.
 TimeoutStartSec=600
 
-CapabilityBoundingSet=CAP_NET_ADMIN CAP_DAC_OVERRIDE CAP_NET_RAW CAP_NET_BIND_SERVICE CAP_SETGID CAP_SETUID CAP_SYS_MODULE CAP_AUDIT_WRITE CAP_KILL CAP_SYS_CHROOT
+CapabilityBoundingSet=CAP_NET_ADMIN CAP_DAC_OVERRIDE CAP_NET_RAW CAP_BPF CAP_NET_BIND_SERVICE CAP_SETGID CAP_SETUID CAP_SYS_MODULE CAP_AUDIT_WRITE CAP_KILL CAP_SYS_CHROOT
 
 ProtectSystem=true
 ProtectHome=read-only

meson.build

@@ -477,19 +477,6 @@ if enable_selinux
 endif
 config_h.set10('HAVE_SELINUX', enable_selinux)
 
-# eBPF support
-ebpf_opt = get_option('ebpf')
-# 'auto' means 'false', because there are still issues.
-if ebpf_opt != 'true'
-  enable_ebpf = false
-else
-  enable_ebpf = true
-  if not cc.has_header('linux/bpf.h')
-    assert(ebpf_opt != 'true', 'eBPF requires kernel support')
-    enable_ebpf = false
-  endif
-endif
-
 # libaudit support
 libaudit = get_option('libaudit')
 enable_libaudit = libaudit.contains('yes')
@@ -1160,6 +1147,5 @@ output +=      'have-nss: ' + crypto_nss_dep.found().to_string() + ')\n'
 output += '  sanitizers: ' + get_option('b_sanitize') + '\n'
 output += '  Mozilla Public Suffix List: ' + enable_libpsl.to_string() + '\n'
 output += '  vapi: ' + enable_vapi.to_string() + '\n'
-output += '  ebpf: ' + enable_ebpf.to_string() + '\n'
 output += '  readline: ' + with_readline + '\n'
 message(output)

meson_options.txt

@@ -44,7 +44,7 @@ option('nmcli', type: 'boolean', value: true, description: 'Build nmcli')
 option('nmtui', type: 'boolean', value: true, description: 'Build nmtui')
 option('nm_cloud_setup', type: 'boolean', value: true, description: 'Build nm-cloud-setup, a tool for automatically configuring networking in cloud')
 option('bluez5_dun', type: 'boolean', value: false, description: 'enable Bluez5 DUN support')
-option('ebpf', type: 'combo', choices: ['auto', 'true', 'false'], description: 'Enable eBPF support')
+option('ebpf', type: 'combo', choices: ['auto', 'true', 'false'], description: 'Enable eBPF support (deprecated)')
 option('nbft', type: 'boolean', value: true, description: 'Enable NBFT support in the initrd generator')
 
 # configuration plugins

src/core/nm-l3cfg.c

@@ -3056,9 +3056,10 @@ handle_start_probing:
         }
 
         _LOGT_acd(acd_data,
-                  "%sstart probing (timeout %u msec, %s)",
+                  "%sstart probing (timeout %u msec, ebpf %s; %s)",
                   orig_state == NM_L3_ACD_ADDR_STATE_INIT ? "" : "re",
                   acd_data->probing_timeout_msec,
+                  n_acd_has_bpf(self->priv.p->nacd) ? "enabled" : "disabled",
                   log_reason);
         return;
     }
@@ -3153,10 +3154,11 @@ handle_start_defending:
         }
 
         _LOGT_acd(acd_data,
-                  "start announcing (defend=%s) (probe created)",
+                  "start announcing (defend=%s) (probe created with ebpf %s)",
                   _l3_acd_defend_type_to_string(acd_data->acd_defend_type_current,
                                                 sbuf256,
-                                                sizeof(sbuf256)));
+                                                sizeof(sbuf256)),
+                  n_acd_has_bpf(self->priv.p->nacd) ? "enabled" : "disabled");
         acd_data->acd_defend_type_is_active = FALSE;
         acd_data->nacd_probe                = probe;
         return;

src/meson.build

@@ -24,7 +24,7 @@ libn_acd = static_library(
     'n-acd/src/n-acd.c',
     'n-acd/src/n-acd-probe.c',
     'n-acd/src/util/timer.c',
-    enable_ebpf ? 'n-acd/src/n-acd-bpf.c' : 'n-acd/src/n-acd-bpf-fallback.c',
+    'n-acd/src/n-acd-bpf.c',
   ),
   include_directories: include_directories(
     'c-list/src',

src/n-acd/.github/workflows/ci.yml

@@ -49,42 +49,6 @@ jobs:
                 "--m32=1" \
                 "--source=/github/workspace"
 
-  ci-no-ebpf:
-    name: CI without eBPF
-    runs-on: ubuntu-latest
-
-    steps:
-    # See above in 'ci' job.
-    - name: Fetch CI
-      uses: actions/checkout@v2
-      with:
-        repository: c-util/automation
-        ref: v1
-        path: automation
-    - name: Build CI
-      working-directory: automation/src/ci-c-util
-      run: docker build --tag ci-c-util:v1 .
-
-    #
-    # Run CI
-    #
-    # This again runs the CI, but this time disables eBPF. We do support the
-    # legacy BPF fallback, so lets make sure we test for it.
-    #
-    - name: Fetch Sources
-      uses: actions/checkout@v2
-      with:
-        path: source
-    - name: Run through C-Util CI
-      run: |
-        docker run \
-                --privileged \
-                -v "$(pwd)/source:/github/workspace" \
-                "ci-c-util:v1" \
-                "--m32=1" \
-                "--mesonargs=-Debpf=false" \
-                "--source=/github/workspace"
-
   ci-valgrind:
     name: CI through Valgrind
     runs-on: ubuntu-latest

src/n-acd/README.md

@@ -41,13 +41,6 @@ meson test
 ninja install
 ```
 
-The following configuration options are available:
-
- * `ebpf`: This boolean controls whether `ebpf` features are used to improve
-           the package filtering performance. If disabled, classic bpf will be
-           used. This feature requires a rather recent kernel (>=3.19).
-           Default is: true
-
 ### Repository:
 
  - **web**:   <https://github.com/nettools/n-acd>

src/n-acd/meson.build

@@ -22,6 +22,4 @@ dep_crbtree = sub_crbtree.get_variable('libcrbtree_dep')
 dep_csiphash = sub_csiphash.get_variable('libcsiphash_dep')
 dep_cstdaux = sub_cstdaux.get_variable('libcstdaux_dep')
 
-use_ebpf = get_option('ebpf')
-
 subdir('src')

src/n-acd/meson_options.txt

@@ -1 +0,0 @@
-option('ebpf', type: 'boolean', value: true, description: 'Enable eBPF packet filtering')

src/n-acd/src/libnacd.sym

@@ -15,6 +15,7 @@ global:
         n_acd_ref;
         n_acd_unref;
         n_acd_get_fd;
+        n_acd_has_bpf;
         n_acd_dispatch;
         n_acd_pop_event;
         n_acd_probe;

src/n-acd/src/meson.build

@@ -15,18 +15,9 @@ libnacd_sources = [
         'n-acd.c',
         'n-acd-probe.c',
         'util/timer.c',
+        'n-acd-bpf.c',
 ]
 
-if use_ebpf
-        libnacd_sources += [
-                'n-acd-bpf.c',
-        ]
-else
-        libnacd_sources += [
-                'n-acd-bpf-fallback.c',
-        ]
-endif
-
 libnacd_private = static_library(
         'nacd-private',
         libnacd_sources,
@@ -77,10 +68,8 @@ endif
 test_api = executable('test-api', ['test-api.c'], link_with: libnacd_shared)
 test('API Symbol Visibility', test_api)
 
-if use_ebpf
-        test_bpf = executable('test-bpf', ['test-bpf.c'], dependencies: libnacd_dep)
-        test('eBPF socket filtering', test_bpf)
-endif
+test_bpf = executable('test-bpf', ['test-bpf.c'], dependencies: libnacd_dep)
+test('eBPF socket filtering', test_bpf)
 
 test_loopback = executable('test-loopback', ['test-loopback.c'], dependencies: libnacd_dep)
 test('Echo Suppression via Loopback', test_loopback)

src/n-acd/src/n-acd-bpf-fallback.c

@@ -1,30 +0,0 @@
-/*
- * A noop implementation of eBPF filter for IPv4 Address Conflict Detection
- *
- * These are a collection of dummy functions that have no effect, but allows
- * n-acd to compile without eBPF support.
- *
- * See n-acd-bpf.c for documentation.
- */
-
-#include <c-stdaux.h>
-#include <stddef.h>
-#include "n-acd-private.h"
-
-int n_acd_bpf_map_create(int *mapfdp, size_t max_entries) {
-        *mapfdp = -1;
-        return 0;
-}
-
-int n_acd_bpf_map_add(int mapfd, struct in_addr *addrp) {
-        return 0;
-}
-
-int n_acd_bpf_map_remove(int mapfd, struct in_addr *addrp) {
-        return 0;
-}
-
-int n_acd_bpf_compile(int *progfdp, int mapfd, struct ether_addr *macp) {
-        *progfdp = -1;
-        return 0;
-}

src/n-acd/src/n-acd-bpf.c

@@ -170,6 +170,10 @@ int n_acd_bpf_map_add(int mapfd, struct in_addr *addrp) {
         uint8_t _dummy = 0;
         int r;
 
+        /* If we don't have a map to update, there is nothing to do. */
+        if (mapfd == -1)
+                return 0;
+
         memset(&attr, 0, sizeof(attr));
         attr = (union bpf_attr){
                 .map_fd = mapfd,
@@ -190,6 +194,10 @@ int n_acd_bpf_map_remove(int mapfd, struct in_addr *addrp) {
         union bpf_attr attr;
         int r;
 
+        /* If we don't have a map to update, there is nothing to do. */
+        if (mapfd == -1)
+                return 0;
+
         memset(&attr, 0, sizeof(attr));
         attr = (union bpf_attr){
                 .map_fd = mapfd,

src/n-acd/src/n-acd.c

@@ -284,6 +284,11 @@ int n_acd_ensure_bpf_map_space(NAcd *acd) {
 
         max_map = 2 * acd->max_bpf_map;
 
+        /* If we didn't succeed in creating a map during n_acd_new(),
+         * let's assume we are unable to create it here, and skip it. */
+        if (!n_acd_has_bpf(acd))
+                goto out;
+
         r = n_acd_bpf_map_create(&fd_map, max_map);
         if (r)
                 return r;
@@ -308,6 +313,8 @@ int n_acd_ensure_bpf_map_space(NAcd *acd) {
                 close(acd->fd_bpf_map);
         acd->fd_bpf_map = fd_map;
         fd_map = -1;
+
+out:
         acd->max_bpf_map = max_map;
         return 0;
 }
@@ -359,13 +366,16 @@ _c_public_ int n_acd_new(NAcd **acdp, NAcdConfig *config) {
 
         acd->max_bpf_map = 8;
 
+        /* Let's try to create a BPF map. If we fail, we want to ignore it
+         * only if we lack permissions to create it, and proceed without eBPF. */
         r = n_acd_bpf_map_create(&acd->fd_bpf_map, acd->max_bpf_map);
-        if (r)
-                return r;
-
-        r = n_acd_bpf_compile(&fd_bpf_prog, acd->fd_bpf_map, (struct ether_addr*) acd->mac);
-        if (r)
+        if (r == 0) {
+                r = n_acd_bpf_compile(&fd_bpf_prog, acd->fd_bpf_map, (struct ether_addr*) acd->mac);
+                if (r)
+                        return r;
+        } else if (r != -EPERM) {
                 return r;
+        }
 
         r = n_acd_socket_new(&acd->fd_socket, fd_bpf_prog, config);
         if (r)
@@ -572,6 +582,20 @@ _c_public_ void n_acd_get_fd(NAcd *acd, int *fdp) {
         *fdp = acd->fd_epoll;
 }
 
+/**
+ * n_acd_has_bpf() - query the usage of eBPF
+ * @acd:                        context object to operate on
+ *
+ * Checks whether the ACD probe is using eBPF or not.
+ *
+ * Return: true if the probe is using eBPF, or
+ *         false if the probe failed to configure eBPF
+ *               (e.g. due to missing capabilities)
+ */
+_c_public_ bool n_acd_has_bpf(NAcd *acd) {
+        return acd->fd_bpf_map != -1;
+}
+
 static int n_acd_handle_timeout(NAcd *acd) {
         NAcdProbe *probe;
         uint64_t now;

src/n-acd/src/n-acd.h

@@ -93,6 +93,7 @@ NAcd *n_acd_ref(NAcd *acd);
 NAcd *n_acd_unref(NAcd *acd);
 
 void n_acd_get_fd(NAcd *acd, int *fdp);
+bool n_acd_has_bpf(NAcd *acd);
 int n_acd_dispatch(NAcd *acd);
 int n_acd_pop_event(NAcd *acd, NAcdEvent **eventp);
 

src/n-acd/src/test-api.c

@@ -56,6 +56,7 @@ static void test_api_functions(void) {
                 (void *)n_acd_ref,
                 (void *)n_acd_unref,
                 (void *)n_acd_get_fd,
+                (void *)n_acd_has_bpf,
                 (void *)n_acd_dispatch,
                 (void *)n_acd_pop_event,
                 (void *)n_acd_probe,

src/n-acd/src/test-bpf.c

@@ -19,6 +19,9 @@
 #include "n-acd-private.h"
 #include "test.h"
 
+// https://mesonbuild.com/Unit-tests.html#skipped-tests-and-hard-errors
+#define RETURN_TEST_SKIPPED 77
+
 #define ETHER_ARP_PACKET_INIT(_op, _mac, _sip, _tip) {                  \
                 .ea_hdr = {                                             \
                         .ar_hrd = htobe16(ARPHRD_ETHER),                \
@@ -43,11 +46,15 @@
                 .arp_tpa[3] =  be32toh((_tip)->s_addr) & 0xff,          \
         }
 
-static void test_map(void) {
+static int test_map(void) {
         int r, mapfd = -1;
         struct in_addr addr = { 1 };
 
         r = n_acd_bpf_map_create(&mapfd, 8);
+        if (r == -EPERM) {
+                return RETURN_TEST_SKIPPED;
+        }
+
         c_assert(r >= 0);
         c_assert(mapfd >= 0);
 
@@ -67,6 +74,7 @@ static void test_map(void) {
         c_assert(r == -ENOENT);
 
         close(mapfd);
+        return 0;
 }
 
 static void verify_success(struct ether_arp *packet, int out_fd, int in_fd) {
@@ -92,7 +100,7 @@ static void verify_failure(struct ether_arp *packet, int out_fd, int in_fd) {
         c_assert(errno == EAGAIN);
 }
 
-static void test_filter(void) {
+static int test_filter(void) {
         uint8_t buf[sizeof(struct ether_arp) + 1] = {};
         struct ether_addr mac1 = { { 0x01, 0x02, 0x03, 0x04, 0x05, 0x06 } };
         struct ether_addr mac2 = { { 0x01, 0x02, 0x03, 0x04, 0x05, 0x07 } };
@@ -103,6 +111,10 @@ static void test_filter(void) {
         int r, mapfd = -1, progfd = -1, pair[2];
 
         r = n_acd_bpf_map_create(&mapfd, 1);
+        if (r == -EPERM) {
+                return RETURN_TEST_SKIPPED;
+        }
+
         c_assert(r >= 0);
 
         r = n_acd_bpf_compile(&progfd, mapfd, &mac1);
@@ -214,13 +226,21 @@ static void test_filter(void) {
         close(pair[1]);
         close(progfd);
         close(mapfd);
-}
-
-int main(int argc, char **argv) {
-        test_setup();
-
-        test_map();
-        test_filter();
+
+        return 0;
+}
+
+int main(int argc, char **argv) {
+        int r;
+        test_setup();
+
+        if ((r = test_map())) {
+                return r;
+        }
+
+        if ((r = test_filter())) {
+                return r;
+        }
 
         return 0;
 }

src/n-dhcp4/src/n-dhcp4-c-connection.c

@@ -379,6 +379,10 @@ void n_dhcp4_c_connection_get_timeout(NDhcp4CConnection *connection,
         *timeoutp = timeout;
 }
 
+void n_dhcp4_c_connection_clear_client_ip(NDhcp4CConnection *connection) {
+        connection->client_ip = INADDR_ANY;
+}
+
 static int n_dhcp4_c_connection_packet_broadcast(NDhcp4CConnection *connection,
                                                  NDhcp4Outgoing *message) {
         int r;

src/n-dhcp4/src/n-dhcp4-c-probe.c

@@ -198,7 +198,7 @@ _c_public_ void n_dhcp4_client_probe_config_set_init_reboot(NDhcp4ClientProbeCon
  *
  * This sets the DSCP property of the configuration object, which specifies
  * the DSCP value to set in the first six bits of the DS field in the IPv4
- * header. If this function is not called, the DSCP will be set to CS0.
+ * header. If this function is not called, the DSCP will be set to CS0 (0).
  */
 _c_public_ void n_dhcp4_client_probe_config_set_dscp(NDhcp4ClientProbeConfig *config, uint8_t dscp) {
         config->dscp = dscp & 0x3F;
@@ -703,6 +703,8 @@ static int n_dhcp4_client_probe_transition_deferred(NDhcp4ClientProbe *probe, ui
 
         switch (probe->state) {
         case N_DHCP4_CLIENT_PROBE_STATE_INIT:
+                /* reset client IP (CIADDR) */
+                n_dhcp4_c_connection_clear_client_ip(&probe->connection);
                 r = n_dhcp4_c_connection_listen(&probe->connection);
                 if (r)
                         return r;

src/n-dhcp4/src/n-dhcp4-private.h

@@ -617,6 +617,8 @@ void n_dhcp4_c_connection_close(NDhcp4CConnection *connection);
 void n_dhcp4_c_connection_get_timeout(NDhcp4CConnection *connection,
                                       uint64_t *timeoutp);
 
+void n_dhcp4_c_connection_clear_client_ip(NDhcp4CConnection *connection);
+
 int n_dhcp4_c_connection_discover_new(NDhcp4CConnection *connection,
                                       NDhcp4Outgoing **request);
 int n_dhcp4_c_connection_select_new(NDhcp4CConnection *connection,