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

Message-ID: <20250410164614.407e6d98@kernel.org>
Date: Thu, 10 Apr 2025 16:46:14 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Jacob Keller <jacob.e.keller@...el.com>
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 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. 

> >  	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.

> > +		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.

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() {
	}