Re: [PATCH net] tun: delete the flow entries of the moved queue on detach

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The fix is incomplete.

--

tun: delete the flow entries of the moved queue on detach

When __tun_detach() moves the last queue into the freed slot, it is
supposed to delete the flow entries that still point at the moved queue's
old index. Before this patch it passed tun->numqueues + 1, an index that
never exists, so nothing was deleted. The patch passes tun->numqueues
instead.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index dcec7311e6011..3b292bce4438c 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -615,7 +615,7 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
>  		}
>  
>  		synchronize_net();
> -		tun_flow_delete_by_queue(tun, tun->numqueues + 1);
> +		tun_flow_delete_by_queue(tun, tun->numqueues);

[Severity: Medium]
This removes the case where every learned flow was misdirected on each
detach. Can a concurrent write() still put back an entry that points at
the old index after this delete has run?

tun_flow_update() reads the queue index before its own rcu_read_lock().
It does not take tun->lock, rtnl, or the consumer_lock that __tun_detach()
uses when it rewrites ntfile->queue_index:

drivers/net/tun.c:tun_flow_update() {
	...
	u16 queue_index = tfile->queue_index;

	head = &tun->flows[tun_hashfn(rxhash)];

	rcu_read_lock();

	e = tun_flow_find(head, rxhash);
	if (likely(e)) {
		/* TODO: keep queueing to old queue until it's empty? */
		if (READ_ONCE(e->queue_index) != queue_index)
			WRITE_ONCE(e->queue_index, queue_index);
	...
	} else {
		spin_lock_bh(&tun->lock);
		...
			tun_flow_create(tun, head, rxhash, queue_index);
	...
}

tun_get_user() also calls it only after it has left its RCU section:

	rcu_read_unlock();

	preempt_disable();
	dev_sw_netstats_rx_add(tun->dev, len);
	preempt_enable();

	if (rxhash)
		tun_flow_update(tun, rxhash, tfile);

So the synchronize_net() before the delete does not seem to wait for this
writer. Here N is the queue count before the detach. One possible
sequence, with write() on the moved queue (ntfile):

CPU1 (write() on ntfile)            CPU2 (__tun_detach())
tun_flow_update()
  queue_index = tfile->queue_index;
  /* N-1 */
  <preempted>
                                    ntfile->queue_index = index;
                                    WRITE_ONCE(tun->numqueues, N-1);
                                    synchronize_net();
                                    tun_flow_delete_by_queue(tun, N-1);
  rcu_read_lock();
  WRITE_ONCE(e->queue_index, N-1);
  /* or tun_flow_create(..., N-1) */

A second sequence: the queue being detached is the last one (index is N-1,
so ntfile == tfile) and the detach is IFF_DETACH_QUEUE. A write() that
passed the !tfile->detached check in tun_get_user() before
tun_disable_queue() can reach tun_flow_update() after the delete. It still
carries queue_index N-1, and nothing rewrites that.

In both cases tun_automq_select_queue() returns the stored index without
checking it against numqueues:

	if (e) {
		tun_flow_save_rps_rxhash(e, txq);
		txq = e->queue_index;
	} else {
		txq = reciprocal_scale(txq, numqueues);
	}

Before tun_set_real_num_queues() runs, tfiles[N-1] is NULL, so
tun_net_xmit() drops the packet. After it runs, netdev_cap_txqueue() sends
the packet to queue 0 and prints the same "selects TX queue N-1, but real
number of TX queues is N-1" warning quoted in the commit message. This
lasts until the flow is written again from a live queue or ages out.

The vhost path (tun_sendmsg()->tun_xdp_one()->tun_flow_update()) runs
inside the rcu_read_lock() taken in tun_sendmsg(), so only the write()
path appears to be affected.

The plain read of e->queue_index in tun_flow_delete_by_queue() also races
with the lockless WRITE_ONCE() in tun_flow_update().

There are two possible fixes. One is to read tfile->queue_index and update
e->queue_index under tun->lock. The other is to have
tun_automq_select_queue() fall back to reciprocal_scale() when
e->queue_index >= numqueues. Would either of these work here?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104174240.138491-1-umangpokhriyall%40gmail.com