netdev - Re: [PATCH v6 net-next 07/15] net-shapers: implement shaper cleanup on queue deletion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8fba5626-f4e0-47c3-b022-a7ca9ca1a93f@redhat.com>
Date: Thu, 5 Sep 2024 20:02:38 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
 Madhu Chittim <madhu.chittim@...el.com>,
 Sridhar Samudrala <sridhar.samudrala@...el.com>,
 Simon Horman <horms@...nel.org>, John Fastabend <john.fastabend@...il.com>,
 Sunil Kovvuri Goutham <sgoutham@...vell.com>,
 Jamal Hadi Salim <jhs@...atatu.com>, Donald Hunter
 <donald.hunter@...il.com>, anthony.l.nguyen@...el.com,
 przemyslaw.kitszel@...el.com, intel-wired-lan@...ts.osuosl.org,
 edumazet@...gle.com
Subject: Re: [PATCH v6 net-next 07/15] net-shapers: implement shaper cleanup
 on queue deletion



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