Re: [PATCH bpf-next 5/7] selftests/bpf: Add netdevsim helper library

On 26/08/31 12:07PM, bot+bpf-ci@kernel.org wrote:

quoted

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index b481b867f372e..c21f89dbe70c0 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile

@@ -862,6 +862,7 @@ TRUNNER_EXTRA_SOURCES := test_progs.c		\
 			 unpriv_helpers.c 	\
 			 sysctl_helpers.c	\
 			 netlink_helpers.c	\
+			 netdevsim_helpers.c	\
 			 jit_disasm_helpers.c	\
 			 io_helpers.c		\
 			 test_loader.c		\

diff --git a/tools/testing/selftests/bpf/netdevsim_helpers.c b/tools/testing/selftests/bpf/netdevsim_helpers.c
new file mode 100644
index 0000000000000..fbed7442b2e8d
--- /dev/null
+++ b/tools/testing/selftests/bpf/netdevsim_helpers.c

@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/rtnetlink.h>
+#include <poll.h>
+#include <stdarg.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/socket.h>
+#include <time.h>
+#include <unistd.h>
+
+#include "netdevsim_helpers.h"
+
+static int echo(const char *path, const char *fmt, ...)
+{
+	char buf[64];
+	va_list ap;
+	int fd, len, err = 0;
+
+	va_start(ap, fmt);
+	len = vsnprintf(buf, sizeof(buf), fmt, ap);
+	va_end(ap);
+
+	fd = open(path, O_WRONLY);
+	if (fd < 0)
+		return -errno;
+	if (write(fd, buf, len) != len)
+		err = -errno;
+	close(fd);
+
+	return err;
+}

A subsystem pattern flags this as potentially concerning: the guide
directs reporting when a test hand-rolls sysfs/file I/O instead of using
existing utilities that the subsystem already provides.

Looking at echo(), it implements the open(O_WRONLY)/write()/close()
sequence that write_sysctl() in test_progs.c already performs for BPF
selftests. Could this reuse that existing helper?

Thanks robot, all comments here and below make sense.
I will simplify this in v2.

quoted

