Re: [PATCH] wifi: cfg80211: avoid holding rtnl_mutex across all cfg80211_leave() calls

Re: [PATCH] wifi: cfg80211: avoid holding rtnl_mutex across all cfg80211_leave() calls

[Ömer Mete Kaya]

Apologies for the noise — I forgot to mark the previous send as v2.
This is v2 of the patch, extending the fix to cover two additional
hung task reports (tun_chr_close and switchdev_deferred_process_work)
that share the same root cause.

Changes in v2:
- Added Reported-by/Closes for tun_chr_close (b0ae8f1a)
- Added Reported-by/Closes for switchdev_deferred_process_work (d6bbe0f5)
- Added linux-kernel@vger.kernel.org to CC
- Updated "three separate" to "five separate" in commit message

In-Reply-To: <20260903111705.472491-1-omermetekaya0@gmail.com>

[PATCH v2] wifi: cfg80211: avoid holding rtnl_mutex across all cfg80211_leave() calls

[Ömer Mete Kaya]

reg_check_chans_work() holds rtnl_mutex for the entire duration of
iterating over all registered devices and calling cfg80211_leave() on
each invalid wdev. cfg80211_leave() can be slow (disconnect, stop AP,
leave mesh), causing rtnl_mutex starvation when many wireless interfaces
are present. This results in tasks waiting for rtnl_mutex for longer
than hung_task_timeout_secs:

  INFO: task hung in inet_rtm_newaddr
  INFO: task hung in inet6_rtm_newaddr
  INFO: task hung in nsim_destroy
  INFO: task hung in tun_chr_close
  INFO: task hung in switchdev_deferred_process_work

Fix by walking cfg80211_rdev_list under RCU and acquiring rtnl per-device,
so other rtnl waiters get a chance to run between devices.

I could not add the Fixes: tag because this patch addresses five separate
hung task reports whose cause bisections all failed, making it impossible
to identify a single introducing commit.

Reported-by: syzbot+adeb8550754921fece20@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=adeb8550754921fece20
Reported-by: syzbot+101224300649c3eb8af4@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=101224300649c3eb8af4
Reported-by: syzbot+8141dcbd23a8f857798a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=8141dcbd23a8f857798a
Reported-by: syzbot+b0ae8f1abf7d891e0426@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=b0ae8f1abf7d891e0426
Reported-by: syzbot+d6bbe0f5705cb8a5aa2b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d6bbe0f5705cb8a5aa2b
Signed-off-by: Ömer Mete Kaya <omermetekaya0@gmail.com>
---
 net/wireless/reg.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index a8336baf85dc..a2e0d1cf8317 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -2466,12 +2466,21 @@ static void reg_check_chans_work(struct work_struct *work)
 	struct cfg80211_registered_device *rdev;
 
 	pr_debug("Verifying active interfaces after reg change\n");
-	rtnl_lock();
-
-	for_each_rdev(rdev)
+	/*
+	 * Acquire rtnl per-device instead of holding it for the entire loop;
+	 * cfg80211_leave() can be slow and starve other rtnl waiters otherwise.
+	 * wiphy_unregister() holds rtnl across list_del_rcu() + synchronize_rcu(),
+	 * so rdev cannot be freed while we hold rtnl_lock() below.
+	 */
+	rcu_read_lock();
+	list_for_each_entry_rcu(rdev, &cfg80211_rdev_list, list) {
+		rcu_read_unlock();
+		rtnl_lock();
 		reg_leave_invalid_chans(&rdev->wiphy);
-
-	rtnl_unlock();
+		rtnl_unlock();
+		rcu_read_lock();
+	}
+	rcu_read_unlock();
 }
 
 void reg_check_channels(void)
-- 
2.55.0

Re: [PATCH v2] wifi: cfg80211: avoid holding rtnl_mutex across all cfg80211_leave() calls

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/20260903151542.486376-2-omermetekaya0@gmail.com?part=1
---
wifi: cfg80211: avoid holding rtnl_mutex across all cfg80211_leave() calls

This commit changes reg_check_chans_work() to iterate over cfg80211_rdev_list
using RCU instead of holding rtnl_mutex for the entire loop. It acquires
rtnl_mutex on a per-device basis to prevent task hangs.

> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index a8336baf85dc6..a2e0d1cf8317c 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -2466,12 +2466,21 @@ static void reg_check_chans_work(struct work_struct *work)
>  	struct cfg80211_registered_device *rdev;
>  
>  	pr_debug("Verifying active interfaces after reg change\n");
> -	rtnl_lock();
> -
> -	for_each_rdev(rdev)
> +	/*
> +	 * Acquire rtnl per-device instead of holding it for the entire loop;
> +	 * cfg80211_leave() can be slow and starve other rtnl waiters otherwise.
> +	 * wiphy_unregister() holds rtnl across list_del_rcu() + synchronize_rcu(),
> +	 * so rdev cannot be freed while we hold rtnl_lock() below.
> +	 */
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(rdev, &cfg80211_rdev_list, list) {
> +		rcu_read_unlock();

[Severity: High]
Does dropping the RCU read lock here remove the lifetime protection of the
current rdev element while waiting for rtnl_lock()?

If a concurrent thread unregisters the device (for example, via
wiphy_unregister()), it could acquire rtnl_lock, remove the device with
list_del_rcu(), and call synchronize_rcu().

Because the RCU read lock was dropped by the work thread here,
synchronize_rcu() could complete and allow the unregister thread to free rdev
before this thread acquires the rtnl lock.

> +		rtnl_lock();
>  		reg_leave_invalid_chans(&rdev->wiphy);

[Severity: High]
Can this regression lead to a use-after-free?

If rdev was freed during the race window while waiting for rtnl_lock(),
accessing rdev->wiphy here would read from freed memory.

> -
> -	rtnl_unlock();
> +		rtnl_unlock();
> +		rcu_read_lock();
> +	}

[Severity: High]
Will the list_for_each_entry_rcu() macro implicitly dereference
rdev->list.next to continue the iteration after the loop body finishes?

If rdev was freed while the lock was dropped, does this result in another
use-after-free when advancing to the next list element?

> +	rcu_read_unlock();
>  }