[PATCH net v4 0/2] tun: bound the NAPI receive queue

[PATCH net v4 0/2] tun: bound the NAPI receive queue

[Antonio Quartulli]

With IFF_NAPI, tun queues written packets for NAPI on an unbounded
list, which an unprivileged user can grow until the system runs out of
memory. Patch 1 bounds it by netdev_max_backlog; patch 2 adds a
kselftest for the vhost-net paths into that queue.

v4:
- Keep the poll's leftovers on a separate queue, like process_backlog():
  v3 did not bound the queue when the poll ran on another CPU (Sashiko)
- Schedule NAPI when dropping: with netdev_max_backlog below
  NAPI_POLL_WEIGHT, MSG_MORE senders (vhost-net) stalled the queue
  (Sashiko)
- Use goto free_skb for the locked drop (Willem)
- Add a vhost-net kselftest (patch 2); the tun kselftest passes (Willem)
- Cc vhost, kselftest and XDP maintainers and the Fixes author/acker

v3: https://lore.kernel.org/netdev/20261031231444.1528758-1-antonio@mandelbit.com/
- Also bound the sendmsg() paths, which share the queue: zerocopy
  buffers are completed when dropped early, and the batched XDP buffers
  of tun_xdp_one() are covered too (Willem)
- Drop the IFF_NAPI_FRAGS exclusion (Willem)
- Recheck under the queue lock before queueing (Willem)
- Compare with > like enqueue_to_backlog() (Willem)
- Keep the includes sorted (Willem)
- Add the reproducer and a testing statement to the commit message

v2: https://lore.kernel.org/netdev/20261030152540.1409884-1-antonio@mandelbit.com/
- Cc netdev and the maintainers

Testing, in an x86-64 VM with 1 and 2 vCPUs:
- Reproducer from patch 1, 15 seconds, 500 and 3000 iptables rules:
  memory use stays flat, no OOM; forwarding rate unchanged (112962
  packets in 3 s without the patch, 113822 with it).
- tun_vhost (patch 2), with vhost_net experimental_zcopytx=0 and =1:
  passes with patch 1. Without it, every variant fails (nothing is
  dropped); removing the tun_xdp_one() check fails the batch variant,
  and removing the zerocopy completion fails the zerocopy one.
- tun kselftest: pass:91 fail:0 xfail:12, as without the patch.

Notes:
- Affected since 943170998b20 (v4.15). Based on net/main 94cea342e891.
- Mitigation: disallow unprivileged user namespaces or restrict
  access to /dev/net/tun.

Antonio Quartulli (2):
  tun: bound the NAPI receive queue
  selftests: net: add a vhost-net test for the tun NAPI queue bound

 drivers/net/tun.c                       |  82 +++++-
 tools/testing/selftests/net/Makefile    |   1 +
 tools/testing/selftests/net/config      |   1 +
 tools/testing/selftests/net/tun_vhost.c | 361 ++++++++++++++++++++++++
 4 files changed, 431 insertions(+), 14 deletions(-)
 create mode 100644 tools/testing/selftests/net/tun_vhost.c

-- 
2.56.0

[PATCH net v4 1/2] tun: bound the NAPI receive queue

[Antonio Quartulli]

With IFF_NAPI, tun_get_user() queues every written packet for NAPI on
an unbounded list. When the poll runs asynchronously (threaded NAPI)
and is slower than the writer, the queue grows until the system runs
out of memory. An unprivileged user can do this from its own user and
network namespaces, and the queued skbs are not charged to its memcg:
in a test VM, a uid 1000 process limited to 128 MB caused a global OOM
in ~6 seconds. The socket send buffer cannot bound this: tun sets it to
INT_MAX, and the build_skb() and XDP batch paths do not check it.

Drop packets once more than netdev_max_backlog of them are queued, like
enqueue_to_backlog() does, and count them as rx_dropped: before
allocating and copying them where possible, and again under the queue
lock before queueing. This covers write(), sendmsg() and the batches of
XDP buffers vhost-net passes to tun_sendmsg(), which share the queue.
Zerocopy buffers of packets dropped early are completed.

The NAPI poll moves the whole queue out, processes up to its budget and
puts the rest back: while it runs, the queue looks empty to writers, so
with the poll on another CPU the backlog still grows without limit. Keep
the leftovers on a queue of their own and take the next batch only once
it is empty, like process_backlog() does, so that neither queue holds
more than the limit.

Dropping because the queue is full also schedules NAPI, in all three
places. vhost-net sends with MSG_MORE while it has more packets, and
tun_get_user() schedules NAPI only after the last one or once more than
NAPI_POLL_WEIGHT are queued. With a netdev_max_backlog below that, the
queue would not drain until vhost-net ran out of packets; with zerocopy,
vhost-net never would, as it stops sending when too many queued buffers
are pending.

