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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 4

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] napi_queue_bounded lowers the system-wide
  net.core.netdev_max_backlog to 8 (tun_vhost.c:379).
- [Low] Claim: the zerocopy variant only tests a dropped zerocopy buffer
  by chance, because vhost_net_tx_select_zcopy() turns zerocopy off once…
- [Low] The commit message says that if a dropped zerocopy buffer is
  never completed, 'closing vhost-net then blocks until the buffer is…
- [Low] The header comment in tun_vhost.c (lines 15-19) says that with
  netdev_max_backlog below 64, only the NAPI schedule done on a queue-…
- [Low] Claim: tv_reap() (tun_vhost.c:353-357) trusts used-ring ids and
  used->idx from vhost, so a bad id or an inflated count could overflow…

--

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

This adds a kselftest that sends 16k packets from vhost-net into an
IFF_NAPI tap device with netdev_max_backlog lowered to 8. It covers the
batch, copy and zerocopy paths. It checks that excess packets are dropped
and counted, that the queue keeps draining, and that every buffer comes
back to the ring.

> to the ring. If a dropped zerocopy buffer is never completed, the test
> fails, and closing vhost-net then blocks until the buffer is released.

[Severity: Low]
Is "blocks until the buffer is released" accurate for the case this test
is meant to catch? A buffer that is never completed is also never
released.

FIXTURE_TEARDOWN(tun_vhost) calls close(self->vhost). That goes through
vhost_net_release()->vhost_net_flush()->vhost_net_ubuf_put_and_wait():

	wait_event(ubufs->wait, !atomic_read(&ubufs->refcount));

This is an uninterruptible wait with no timeout.

The harness still records a failure. After 30s, __wait_for_test() sends
SIGKILL to the process group and prints "Test terminated by timeout".

The child stuck in D state still holds its inherited stdout/stderr,
though. So the tap_prefix side of the pipeline in run_one() in
kselftest/runner.sh never sees EOF, and run_kselftest.sh hangs. The stuck
process also pins the vhost_net module and triggers hung task warnings.

Could the commit message say that on an affected kernel this variant
leaves an unkillable process and can hang the runner?

> diff --git a/tools/testing/selftests/net/tun_vhost.c b/tools/testing/selftests/net/tun_vhost.c
> new file mode 100644
> index 0000000000000..ed35435df90eb
> --- /dev/null
> +++ b/tools/testing/selftests/net/tun_vhost.c
> @@ -0,0 +1,409 @@

[ ... ]

> + * Tun schedules NAPI after a batch, after the last of a series of MSG_MORE
> + * packets, once more than NAPI_POLL_WEIGHT (64) packets are queued, and
> + * when it drops a packet because the queue is full. With netdev_max_backlog
> + * below 64, the queue always overflows and only the last of these keeps it
> + * draining while the sender keeps sending.

[Severity: Low]
Is this accurate for the batch variant?

tun_sendmsg() handles the whole TUN_MSG_PTR batch under
local_bh_disable(). It schedules NAPI whenever anything was queued:

	if (tfile->napi_enabled && queued > 0)
		napi_schedule(&tfile->napi);

local_bh_enable() follows, which runs the NET_RX softirq inline and drains
the queue. Each batch therefore starts with an empty queue. About 9 of
every 64 XDP buffers get queued, so queued > 0 is always true.

