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 <[email protected]>
---
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
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 <[email protected]>
Thanks.
> + cfg80211_ch_switch_notify(netdev, &priv->dfs_chandef);
> + mutex_unlock(&netdev->ieee80211_ptr->mtx);
> }
> --
> 2.35.1
>
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 <[email protected]>
>
> Thanks.
>
>> + cfg80211_ch_switch_notify(netdev, &priv->dfs_chandef);
>> + mutex_unlock(&netdev->ieee80211_ptr->mtx);
>> }
>> --
>> 2.35.1
>>