Reproducer, as an unprivileged user in a new user and network
namespace, without kernel modifications or fault injection:
  ip tuntap add t0 mode tun; ip addr add 10.11.0.1/24 dev t0
  ip link add d0 type dummy; ip addr add 10.20.0.1/24 dev d0
  ip link set t0 up; ip link set d0 up; sysctl net.ipv4.ip_forward=1
  install ~500 iptables FORWARD rules and a MASQUERADE rule on d0
  open t0 with IFF_TUN | IFF_NO_PI | IFF_NAPI
  echo 1 > /sys/class/net/t0/threaded
  write() 1400-byte IPv4 packets for 10.20.0.2 in a loop

Tested in an x86-64 VM with 1 and 2 vCPUs: with the reproducer running
for 15 seconds, memory use stays flat and the forwarding rate is
unchanged; the tun kselftest passes, as it does without the patch; and
the tun_vhost kselftest added by the next patch, which drives the
vhost-net batch, copy and zerocopy paths, passes, while it fails without
the patch.

Still open: the limit counts packets rather than bytes and applies per
queue, up to about twice the limit can be in flight (queued plus taken
by the poll), and the queued skbs are still not charged to the writer's
memcg.

Fixes: 943170998b20 ("tun: enable NAPI for TUN/TAP driver")
Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
---
 drivers/net/tun.c | 82 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 68 insertions(+), 14 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 770b8fe085c3..ea99ee0aee2b 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -58,6 +58,7 @@
 #include <linux/nsproxy.h>
 #include <linux/virtio_net.h>
 #include <linux/rcupdate.h>
+#include <net/hotdata.h>
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
 #include <net/rtnetlink.h>
@@ -140,6 +141,8 @@ struct tun_file {
 		unsigned int ifindex;
 	};
 	struct napi_struct napi;
+	/* NAPI poll only: skbs taken off sk_write_queue, not yet received */
+	struct sk_buff_head napi_process_queue;
 	bool napi_enabled;
 	bool napi_frags_enabled;
 	struct mutex napi_mutex;	/* Protects access to the above napi */
@@ -227,31 +230,40 @@ struct veth {
 static void tun_flow_init(struct tun_struct *tun);
 static void tun_flow_uninit(struct tun_struct *tun);
 
+/* In IFF_NAPI mode written packets wait in the tfile's sk_write_queue for the
+ * NAPI poll, which may run later in a kthread or ksoftirqd. Bound that queue
+ * like the per-CPU backlog of netif_rx().
+ */
+static bool tun_napi_queue_full(const struct sk_buff_head *queue)
+{
+	return skb_queue_len_lockless(queue) >
+	       READ_ONCE(net_hotdata.max_backlog);
+}
+
 static int tun_napi_receive(struct napi_struct *napi, int budget)
 {
 	struct tun_file *tfile = container_of(napi, struct tun_file, napi);
+	struct sk_buff_head *process_queue = &tfile->napi_process_queue;
 	struct sk_buff_head *queue = &tfile->sk.sk_write_queue;
-	struct sk_buff_head process_queue;
 	struct sk_buff *skb;
 	int received = 0;
 
-	__skb_queue_head_init(&process_queue);
-
-	spin_lock(&queue->lock);
-	skb_queue_splice_tail_init(queue, &process_queue);
-	spin_unlock(&queue->lock);
-
-	while (received < budget && (skb = __skb_dequeue(&process_queue))) {
+	while (received < budget) {
+		/* Refill only when empty: leftovers are not put back on the
+		 * queue writers check, so neither queue exceeds the limit.
+		 */
+		if (skb_queue_empty(process_queue)) {
+			spin_lock(&queue->lock);
+			skb_queue_splice_tail_init(queue, process_queue);
+			spin_unlock(&queue->lock);
+			if (skb_queue_empty(process_queue))
+				break;
+		}
+		skb = __skb_dequeue(process_queue);
 		napi_gro_receive(napi, skb);
 		++received;
 	}
 
-	if (!skb_queue_empty(&process_queue)) {
-		spin_lock(&queue->lock);
-		skb_queue_splice(&process_queue, queue);
-		spin_unlock(&queue->lock);
-	}
-
 	return received;
 }
 
@@ -572,6 +584,8 @@ static void tun_queue_purge(struct tun_file *tfile)
 		tun_ptr_free(ptr);
 
 	skb_queue_purge(&tfile->sk.sk_write_queue);
+	/* NAPI is disabled by now */
+	__skb_queue_purge(&tfile->napi_process_queue);
 	skb_queue_purge(&tfile->sk.sk_error_queue);
 }
 
@@ -1856,6 +1870,23 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 			zerocopy = true;
 	}
 
