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

[Hans de Goede]

Hi,

On 1/5/21 10:24 AM, Peer, Ilan wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@xxxxxxxxxx>
>> Sent: Monday, January 04, 2021 19:07
>> To: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>; David S . Miller
>> <davem@xxxxxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Rojewski,
>> Cezary <cezary.rojewski@xxxxxxxxx>; Pierre-Louis Bossart <pierre-
>> louis.bossart@xxxxxxxxxxxxxxx>; Liam Girdwood
>> <liam.r.girdwood@xxxxxxxxxxxxxxx>; Jie Yang <yang.jie@xxxxxxxxxxxxxxx>;
>> Mark Brown <broonie@xxxxxxxxxx>
>> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>; linux-
>> wireless@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
>> kernel@xxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; Peer, Ilan
>> <ilan.peer@xxxxxxxxx>
>> 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 <ilan.peer@xxxxxxxxx>
>> Fixes: beee24695157 ("cfg80211: Save the regulatory domain when setting
>> custom regulator")
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>>  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
>> *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.
> 
> 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.

Regards,

Hans