+
+void netdevsim_destroy(unsigned int id)
+{
+	echo("/sys/bus/netdevsim/del_device", "%u", id);
+}
+
+static int create_new_device(void)
+{
+	unsigned int id;
+	int err;
+
+	/* if 10K is not enough, then something is clearly not right */
+	for (id = 0; id < 10000; id++) {
+		err = echo("/sys/bus/netdevsim/new_device", "%u", id);
+		if (!err)
+			return id;
+		if (err != -ENOSPC)
+			return err;
+	}
+
+	return -ENOSPC;
+}
+
+static int open_link_socket(void)
+{
+	struct sockaddr_nl addr = {
+		.nl_family = AF_NETLINK,
+		.nl_groups = RTMGRP_LINK,
+	};
+	int fd;
+
+	fd = socket(AF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
+	if (fd < 0)
+		return -errno;
+	if (bind(fd, (struct sockaddr *)&addr, sizeof(addr))) {
+		int err = -errno;
+
+		close(fd);
+		return err;
+	}
+
+	return fd;
+}

The subsystem guide also flags this pattern: open_link_socket() appears
to duplicate rtnl_open(&rth, RTMGRP_LINK) from netlink_helpers.c, which
does the same socket(AF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE)
plus bind() with nl_groups set to the subscriptions parameter.

In particular, it looks structurally similar to genl_open() in
netlink_helpers.c, which the immediately preceding commit in this series
(a1205972510b7 "selftests/bpf: Extract some helpers from tests to the
netlink library") added explicitly for sharing netlink plumbing.

The re-implementation also appears to omit the SO_RCVBUF = 1024 * 1024
setting that rtnl_open_byproto() sets in netlink_helpers.c, so the new
socket runs on the default net.core.rmem_default receive buffer.

Similarly, the receive/poll loop in recv_device_ifindex() appears to
hand-roll what rtnl_recvmsg()/__rtnl_recvmsg() in netlink_helpers.c
already do. Notably, those use MSG_PEEK | MSG_TRUNC to size the receive
buffer, whereas the new loop uses a fixed 16 KiB buffer with a plain
recv(fd, buf, sizeof(buf), 0) and no MSG_TRUNC check. Additionally,
__rtnl_recvmsg() retries on EINTR/EAGAIN where the implementation below
returns -errno.

The agent notes that functionally these hand-rolled versions are correct
for their specific use case, and this is selftest-only code. However,
could the new helpers reuse the existing netlink infrastructure?

quoted

+
+static int remaining_timeout_ms(const struct timespec *deadline)
+{
+	struct timespec now;
+	long long remaining;
+
+	if (clock_gettime(CLOCK_MONOTONIC, &now))
+		return -errno;
+
+	remaining = (deadline->tv_sec - now.tv_sec) * 1000 +
+		    (deadline->tv_nsec - now.tv_nsec) / 1000000;
+
+	return remaining > 0 ? remaining : 0;
+}

This isn't a bug, but since clock_gettime(CLOCK_MONOTONIC, ...) on a
valid stack timespec can't really fail, would it be simpler to have
remaining_timeout_ms() just return milliseconds? That way the timeout
check at the end of recv_device_ifindex() (which currently needs
`return timeout < 0 ? timeout : -ETIMEDOUT;` to forward the
clock_gettime error) could be a plain `return -ETIMEDOUT;`.

quoted

+
+static int recv_device_ifindex(int fd, unsigned int id, unsigned int *ifindex)
+{
+	char parent_name[32], buf[16 * 1024];
+	struct pollfd pfd = {
+		.fd = fd,
+		.events = POLLIN,
+	};
+	struct timespec deadline;
+	struct nlmsghdr *nlh;
+	int len, ret, timeout;
+
+	snprintf(parent_name, sizeof(parent_name), "netdevsim%u", id);
+	if (clock_gettime(CLOCK_MONOTONIC, &deadline))
+		return -errno;
+	deadline.tv_sec += 5;
+
+	for (timeout = remaining_timeout_ms(&deadline); timeout > 0;
+	     timeout = remaining_timeout_ms(&deadline)) {
+		ret = poll(&pfd, 1, timeout);
+		if (ret < 0) {
+			if (errno == EINTR)
+				continue;
+			return -errno;
+		}
+		if (!ret)
+			return -ETIMEDOUT;
+		if (!(pfd.revents & POLLIN))
+			return -EIO;
+
+		len = recv(fd, buf, sizeof(buf), 0);
+		if (len < 0)
+			return -errno;
+
+		for (nlh = (struct nlmsghdr *)buf; NLMSG_OK(nlh, len);
+		     nlh = NLMSG_NEXT(nlh, len)) {
+			struct ifinfomsg *ifm;
+			struct rtattr *attr;
+			int attr_len;
+
+			if (nlh->nlmsg_type != RTM_NEWLINK)
+				continue;
+
+			ifm = NLMSG_DATA(nlh);
+			attr = IFLA_RTA(ifm);
+			attr_len = IFLA_PAYLOAD(nlh);
+			for (; RTA_OK(attr, attr_len);
+			     attr = RTA_NEXT(attr, attr_len)) {
+				if (attr->rta_type != IFLA_PARENT_DEV_NAME)
+					continue;
+				if (strcmp(RTA_DATA(attr), parent_name))
+					continue;
+
+				*ifindex = ifm->ifi_index;
+				return 0;
+			}
+		}
+	}
+
+	return timeout < 0 ? timeout : -ETIMEDOUT;
+}
+
+int netdevsim_create(unsigned int *ifindex)
+{
+	int fd, id, err;
+
+	fd = open_link_socket();
+	if (fd < 0)
+		return fd;
+
+	id = create_new_device();
+	if (id < 0) {
+		close(fd);
+		return id;
+	}
+
+	err = recv_device_ifindex(fd, id, ifindex);
+	close(fd);
+	if (err) {
+		netdevsim_destroy(id);
+		return err;
+	}
+
+	return id;
+}

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33386073074