On 9/5/24 03:33, Jakub Kicinski wrote:
On Wed, 4 Sep 2024 15:53:39 +0200 Paolo Abeni wrote:
+ net_shaper_set_real_num_tx_queues(dev, txq);
+
dev_qdisc_change_real_num_tx(dev, txq);
dev->real_num_tx_queues = txq;
The dev->lock has to be taken here, around those three lines,
and then set / group must check QUEUE ids against
dev->real_num_tx_queues, no? Otherwise the work
net_shaper_set_real_num_tx_queues() does is prone to races?
Yes, I think such race exists, but I'm unsure that tacking the lock
around the above code will be enough.
i.e. if the relevant devices has 16 channel queues the set() races with
a channel reconf on different CPUs:
CPU 1 CPU 2
set_channels(8)
driver_set_channel()
// actually change the number of queues to
// 8, dev->real_num_tx_queues is still 16
// dev->lock is not held yet because the
// driver still has to call
// netif_set_real_num_tx_queues()
set(QUEUE_15,...)
// will pass validation
// but queue 15 does not
// exist anymore
Acquiring dev->lock around set_channel() will not be enough: some driver
change the channels number i.e. when enabling XDP.
I think/fear we need to replace the dev->lock with the rtnl lock to
solve the race for good.
/P
