netdev - Re: [PATCH net] net: don't mix device locking in dev_close

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: <Z_w_M95kn0UFXfDm@mini-arch>
Date: Sun, 13 Apr 2025 15:48:19 -0700
From: Stanislav Fomichev <stfomichev@...il.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,
	syzbot+6f588c78bf765b62b450@...kaller.appspotmail.com,
	sdf@...ichev.me
Subject: Re: [PATCH net] net: don't mix device locking in dev_close_many()
 calls

On 04/12, Jakub Kicinski wrote:
> Lockdep found the following dependency:
> 
>   &dev_instance_lock_key#3 -->
>      &rdev->wiphy.mtx -->
>         &net->xdp.lock -->
> 	   &xs->mutex -->
> 	      &dev_instance_lock_key#3
> 
> The first dependency is the problem. wiphy mutex should be outside
> the instance locks. The problem happens in notifiers (as always)
> for CLOSE. We only hold the instance lock for ops locked devices
> during CLOSE, and WiFi netdevs are not ops locked. Unfortunately,
> when we dev_close_many() during netns dismantle we may be holding
> the instance lock of _another_ netdev when issuing a CLOSE for
> a WiFi device.
> 
> Lockdep's "Possible unsafe locking scenario" only prints 3 locks
> and we have 4, plus I think we'd need 3 CPUs, like this:
> 
>        CPU0                 CPU1              CPU2
>        ----                 ----              ----
>   lock(&xs->mutex);
>                        lock(&dev_instance_lock_key#3);
>                                          lock(&rdev->wiphy.mtx);
>                                          lock(&net->xdp.lock);
>                                          lock(&xs->mutex);
>                        lock(&rdev->wiphy.mtx);
>   lock(&dev_instance_lock_key#3);
> 
> Tho, I don't think that's possible as CPU1 and CPU2 would
> be under rtnl_lock. Even if we have per-netns rtnl_lock and
> wiphy can span network namespaces - CPU0 and CPU1 must be
> in the same netns to see dev_instance_lock, so CPU0 can't
> be installing a socket as CPU1 is tearing the netns down.
> 
> Regardless, our expected lock ordering is that wiphy lock
> is taken before instance locks, so let's fix this.
> 
> Go over the ops locked and non-locked devices separately.
> Note that calling dev_close_many() on an empty list is perfectly
> fine. All processing (including RCU syncs) are conditional
> on the list not being empty, already.
> 
> Fixes: 7e4d784f5810 ("net: hold netdev instance lock during rtnetlink operations")
> Reported-by: syzbot+6f588c78bf765b62b450@...kaller.appspotmail.com
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

Acked-by: Stanislav Fomichev <sdf@...ichev.me>