+	/* NAPI queue full: drop before allocating, and schedule NAPI, which a
+	 * MSG_MORE sender may not have done yet. Rechecked under the lock.
+	 */
+	if (tfile->napi_enabled &&
+	    unlikely(tun_napi_queue_full(&tfile->sk.sk_write_queue))) {
+		local_bh_disable();
+		napi_schedule(&tfile->napi);
+		local_bh_enable();
+		if (msg_control) {
+			struct ubuf_info *uarg = msg_control;
+
+			uarg->ops->complete(NULL, uarg, false);
+		}
+		dev_core_stats_rx_dropped_inc(tun->dev);
+		return total_len;
+	}
+
 	if (!frags && tun_can_build_skb(tun, tfile, len, noblock, zerocopy)) {
 		/* For the packet that is not easy to be processed
 		 * (e.g gso or jumbo packet), we will do it at after
@@ -2045,6 +2076,17 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 			goto free_skb;
 		}
 
+		/* NAPI queue full: drop, as above */
+		if (unlikely(tun_napi_queue_full(queue))) {
+			spin_unlock(&queue->lock);
+			napi_schedule(&tfile->napi);
+			local_bh_enable();
+			rcu_read_unlock();
+			dev_core_stats_rx_dropped_inc(tun->dev);
+			drop_reason = SKB_DROP_REASON_FULL_RING;
+			goto free_skb;
+		}
+
 		__skb_queue_tail(queue, skb);
 		queue_len = skb_queue_len(queue);
 		spin_unlock(&queue->lock);
@@ -2649,6 +2691,17 @@ static int tun_xdp_one(struct tun_struct *tun,
 			return -EBUSY;
 		}
 
+		/* NAPI queue full: drop, and schedule NAPI so that it drains
+		 * even if nothing in this batch gets queued
+		 */
+		if (unlikely(tun_napi_queue_full(queue))) {
+			spin_unlock(&queue->lock);
+			napi_schedule(&tfile->napi);
+			kfree_skb_reason(skb, SKB_DROP_REASON_FULL_RING);
+			dev_core_stats_rx_dropped_inc(tun->dev);
+			return 0;
+		}
+
 		__skb_queue_tail(queue, skb);
 		spin_unlock(&queue->lock);
 		ret = 1;
@@ -3571,6 +3624,7 @@ static int tun_chr_open(struct inode *inode, struct file * file)
 	}
 
 	mutex_init(&tfile->napi_mutex);
+	__skb_queue_head_init(&tfile->napi_process_queue);
 	RCU_INIT_POINTER(tfile->tun, NULL);
 	tfile->flags = 0;
 	tfile->ifindex = 0;
-- 
2.56.0

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

[Antonio Quartulli]

Send 16k packets from vhost-net into a tap device with IFF_NAPI and
netdev_max_backlog set to 8, in the three ways vhost-net passes packets
to tun: batches of XDP buffers, one packet per sendmsg() with MSG_MORE,
and zerocopy buffers. Check that excess packets are dropped and counted,
that the queue keeps draining while vhost-net keeps sending, that every
packet is either delivered or dropped, and that every buffer comes back
to the ring, which it does not if a dropped zerocopy buffer is never
completed.

The zerocopy variant needs vhost_net loaded with experimental_zcopytx=1
and is skipped otherwise; with it, vhost-net does not batch and the
batch variant is skipped instead.

The test drives a minimal split virtqueue itself. It needs root and
CONFIG_VHOST_NET, and restores netdev_max_backlog, which is global, on
exit.

Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
---
 tools/testing/selftests/net/Makefile    |   1 +
 tools/testing/selftests/net/config      |   1 +
 tools/testing/selftests/net/tun_vhost.c | 361 ++++++++++++++++++++++++
 3 files changed, 363 insertions(+)
 create mode 100644 tools/testing/selftests/net/tun_vhost.c

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index df215e1a6262..de4cdaac99a0 100644
--- 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
 
diff --git a/tools/testing/selftests/net/config b/tools/testing/selftests/net/config
index cb8cb4696df7..1dce372caf43 100644
--- a/tools/testing/selftests/net/config
+++ b/tools/testing/selftests/net/config
@@ -136,6 +136,7 @@ CONFIG_TRACEPOINTS=y
 CONFIG_TUN=y
 CONFIG_USER_NS=y
 CONFIG_VETH=y
+CONFIG_VHOST_NET=m
 CONFIG_VLAN_8021Q=y
 CONFIG_VXLAN=m
 CONFIG_XFRM_INTERFACE=m
