Re: [PATCH net v4 2/2] selftests: net: add a vhost-net test for the tun NAPI queue bound

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests: net: native-endian vring access in tv_post() and tv_reap()
- [Medium] selftests: net: global netdev_max_backlog leaked on setup fail or teardown
- [Low] selftests: net: missing tun_vhost entry in selftests/net/.gitignore
--

commit 428d9005b1a3d09435bb7a0e0e6230535e135403
Author: Antonio Quartulli <antonio@mandelbit.com>

selftests: net: add a vhost-net test for the tun NAPI queue bound

This commit adds a tun_vhost selftest that sends packets from vhost-net into a
tap device with IFF_NAPI and netdev_max_backlog set to 8 across batch, copy,
and zerocopy variants to verify that excess packets are dropped, the queue
continues draining, and all ring buffers are returned.

> diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> index 9180b9bec5155..babb684d6f77a 100644
> --- a/tools/testing/selftests/net/Makefile
> +++ b/tools/testing/selftests/net/Makefile
> @@ -202,6 +202,7 @@ TEST_GEN_PROGS := \
>  	tap \
>  	tcp_port_share \
>  	tls \
> +	tun_vhost \
>  	udp_splice_checksum \
>  # end of TEST_GEN_PROGS

[Severity: Low]
This isn't a bug, but since tun_vhost is added to TEST_GEN_PROGS in
tools/testing/selftests/net/Makefile, should tun_vhost also be added to
tools/testing/selftests/net/.gitignore like the other generated test binaries
in this directory?

