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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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