diff --git a/tools/testing/selftests/net/tun_vhost.c b/tools/testing/selftests/net/tun_vhost.c
new file mode 100644
index 000000000000..dbf76d324a7e
--- /dev/null
+++ b/tools/testing/selftests/net/tun_vhost.c
@@ -0,0 +1,361 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Send packets from vhost-net into a tap device with IFF_NAPI and check
+ * that the NAPI queue is bounded by netdev_max_backlog: excess packets
+ * are dropped and counted, and every buffer is returned to the driver,
+ * zerocopy ones included.
+ *
+ * vhost-net passes packets to tun in three ways:
+ *  - batch: batches of XDP buffers (tun_xdp_one()), the default when the
+ *    socket send buffer is unlimited and zerocopy is disabled
+ *  - copy: one packet per sendmsg(), when the send buffer is limited
+ *  - zerocopy: one packet per sendmsg() with a ubuf_info to complete,
+ *    only with vhost_net experimental_zcopytx=1 (else skipped)
+ *
+ * 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.
+ *
+ * Author: Antonio Quartulli <antonio@mandelbit.com>
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <poll.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <linux/if.h>
+#include <linux/if_arp.h>
+#include <linux/if_ether.h>
+#include <linux/if_tun.h>
+#include <linux/vhost.h>
+#include <linux/virtio_config.h>
+#include <linux/virtio_net.h>
+#include <linux/virtio_ring.h>
+#include <sys/eventfd.h>
+#include <sys/ioctl.h>
+#include <sys/socket.h>
+
+#include "kselftest_harness.h"
+
+#define TV_IFNAME	"tvh0"
+#define TV_RING_NUM	256
+#define TV_BUF_SIZE	2048
+#define TV_HDR_LEN	sizeof(struct virtio_net_hdr_v1)
+#define TV_PKTS		(64 * TV_RING_NUM)
+#define TV_BACKLOG	"8"
+#define TV_TX_VQ	1
+
+static const unsigned char tv_mac_tap[ETH_ALEN] = { 2, 0, 0, 0, 0, 1 };
+static const unsigned char tv_mac_dst[ETH_ALEN] = { 2, 0, 0, 0, 0, 2 };
+static const unsigned char tv_mac_src[ETH_ALEN] = { 2, 0, 0, 0, 0, 3 };
+
+struct tv_stats {
+	unsigned long long rx_packets;
+	unsigned long long rx_dropped;
+};
+
+static int tv_read_stats(const char *ifname, struct tv_stats *st)
+{
+	char line[512], *p;
+	FILE *f;
+	int ret = -1;
+
+	f = fopen("/proc/net/dev", "r");
+	if (!f)
+		return -1;
+	while (fgets(line, sizeof(line), f)) {
+		p = strchr(line, ':');
+		if (!p)
+			continue;
+		*p = '\0';
+		if (strcmp(line + strspn(line, " "), ifname))
+			continue;
+		if (sscanf(p + 1, "%*u %llu %*u %llu", &st->rx_packets,
+			   &st->rx_dropped) == 2)
+			ret = 0;
+		break;
+	}
+	fclose(f);
+	return ret;
+}
+
+FIXTURE(tun_vhost)
+{
+	int backlog_fd;
+	char backlog[32];
+	int tap;
+	int vhost;
+	int kick;
+	int call;
+	void *ring;
+	struct vring vr;
+	char *bufs;
+	unsigned short avail_idx;
+	unsigned short used_idx;
+};
+
+FIXTURE_VARIANT(tun_vhost)
+{
+	bool batch;
+	bool limit_sndbuf;
+	bool zerocopy;
+	int pktlen;
+};
+
+FIXTURE_VARIANT_ADD(tun_vhost, batch)
+{
+	.batch = true,
+	.pktlen = 64,
+};
+
+FIXTURE_VARIANT_ADD(tun_vhost, copy)
+{
+	.limit_sndbuf = true,
+	.pktlen = 64,
+};
+
+FIXTURE_VARIANT_ADD(tun_vhost, zerocopy)
+{
+	.limit_sndbuf = true,
+	.zerocopy = true,
+	.pktlen = 1000,
+};
+
+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';
+}
+
+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);
+
+	/* buffer addresses are passed as is: map them 1:1 */
+	mem = calloc(1, memsz);
+	ASSERT_NE(mem, NULL);
+	mem->nregions = 1;
+	mem->regions[0].guest_phys_addr = (unsigned long)self->bufs;
+	mem->regions[0].userspace_addr = (unsigned long)self->bufs;
+	mem->regions[0].memory_size = TV_RING_NUM * TV_BUF_SIZE;
+	ASSERT_EQ(ioctl(self->vhost, VHOST_SET_MEM_TABLE, mem), 0);
+	free(mem);
+
+	state.num = TV_RING_NUM;
+	ASSERT_EQ(ioctl(self->vhost, VHOST_SET_VRING_NUM, &state), 0);
+	state.num = 0;
+	ASSERT_EQ(ioctl(self->vhost, VHOST_SET_VRING_BASE, &state), 0);
+	addr.desc_user_addr = (unsigned long)self->vr.desc;
+	addr.avail_user_addr = (unsigned long)self->vr.avail;
+	addr.used_user_addr = (unsigned long)self->vr.used;
+	ASSERT_EQ(ioctl(self->vhost, VHOST_SET_VRING_ADDR, &addr), 0);
+	file.fd = self->kick;
+	ASSERT_EQ(ioctl(self->vhost, VHOST_SET_VRING_KICK, &file), 0);
+	file.fd = self->call;
+	ASSERT_EQ(ioctl(self->vhost, VHOST_SET_VRING_CALL, &file), 0);
+	file.fd = self->tap;
+	ASSERT_EQ(ioctl(self->vhost, VHOST_NET_SET_BACKEND, &file), 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);
+	ifr.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_VNET_HDR | IFF_NAPI;
+	memcpy(ifr.ifr_name, TV_IFNAME, sizeof(TV_IFNAME));
+	ASSERT_EQ(ioctl(self->tap, TUNSETIFF, &ifr), 0);
+	ASSERT_EQ(ioctl(self->tap, TUNSETVNETHDRSZ, &hdrlen), 0);
+	if (variant->limit_sndbuf)
+		ASSERT_EQ(ioctl(self->tap, TUNSETSNDBUF, &sndbuf), 0);
+
+	fd = socket(AF_INET, SOCK_DGRAM, 0);
+	ASSERT_GE(fd, 0);
+	ifr.ifr_hwaddr.sa_family = ARPHRD_ETHER;
+	memcpy(ifr.ifr_hwaddr.sa_data, tv_mac_tap, ETH_ALEN);
+	ASSERT_EQ(ioctl(fd, SIOCSIFHWADDR, &ifr), 0);
+	ASSERT_EQ(ioctl(fd, SIOCGIFFLAGS, &ifr), 0);
+	ifr.ifr_flags |= IFF_UP;
+	ASSERT_EQ(ioctl(fd, SIOCSIFFLAGS, &ifr), 0);
+	close(fd);
+
+	/* every buffer: zeroed vnet header + IPv4 frame for another host,
+	 * which the stack drops without counting it in rx_dropped
+	 */
+	ASSERT_EQ(posix_memalign((void **)&self->bufs, 4096,
+				 TV_RING_NUM * TV_BUF_SIZE), 0);
+	memset(self->bufs, 0, TV_RING_NUM * TV_BUF_SIZE);
+	for (i = 0; i < TV_RING_NUM; i++) {
+		unsigned char *eth = (unsigned char *)self->bufs +
+				     i * TV_BUF_SIZE + TV_HDR_LEN;
+
+		memcpy(eth, tv_mac_dst, ETH_ALEN);
+		memcpy(eth + ETH_ALEN, tv_mac_src, ETH_ALEN);
+		eth[12] = ETH_P_IP >> 8;
+		eth[13] = ETH_P_IP & 0xff;
+		eth[14] = 0x45;
+	}
+
+	ASSERT_EQ(posix_memalign(&self->ring, 4096,
+				 vring_size(TV_RING_NUM, 4096)), 0);
+	memset(self->ring, 0, vring_size(TV_RING_NUM, 4096));
+	vring_init(&self->vr, TV_RING_NUM, self->ring, 4096);
+
+	self->kick = eventfd(0, EFD_NONBLOCK);
+	self->call = eventfd(0, EFD_NONBLOCK);
+	ASSERT_GE(self->kick, 0);
+	ASSERT_GE(self->call, 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);
+}
+
+/* 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;
+		}
+		if (ms <= 0 || poll(&pfd, 1, 100) < 0)
+			return 0;
+		if (pfd.revents & POLLIN && read(self->call, &v, sizeof(v)) < 0)
+			return -1;
+		ms -= 100;
+	}
+}
+
+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);
+
+	while (reaped < TV_PKTS) {
+		n = tv_post(self, variant->pktlen, TV_PKTS - posted);
+		ASSERT_GE(n, 0);
+		posted += n;
+		n = tv_reap(self, 5000);
+		ASSERT_GE(n, 0);
+		if (!n) {
+			TH_LOG("%d of %d buffers not returned",
+			       posted - reaped, posted);
+			ASSERT_GT(n, 0);
+		}
+		reaped += n;
+	}
+
+	ASSERT_EQ(tv_read_stats(TV_IFNAME, &after), 0);
+	delivered = after.rx_packets - before.rx_packets;
+	dropped = after.rx_dropped - before.rx_dropped;
+	TH_LOG("sent %d, delivered %llu, dropped %llu", TV_PKTS, delivered,
+	       dropped);
+	/* bounded, */
+	EXPECT_GT(dropped, 0);
+	/* still draining, */
+	EXPECT_GE(delivered, TV_PKTS / 64);
+	/* and every packet accounted for */
+	EXPECT_EQ(delivered + dropped, TV_PKTS);
+}
+
+TEST_HARNESS_MAIN
-- 
2.56.0