> diff --git a/tools/testing/selftests/net/tun_vhost.c b/tools/testing/selftests/net/tun_vhost.c
> new file mode 100644
> index 0000000000000..dbf76d324a7e2
> --- /dev/null
> +++ b/tools/testing/selftests/net/tun_vhost.c
[ ... ]
> +static void tv_vhost_setup(struct __test_metadata *_metadata,
> +			   FIXTURE_DATA(tun_vhost) *self)
> +{
> +	struct vhost_vring_state state = { .index = TV_TX_VQ };
> +	struct vhost_vring_file file = { .index = TV_TX_VQ };
> +	struct vhost_vring_addr addr = { .index = TV_TX_VQ };
> +	size_t memsz = sizeof(struct vhost_memory) +
> +		       sizeof(struct vhost_memory_region);
> +	unsigned long long features;
> +	struct vhost_memory *mem;
> +
> +	ASSERT_EQ(ioctl(self->vhost, VHOST_SET_OWNER, NULL), 0);
> +	ASSERT_EQ(ioctl(self->vhost, VHOST_GET_FEATURES, &features), 0);
> +	ASSERT_TRUE(features & (1ULL << VIRTIO_F_VERSION_1));
> +	features = 1ULL << VIRTIO_F_VERSION_1;
> +	ASSERT_EQ(ioctl(self->vhost, VHOST_SET_FEATURES, &features), 0);
[ ... ]
> +FIXTURE_SETUP(tun_vhost)
> +{
> +	struct ifreq ifr = {};
> +	int hdrlen = TV_HDR_LEN;
> +	int sndbuf = 1 << 20;
> +	ssize_t n;
> +	int i, fd;
> +
> +	self->tap = -1;
> +	self->vhost = open("/dev/vhost-net", O_RDWR);
> +	if (self->vhost < 0)
> +		SKIP(return, "no /dev/vhost-net");
> +	if (variant->zerocopy && !tv_zerocopy_enabled())
> +		SKIP(return, "vhost_net experimental_zcopytx not enabled");
> +	if (variant->batch && tv_zerocopy_enabled())
> +		SKIP(return, "vhost_net does not batch with zerocopy");
> +
> +	/* global, and only visible from init_net: keep it open to restore */
> +	self->backlog_fd = open("/proc/sys/net/core/netdev_max_backlog",
> +				O_RDWR);
> +	ASSERT_GE(self->backlog_fd, 0);
> +	n = read(self->backlog_fd, self->backlog, sizeof(self->backlog) - 1);
> +	ASSERT_GT(n, 0);
> +	self->backlog[n] = '\0';
> +	ASSERT_EQ(pwrite(self->backlog_fd, TV_BACKLOG, strlen(TV_BACKLOG), 0),
> +		  strlen(TV_BACKLOG));
> +
> +	ASSERT_EQ(unshare(CLONE_NEWNET), 0);
> +
> +	self->tap = open("/dev/net/tun", O_RDWR);
> +	ASSERT_GE(self->tap, 0);
[ ... ]
> +	tv_vhost_setup(_metadata, self);
> +}
> +
> +FIXTURE_TEARDOWN(tun_vhost)
> +{
> +	if (self->tap < 0)
> +		return;
> +	/* waits for outstanding zerocopy buffers */
> +	close(self->vhost);
> +	close(self->tap);
> +	if (pwrite(self->backlog_fd, self->backlog, strlen(self->backlog),
> +		   0) < 0)
> +		TH_LOG("cannot restore netdev_max_backlog to %s",
> +		       self->backlog);
> +	close(self->backlog_fd);
> +}

[Severity: Medium]
Can this leave the host-global /proc/sys/net/core/netdev_max_backlog sysctl
clobbered at 8 if setup fails or if teardown hangs waiting for zerocopy
buffers?

In FIXTURE_SETUP(tun_vhost), TV_BACKLOG is written to self->backlog_fd before
unshare(CLONE_NEWNET), open("/dev/net/tun"), TUNSETIFF, socket setup, and
tv_vhost_setup(). In tools/testing/selftests/kselftest_harness.h
(__TEST_F_IMPL), _metadata->no_teardown remains true until FIXTURE_SETUP
returns, so if any subsequent ASSERT_* fails during setup, teardown is skipped
entirely (and FIXTURE_TEARDOWN also returns early when self->tap < 0).

In addition, in FIXTURE_TEARDOWN(tun_vhost), close(self->vhost) is called
before restoring self->backlog_fd. If the zerocopy variant fails because a
dropped zerocopy packet never completes its ubuf_info, close(self->vhost) ->
vhost_net_release() -> vhost_net_flush() -> vhost_net_ubuf_put_and_wait() will
block in uninterruptible sleep until the test harness kills the process with
SIGKILL, preventing the pwrite() restore from running.

Should netdev_max_backlog be written only at the end of FIXTURE_SETUP(tun_vhost)
after all fallible setup steps succeed, and restored in
FIXTURE_TEARDOWN(tun_vhost) before calling close(self->vhost)?

> +/* queue up to @n buffers, return how many */
> +static int tv_post(FIXTURE_DATA(tun_vhost) *self, int pktlen, int n)
> +{
> +	int inflight = (unsigned short)(self->avail_idx - self->used_idx);
> +	unsigned long long one = 1;
> +	int i;
> +
> +	for (i = 0; i < n && inflight < TV_RING_NUM; i++, inflight++) {
> +		unsigned int id = self->avail_idx % TV_RING_NUM;
> +
> +		self->vr.desc[id].addr = (unsigned long)self->bufs +
> +					 id * TV_BUF_SIZE;
> +		self->vr.desc[id].len = TV_HDR_LEN + pktlen;
> +		self->vr.desc[id].flags = 0;
> +		self->vr.avail->ring[id] = id;
> +		self->avail_idx++;
> +	}
> +	__atomic_store_n(&self->vr.avail->idx, self->avail_idx,
> +			 __ATOMIC_RELEASE);
> +	if (i && write(self->kick, &one, sizeof(one)) != sizeof(one))
> +		return -1;
> +	return i;
> +}
> +
> +/* reap returned buffers, waiting up to @ms for at least one */
> +static int tv_reap(FIXTURE_DATA(tun_vhost) *self, int ms)
> +{
> +	struct pollfd pfd = { .fd = self->call, .events = POLLIN };
> +	unsigned short used;
> +	unsigned long long v;
> +
> +	for (;;) {
> +		used = __atomic_load_n(&self->vr.used->idx, __ATOMIC_ACQUIRE);
> +		if (used != self->used_idx) {
> +			int n = (unsigned short)(used - self->used_idx);
> +
> +			self->used_idx = used;
> +			return n;
> +		}

[Severity: Medium]
Will tv_post() and tv_reap() fail on big-endian architectures such as s390x?

In tv_vhost_setup(), the test negotiates VIRTIO_F_VERSION_1 via
ioctl(self->vhost, VHOST_SET_FEATURES, &features), which causes vhost to
interpret all vring fields as little-endian (vq->is_le = true in
vhost_init_is_le()).

However, tv_post() stores self->vr.desc[id].addr, self->vr.desc[id].len,
self->vr.avail->ring[id], and self->vr.avail->idx in CPU-native byte order,
and tv_reap() loads and subtracts self->vr.used->idx in CPU-native byte order.
On big-endian hosts, vhost will byte-swap those native values when reading
avail->idx and descriptors, and tv_reap() will misread used->idx.

Should these vring accesses use htole64(), htole32(), htole16(), and le16toh()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103142232.1869185-1-antonio@mandelbit.com?part=2