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

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

[Umang Pokhriyal]

When a queue is detached, __tun_detach() moves the last queue into the
freed slot and decrements tun->numqueues. The flow entries that point at
the old index of the moved queue, now tun->numqueues, are meant to be
deleted, but the call passes tun->numqueues + 1, an index that never
exists, so nothing is deleted.

Until those entries are refreshed by a write on their flow or age out,
tun_automq_select_queue() returns an index past the last queue, and
netdev_core_pick_tx() sends every such flow to queue 0 and warns:

  tapq0 selects TX queue 3, but real number of TX queues is 3

Found with an LLM while testing how a VMM detaches tap queues when a
guest lowers its virtio-net queue pairs. With four queues, 30 flows
learned on queue 3 and queue 1 detached, all 30 went to queue 0 with the
warning; with this patch they are spread by hash and no warning is
printed (3 runs each, in a guest booted on net).

Pass tun->numqueues.

Fixes: 96442e42429e ("tuntap: choose the txq based on rxq")
Signed-off-by: Umang Pokhriyal <umangpokhriyall@gmail.com>
---
 drivers/net/tun.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index dcec7311e..3b292bce4 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);
 		/* Drop read queue */
 		tun_queue_purge(tfile);
 		spin_lock_bh(&ntfile->tx_ring.consumer_lock);
-- 
2.53.0

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

[Willem de Bruijn]

Umang Pokhriyal wrote:
> When a queue is detached, __tun_detach() moves the last queue into the
> freed slot and decrements tun->numqueues. The flow entries that point at
> the old index of the moved queue, now tun->numqueues, are meant to be
> deleted, but the call passes tun->numqueues + 1, an index that never
> exists, so nothing is deleted.
> 
> Until those entries are refreshed by a write on their flow or age out,
> tun_automq_select_queue() returns an index past the last queue, and
> netdev_core_pick_tx() sends every such flow to queue 0 and warns:
> 
>   tapq0 selects TX queue 3, but real number of TX queues is 3
> 
> Found with an LLM while testing how a VMM detaches tap queues when a
> guest lowers its virtio-net queue pairs. With four queues, 30 flows
> learned on queue 3 and queue 1 detached, all 30 went to queue 0 with the
> warning; with this patch they are spread by hash and no warning is
> printed (3 runs each, in a guest booted on net).
> 
> Pass tun->numqueues.
> 
> Fixes: 96442e42429e ("tuntap: choose the txq based on rxq")
> Signed-off-by: Umang Pokhriyal <umangpokhriyall@gmail.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

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

[netdev-bot+sashiko]

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