diff mbox series

cfg80211: Fix "suspicious RCU usage in wiphy_apply_custom_regulatory" warning/backtrace

Message ID 20210104170713.66956-1-hdegoede@redhat.com
State New
Headers show
Series cfg80211: Fix "suspicious RCU usage in wiphy_apply_custom_regulatory" warning/backtrace | expand

Commit Message

Hans de Goede Jan. 4, 2021, 5:07 p.m. UTC
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:

Comments

Peer, Ilan Jan. 5, 2021, 9:24 a.m. UTC | #1
Hi,

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Monday, January 04, 2021 19:07
> To: Johannes Berg <johannes@sipsolutions.net>; David S . Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Rojewski,
> Cezary <cezary.rojewski@intel.com>; Pierre-Louis Bossart <pierre-
> louis.bossart@linux.intel.com>; Liam Girdwood
> <liam.r.girdwood@linux.intel.com>; Jie Yang <yang.jie@linux.intel.com>;
> Mark Brown <broonie@kernel.org>
> Cc: Hans de Goede <hdegoede@redhat.com>; linux-
> wireless@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; alsa-devel@alsa-project.org; Peer, Ilan
> <ilan.peer@intel.com>
> 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@intel.com>
> Fixes: beee24695157 ("cfg80211: Save the regulatory domain when setting
> custom regulator")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  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.

Thanks for addressing the bug,

Ilan.
Hans de Goede Jan. 18, 2021, 9:09 p.m. UTC | #2
Hi,

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

> 

>> -----Original Message-----

>> From: Hans de Goede <hdegoede@redhat.com>

>> Sent: Monday, January 04, 2021 19:07

>> To: Johannes Berg <johannes@sipsolutions.net>; David S . Miller

>> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Rojewski,

>> Cezary <cezary.rojewski@intel.com>; Pierre-Louis Bossart <pierre-

>> louis.bossart@linux.intel.com>; Liam Girdwood

>> <liam.r.girdwood@linux.intel.com>; Jie Yang <yang.jie@linux.intel.com>;

>> Mark Brown <broonie@kernel.org>

>> Cc: Hans de Goede <hdegoede@redhat.com>; linux-

>> wireless@vger.kernel.org; netdev@vger.kernel.org; linux-

>> kernel@vger.kernel.org; alsa-devel@alsa-project.org; Peer, Ilan

>> <ilan.peer@intel.com>

>> 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@intel.com>

>> Fixes: beee24695157 ("cfg80211: Save the regulatory domain when setting

>> custom regulator")

>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

>> ---

>>  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
Peer, Ilan Jan. 19, 2021, 3:24 p.m. UTC | #3
Hi,

This fix can be found here. It was merged to the mac80211 tree.

https://patchwork.kernel.org/project/linux-wireless/patch/iwlwifi.20210105165657.613e9a876829.Ia38d27dbebea28bf9c56d70691d243186ede70e7@changeid/

Regards,

Ilan.

> -----Original Message-----

> From: Hans de Goede <hdegoede@redhat.com>

> Sent: Monday, January 18, 2021 23:09

> To: Peer, Ilan <ilan.peer@intel.com>; Johannes Berg

> <johannes@sipsolutions.net>; David S . Miller <davem@davemloft.net>;

> Jakub Kicinski <kuba@kernel.org>; Rojewski, Cezary

> <cezary.rojewski@intel.com>; Pierre-Louis Bossart <pierre-

> louis.bossart@linux.intel.com>; Liam Girdwood

> <liam.r.girdwood@linux.intel.com>; Jie Yang <yang.jie@linux.intel.com>;

> Mark Brown <broonie@kernel.org>

> 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:

> > Hi,

> >

> >> -----Original Message-----

> >> From: Hans de Goede <hdegoede@redhat.com>

> >> Sent: Monday, January 04, 2021 19:07

> >> To: Johannes Berg <johannes@sipsolutions.net>; David S . Miller

> >> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Rojewski,

> >> Cezary <cezary.rojewski@intel.com>; Pierre-Louis Bossart <pierre-

> >> louis.bossart@linux.intel.com>; Liam Girdwood

> >> <liam.r.girdwood@linux.intel.com>; Jie Yang

> >> <yang.jie@linux.intel.com>; Mark Brown <broonie@kernel.org>

> >> Cc: Hans de Goede <hdegoede@redhat.com>; linux-

> >> wireless@vger.kernel.org; netdev@vger.kernel.org; linux-

> >> kernel@vger.kernel.org; alsa-devel@alsa-project.org; Peer, Ilan

> >> <ilan.peer@intel.com>

> >> 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@intel.com>

> >> Fixes: beee24695157 ("cfg80211: Save the regulatory domain when

> >> setting custom regulator")

> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

> >> ---

> >>  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
diff mbox series

Patch

=============================
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_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@intel.com>
Fixes: beee24695157 ("cfg80211: Save the regulatory domain when setting custom regulator")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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);
 }