Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: SkHVsz57ov7p7pD4agS65koVxAnN0ZNRMi/k6FINPCF/0nE5v7u30voLgntK1dXkyehU4espuB
 U3fq17NoAHPQ==
IronPort-SDR: Pl8oQ3hM5bRdh7q1cTzekIo0rtCiGfdaQGg95udW8JwXyeBTJiXeyvFITPQn8g32/HbxJyJ3uO
 Y5F6AoASrpoA==
From:   "Peer, Ilan" <ilan.peer@intel.com>
To:     Hans de Goede <hdegoede@redhat.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" <linux-wireless@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>
Subject: RE: [PATCH] cfg80211: Fix "suspicious RCU usage in
 wiphy_apply_custom_regulatory" warning/backtrace
Thread-Topic: [PATCH] cfg80211: Fix "suspicious RCU usage in
 wiphy_apply_custom_regulatory" warning/backtrace
Thread-Index: AQHW4rwYhFgUlszMR0OBRT1c2rvcL6oYwsJQ
Date:   Tue, 5 Jan 2021 09:24:58 +0000
Message-ID: <BN7PR11MB2610DDFBC90739A9D1FAED52E9D10@BN7PR11MB2610.namprd11.prod.outlook.com>
References: <20210104170713.66956-1-hdegoede@redhat.com>
In-Reply-To: <20210104170713.66956-1-hdegoede@redhat.com>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.5.1.3
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 372527cb-7190-4a2c-e7e5-08d8b15bc5ac
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Jan 2021 09:24:58.8149
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: e8+AWDWfefuTXKA+N+GiJj3nZL4iOmdBLxNZgtMBpg7flDUOyu6PSi0RLl11Wt2Pn6E/HV/bW+fmMNXMz5sKUg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR11MB3810
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org

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
>=20
> 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 regi=
stered
> and at this point wiphy->regd will typically by NULL and calling
> rcu_dereference_rtnl on a NULL pointer causes the following
> warning/backtrace:
>=20
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
> WARNING: suspicious RCU usage
> 5.11.0-rc1+ #19 Tainted: G        W
> -----------------------------
> net/wireless/reg.c:144 suspicious rcu_dereference_check() usage!
>=20
> other info that might help us debug this:
>=20
> rcu_scheduler_active =3D 2, debug_locks =3D 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
>=20
> 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
>=20
> Add a check for wiphy->regd being NULL before calling get_wiphy_regdom
> (as is already done in other places) to fix this.
>=20
> wiphy->regd will likely always be NULL when
> wiphy->wiphy_apply_custom_regulatory
> gets called, so arguably the tmp =3D 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.
>=20
> 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(-)
>=20
> 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 =3D NULL;
>  	enum nl80211_band band;
>  	unsigned int bands_set =3D 0;
>=20
> @@ -2571,7 +2571,8 @@ void wiphy_apply_custom_regulatory(struct wiphy
> *wiphy,
>  	if (IS_ERR(new_regd))
>  		return;
>=20
> -	tmp =3D get_wiphy_regdom(wiphy);
> +	if (wiphy->regd)
> +		tmp =3D 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 handl=
e the wrong RCU usage in other cases.

I'll prepare a fix for this.

Thanks for addressing the bug,

Ilan.