bpf: unit tests and fixes for prepend_name function

.github/workflows/bpf-unit-tests.yml

@@ -0,0 +1,32 @@
+name: BPF Unit Tests
+on:
+  pull_request:
+    paths:
+      - 'bpf/**'
+  push:
+    branches:
+      - main
+    paths:
+      - 'bpf/**'
+
+jobs:
+  build:
+    runs-on: ${{ matrix.os }}
+    strategy:
+      matrix:
+        os: [ ubuntu-22.04, actuated-arm64-4cpu-8gb ]
+    steps:
+    - name: Checkout code
+      uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
+
+    - name: Install Go
+      uses: actions/setup-go@0c52d547c9bc32b1aa3301fd7a9cb496313a4491 # v5.0.0
+      with:
+        # renovate: datasource=golang-version depName=go
+        go-version: '1.21.5'
+
+    - name: Install LLVM
+      run: sudo apt-get -y install clang llvm
+
+    - name: Run BPF unit test
+      run: make bpf-test BPFGOTESTFLAGS="-v"

Makefile

@@ -158,6 +158,10 @@ tetragon-bpf-container:
 	$(CONTAINER_ENGINE) run -v $(CURDIR):/tetragon:Z -u $$(id -u) -e BPF_TARGET_ARCH=$(BPF_TARGET_ARCH) --name tetragon-clang $(CLANG_IMAGE) $(MAKE) -C /tetragon/bpf -j$(JOBS) $(__BPF_DEBUG_FLAGS)
 	$(CONTAINER_ENGINE) rm tetragon-clang
 
+.PHONY: bpf-test
+bpf-test:
+	$(MAKE) -C ./bpf run-test
+
 .PHONY: verify
 verify: tetragon-bpf
 	sudo contrib/verify/verify.sh bpf/objs

bpf/Makefile

@@ -1,21 +1,7 @@
 .PHONY: all clean
 .SUFFIXES:
 
-SHELL=/bin/bash # needed for the *.{o,ll,i,s} pattern in the clean target
-
-CLANG  ?= clang
-LLC    ?= llc
-
-# Build the BPF programs for the detected architecture, default to x86, and
-# allow easy overriding by using ?= for cross-compilation
-UNAME_M := $(shell uname -m)
-ifeq ($(UNAME_M),x86_64)
-	BPF_TARGET_ARCH ?= x86
-endif
-ifeq ($(UNAME_M),aarch64)
-	BPF_TARGET_ARCH ?= arm64
-endif
-BPF_TARGET_ARCH ?= x86
+include ./Makefile.defs
 
 ALIGNCHECKERDIR = alignchecker/
 PROCESSDIR := process/
@@ -40,25 +26,6 @@ PROCESS = bpf_execve_event.o bpf_execve_event_v53.o bpf_fork.o bpf_exit.o bpf_ge
 CGROUP = bpf_cgroup_mkdir.o bpf_cgroup_rmdir.o bpf_cgroup_release.o
 BPFTEST = bpf_lseek.o bpf_globals.o
 
-IDIR = ./include/
-LIBBPF = ./libbpf/
-LDIR = ./lib
-DEPS = $(patsubst %,$(IDIR)/%,$(_DEPS))
-
-FLAGS := -I. \
-	 -Wall -Werror \
-	 -Wno-address-of-packed-member -Wno-compare-distinct-pointer-types -Wno-unknown-warning-option \
-	 -O2
-
-DEBUG ?= 0
-ifeq ($(DEBUG),1)
-	__DEBUG_FLAGS = -DTETRAGON_BPF_DEBUG
-endif
-
-CLANG_FLAGS += $(FLAGS) -I $(LIBBPF) -I $(IDIR) -I $(LDIR) -target bpf -emit-llvm -g -D__TARGET_ARCH_$(BPF_TARGET_ARCH) -fdebug-default-version=4 $(__DEBUG_FLAGS)
-LLC_FLAGS   := -march=bpf -mcpu=v2 -mattr=dwarfris
-LLC_FLAGS_ALU32 := -march=bpf -mcpu=v3 -mattr=dwarfris
-
 OBJSDIR       := objs/
 DEPSDIR       := deps/
 
@@ -157,11 +124,17 @@ $(DEPSDIR)%.d: $(CGROUPDIR)%.c
 objs/%.o: objs/%.ll
 	$(LLC) $(LLC_FLAGS) -filetype=obj $< -o $@
 
-# include dependencies
-ifneq ($(MAKECMDGOALS),clean)
+# include dependencies, see https://lists.gnu.org/archive/html/make-w32/2004-03/msg00062.html
+ifeq (,$(filter $(MAKECMDGOALS),clean run-test))
 -include $(DEPS)
 endif
 