Re: [PATCH net v4 0/2] tun: bound the NAPI receive queue

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v4 1/2] tun: bound the NAPI receive queue

[sashiko-bot]

> With IFF_NAPI, tun_get_user() queues every written packet for NAPI on
> an unbounded list. When the poll runs asynchronously (threaded NAPI)
> and is slower than the writer, the queue grows until the system runs
> out of memory. An unprivileged user can do this from its own user and
> network namespaces, and the queued skbs are not charged to its memcg:
> in a test VM, a uid 1000 process limited to 128 MB caused a global OOM
> in ~6 seconds. The socket send buffer cannot bound this: tun sets it to
> INT_MAX, and the build_skb() and XDP batch paths do not check it.
> 
> Drop packets once more than netdev_max_backlog of them are queued, like
> enqueue_to_backlog() does, and count them as rx_dropped: before
> allocating and copying them where possible, and again under the queue
> lock before queueing. This covers write(), sendmsg() and the batches of
> XDP buffers vhost-net passes to tun_sendmsg(), which share the queue.
> Zerocopy buffers of packets dropped early are completed.
> [ ... ]
> by the poll), and the queued skbs are still not charged to the writer's
> memcg.
> 
> Fixes: 943170998b20 ("tun: enable NAPI for TUN/TAP driver")
> Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>

