Re: [PATCH net v2] net: tun: fix race condition between tun_attach and tun_get_user with XDP

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 (2) · 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] Incomplete fix: the new "publish tfile->tun last" ordering is a
  no-op on the TUNSETQUEUE/IFF_ATTACH_QUEUE re-attach path, so the…

Pre-existing issues:
- [Medium] Mirror-image ordering problem on the detach path is untouched
  and reproduces the same netif_get_rxqueue() WARN_ONCE.
- [Medium] tun_set_real_num_queues() (drivers/net/tun.c:526-530) is void
  and discards the documented error returns of…

--

net: tun: fix race condition between tun_attach and tun_get_user with XDP

This moves the rcu_assign_pointer(tfile->tun, tun) publication in
tun_attach() to after tun_set_real_num_queues(), so that
dev->real_num_rx_queues is raised before a concurrent writer can observe
the newly attached tun_file.  The intent is to stop the WARN_ONCE in
netif_get_rxqueue() that fires when a recorded rx queue index is not
below dev->real_num_rx_queues.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 5a302709a68a..2dfca25b86e9 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -811,11 +811,11 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>  	 * initialized tfile; otherwise we risk using half-initialized
>  	 * object.
>  	 */
> -	if (publish_tun)
> -		rcu_assign_pointer(tfile->tun, tun);
>  	rcu_assign_pointer(tun->tfiles[tun->numqueues], tfile);
>  	WRITE_ONCE(tun->numqueues, tun->numqueues + 1);
>  	tun_set_real_num_queues(tun);

[Severity: Medium]
This isn't a bug introduced by this patch, but the new ordering guarantee
now rests on tun_set_real_num_queues() having actually taken effect, and
that function drops both return values:

drivers/net/tun.c:
static void tun_set_real_num_queues(struct tun_struct *tun)
{
	netif_set_real_num_tx_queues(tun->dev, tun->numqueues);
	netif_set_real_num_rx_queues(tun->dev, tun->numqueues);
}

For an already registered device, netif_set_real_num_rx_queues() returns
early without assigning dev->real_num_rx_queues:

net/core/dev.c:netif_set_real_num_rx_queues() {
		rc = net_rx_queue_update_kobjects(dev, dev->real_num_rx_queues,
						  rxq);
		if (rc)
			return rc;
	}

	dev->real_num_rx_queues = rxq;
}

and net_rx_queue_update_kobjects() can fail on rx_queue_add_kobject():

net/core/net-sysfs.c:net_rx_queue_update_kobjects() {
		error = rx_queue_add_kobject(dev, i);
		if (error) {
			new_num = old_num;
			break;
		}
}

In that case tun_attach() has already published tun->tfiles[] and bumped
tun->numqueues, and then publishes tfile->tun and returns 0.  Does that
leave tfile->queue_index == dev->real_num_rx_queues permanently, so that
every write() on that queue going through the generic XDP path hits the
same netif_get_rxqueue() warning the commit message is trying to remove?
Should the error be propagated and the numqueues/tfiles publication
unwound instead?

[Severity: Medium]
This is a pre-existing issue and the patch does not touch __tun_detach(),
but the decreasing direction looks like the same mismatch.  In
__tun_detach() the surviving queue's index is lowered and numqueues is
decremented well before the real queue count is reduced:

drivers/net/tun.c:__tun_detach() {
		spin_lock(&ntfile->tx_ring.consumer_lock);
		ntfile->queue_index = index;
		ntfile->xdp_rxq.queue_index = index;
		...
		WRITE_ONCE(tun->numqueues, tun->numqueues - 1);
		...
		synchronize_net();
		...
		tun_set_real_num_queues(tun);
}

The moved tfile keeps its ->tun published, and the queue_index sample in
tun_get_user() happens before the rcu_read_lock() around the XDP call:

	skb_record_rx_queue(skb, tfile->queue_index);

	if (skb_xdp) {
		...
		local_bh_disable();
		rcu_read_lock();

so the synchronize_net() above does not fence a writer that already
sampled the old, higher index.  Can a write() racing with
TUNSETQUEUE|IFF_DETACH_QUEUE reach do_xdp_generic() after
dev->real_num_rx_queues has shrunk and hit the same WARN_ONCE?  Also, for
the clean == false case, tun_disable_queue() never clears tfile->tun and
the stale queue_index stays in place while the fd is still writable.
Should the commit message scope its claim to the attach path, or should
the detach direction be handled too?

> +	if (publish_tun)
> +		rcu_assign_pointer(tfile->tun, tun);
>  out:
>  	return err;
>  }

[Severity: High]
Is this reordering effective on the TUNSETQUEUE re-attach path?  A queue
detached with IFF_DETACH_QUEUE goes through __tun_detach(tfile, false),
where clearing tfile->tun only happens in the clean case:

drivers/net/tun.c:__tun_detach() {
		if (clean) {
			RCU_INIT_POINTER(tfile->tun, NULL);
			sock_put(&tfile->sk);
		} else {
			tun_disable_queue(tun, tfile);
			tun_napi_disable(tfile);
		}
}

So on that fd tfile->tun stays non-NULL and visible.  tun_set_queue()
then re-attaches with publish_tun = true:

		ret = tun_attach(tun, file, false, tun->flags & IFF_NAPI,
				 tun->flags & IFF_NAPI_FRAGS, true);

and tun_attach() writes the new, higher index long before the queue count
is raised:

	spin_lock(&tfile->tx_ring.consumer_lock);
	tfile->queue_index = tun->numqueues;
	spin_unlock(&tfile->tx_ring.consumer_lock);

Since the moved rcu_assign_pointer(tfile->tun, tun) stores a value that
readers can already see, does it order anything for them on this path?  A
concurrent write() on the same fd still passes tun_get(), records the new
index and calls into XDP:

	skb_record_rx_queue(skb, tfile->queue_index);

	if (skb_xdp) {

reaching net/core/dev.c:netif_get_rxqueue():

		if (unlikely(index >= dev->real_num_rx_queues)) {
			WARN_ONCE(dev->real_num_rx_queues > 1,

which is the warning quoted in the commit message, plus the silent
fallback to dev->_rx[0] so the program runs against the wrong
netdev_rx_queue.  The window is not small either, because
netif_set_real_num_rx_queues() does GFP_KERNEL sysfs work.  Would either
clearing tfile->tun on the non-clean detach, or publishing
tfile->queue_index only after tun_set_real_num_queues(), close the attach
path in all cases rather than only when tfile->tun transitions from NULL?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260920030240.443223-1-xietangxin%40h-partners.com