Re: [DO_NOT_APPLY 1/2] integrate xdp-forwarder into net-vm

Yureka
2025-08-31T20:50:21Z

On 8/30/25 12:59, Alyssa Ross wrote:
> I don't know much about BPF (or networking really, for that matter), so
> I'll focus my comments on the integration into Spectrum. Hopefully
> somebody else is able to help with reviewing the actual functionality.
>
> Yureka Lilian<yureka@cyberchaos.dev> writes:
>
>> Signed-off-by: Yureka Lilian<yureka@cyberchaos.dev>
>> ---
>> vm/sys/net/Makefile | 8 +-
>> vm/sys/net/default.nix | 23 ++--
>> vm/sys/net/etc/fstab | 1 +
>> vm/sys/net/etc/mdev/iface | 23 +---
>> vm/sys/net/etc/nftables.conf | 8 --
>> vm/sys/net/etc/s6-rc/connman/dependencies | 4 -
>> vm/sys/net/etc/s6-rc/connman/type | 1 -
>> vm/sys/net/etc/s6-rc/connman/type.license | 2 -
>> vm/sys/net/etc/s6-rc/nftables/type | 1 -
>> vm/sys/net/etc/s6-rc/nftables/type.license | 2 -
>> vm/sys/net/etc/s6-rc/nftables/up | 6 -
>> vm/sys/net/xdp-forwarder/README.md | 9 ++
>> vm/sys/net/xdp-forwarder/default.nix | 35 ++++++
>> .../xdp-forwarder/include/parsing_helpers.h | 38 +++++++
>> .../xdp-forwarder/include/rewrite_helpers.h | 103 ++++++++++++++++++
>> vm/sys/net/xdp-forwarder/load_physical | 4 +
>> vm/sys/net/xdp-forwarder/load_router | 6 +
>> vm/sys/net/xdp-forwarder/prog_physical.c | 28 +++++
>> vm/sys/net/xdp-forwarder/prog_router.c | 34 ++++++
>> vm/sys/net/xdp-forwarder/set_router_iface.c | 31 ++++++
> Could this be integrated into tools/? I'm hoping we can work towards
> having a common build system for all our compiled code. Currently it
> only has a host/guest distinction, but we probably should change that to
> host/driver/app.
Is moved to tools/ in the next iteration.
>> 20 files changed, 308 insertions(+), 59 deletions(-)
>> delete mode 100644 vm/sys/net/etc/nftables.conf
>> delete mode 100644 vm/sys/net/etc/s6-rc/connman/dependencies
>> delete mode 100644 vm/sys/net/etc/s6-rc/connman/type
>> delete mode 100644 vm/sys/net/etc/s6-rc/connman/type.license
>> delete mode 100644 vm/sys/net/etc/s6-rc/nftables/type
>> delete mode 100644 vm/sys/net/etc/s6-rc/nftables/type.license
>> delete mode 100644 vm/sys/net/etc/s6-rc/nftables/up
>> create mode 100644 vm/sys/net/xdp-forwarder/README.md
>> create mode 100644 vm/sys/net/xdp-forwarder/default.nix
>> create mode 100644 vm/sys/net/xdp-forwarder/include/parsing_helpers.h
>> create mode 100644 vm/sys/net/xdp-forwarder/include/rewrite_helpers.h
>> create mode 100755 vm/sys/net/xdp-forwarder/load_physical
>> create mode 100755 vm/sys/net/xdp-forwarder/load_router
>> create mode 100644 vm/sys/net/xdp-forwarder/prog_physical.c
>> create mode 100644 vm/sys/net/xdp-forwarder/prog_router.c
>> create mode 100644 vm/sys/net/xdp-forwarder/set_router_iface.c
>>
>> diff --git a/vm/sys/net/Makefile b/vm/sys/net/Makefile
>> index e681940..9576a92 100644
>> --- a/vm/sys/net/Makefile
>> +++ b/vm/sys/net/Makefile
>> @@ -34,12 +34,11 @@ VM_FILES = \
>> etc/init \
>> etc/mdev.conf \
>> etc/mdev/iface \
>> - etc/nftables.conf \
>> etc/passwd \
>> etc/s6-linux-init/run-image/service/getty-hvc0/run \
>> etc/s6-linux-init/scripts/rc.init \
>> etc/sysctl.conf
>> -VM_DIRS = dev etc/s6-linux-init/env run proc sys var/lib/connman
>> +VM_DIRS = dev etc/s6-linux-init/env run proc sys
>>
>> # These are separate because they need to be included, but putting
>> # them as make dependencies would confuse make.
>> @@ -59,9 +58,6 @@ build/rootfs.erofs: ../../../scripts/make-erofs.sh $(PACKAGES_FILE) $(VM_FILES)
>> ) | ../../../scripts/make-erofs.sh $@
>>
>> VM_S6_RC_FILES = \
>> - etc/s6-rc/connman/dependencies \
>> - etc/s6-rc/connman/run \
>> - etc/s6-rc/connman/type \
>> etc/s6-rc/dbus/notification-fd \
>> etc/s6-rc/dbus/run \
>> etc/s6-rc/dbus/type \
>> @@ -71,8 +67,6 @@ VM_S6_RC_FILES = \
>> etc/s6-rc/mdevd/notification-fd \
>> etc/s6-rc/mdevd/run \
>> etc/s6-rc/mdevd/type \
>> - etc/s6-rc/nftables/type \
>> - etc/s6-rc/nftables/up \
>> etc/s6-rc/ok-all/contents \
>> etc/s6-rc/ok-all/type \
>> etc/s6-rc/sysctl/type \
>> diff --git a/vm/sys/net/default.nix b/vm/sys/net/default.nix
>> index b5873eb..335c938 100644
>> --- a/vm/sys/net/default.nix
>> +++ b/vm/sys/net/default.nix
>> @@ -1,23 +1,26 @@
>> # SPDX-License-Identifier: MIT
>> # SPDX-FileCopyrightText: 2021-2023 Alyssa Ross<hi@alyssa.is>
>> +# SPDX-FileCopyrightText: 2025 Yureka Lilian<yureka@cyberchaos.dev>
>>
>> -import ../../../lib/call-package.nix ({ lseek, src, terminfo, pkgsStatic }:
>> -pkgsStatic.callPackage (
>> +import ../../../lib/call-package.nix ({ lseek, src, terminfo, pkgsMusl }:
>> +pkgsMusl.callPackage (
>>
>> { lib, stdenvNoCC, nixos, runCommand, writeClosure
>> , erofs-utils, jq, s6-rc, util-linux, xorg
>> -, busybox, connmanMinimal, dbus, execline, kmod, linux_latest, mdevd, nftables
>> -, s6, s6-linux-init
>> +, busybox, dbus, execline, kmod, linux_latest, mdevd
>> +, s6, s6-linux-init, xdp-tools
>> }:
>>
>> let
>> inherit (lib) concatMapStringsSep;
>> inherit (nixosAllHardware.config.hardware) firmware;
>>
>> - connman = connmanMinimal;
>> -
>> packages = [
>> - connman dbus execline kmod mdevd s6 s6-linux-init s6-rc
>> + dbus execline kmod mdevd s6 s6-linux-init s6-rc xdp-tools
>> +
>> + (pkgsMusl.callPackage ./xdp-forwarder {
>> + linux = kernel;
>> + })
>>
>> (busybox.override {
>> extraConfig = ''
>> @@ -30,13 +33,13 @@ let
>> CONFIG_RMMOD n
>> '';
>> })
>> -
>> - (nftables.override { withCli = false; })
>> ];
>>
>> # Packages that should be fully linked into /usr,
>> # (not just their bin/* files).
>> - usrPackages = [ connman dbus firmware kernel terminfo ];
>> + usrPackages = [
>> + dbus firmware kernel terminfo
>> + ];
>>
>> packagesSysroot = runCommand "packages-sysroot" {
>> inherit packages;
>> diff --git a/vm/sys/net/etc/fstab b/vm/sys/net/etc/fstab
>> index 6a82ecc..06f26ff 100644
>> --- a/vm/sys/net/etc/fstab
>> +++ b/vm/sys/net/etc/fstab
>> @@ -4,3 +4,4 @@ proc /proc proc defaults 0 0
>> devpts /dev/pts devpts defaults,gid=4,mode=620 0 0
>> tmpfs /dev/shm tmpfs defaults 0 0
>> sysfs /sys sysfs defaults 0 0
>> +bpffs /sys/fs/bpf bpf defaults 0 0
>> diff --git a/vm/sys/net/etc/mdev/iface b/vm/sys/net/etc/mdev/iface
>> index 2306575..9724690 100755
>> --- a/vm/sys/net/etc/mdev/iface
>> +++ b/vm/sys/net/etc/mdev/iface
>> @@ -1,6 +1,7 @@
>> #!/bin/execlineb -P
>> # SPDX-License-Identifier: EUPL-1.2+
>> # SPDX-FileCopyrightText: 2020-2021 Alyssa Ross<hi@alyssa.is>
>> +# SPDX-FileCopyrightText: 2025 Yureka Lilian<yureka@cyberchaos.dev>
>>
>> importas -Si INTERFACE
>>
>> @@ -8,29 +9,15 @@ ifte
>>
>> {
>> # This interface is connected to another VM.
> This comment should probably be reworded a bit.
Will be in the next iteration
>> -
>> - # The other VM's IP is encoded in the NIC-specific portion of the
>> - # interface's MAC address.
>> - backtick -E CLIENT_IP {
>> - awk -F: "{printf \"100.64.%d.%d\\n\", \"0x\" $5, \"0x\" $6}"
>> - /sys/class/net/${INTERFACE}/address
>> - }
>> -
>> - if { ip address add 169.254.0.1/32 dev $INTERFACE }
>> - if { ip link set $INTERFACE up }
>> - ip route add $CLIENT_IP dev $INTERFACE
>> + if { load_router $INTERFACE }
>> + ip link set $INTERFACE up
>> }
>>
>> {
>> if { test $INTERFACE != lo }
>> # This is a physical connection to a network device.
>> - background { s6-rc -bu change connman }
>> - if { s6-rc -bu change nftables }
>> - if {
>> - forx -pE module { nft_counter nft_masq }
>> - modprobe $module
>> - }
>> - nft add rule ip nat postrouting oifname $INTERFACE counter masquerade
>> + if { load_physical $INTERFACE }
>> + ip link set $INTERFACE up
>> }
>>
>> grep -iq ^02:01: /sys/class/net/${INTERFACE}/address
>> diff --git a/vm/sys/net/xdp-forwarder/README.md b/vm/sys/net/xdp-forwarder/README.md
>> new file mode 100644
>> index 0000000..2aa6585
>> --- /dev/null
>> +++ b/vm/sys/net/xdp-forwarder/README.md
>> @@ -0,0 +1,9 @@
>> +### xdp-forwarder
>> +
>> +The xdp forwarder consists of a redirect_kern.c (which is the actual XDP program, basically just a one-liner) and redirect_user.c (which loads the XDP program and sets up the maps with the interfaces passed on the command line).
>> +
>> +It behaves like an ethernet hub between two interfaces.
>> +
>> +Temporary repository, to be inlined into the [Spectrum](https://spectrum-os.org/) monorepo.
>> +
>> +This work was funded by [NGI Zero](https://www.ngi.eu/ngi-projects/ngi-zero/), an initiative by the Digital Single Market of the European Commission.
> This should be integrated into the Documentation/ we already have. It's
> a bit of a mess in ther at the moment, though. I expect we'll be
> getting some help with that early next year, so would be fine to be
> without documentation until then.
I added a more up-to-date text to the architecture page
>> diff --git a/vm/sys/net/xdp-forwarder/default.nix b/vm/sys/net/xdp-forwarder/default.nix
>> new file mode 100644
>> index 0000000..75b1d66
>> --- /dev/null
>> +++ b/vm/sys/net/xdp-forwarder/default.nix
>> @@ -0,0 +1,35 @@
>> +# SPDX-License-Identifier: MIT
>> +# SPDX-FileCopyrightText: 2025 Yureka Lilian<yureka@cyberchaos.dev>
>> +
>> +{ lib, runCommand, stdenv, llvmPackages, libbpf, linux, bpftools }:
>> +
>> +stdenv.mkDerivation {
>> + pname = "xdp-forwarder";
>> + version = "0";
>> +
>> + src = lib.fileset.toSource {
>> + root = ./.;
>> + fileset = lib.fileset.fileFilter
>> + ({ hasExt, ... }: !(hasExt "nix") && !(hasExt "md")) ./.;
>> + };
>> +
>> + buildInputs = [ libbpf ];
>> + nativeBuildInputs = [ llvmPackages.clang-unwrapped bpftools ];
>> +
>> + buildPhase = ''
>> + bpftool btf dump file ${linux.dev}/vmlinux format c > include/vmlinux.h
> I guess we're still missing a vmlinux.h package in Nixpkgs? That would
> be much cleaner.
Fixed by not relying on vmlinux.h
>
>> + for prog in physical router; do
>> + clang $NIX_CFLAGS_COMPILE -O2 -g -Wall -target bpf -I include -c prog_$prog.c -o prog_$prog.o
>> + substituteInPlace load_$prog --replace-fail prog_$prog.o $out/lib/prog_$prog.o
>> + done
>> + $CC -lbpf -I include -o set_router_iface set_router_iface.c
>> + '';
>> +
>> + installPhase = ''
>> + for prog in physical router; do
>> + install -Dm644 prog_$prog.o $out/lib/prog_$prog.o
>> + install -Dm755 load_$prog $out/bin/load_$prog
>> + done
>> + install -Dm755 set_router_iface $out/bin/set_router_iface
>> + '';
>> +}
>> diff --git a/vm/sys/net/xdp-forwarder/include/parsing_helpers.h b/vm/sys/net/xdp-forwarder/include/parsing_helpers.h
>> new file mode 100644
>> index 0000000..2446e56
>> --- /dev/null
>> +++ b/vm/sys/net/xdp-forwarder/include/parsing_helpers.h
>> @@ -0,0 +1,38 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-clause) */
>> +/* SPDX-FileCopyrightText: 2021 The xdp-tutorial Authors */
>> +/* SPDX-FileCopyrightText: 2025 Yureka Lilian<yureka@cyberchaos.dev> */
>> +/* Based onhttps://github.com/xdp-project/xdp-tutorial/blob/main/common/parsing_helpers.h */
>> +
>> +#ifndef __PARSING_HELPERS_H
>> +#define __PARSING_HELPERS_H
>> +
>> +#include <bpf/bpf_endian.h>
>> +
>> +//#include <linux/if_ether.h>
>> +#define ETH_P_8021Q 0x8100 /* 802.1Q VLAN Extended Header */
>> +#define ETH_P_8021AD 0x88A8 /* 802.1ad Service VLAN */
> Looks like there's still something to do here?
Fixed by using linux/if_ether.h
>> +
>> +static __always_inline int proto_is_vlan(__u16 h_proto)
>> +{
>> + return !!(h_proto == bpf_htons(ETH_P_8021Q) ||
>> + h_proto == bpf_htons(ETH_P_8021AD));
>> +}
>> +
>> +static __always_inline int parse_ethhdr(struct xdp_md *ctx,
>> + struct ethhdr **ethhdr)
>> +{
>> + void *data_end = (void *)(long)ctx->data_end;
>> + struct ethhdr *eth = (void *) (long) ctx->data;
> uintptr_t might be more appropriate than long?
>
> There's some inconsistent formatting here too. I've pushed an
> uncrustify config file that resolves the inconsistencies I noticed
> though, so should be possible to avoid going forward without having to
> think about it. :)
This file has been replaced by the original (untouched, besides
formatting) header from xdp-tutorial. This version doesn't do the cast.
>
>> +
>> + /* Byte-count bounds check; check if current pointer + size of header
>> + * is after data_end.
>> + */
>> + if ((void *) (eth + 1) > data_end)
>> + return -1;
> This is checking that there's more data after the header, right? Is
> that something it's important for us to check?

The intent is to check that the entire eth hdr, which we casted a
pointer to, is within the data (length) of the packet before we
de-reference the pointer. So essentially, skipping packets which do not
have a full ethernet header, instead of reading from addresses which we
are not supposed to read from.

When loading the XDP program, it is tested against an empty or very
small packet, and if it tries to access memory outside of the packet
bounds, it will refuse to load. So the BPF/XDP system ensures that these
kinds of packets are handled properly.

>> +
>> + *ethhdr = eth;
>> +
>> + return eth->h_proto; /* network-byte-order */
> Why return this when we're already outputting the whole struct ethhdr?
No point really, but it's the type signatures is from the xdp-tutorial
code, and I want to stick to that as much as possible.
>> +}
>> +
>> +#endif
>> diff --git a/vm/sys/net/xdp-forwarder/include/rewrite_helpers.h b/vm/sys/net/xdp-forwarder/include/rewrite_helpers.h
>> new file mode 100644
>> index 0000000..7fa6e2c
>> --- /dev/null
>> +++ b/vm/sys/net/xdp-forwarder/include/rewrite_helpers.h
>> @@ -0,0 +1,103 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-clause) */
>> +/* SPDX-FileCopyrightText: 2019 The xdp-tutorial Authors */
>> +/* SPDX-FileCopyrightText: 2025 Yureka Lilian<yureka@cyberchaos.dev> */
>> +/* Based on:https://github.com/xdp-project/xdp-tutorial/blob/main/common/rewrite_helpers.h */
>> +
>> +#ifndef __REWRITE_HELPERS_H
>> +#define __REWRITE_HELPERS_H
>> +
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_endian.h>
>> +
>> +/* Pops the outermost VLAN tag off the packet. Returns the popped VLAN ID on
>> + * success or negative errno on failure.
>> + */
>> +static __always_inline int vlan_tag_pop(struct xdp_md *ctx, struct ethhdr *eth)
>> +{
>> + void *data_end = (void *)(long)ctx->data_end;
>> + struct ethhdr eth_cpy;
>> + struct vlan_hdr *vlh;
>> + __be16 h_proto;
>> + int vlid;
>> +
>> + if (!proto_is_vlan(eth->h_proto))
>> + return -1;
>> +
>> + /* Careful with the parenthesis here */
>> + vlh = (void *)(eth + 1);
>> +
>> + /* Still need to do bounds checking */
>> + if ((void *) (vlh + 1) > data_end)
>> + return -1;
>> +
>> + /* Save vlan ID for returning, h_proto for updating Ethernet header */
>> + vlid = bpf_ntohs(vlh->h_vlan_TCI);
>> + h_proto = vlh->h_vlan_encapsulated_proto;
>> +
>> + /* Make a copy of the outer Ethernet header before we cut it off */
>> + __builtin_memcpy(&eth_cpy, eth, sizeof(eth_cpy));
>> +
>> + /* Actually adjust the head pointer */
>> + if (bpf_xdp_adjust_head(ctx, (int)sizeof(*vlh)))
>> + return -1;
>> +
>> + /* Need to re-evaluate data *and* data_end and do new bounds checking
>> + * after adjusting head
>> + */
>> + eth = (void *)(long)ctx->data;
>> + data_end = (void *)(long)ctx->data_end;
>> + if ((void *) (eth + 1) > data_end)
>> + return -1;
>> +
>> + /* Copy back the old Ethernet header and update the proto type */
>> + __builtin_memcpy(eth, &eth_cpy, sizeof(*eth));
>> + eth->h_proto = h_proto;
>> +
>> + return vlid;
>> +}
>> +
>> +/* Pushes a new VLAN tag after the Ethernet header. Returns 0 on success,
>> + * -1 on failure.
>> + */
>> +static __always_inline int vlan_tag_push(struct xdp_md *ctx,
>> + struct ethhdr *eth, int vlid)
>> +{
>> + void *data_end = (void *)(long)ctx->data_end;
>> + struct ethhdr eth_cpy;
>> + struct vlan_hdr *vlh;
>> +
>> + /* First copy the original Ethernet header */
>> + __builtin_memcpy(&eth_cpy, eth, sizeof(eth_cpy));
>> +
>> + /* Then add space in front of the packet */
>> + if (bpf_xdp_adjust_head(ctx, 0 - (int)sizeof(*vlh)))
>> + return -1;
>> +
>> + /* Need to re-evaluate data_end and data after head adjustment, and
>> + * bounds check, even though we know there is enough space (as we
>> + * increased it).
>> + */
>> + data_end = (void *)(long)ctx->data_end;
>> + eth = (void *)(long)ctx->data;
>> +
>> + if ((void *) (eth + 1) > data_end)
> These bound checks still look really weird to me. Anyway, the last
> doesn't do anything here, right?

Yes, it's likely optimized away, because we already checked that the
vlan hdr (which comes after the eth hdr) is valid.

However, we should always check before de-referencing a pointer that we
don't read from invalid memory.

>> + return -1;
>> +
>> + /* Copy back Ethernet header in the right place, populate VLAN tag with
>> + * ID and proto, and set outer Ethernet header to VLAN type.
>> + */
>> + __builtin_memcpy(eth, &eth_cpy, sizeof(*eth));
>> +
>> + vlh = (void *)(eth + 1);
>> +
>> + if ((void *) (vlh + 1) > data_end)
>> + return -1;
>> +
>> + vlh->h_vlan_TCI = bpf_htons(vlid);
>> + vlh->h_vlan_encapsulated_proto = eth->h_proto;
>> +
>> + eth->h_proto = bpf_htons(ETH_P_8021Q);
>> + return 0;
>> +}
>> +
>> +#endif
>> diff --git a/vm/sys/net/xdp-forwarder/load_physical b/vm/sys/net/xdp-forwarder/load_physical
>> new file mode 100755
>> index 0000000..13473e4
>> --- /dev/null
>> +++ b/vm/sys/net/xdp-forwarder/load_physical
>> @@ -0,0 +1,4 @@
>> +#!/bin/execlineb -S1
>> +# SPDX-License-Identifier: EUPL-1.2+
>> +# SPDX-FileCopyrightText: 2025 Yureka Lilian<yureka@cyberchaos.dev>
>> +xdp-loader load ${1} prog_physical.o -m skb -p /sys/fs/bpf
>> diff --git a/vm/sys/net/xdp-forwarder/load_router b/vm/sys/net/xdp-forwarder/load_router
>> new file mode 100755
>> index 0000000..d16c26d
>> --- /dev/null
>> +++ b/vm/sys/net/xdp-forwarder/load_router
>> @@ -0,0 +1,6 @@
>> +#!/bin/execlineb -S1
>> +# SPDX-License-Identifier: EUPL-1.2+
>> +# SPDX-FileCopyrightText: 2025 Yureka Lilian<yureka@cyberchaos.dev>
>> +if { xdp-loader load ${1} prog_router.o -m skb -p /sys/fs/bpf }
>> +if { ip link set ${1} promisc on }
>> +set_router_iface ${1}
> These scripts are only used in one place, right? I think they're simple
> enough it would be clearer to just inline them.
Done in the next iteration
>> diff --git a/vm/sys/net/xdp-forwarder/set_router_iface.c b/vm/sys/net/xdp-forwarder/set_router_iface.c
>> new file mode 100644
>> index 0000000..c828ee3
>> --- /dev/null
>> +++ b/vm/sys/net/xdp-forwarder/set_router_iface.c
>> @@ -0,0 +1,31 @@
>> +// SPDX-License-Identifier: EUPL-1.2+
>> +// SPDX-FileCopyrightText: 2025 Yureka Lilian<yureka@cyberchaos.dev>
>> +
>> +#include <stdio.h>
>> +#include <net/if.h>
>> +#include <bpf/bpf.h>
>> +
>> +int main(int argc, char **argv) {
>> + if (argc < 2) {
>> + fprintf(stderr, "missing interface name\n");
>> + return 1;
>> + }
>> +
>> + int router_idx = if_nametoindex(argv[1]);
>> + if (router_idx <= 0) {
>> + perror("error getting router interface");
>> + return 1;
>> + }
>> +
>> + int map_fd = bpf_obj_get("/sys/fs/bpf/router_iface");
>> + if (map_fd < 0) {
>> + perror("failed to open bpf map");
>> + return 1;
>> + }
>> +
>> + int id = 0;
>> + if (bpf_map_update_elem(map_fd, &id, &router_idx, 0) < 0) {
>> + perror("failed to update bpf map");
>> + return 1;
>> + }
>> +}
> No existing CLI for trivial map updates like this?
There is a bpftool map update command, but the way we'd need to pass the
interface id as value is very inconvenient to produce in a shell script,
since it takes individual bytes.