Sashiko has reviewed this patch and found no issues.

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

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

[sashiko-bot]

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

Re: [PATCH net v4 1/2] tun: bound the NAPI receive queue

[Willem de Bruijn]

Antonio Quartulli wrote:
> With IFF_NAPI, tun_get_user() queues every written packet for NAPI on
> an unbounded list. When the poll runs asynchronously (threaded NAPI)
> and is slower than the writer, the queue grows until the system runs
> out of memory. An unprivileged user can do this from its own user and
> network namespaces, and the queued skbs are not charged to its memcg:
> in a test VM, a uid 1000 process limited to 128 MB caused a global OOM
> in ~6 seconds. The socket send buffer cannot bound this: tun sets it to
> INT_MAX, and the build_skb() and XDP batch paths do not check it.
> 
> Drop packets once more than netdev_max_backlog of them are queued, like
> enqueue_to_backlog() does, and count them as rx_dropped: before
> allocating and copying them where possible, and again under the queue
> lock before queueing. This covers write(), sendmsg() and the batches of
> XDP buffers vhost-net passes to tun_sendmsg(), which share the queue.
> Zerocopy buffers of packets dropped early are completed.
> 
> The NAPI poll moves the whole queue out, processes up to its budget and
> puts the rest back: while it runs, the queue looks empty to writers, so
> with the poll on another CPU the backlog still grows without limit. Keep
> the leftovers on a queue of their own and take the next batch only once
> it is empty, like process_backlog() does, so that neither queue holds
> more than the limit.
> 
> Dropping because the queue is full also schedules NAPI, in all three
> places. vhost-net sends with MSG_MORE while it has more packets, and
> tun_get_user() schedules NAPI only after the last one or once more than
> NAPI_POLL_WEIGHT are queued. With a netdev_max_backlog below that, the
> queue would not drain until vhost-net ran out of packets; with zerocopy,
> vhost-net never would, as it stops sending when too many queued buffers
> are pending.
> 
> Reproducer, as an unprivileged user in a new user and network
> namespace, without kernel modifications or fault injection:
>   ip tuntap add t0 mode tun; ip addr add 10.11.0.1/24 dev t0
>   ip link add d0 type dummy; ip addr add 10.20.0.1/24 dev d0
>   ip link set t0 up; ip link set d0 up; sysctl net.ipv4.ip_forward=1
>   install ~500 iptables FORWARD rules and a MASQUERADE rule on d0
>   open t0 with IFF_TUN | IFF_NO_PI | IFF_NAPI
>   echo 1 > /sys/class/net/t0/threaded
>   write() 1400-byte IPv4 packets for 10.20.0.2 in a loop
> 
> Tested in an x86-64 VM with 1 and 2 vCPUs: with the reproducer running
> for 15 seconds, memory use stays flat and the forwarding rate is
> unchanged; the tun kselftest passes, as it does without the patch; and
> the tun_vhost kselftest added by the next patch, which drives the
> vhost-net batch, copy and zerocopy paths, passes, while it fails without
> the patch.
> 
> Still open: the limit counts packets rather than bytes and applies per
> queue, up to about twice the limit can be in flight (queued plus taken
> by the poll), and the queued skbs are still not charged to the writer's
> memcg.
> 
> Fixes: 943170998b20 ("tun: enable NAPI for TUN/TAP driver")
> Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>

This is quite a complex solution.

Can we perhaps simplify it a bit? For instance:

On tx only keep the lockless check. Number of packets that can bypass
that is bounded, by the number of competing threads.

On rx avoid the new queue, by not temporary presenting an empty queue:

        spin_lock(&queue->lock);
        if (skb_queue_len(queue) <= budget) {
            skb_queue_splice_tail_init(queue, &process_queue);
        } else {
            while (skb_queue_len(&process_queue) < budget &&
                   (skb = __skb_dequeue(queue)))
                __skb_queue_tail(&process_queue, skb);
        }
        spin_unlock(&queue->lock);