That means the drop-time napi_schedule() in tun_xdp_one() never runs in
the case it exists for, where nothing in the batch gets queued:

	if (tfile->napi_enabled &&
	    unlikely(tun_napi_queue_full(&tfile->sk.sk_write_queue))) {
		napi_schedule(&tfile->napi);

If that schedule were removed, delivery would stay around 2300 of 16384.
That is well above TV_PKTS / 64.

So can EXPECT_GE(delivered, TV_PKTS / 64) catch a regression of that
schedule in the batch variant?

The copy and zerocopy variants do seem to depend on the drop-time schedule
in tun_get_user(). There the last packet without MSG_MORE is itself
dropped before it can reach the !more napi_schedule().

[ ... ]

> +FIXTURE_VARIANT_ADD(tun_vhost, zerocopy)
> +{
> +	.limit_sndbuf = true,
> +	.zerocopy = true,
> +	.pktlen = 1000,
> +};

[Severity: Low]
Does this variant always reach the dropped zerocopy path?
vhost_net_tx_select_zcopy() turns zerocopy off once errors build up:

	return !net->tx_flush &&
		net->tx_packets / 64 >= net->tx_zcopy_err;

>From the current code, it looks like it does reach that path on every run:

- VHOST_NET_SET_BACKEND resets tx_packets and tx_zcopy_err, and
  vhost_net_tx_packet() resets them again every 1024 packets.
- tv_post() publishes all 256 descriptors at once, so MSG_MORE is set on
  every packet but the last.
- Packet 10 is then dropped in tun_get_user() while tx_zcopy_err is still
  0:

	if (tfile->napi_enabled &&
	    unlikely(tun_napi_queue_full(&tfile->sk.sk_write_queue))) {
		...
			uarg->ops->complete(NULL, uarg, false);

So the coverage claim in the commit message appears to hold, and no change
seems needed here. The test just doesn't assert that a zerocopy drop
actually happened.

[ ... ]

> +/* reap returned buffers, waiting up to @ms for at least one */
> +static int tv_reap(struct tv_ring *r, int ms)
> +{

[ ... ]

> +		while (r->used_idx != used) {
> +			struct vring_used_elem *e;
> +
> +			e = &r->vr.used->ring[r->used_idx % TV_RING_NUM];
> +			r->free[r->nfree++] = le32toh(e->id);

[Severity: Low]
This isn't a bug, but tv_reap() stores the id written by the kernel into
free[] without checking it against TV_RING_NUM. tv_post() later uses that
id to index vr.desc[].

vhost_get_vq_desc() already rejects heads outside the ring:

	if (unlikely(head >= vq->num)) {
		vq_err(vq, "Guest says index %u > %u is available",

Used entries also only come from heads the test made available. So with a
correct kernel this can't overflow free[], and a broken vhost could at
worst crash the test process itself.

[ ... ]

> +TEST_F(tun_vhost, napi_queue_bounded)
> +{
> +	struct tv_stats before, after;
> +	unsigned long long delivered, dropped;
> +	int posted = 0, reaped = 0, n;
> +
> +	ASSERT_EQ(tv_read_stats(TV_IFNAME, &before), 0);
> +	/* lowered only now, so that a failed setup leaves it alone */
> +	ASSERT_EQ(pwrite(self->backlog_fd, TV_BACKLOG, strlen(TV_BACKLOG), 0),
> +		  strlen(TV_BACKLOG));
> +
> +	while (reaped < TV_PKTS) {
> +		n = tv_post(&self->ring, variant->pktlen, TV_PKTS - posted);
> +		ASSERT_GE(n, 0);
> +		posted += n;
> +		n = tv_reap(&self->ring, 5000);

[Severity: Medium]
What restores the global net.core.netdev_max_backlog if the test is killed
before FIXTURE_TEARDOWN(tun_vhost) runs?

That teardown only runs in the forked test child. With a plain
FIXTURE_TEARDOWN, the parent-side teardown_fn(true, ...) call in
__TEST_F_IMPL() does nothing.

On the default 30s harness timeout, __wait_for_test() does:

	kill(-(t->pid), SIGKILL);

That kills the whole process group, wrapper and child, without running any
teardown. SIGINT, SIGSEGV or an OOM kill of the child have the same
effect.

The loop has no overall deadline either. Each tv_reap(..., 5000) returns
as soon as one buffer comes back, and at least 64 rounds are needed. A
slow kernel that keeps returning buffers slowly can therefore run past 30s
without any ASSERT firing.

In that case the host is left with netdev_max_backlog set to 8 until a
reboot or manual reset. This affects later tests in the same run and any
traffic that uses the per-CPU backlog.

FIXTURE_TEARDOWN_PARENT alone would not help, because the wrapper is in
the killed process group too. Would a per-loop deadline well under the
harness timeout, and/or TEST_F_TIMEOUT, make sure the restore path runs?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111014208.1795777-1-antonio%40mandelbit.com