[PATCH RFT] mwifiex: add mutex lock for call in mwifiex_dfs_chan_sw_work_queue

[PATCH RFT] mwifiex: add mutex lock for call in mwifiex_dfs_chan_sw_work_queue

[Niels Dossche]

cfg80211_ch_switch_notify uses ASSERT_WDEV_LOCK to assert that
net_device->ieee80211_ptr->mtx is held during the function's execution.
mwifiex_dfs_chan_sw_work_queue is one of its callers, which does not
hold that lock, therefore violating the assertion.
Add a lock around the call.

Disclaimer:
I am currently working on a static analyser to detect missing locks.
This was a reported case. I manually verified the report by looking
at the code, so that I do not send wrong information or patches.
After concluding that this seems to be a true positive, I created
this patch.
However, as I do not in fact have this particular hardware,
I was unable to test it.

Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
---
 drivers/net/wireless/marvell/mwifiex/11h.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/11h.c b/drivers/net/wireless/marvell/mwifiex/11h.c
index d2ee6469e67b..f2cba764024e 100644
--- a/drivers/net/wireless/marvell/mwifiex/11h.c
+++ b/drivers/net/wireless/marvell/mwifiex/11h.c
@@ -285,6 +285,7 @@ void mwifiex_dfs_chan_sw_work_queue(struct work_struct *work)
 	struct mwifiex_private *priv =
 			container_of(delayed_work, struct mwifiex_private,
 				     dfs_chan_sw_work);
+	struct net_device *netdev;
 
 	bss_cfg = &priv->bss_cfg;
 	if (!bss_cfg->beacon_period) {
@@ -301,7 +302,11 @@ void mwifiex_dfs_chan_sw_work_queue(struct work_struct *work)
 		return;
 	}
 
+	netdev = priv->netdev;
+
 	mwifiex_dbg(priv->adapter, MSG,
 		    "indicating channel switch completion to kernel\n");
-	cfg80211_ch_switch_notify(priv->netdev, &priv->dfs_chandef);
+	mutex_lock(&netdev->ieee80211_ptr->mtx);
+	cfg80211_ch_switch_notify(netdev, &priv->dfs_chandef);
+	mutex_unlock(&netdev->ieee80211_ptr->mtx);
 }
-- 
2.35.1

Re: [PATCH RFT] mwifiex: add mutex lock for call in mwifiex_dfs_chan_sw_work_queue

[Brian Norris]

On Sat, Mar 19, 2022 at 11:47:52PM +0100, Niels Dossche wrote:
> --- a/drivers/net/wireless/marvell/mwifiex/11h.c
> +++ b/drivers/net/wireless/marvell/mwifiex/11h.c
> @@ -285,6 +285,7 @@ void mwifiex_dfs_chan_sw_work_queue(struct work_struct *work)
>  	struct mwifiex_private *priv =
>  			container_of(delayed_work, struct mwifiex_private,
>  				     dfs_chan_sw_work);
> +	struct net_device *netdev;
>  
>  	bss_cfg = &priv->bss_cfg;
>  	if (!bss_cfg->beacon_period) {
> @@ -301,7 +302,11 @@ void mwifiex_dfs_chan_sw_work_queue(struct work_struct *work)
>  		return;
>  	}
>  
> +	netdev = priv->netdev;
> +
>  	mwifiex_dbg(priv->adapter, MSG,
>  		    "indicating channel switch completion to kernel\n");
> -	cfg80211_ch_switch_notify(priv->netdev, &priv->dfs_chandef);
> +	mutex_lock(&netdev->ieee80211_ptr->mtx);

A more appropriate route to this object might be priv->wdev.mtx. But
otherwise, I think this makes sense, and matches what
ath6kl_cfg80211_ch_switch_notify() and qtnf_event_handle_freq_change()
do. With the suggested change:

Reviewed-by: Brian Norris <briannorris@chromium.org>

Thanks.

> +	cfg80211_ch_switch_notify(netdev, &priv->dfs_chandef);
> +	mutex_unlock(&netdev->ieee80211_ptr->mtx);
>  }
> -- 
> 2.35.1
> 

Re: [PATCH RFT] mwifiex: add mutex lock for call in mwifiex_dfs_chan_sw_work_queue

[Niels Dossche]

On 21/03/2022 21:17, Brian Norris wrote:
> On Sat, Mar 19, 2022 at 11:47:52PM +0100, Niels Dossche wrote:
>> --- a/drivers/net/wireless/marvell/mwifiex/11h.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/11h.c
>> @@ -285,6 +285,7 @@ void mwifiex_dfs_chan_sw_work_queue(struct work_struct *work)
>>  	struct mwifiex_private *priv =
>>  			container_of(delayed_work, struct mwifiex_private,
>>  				     dfs_chan_sw_work);
>> +	struct net_device *netdev;
>>  
>>  	bss_cfg = &priv->bss_cfg;
>>  	if (!bss_cfg->beacon_period) {
>> @@ -301,7 +302,11 @@ void mwifiex_dfs_chan_sw_work_queue(struct work_struct *work)
>>  		return;
>>  	}
>>  
>> +	netdev = priv->netdev;
>> +
>>  	mwifiex_dbg(priv->adapter, MSG,
>>  		    "indicating channel switch completion to kernel\n");
>> -	cfg80211_ch_switch_notify(priv->netdev, &priv->dfs_chandef);
>> +	mutex_lock(&netdev->ieee80211_ptr->mtx);
> 
> A more appropriate route to this object might be priv->wdev.mtx. But
> otherwise, I think this makes sense, and matches what
> ath6kl_cfg80211_ch_switch_notify() and qtnf_event_handle_freq_change()
> do. With the suggested change:

Thanks for the review, will change that and send a v2.

> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> 
> Thanks.
> 
>> +	cfg80211_ch_switch_notify(netdev, &priv->dfs_chandef);
>> +	mutex_unlock(&netdev->ieee80211_ptr->mtx);
>>  }
>> -- 
>> 2.35.1
>>