> ---
>  drivers/net/tun.c | 82 +++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 68 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 770b8fe085c3..ea99ee0aee2b 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -58,6 +58,7 @@
>  #include <linux/nsproxy.h>
>  #include <linux/virtio_net.h>
>  #include <linux/rcupdate.h>
> +#include <net/hotdata.h>
>  #include <net/net_namespace.h>
>  #include <net/netns/generic.h>
>  #include <net/rtnetlink.h>
> @@ -140,6 +141,8 @@ struct tun_file {
>  		unsigned int ifindex;
>  	};
>  	struct napi_struct napi;
> +	/* NAPI poll only: skbs taken off sk_write_queue, not yet received */
> +	struct sk_buff_head napi_process_queue;
>  	bool napi_enabled;
>  	bool napi_frags_enabled;
>  	struct mutex napi_mutex;	/* Protects access to the above napi */
> @@ -227,31 +230,40 @@ struct veth {
>  static void tun_flow_init(struct tun_struct *tun);
>  static void tun_flow_uninit(struct tun_struct *tun);
>  
> +/* In IFF_NAPI mode written packets wait in the tfile's sk_write_queue for the
> + * NAPI poll, which may run later in a kthread or ksoftirqd. Bound that queue
> + * like the per-CPU backlog of netif_rx().
> + */
> +static bool tun_napi_queue_full(const struct sk_buff_head *queue)
> +{
> +	return skb_queue_len_lockless(queue) >
> +	       READ_ONCE(net_hotdata.max_backlog);
> +}
> +
>  static int tun_napi_receive(struct napi_struct *napi, int budget)
>  {
>  	struct tun_file *tfile = container_of(napi, struct tun_file, napi);
> +	struct sk_buff_head *process_queue = &tfile->napi_process_queue;
>  	struct sk_buff_head *queue = &tfile->sk.sk_write_queue;
> -	struct sk_buff_head process_queue;
>  	struct sk_buff *skb;
>  	int received = 0;
>  
> -	__skb_queue_head_init(&process_queue);
> -
> -	spin_lock(&queue->lock);
> -	skb_queue_splice_tail_init(queue, &process_queue);
> -	spin_unlock(&queue->lock);
> -
> -	while (received < budget && (skb = __skb_dequeue(&process_queue))) {
> +	while (received < budget) {
> +		/* Refill only when empty: leftovers are not put back on the
> +		 * queue writers check, so neither queue exceeds the limit.
> +		 */
> +		if (skb_queue_empty(process_queue)) {
> +			spin_lock(&queue->lock);
> +			skb_queue_splice_tail_init(queue, process_queue);
> +			spin_unlock(&queue->lock);
> +			if (skb_queue_empty(process_queue))
> +				break;
> +		}
> +		skb = __skb_dequeue(process_queue);
>  		napi_gro_receive(napi, skb);
>  		++received;
>  	}
>  
> -	if (!skb_queue_empty(&process_queue)) {
> -		spin_lock(&queue->lock);
> -		skb_queue_splice(&process_queue, queue);
> -		spin_unlock(&queue->lock);
> -	}
> -
>  	return received;
>  }
>  
> @@ -572,6 +584,8 @@ static void tun_queue_purge(struct tun_file *tfile)
>  		tun_ptr_free(ptr);
>  
>  	skb_queue_purge(&tfile->sk.sk_write_queue);
> +	/* NAPI is disabled by now */
> +	__skb_queue_purge(&tfile->napi_process_queue);
>  	skb_queue_purge(&tfile->sk.sk_error_queue);
>  }
>  
> @@ -1856,6 +1870,23 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  			zerocopy = true;
>  	}
>  
> +	/* NAPI queue full: drop before allocating, and schedule NAPI, which a
> +	 * MSG_MORE sender may not have done yet. Rechecked under the lock.
> +	 */
> +	if (tfile->napi_enabled &&
> +	    unlikely(tun_napi_queue_full(&tfile->sk.sk_write_queue))) {
> +		local_bh_disable();
> +		napi_schedule(&tfile->napi);
> +		local_bh_enable();
> +		if (msg_control) {
> +			struct ubuf_info *uarg = msg_control;
> +
> +			uarg->ops->complete(NULL, uarg, false);
> +		}
> +		dev_core_stats_rx_dropped_inc(tun->dev);
> +		return total_len;
> +	}
> +
>  	if (!frags && tun_can_build_skb(tun, tfile, len, noblock, zerocopy)) {
>  		/* For the packet that is not easy to be processed
>  		 * (e.g gso or jumbo packet), we will do it at after
> @@ -2045,6 +2076,17 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  			goto free_skb;
>  		}
>  
> +		/* NAPI queue full: drop, as above */
> +		if (unlikely(tun_napi_queue_full(queue))) {
> +			spin_unlock(&queue->lock);
> +			napi_schedule(&tfile->napi);
> +			local_bh_enable();
> +			rcu_read_unlock();
> +			dev_core_stats_rx_dropped_inc(tun->dev);
> +			drop_reason = SKB_DROP_REASON_FULL_RING;
> +			goto free_skb;
> +		}
> +
>  		__skb_queue_tail(queue, skb);
>  		queue_len = skb_queue_len(queue);
>  		spin_unlock(&queue->lock);
> @@ -2649,6 +2691,17 @@ static int tun_xdp_one(struct tun_struct *tun,
>  			return -EBUSY;
>  		}
>  
> +		/* NAPI queue full: drop, and schedule NAPI so that it drains
> +		 * even if nothing in this batch gets queued
> +		 */
> +		if (unlikely(tun_napi_queue_full(queue))) {
> +			spin_unlock(&queue->lock);
> +			napi_schedule(&tfile->napi);
> +			kfree_skb_reason(skb, SKB_DROP_REASON_FULL_RING);
> +			dev_core_stats_rx_dropped_inc(tun->dev);
> +			return 0;
> +		}
> +
>  		__skb_queue_tail(queue, skb);
>  		spin_unlock(&queue->lock);
>  		ret = 1;
> @@ -3571,6 +3624,7 @@ static int tun_chr_open(struct inode *inode, struct file * file)
>  	}
>  
>  	mutex_init(&tfile->napi_mutex);
> +	__skb_queue_head_init(&tfile->napi_process_queue);
>  	RCU_INIT_POINTER(tfile->tun, NULL);
>  	tfile->flags = 0;
>  	tfile->ifindex = 0;
> -- 
> 2.56.0
> 

