RE: [PATCH] cfg80211: Fix "suspicious RCU usage in wiphy_apply_custom_regulatory" warning/backtrace

-----Original Message-----
From: Hans de Goede <redacted>
Sent: Monday, January 18, 2021 23:09
To: Peer, Ilan <redacted>; Johannes Berg
[off-list ref]; David S . Miller [off-list ref];
Jakub Kicinski [off-list ref]; Rojewski, Cezary
[off-list ref]; Pierre-Louis Bossart <pierre-
louis.bossart@linux.intel.com>; Liam Girdwood
[off-list ref]; Jie Yang [off-list ref];
Mark Brown [off-list ref]
Cc: linux-wireless@vger.kernel.org; netdev@vger.kernel.org; linux-
kernel@vger.kernel.org; alsa-devel@alsa-project.org
Subject: Re: [PATCH] cfg80211: Fix "suspicious RCU usage in
wiphy_apply_custom_regulatory" warning/backtrace

Hi,

On 1/5/21 10:24 AM, Peer, Ilan wrote:

quoted

Hi,

quoted

-----Original Message-----
From: Hans de Goede <redacted>
Sent: Monday, January 04, 2021 19:07
To: Johannes Berg <johannes@sipsolutions.net>; David S . Miller
[off-list ref]; Jakub Kicinski [off-list ref]; Rojewski,
Cezary [off-list ref]; Pierre-Louis Bossart <pierre-
louis.bossart@linux.intel.com>; Liam Girdwood
[off-list ref]; Jie Yang
[off-list ref]; Mark Brown [off-list ref]
Cc: Hans de Goede <redacted>; linux-
wireless@vger.kernel.org; netdev@vger.kernel.org; linux-
kernel@vger.kernel.org; alsa-devel@alsa-project.org; Peer, Ilan
[off-list ref]
Subject: [PATCH] cfg80211: Fix "suspicious RCU usage in
wiphy_apply_custom_regulatory" warning/backtrace

Commit beee24695157 ("cfg80211: Save the regulatory domain when
setting custom regulatory") adds a get_wiphy_regdom call to
wiphy_apply_custom_regulatory. But as the comment above
wiphy_apply_custom_regulatory says:
"/* Used by drivers prior to wiphy registration */"
this function is used by driver's probe function before the wiphy is
registered and at this point wiphy->regd will typically by NULL and
calling rcu_dereference_rtnl on a NULL pointer causes the following
warning/backtrace:

=============================
WARNING: suspicious RCU usage
5.11.0-rc1+ #19 Tainted: G        W
-----------------------------
net/wireless/reg.c:144 suspicious rcu_dereference_check() usage!

other info that might help us debug this:

rcu_scheduler_active = 2, debug_locks = 1
2 locks held by kworker/2:0/22:
 #0: ffff9a4bc104df38 ((wq_completion)events){+.+.}-{0:0}, at:
process_one_work+0x1ee/0x570
 #1: ffffb6e94010be78
((work_completion)(&fw_work->work)){+.+.}-{0:0},
at: process_one_work+0x1ee/0x570

stack backtrace:
CPU: 2 PID: 22 Comm: kworker/2:0 Tainted: G        W         5.11.0-rc1+ #19
Hardware name: LENOVO 60073/INVALID, BIOS 01WT17WW 08/01/2014
Workqueue: events request_firmware_work_func Call Trace:
 dump_stack+0x8b/0xb0
 get_wiphy_regdom+0x57/0x60 [cfg80211]
 wiphy_apply_custom_regulatory+0xa0/0xf0 [cfg80211]
 brcmf_cfg80211_attach+0xb02/0x1360 [brcmfmac]
 brcmf_attach+0x189/0x460 [brcmfmac]
 brcmf_sdio_firmware_callback+0x78a/0x8f0 [brcmfmac]
 brcmf_fw_request_done+0x67/0xf0 [brcmfmac]
 request_firmware_work_func+0x3d/0x70
 process_one_work+0x26e/0x570
 worker_thread+0x55/0x3c0
 ? process_one_work+0x570/0x570
 kthread+0x137/0x150
 ? __kthread_bind_mask+0x60/0x60
 ret_from_fork+0x22/0x30

Add a check for wiphy->regd being NULL before calling
get_wiphy_regdom (as is already done in other places) to fix this.

wiphy->regd will likely always be NULL when
wiphy->wiphy_apply_custom_regulatory
gets called, so arguably the tmp = get_wiphy_regdom() and
rcu_free_regdom(tmp) calls should simply be dropped, this patch keeps
the
2 calls, to allow drivers to call wiphy_apply_custom_regulatory more
then once if necessary.

Cc: Ilan Peer <redacted>
Fixes: beee24695157 ("cfg80211: Save the regulatory domain when
setting custom regulator")
Signed-off-by: Hans de Goede <redacted>
---
 net/wireless/reg.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c index

bb72447ad960..9254b9cbaa21 100644

--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c

@@ -2547,7 +2547,7 @@ static void handle_band_custom(struct wiphy

*wiphy,  void wiphy_apply_custom_regulatory(struct wiphy *wiphy,
 				   const struct ieee80211_regdomain *regd)  {
-	const struct ieee80211_regdomain *new_regd, *tmp;
+	const struct ieee80211_regdomain *new_regd, *tmp = NULL;
 	enum nl80211_band band;
 	unsigned int bands_set = 0;

@@ -2571,7 +2571,8 @@ void wiphy_apply_custom_regulatory(struct

wiphy

quoted

quoted

*wiphy,
 	if (IS_ERR(new_regd))
 		return;

-	tmp = get_wiphy_regdom(wiphy);
+	if (wiphy->regd)
+		tmp = get_wiphy_regdom(wiphy);
 	rcu_assign_pointer(wiphy->regd, new_regd);
 	rcu_free_regdom(tmp);

This only fixes the first case where the pointer in NULL and does not handle

the wrong RCU usage in other cases.

quoted

I'll prepare a fix for this.

Any luck with this? This is a regression in 5.11, so this really should be fixed in
a future 5.11-rc and the clock is running out.

Yes. The fix is ready. We'll send it.

Regards,

Ilan.