Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 3 · Low: 5
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] FIXTURE_SETUP(tun_vhost) sets the global sysctl
net.core.netdev_max_backlog to 8 early on: it writes TV_BACKLOG with…
- [Medium] The commit message says the zerocopy variant checks that every
buffer comes back to the ring, "which it does not if a dropped…
- [Medium] tv_vhost_setup() negotiates only VIRTIO_F_VERSION_1.
- [Low] The file header comment in tun_vhost.c says "Tun schedules NAPI
only after a batch, or after the last of a series of MSG_MORE packets".
- [Low] tv_post() picks the next descriptor as id = avail_idx %
TV_RING_NUM and treats it as free whenever avail_idx - used_idx <…
- [Low] tv_zerocopy_enabled() reads the first byte of
/sys/module/vhost_net/parameters/experimental_zcopytx and returns `c…
- [Low] The test sets the system-wide net.core.netdev_max_backlog
(net_hotdata.max_backlog, read by tun_napi_queue_full() at…
- [Low] selftests: net: missing tun_vhost entry in
selftests/net/.gitignore
--
selftests: net: add a vhost-net test for the tun NAPI queue bound
This adds a kselftest that runs its own minimal split virtqueue through
vhost-net into an IFF_NAPI tap device, with netdev_max_backlog set to 8.
It covers the batch, copy and zerocopy transmit paths. It checks that
excess packets are dropped and counted, that the queue keeps draining,
and that every buffer comes back to the ring.
quoted hunk ↗ jump to hunk
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
@@ -0,0 +1,361 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
[ ... ]
quoted hunk ↗ jump to hunk
+ * Tun schedules NAPI only after a batch, or after the last of a series of
+ * MSG_MORE packets, so with a small netdev_max_backlog the queue always
+ * overflows. It must keep draining while the sender keeps sending.
[Severity: Low]
Is this description of tun accurate? tun_get_user() also schedules NAPI
when the queue grows past NAPI_POLL_WEIGHT:
drivers/net/tun.c:tun_get_user() {
...
if (!more || queue_len > NAPI_POLL_WEIGHT)
napi_schedule(&tfile->napi);
...
}
The queue-full drop paths in tun_get_user() and tun_xdp_one() also call
napi_schedule().
TV_BACKLOG (8) is below NAPI_POLL_WEIGHT (64), so the queue_len trigger
never fires in this test. The "still draining" check is really testing
the scheduling done on the drop paths.
Could the comment mention both of these dependencies?
[ ... ]
quoted hunk ↗ jump to hunk
+static bool tv_zerocopy_enabled(void)
+{
+ char c = '0';
+ int fd;
+
+ fd = open("/sys/module/vhost_net/parameters/experimental_zcopytx",
+ O_RDONLY);
+ if (fd < 0)
+ return false;
+ if (read(fd, &c, 1) != 1)
+ c = '0';
+ close(fd);
+ return c == '1';
+}
[Severity: Low]
Does this check match how vhost-net reads the parameter?
experimental_zcopytx is an int module parameter, and vhost-net enables
zerocopy for any nonzero value:
drivers/vhost/net.c:vhost_sock_zcopy() {
return unlikely(experimental_zcopytx) &&
sock_flag(sock->sk, SOCK_ZEROCOPY);
}
vhost_net_init() also only tests if (experimental_zcopytx).
With experimental_zcopytx=2 or -1, the zerocopy variant is skipped. The
batch variant still runs, but handle_tx() goes through
handle_tx_zerocopy(). The batch variant therefore passes without ever
taking the XDP batch path.
quoted hunk ↗ jump to hunk
+
+static void tv_vhost_setup(struct __test_metadata *_metadata,
+ FIXTURE_DATA(tun_vhost) *self)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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);
[Severity: Medium]
Does this work on big-endian hosts? Once VIRTIO_F_VERSION_1 is
negotiated, vhost treats every ring field as little-endian:
drivers/vhost/vhost.c:vhost_init_is_le() {
vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1)
|| virtio_legacy_is_little_endian();
}
tv_post() stores desc[id].addr, desc[id].len, desc[id].flags,
avail->ring[id] and avail->idx in native byte order. tv_reap() reads
used->idx the same way. Neither does any htole64/32/16 or le16toh
conversion.
On s390x or ppc64 BE, vhost would see a byte-swapped avail->idx, and
descriptor addresses that do not translate through the single memory
region. tv_reap() would then time out and report "N of M buffers not
returned", even though tun is behaving correctly.
Should the ring accesses be converted to little-endian, or the test be
skipped on big-endian?
[ ... ]
+FIXTURE_SETUP(tun_vhost)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ /* 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);
[Severity: Low]
This changes the system-wide netdev_max_backlog for the whole run, and
the unshare(CLONE_NEWNET) below does not contain it. tun reads the
global value:
drivers/net/tun.c:tun_napi_queue_full() {
return skb_queue_len_lockless(queue) >
READ_ONCE(net_hotdata.max_backlog);
}
enqueue_to_backlog() reads the same global value.
If other networking tests or workloads run on the same kernel at the
same time, every netdev in every netns gets an 8 packet per-CPU backlog.
Could loopback, netif_rx or RPS traffic in those tests then drop
packets and fail intermittently?
The standard kselftest runner is sequential, so this only affects
parallel or custom runners.
quoted hunk ↗ jump to hunk
+ 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);
[Severity: Medium]
Is netdev_max_backlog restored if a later step in setup fails? Every
ASSERT after this pwrite can abort setup. That includes unshare(), the
tun open and ioctls, the SIOCSIFHWADDR/SIOCGIFFLAGS/SIOCSIFFLAGS
ioctls, both posix_memalign() calls, both eventfd() calls, and every
vhost ioctl in tv_vhost_setup().
In kselftest_harness.h, __TEST_F_IMPL clears no_teardown only after
setup succeeds:
fixture_name##_setup(_metadata, self, variant->data);
/* Let setup failure terminate early. */
if (_metadata->exit_code)
_exit(0);
*_metadata->no_teardown = false;
When an ASSERT fails during setup, __bail() calls teardown_fn(). That
sees no_teardown still set, skips FIXTURE_TEARDOWN, and then calls
abort(). There is no FIXTURE_TEARDOWN_PARENT either. FIXTURE_TEARDOWN
also returns early when self->tap < 0, before the restore.
Would the host then stay at netdev_max_backlog=8? The next variant's
setup would read "8" and save it as the original value, so the real
value would be lost for the rest of the run.
That seems to contradict the commit message:
"and restores netdev_max_backlog, which is global, on exit."
[ ... ]
quoted hunk ↗ jump to hunk
+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]
What happens here in the failure the zerocopy variant is meant to
catch? Per the commit message, a buffer does not come back "if a
dropped zerocopy buffer is never completed". In that case tv_reap()
returns 0 after 5 seconds, ASSERT_GT(n, 0) fails, and this teardown
runs.
close(self->vhost) then waits uninterruptibly with no timeout:
vhost_net_release()
vhost_net_flush()
vhost_net_ubuf_put_and_wait()
wait_event(ubufs->wait, !atomic_read(&ubufs->refcount));
vhost_net_release() drops its tx_sock reference only after that flush:
if (tx_sock)
sockfd_put(tx_sock);
So closing the tap fd cannot let tun purge the queued skbs and release
the ubufs.
Would the process then sit in D state, where the harness timeout's
SIGKILL cannot reap it? The netdev_max_backlog restore after the two
close() calls would never run. The result would be a hung task warning,
a host left at 8, and pinned vhost/tap objects, rather than a clean
FAIL.
quoted hunk ↗ jump to hunk
+
+/* 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;
[Severity: Low]
Does this assume vhost returns buffers in order? The descriptor id
comes from avail_idx alone. tv_reap() only consumes used->idx and never
reads used->ring[].id.
In the zerocopy variant, handle_tx_zerocopy() puts copy-path heads on
the used ring right away. Zerocopy heads wait for their ubuf to
complete:
drivers/vhost/net.c:handle_tx_zerocopy() {
...
if (!zcopy_used)
vhost_add_used_and_signal(&net->dev, vq, head, 0);
else
vhost_zerocopy_signal_used(net, vq);
...
}
This test produces both kinds of head. The tun queue-full drop path
completes ubufs with success=false, which raises tx_zcopy_err and makes
vhost_net_tx_select_zcopy() return false for a while.
vhost_exceeds_maxpend() has the same effect.
The ring is kept full. Could the next tv_post() then rewrite and
re-post the oldest slot while its zerocopy head is still pending? That
would put the same head in flight twice, which the split ring rules do
not allow. The test would also lose track of which buffer was never
returned.
The rewritten descriptor has identical contents, so no data corruption
is expected here.
quoted hunk ↗ jump to hunk
+
+ 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++;
+ }
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -203,6 +203,7 @@ TEST_GEN_PROGS := \
tap \
tcp_port_share \
tls \
+ tun_vhost \
udp_splice_checksum \
# end of TEST_GEN_PROGS
[Severity: Low]
Should tun_vhost also be added to
tools/testing/selftests/net/.gitignore? The neighbouring
TEST_GEN_PROGS entries, such as tap, tcp_port_share, tls and
udp_splice_checksum, all have matching .gitignore entries. This patch
adds tun_vhost to TEST_GEN_PROGS but does not add a .gitignore entry
for it.
As a result, an in-tree build of the net selftests leaves an
untracked tools/testing/selftests/net/tun_vhost binary in the working
tree, and it shows up in git status.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103142232.1869185-1-antonio%40mandelbit.com