Re: [PATCH net v4 1/2] tun: bound the NAPI receive queue

[netdev-bot+sashiko]

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

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

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).

- [High] The fix limits the IFF_NAPI receive queue by packet count,
  separately for each tun_file.

--

tun: bound the NAPI receive queue

This commit limits the IFF_NAPI receive queue (sk_write_queue) of each
tun_file to netdev_max_backlog packets. Packets beyond that are dropped
and counted as rx_dropped in tun_get_user() and tun_xdp_one(). It also
moves the NAPI poll leftovers to a new per-tfile napi_process_queue,
which is only refilled once it is empty.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index dcec7311e6011..a86750626953c 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> @@ -227,31 +230,40 @@ struct veth {
>  static void tun_flow_init(struct tun_struct *tun);
>  static void tun_flow_uninit(struct tun_struct *tun);
>  
> +/* In IFF_NAPI mode written packets wait in the tfile's sk_write_queue for the
> + * NAPI poll, which may run later in a kthread or ksoftirqd. Bound that queue
> + * like the per-CPU backlog of netif_rx().
> + */
> +static bool tun_napi_queue_full(const struct sk_buff_head *queue)
> +{
> +	return skb_queue_len_lockless(queue) >
> +	       READ_ONCE(net_hotdata.max_backlog);
> +}

[Severity: High]

Is a packet count per queue enough to stop the unprivileged global OOM
described in the commit message?

tun_napi_queue_full() counts skbs but does not look at their size. In
tun_get_user(), len comes from iov_iter_count() with no MTU or byte
cap, and sk_sndbuf is INT_MAX. tun_alloc_skb() puts anything beyond the
frag limit (about 544 KB with 4K pages) into the linear head:

drivers/net/tun.c:tun_alloc_skb() {
    ...
	if (len - linear > MAX_SKB_FRAGS * (PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER))
		linear = len - MAX_SKB_FRAGS * (PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER);
	skb = sock_alloc_send_pskb(sk, prepad + linear, len - linear, noblock,
				   &err, PAGE_ALLOC_COSTLY_ORDER);
    ...
}

napi_process_queue in tun_napi_receive() holds one batch while writers
refill sk_write_queue. So a single queue can pin about 2 * max_backlog
skbs, which is about 2000 by default:

- With 64 KB writes that is roughly 136 MB per queue.
- With writes near 1 MB it can reach the INT_MAX sk_wmem_alloc ceiling.

None of this memory is charged to the writer's memcg.

Packets don't even need to be oversized. tun_net_init() sets max_mtu to
MAX_MTU - hard_header_len, about 64 KB. MTU-sized packets are therefore
about 45 times larger than the 1400-byte packets in the reproducer.

The limit can also be multiplied without any privilege in the init
namespace. tun_set_iff() only checks CAP_NET_ADMIN in the device's user
namespace before allowing IFF_MULTI_QUEUE:

drivers/net/tun.c:tun_set_iff() {
    ...
		int queues = ifr->ifr_flags & IFF_MULTI_QUEUE ?
			     MAX_TAP_QUEUES : 1;

		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
			return -EPERM;
    ...
}

Each of up to 256 queues gets its own limit. On top of that, the same
user can create more tun devices and network namespaces.

The reproducer in the commit message writes 1400-byte packets on one
queue. Would the same reproducer still exhaust global memory with larger
writes, or with more queues or devices?

The "Still open" paragraph lists the packets-not-bytes and per-queue
limits. It doesn't say that the unprivileged OOM is still reachable,
even though the patch carries a Fixes: tag. The following tun_vhost
selftest patch in the series doesn't change drivers/net/tun.c, so this
gap seems to remain at the end of the series.

Would closing this need accounting by skb truesize or bytes, or charging
the queued skbs to the writer's memcg, rather than a packet count per
queue?

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

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

[netdev-bot+sashiko]

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.

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

[ ... ]

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

[ ... ]

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

> +
> +static void tv_vhost_setup(struct __test_metadata *_metadata,
> +			   FIXTURE_DATA(tun_vhost) *self)
> +{

[ ... ]

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

[ ... ]

> +	/* 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.

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

[ ... ]

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

> +
> +/* 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.

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

[ ... ]

> 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