netdev - Re: [PATCH net-next v2 8/8] netdev: depend on netdev->lock for qstats in ops locked drivers

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] [day] [month] [year] [list]

Message-ID: <202fbd8a-4863-4655-9e2d-8c8e8902dc2c@intel.com>
Date: Fri, 11 Apr 2025 14:29:27 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <davem@...emloft.net>, <netdev@...r.kernel.org>, <edumazet@...gle.com>,
	<pabeni@...hat.com>, <andrew+netdev@...n.ch>, <horms@...nel.org>,
	<sdf@...ichev.me>, <hramamurthy@...gle.com>, <kuniyu@...zon.com>,
	<jdamato@...tly.com>
Subject: Re: [PATCH net-next v2 8/8] netdev: depend on netdev->lock for qstats
 in ops locked drivers



On 4/10/2025 4:46 PM, Jakub Kicinski wrote:
> On Wed, 9 Apr 2025 22:23:28 -0700 Jacob Keller wrote:
>>> +struct netdev_stat_ops
>>> +----------------------
>>> +
>>> +"qstat" ops are invoked under the instance lock for "ops locked" drivers,
>>> +and under rtnl_lock for all other drivers.
>>> +
>>>  struct net_shaper_ops
>>>  ---------------------
>>>    
>>
>> What determines if a driver is "ops locked"? Is that defined above this
>> chunk in the doc? I see its when netdev_need_ops_lock() is set? Ok.
> 
> Yup, it was hiding in the previous patch:
> 
>    Code comments and docs refer to drivers which have ops called under
>    the instance lock as "ops locked".
> 
>> Sounds like it would be good to start migrating drivers over to this
>> locking paradigm over time.
> 
> At least for the drivers which implement queue stats its nice to be able 
> to dump stats without taking the global lock. 
> 

Yep. Lots of good reasons to do this work, even if it takes a long time
because of how interconnected the problems are. A measured approach
where we do things slowly is great for reducing the risk of such a big
refactor.

>>>  	if (ifindex) {
>>> -		netdev = __dev_get_by_index(net, ifindex);
>>> -		if (netdev && netdev->stat_ops) {
>>> +		netdev = netdev_get_by_index_lock_ops_compat(net, ifindex);
>>> +		if (!netdev) {
>>> +			NL_SET_BAD_ATTR(info->extack,
>>> +					info->attrs[NETDEV_A_QSTATS_IFINDEX]);
>>> +			return -ENODEV;
>>> +		}  
>>
>> I guess netdev_get_by_index_lock_ops_compat acquires the lock when it
>> returns success?
> 
> Yes.
> 

Thanks!

>>> +		if (netdev->stat_ops) {
>>>  			err = netdev_nl_qstats_get_dump_one(netdev, scope, skb,
>>>  							    info, ctx);
>>>  		} else {
>>>  			NL_SET_BAD_ATTR(info->extack,
>>>  					info->attrs[NETDEV_A_QSTATS_IFINDEX]);
>>> -			err = netdev ? -EOPNOTSUPP : -ENODEV;
>>> -		}
>>> -	} else {  
>>
>> But there's an else branch here so now I'm confused with how this
>> locking works.
> 

Ugh.. Yea I should have noticed that :D

> The diff is really hard to read, sorry, I should have done two patches.
> The else branch is _removed_. The code is now:
> 
> 	if (ifindex) {
> 		netdev = netdev_get_by_index_lock_ops_compat(net, ifindex);
> 		...
> 		netdev_unlock_ops_compat(netdev);  
> 		return ;
> 	}
> 
> 	for_each_lock_scoped() {
> 	}

I should have fetched this to review locally, that is much better.