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

[netdev-bot+sashiko@kernel.org]

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

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

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 new bound is a packet count on each tun_file's
  sk_write_queue, compared against net_hotdata.max_backlog (default…
- [Low] The new full-queue check runs before every XDP hook, in both
  paths the patch changes.
- [Low] The new comment in tun_napi_receive() says it takes "at most
  budget skbs, so that the queue never looks emptier to writers than it…

--

tun: bound the NAPI receive queue

In IFF_NAPI mode, this patch drops new packets in tun_get_user() and
tun_xdp_one() once the tfile's sk_write_queue holds more than
netdev_max_backlog packets, and counts them as rx_dropped. It also changes
tun_napi_receive() to take at most budget skbs from the queue instead of
splicing out the whole queue and putting the rest back.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 3b292bce4438c..e2fda570b90f6 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c

[ ... ]

> @@ -227,6 +228,16 @@ 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]
Does a per-queue packet count bound the uncharged memory that an
unprivileged user can pin?

This limit is about 1000 packets on one tfile's sk_write_queue. It has no
byte accounting and no limit across tfiles. The same user who can run the
reproducer can also open IFF_MULTI_QUEUE devices. tun_set_iff() only checks
CAP_NET_ADMIN in the user's own user namespace:

    int queues = ifr->ifr_flags & IFF_MULTI_QUEUE ?
                 MAX_TAP_QUEUES : 1;

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

That gives up to 256 queues per device, and as many devices as
RLIMIT_NOFILE allows. Each tfile still has sk_sndbuf = INT_MAX from
tun_chr_open(), and its queued skbs are not charged to the writer's memcg.
The only admission check on the write path is this one in tun_get_user()
and tun_xdp_one().

The commit message says:

    Still open: the limit counts packets rather than bytes and applies per
    queue, so with large packets and many queues (IFF_MULTI_QUEUE works in a
    user namespace too) the queued skbs, which are not charged to the
    writer's memcg, can still add up to an OOM. This is hard to reach in
    practice: with large packets the writer's copy is slower than the NAPI
    poll, so the queue does not fill.

That argument covers large packets only. The reproducer itself shows
1400-byte writes outrunning threaded NAPI. All queues drain on the same
CPUs, so if the writer cycles small packets through every queue fd, can
each queue reach max_backlog?

With roughly 2.3 KB of truesize per 1400-byte skb, about 1000 tfiles (the
default soft RLIMIT_NOFILE) would pin around 2.3 GB, and a higher fd limit
pins more. Does that still allow the global OOM from a memcg-limited user
that this patch is meant to fix?

A bound that does not scale with the number of tfiles would close this.
Two options would be charging queued skb truesize to the socket or memcg on
every enqueue path, or a per-device or per-netns aggregate limit. Either
would replace the per-queue packet count.

> +
>  static int tun_napi_receive(struct napi_struct *napi, int budget)
>  {
>  	struct tun_file *tfile = container_of(napi, struct tun_file, napi);
> @@ -237,21 +248,24 @@ static int tun_napi_receive(struct napi_struct *napi, int budget)
>  
>  	__skb_queue_head_init(&process_queue);
>  
> +	/* Take at most budget skbs, so that the queue never looks emptier to
> +	 * writers than it is.
> +	 */
>  	spin_lock(&queue->lock);
> -	skb_queue_splice_tail_init(queue, &process_queue);
> +	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);
>  
> -	while (received < budget && (skb = __skb_dequeue(&process_queue))) {
> +	while ((skb = __skb_dequeue(&process_queue))) {
>  		napi_gro_receive(napi, skb);
>  		++received;
>  	}

[Severity: Low]
Is the "never looks emptier to writers than it is" part of this comment
accurate?

When skb_queue_len(queue) <= budget, skb_queue_splice_tail_init() resets
the queue's qlen to 0. Otherwise each __skb_dequeue() lowers qlen by one,
for up to budget skbs.

In both cases, while napi_gro_receive() works through process_queue,
possibly on another CPU with threaded NAPI, tun_napi_queue_full() sees up to
budget fewer pending skbs than are really pending. The real bound comes out
to about max_backlog + budget, plus one per concurrent lockless writer.

This is much better than hiding the whole backlog as before. Could the
comment say the queue looks emptier by at most budget, so later code does
not rely on a strict invariant?

[ ... ]

> @@ -2561,6 +2593,17 @@ static int tun_xdp_one(struct tun_struct *tun,
>  		return -EIO;
>  	}
>  
> +	/* NAPI queue full: drop, and schedule NAPI so that it drains even if
> +	 * nothing in this batch gets queued
> +	 */
> +	if (tfile->napi_enabled &&
> +	    unlikely(tun_napi_queue_full(&tfile->sk.sk_write_queue))) {
> +		napi_schedule(&tfile->napi);
> +		put_page(virt_to_head_page(xdp->data));
> +		dev_core_stats_rx_dropped_inc(tun->dev);
> +		return 0;
> +	}
> +
>  	xdp_prog = rcu_dereference(tun->xdp_prog);

[Severity: Low]
Should this check come after the XDP program runs?

Here it sits before rcu_dereference(tun->xdp_prog) and bpf_prog_run_xdp().
In tun_get_user() the new check also comes before tun_build_skb(), which
runs native XDP and returns NULL when the program consumes the packet, and
before do_xdp_generic().

Packets the program would REDIRECT, TX or DROP never reach sk_write_queue.
They add nothing to the queue being bounded.

Once XDP_PASS traffic fills the queue past netdev_max_backlog, those packets
are dropped without the program seeing them. They are also counted in
rx_dropped. Would redirected or XDP_TX flows on that tfile stall whenever
the stack path is congested?

The commit message only says packets are dropped "before allocating and
copying them". If this trade-off is intended, could the commit message
mention the effect on XDP?

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