Re: [PATCH bpf-next 5/7] selftests/bpf: Add netdevsim helper library
From: bot+bpf-ci@kernel.org
Date: 2026-08-31 12:07:10
Also in:
bpf, netdev
quoted hunk ↗ jump to hunk
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?
+
+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?
+
+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;`.
+
+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