+# the 'test' target is already taken
+run-test:
+	$(MAKE) -C tests test
+
+SUBDIRS=tests
+
 clean:
 	@$(ECHO_CLEAN)
 	$(QUIET) $(foreach TARGET,$(SUBDIRS), \

bpf/Makefile.defs

@@ -0,0 +1,36 @@
+SHELL=/bin/bash # needed for the *.{o,ll,i,s} pattern in the clean target
+
+CLANG  ?= clang
+LLC    ?= llc
+
+# Build the BPF programs for the detected architecture, default to x86, and
+# allow easy overriding by using ?= for cross-compilation
+UNAME_M := $(shell uname -m)
+ifeq ($(UNAME_M),x86_64)
+	BPF_TARGET_ARCH ?= x86
+endif
+ifeq ($(UNAME_M),aarch64)
+	BPF_TARGET_ARCH ?= arm64
+endif
+BPF_TARGET_ARCH ?= x86
+
+ROOT_DIR := $(dir $(lastword $(MAKEFILE_LIST)))
+
+IDIR = $(ROOT_DIR)include/
+LIBBPF = $(ROOT_DIR)libbpf/
+LDIR = $(ROOT_DIR)lib
+DEPS = $(patsubst %,$(IDIR)/%,$(_DEPS))
+
+FLAGS := -I$(ROOT_DIR) \
+	 -Wall -Werror \
+	 -Wno-address-of-packed-member -Wno-compare-distinct-pointer-types -Wno-unknown-warning-option \
+	 -O2
+
+DEBUG ?= 0
+ifeq ($(DEBUG),1)
+	__DEBUG_FLAGS = -DTETRAGON_BPF_DEBUG
+endif
+
+CLANG_FLAGS += $(FLAGS) -I $(LIBBPF) -I $(IDIR) -I $(LDIR) -target bpf -emit-llvm -g -D__TARGET_ARCH_$(BPF_TARGET_ARCH) -fdebug-default-version=4 $(__DEBUG_FLAGS)
+LLC_FLAGS   := -march=bpf -mcpu=v2 -mattr=dwarfris
+LLC_FLAGS_ALU32 := -march=bpf -mcpu=v3 -mattr=dwarfris

bpf/process/bpf_process_event.h

@@ -11,8 +11,14 @@
 
 #define ENAMETOOLONG 36 /* File name too long */
 
+#define MAX_BUF_LEN 256
+
 struct buffer_heap_map_value {
-	unsigned char buf[PATH_MAP_SIZE];
+	// Buffer is twice the needed size because of the verifier. In prepend_name
+	// unit tests, the verifier figures out that 255 is enough and that the
+	// buffer_offset will not overflow, but in the real use-case it looks like
+	// it's forgetting about that.
+	unsigned char buf[MAX_BUF_LEN * 2];
 };
 
 struct {
@@ -108,42 +114,49 @@ d_unlinked(struct dentry *dentry)
 }
 
 static inline __attribute__((always_inline)) int
-prepend_name(char *bf, char **buffer, int *buflen, const char *name, u32 dlen)
+prepend_name(char *buf, char **bufptr, int *buflen, const char *name, u32 namelen)
 {
-	char slash = '/';
-	u64 buffer_offset = (u64)(*buffer) - (u64)bf;
-
-	// Change dlen (the dentry name length) to fit in the buffer.
-	// We prefer to store the part of it that fits rather that discard it.
-	if (dlen + 1 /* for the slash */ >= *buflen)
-		dlen = *buflen - 1 /* for the slash */ -
-		       1 /* in order to avoid the case to do *buflen == 0 */;
-
-	*buflen -= (dlen + 1);
-	// This will not happen as in the previous if-clause ensures that *buflen will be > 0
-	// Needed to make the verifier happy in older kernels.
-	if (*buflen <= 0)
+	// contains 1 if the buffer is large enough to contain the whole name and a slash prefix
+	bool write_slash = 1;
+
+	u64 buffer_offset = (u64)(*bufptr) - (u64)buf;
+
+	// Change name and namelen to fit in the buffer.
+	// We prefer to store the part of it that fits rather than discard it.
+	if (namelen >= *buflen) {
+		name += namelen - *buflen;
+		namelen = *buflen;
+		write_slash = 0;
+	}
+
+	*buflen -= (namelen + write_slash);
+
+	// This will not happen as buffer_offset cannot be above 256 and namelen is
+	// bound to 255. Needed to make the verifier happy in older kernels.
+	if (namelen + write_slash > buffer_offset)
 		return -ENAMETOOLONG;
 
-	buffer_offset -= (dlen + 1);
+	buffer_offset -= (namelen + write_slash);
 
 	// This will never happen. buffer_offset is the diff of the initial buffer pointer
 	// with the current buffer pointer. This will be at max 256 bytes (similar to the initial
 	// size).
 	// Needed to bound that for probe_read call.
-	if (buffer_offset > PATH_MAP_SIZE - 256)
+	if (buffer_offset >= MAX_BUF_LEN)
 		return -ENAMETOOLONG;
 
-	probe_read(bf + buffer_offset, sizeof(char), &slash);
-	// This ensures that dlen is < 256, which is aligned with kernel's max dentry name length
+	if (write_slash)
+		buf[buffer_offset] = '/';
+
+	// This ensures that namelen is < 256, which is aligned with kernel's max dentry name length
 	// that is 255 (https://elixir.bootlin.com/linux/v5.10/source/include/uapi/linux/limits.h#L12).
 	// Needed to bound that for probe_read call.
-	asm volatile("%[dlen] &= 0xff;\n" ::[dlen] "+r"(dlen)
+	asm volatile("%[namelen] &= 0xff;\n" ::[namelen] "+r"(namelen)
 		     :);
-	probe_read(bf + buffer_offset + 1, dlen * sizeof(char), name);
+	probe_read(buf + buffer_offset + write_slash, namelen * sizeof(char), name);
 
-	*buffer = bf + buffer_offset;
-	return 0;
+	*bufptr = buf + buffer_offset;
+	return write_slash ? 0 : -ENAMETOOLONG;
 }
 
 /*
@@ -357,10 +370,10 @@ d_path_local(const struct path *path, int *buflen, int *error)
 	if (!buffer)
 		return 0;
 
-	*buflen = 256;
+	*buflen = MAX_BUF_LEN;
 	buffer = __d_path_local(path, buffer, buflen, error);
 	if (*buflen > 0)
-		*buflen = 256 - *buflen;
+		*buflen = MAX_BUF_LEN - *buflen;
 
 	return buffer;
 }

bpf/tests/Makefile

@@ -0,0 +1,30 @@
+include ../Makefile.defs
+
+TESTS = prepend_name_test.o
+
+OBJSDIR  := objs/
+OBJS := $(addprefix $(OBJSDIR),$(TESTS))
+SUDO ?= sudo
+
+.PHONY: all
+all: $(OBJS)
+
+.PHONY: test
+test: $(OBJS)
+	go test -exec "$(SUDO)" ./ $(BPFGOTESTFLAGS)
+
+$(OBJS): | $(OBJSDIR)
+
+$(OBJSDIR):
+	mkdir $(OBJSDIR)
+
+objs/%.ll: %.c
+	$(CLANG) $(CLANG_FLAGS) -I ../ -c $< -o $@
+
+objs/%.o: objs/%.ll
+	$(LLC) $(LLC_FLAGS) -filetype=obj $< -o $@
+
+.PHONY: clean
+clean:
+	@$(ECHO_CLEAN)
+	$(QUIET)rm -f $(OBJSDIR)*.{o,ll,i,s}

bpf/tests/prepend_name_test.c

@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+/* Copyright Authors of Cilium */
+
+//go:build ignore
+
+#include "vmlinux.h"
+
+#include "bpf_tracing.h" // bpf_printk
+
+#include "bpf_task.h"
+#include "process/bpf_process_event.h"
+
+char _license[] __attribute__((section("license"), used)) = "Dual BSD/GPL";
+
+#define TEST_MAX_BUF_LEN 256
+#define NAME_MAX	 255
+
+struct test_prepend_name_state_map_value {
+	char buf[TEST_MAX_BUF_LEN];
+	u64 buflen;
+	char dname[NAME_MAX];
+	char pad;
+	u32 dlen;
+	u32 offset;
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, struct test_prepend_name_state_map_value);
+} test_prepend_name_state_map SEC(".maps");
+
+__attribute__((section("raw_tracepoint/test"), used)) int
+test_prepend_name()
+{
+	struct test_prepend_name_state_map_value *ts;
+	int zero = 0;
+
+	ts = map_lookup_elem(&test_prepend_name_state_map, &zero);
+	if (!ts)
+		return 1;
+
+	if (ts->buflen < 0 || ts->buflen > TEST_MAX_BUF_LEN)
+		return 2;
+
+	char *bufptr = ts->buf + ts->buflen;
+
+	ts->dlen &= 255;
+
+	int ret = prepend_name((char *)&ts->buf, &bufptr, (int *)&ts->buflen, ts->dname, ts->dlen);
+
+	ts->offset = bufptr - (char *)&ts->buf;
+
+	return ret;
+}