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

[xietangxin <xietangxin@h-partners.com>]

On 9/21/2026 10:53 AM, netdev-bot+sashiko@kernel.org wrote:
> 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:
This is a pre-existing issue unrelated to the race being fixed.
Maybe address it in a separate follow-up patch.
> 
> 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:
>
This is a deterministic bug (no race needed).
The XDP path in tun_get_user() lacks a tfile->detached check,
Write on a detached fd records a stale queue_index via
skb_record_rx_queue(), then enters do_xdp_generic() which calls
netif_get_rxqueue() with a stale index and trigger same WARN_ONCE.

> 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:
> 
Correct. __tun_detach(clean=false) does not clear tfile->tun,
so rcu_assign_pointer(tfile->tun, tun) writes the same pointer value that
readers already observe -- the v2 publish ordering has no effect.
Another reattach and tun_get_user race issue:
  CPU 0 (re-attach)             CPU 1 (tun_get_user)
  // tfile->tun isn't null
  tfile->queue_index = 2
  tun_enable_queue(tfile)
    tfile->detached = NULL
                                skb_record_rx_queue(skb, 2)
                                do_xdp_generic()
                                  netif_get_rxqueue()
                                  // index(2) >= real(2) -> WARN
  tun_set_real_num_queues()
    real_num_rx_queues = 3

pw-bot: cr
> 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?
> 

-- 
Best regards,
Tangxin Xie