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

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 18477 bytes --]

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.

>  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.

> -
> -  # 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.

> 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.

> +    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 on  https://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?

> +
> +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. :)

> +
> +	/* 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?

> +
> +	*ethhdr = eth;
> +
> +	return eth->h_proto; /* network-byte-order */

Why return this when we're already outputting the whole struct ethhdr?

> +}
> +
> +#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?

> +		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.

> 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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]