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

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 26/08/31 12:07PM, bot+bpf-ci@kernel.org wrote:
> > 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.

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