| Message ID | 20220319224751.72241-1-dossche.niels@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFT] mwifiex: add mutex lock for call in mwifiex_dfs_chan_sw_work_queue | expand |
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 >
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 >>
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); }
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(-)