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 <[email protected]> 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/[email protected]?part=2
