linux-kernel - Re: [PATCH net-next v1 4/4] net: drop rtnl_lock for queue

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: <Z8uGQ33kaodoZykm@mini-arch>
Date: Fri, 7 Mar 2025 15:50:27 -0800
From: Stanislav Fomichev <stfomichev@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Stanislav Fomichev <sdf@...ichev.me>, netdev@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
	linux-kernel@...r.kernel.org, horms@...nel.org,
	donald.hunter@...il.com, michael.chan@...adcom.com,
	pavan.chebbi@...adcom.com, andrew+netdev@...n.ch,
	jdamato@...tly.com, xuanzhuo@...ux.alibaba.com,
	almasrymina@...gle.com, asml.silence@...il.com, dw@...idwei.uk
Subject: Re: [PATCH net-next v1 4/4] net: drop rtnl_lock for queue_mgmt
 operations

On 03/07, Jakub Kicinski wrote:
> On Fri,  7 Mar 2025 07:57:25 -0800 Stanislav Fomichev wrote:
> > All drivers that use queue API are already converted to use
> > netdev instance lock. Move netdev instance lock management to
> > the netlink layer and drop rtnl_lock.
> 
> > @@ -860,12 +854,11 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> >  	}
> >  
> >  	mutex_lock(&priv->lock);
> > -	rtnl_lock();
> >  
> > -	netdev = __dev_get_by_index(genl_info_net(info), ifindex);
> > +	netdev = netdev_get_by_index_lock(genl_info_net(info), ifindex);
> >  	if (!netdev || !netif_device_present(netdev)) {
> >  		err = -ENODEV;
> > -		goto err_unlock;
> > +		goto err_unlock_sock;
> >  	}
> >  
> >  	if (dev_xdp_prog_count(netdev)) {
> > @@ -918,14 +911,15 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> >  	if (err)
> >  		goto err_unbind;
> >  
> > -	rtnl_unlock();
> > +	netdev_unlock(netdev);
> 
> Ah, here's the unlock :)

mutex_unlock(&priv->lock) is still missing :(

> Looks good for the devmem binding, I think, the other functions will
> need a bit more careful handling. So perhaps drop the queue get changes?
> I'm cooking some patches for the queue get and queue stats.
> AFAIU we need helpers which will go over netdevs and either take rtnl
> lock or instance lock, depending on whether the driver is "ops locked"

Here is what I was tentatively playing with (rtnl_netdev_{,un}lock_ops
abomination):
https://github.com/fomichev/linux/commit/f791a23c358c7db0e798bc4181dc6c243c8ff77d

Which sort of does what you're suggesting in:
https://github.com/fomichev/linux/commit/392ae1f3ca823dc412a2dac2263b6c8355f6925d

Although I'm still unconditionally holding rtnl_lock during
for_each_netdev_dump..