* [PATCH v2] mac80211: fix race in ieee80211_register_hw()
@ 2020-04-07 10:10 Sumit Garg
2020-04-07 19:25 ` Krishna Chaitanya
0 siblings, 1 reply; 8+ messages in thread
From: Sumit Garg @ 2020-04-07 10:10 UTC (permalink / raw)
To: linux-wireless, johannes
Cc: davem, kuba, kvalo, chaitanya.mgit, netdev, linux-kernel,
matthias.schoepfer, Philipp.Berg, Michael.Weitner,
daniel.thompson, loic.poulain, Sumit Garg, stable
A race condition leading to a kernel crash is observed during invocation
of ieee80211_register_hw() on a dragonboard410c device having wcn36xx
driver built as a loadable module along with a wifi manager in user-space
waiting for a wifi device (wlanX) to be active.
Sequence diagram for a particular kernel crash scenario:
user-space ieee80211_register_hw() ieee80211_tasklet_handler()
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
| | |
|<---phy0----wiphy_register() |
|-----iwd if_add---->| |
| |<---IRQ----(RX packet)
| Kernel crash |
| due to unallocated |
| workqueue. |
| | |
| alloc_ordered_workqueue() |
| | |
| Misc wiphy init. |
| | |
| ieee80211_if_add() |
| | |
As evident from above sequence diagram, this race condition isn't specific
to a particular wifi driver but rather the initialization sequence in
ieee80211_register_hw() needs to be fixed. So re-order the initialization
sequence and the updated sequence diagram would look like:
user-space ieee80211_register_hw() ieee80211_tasklet_handler()
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
| | |
| alloc_ordered_workqueue() |
| | |
| Misc wiphy init. |
| | |
|<---phy0----wiphy_register() |
|-----iwd if_add---->| |
| |<---IRQ----(RX packet)
| | |
| ieee80211_if_add() |
| | |
Cc: <stable@vger.kernel.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
Changes in v2:
- Move rtnl_unlock() just after ieee80211_init_rate_ctrl_alg().
- Update sequence diagrams in commit message for more clarification.
net/mac80211/main.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 4c2b5ba..d497129 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1051,7 +1051,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
local->hw.wiphy->signal_type = CFG80211_SIGNAL_TYPE_UNSPEC;
if (hw->max_signal <= 0) {
result = -EINVAL;
- goto fail_wiphy_register;
+ goto fail_workqueue;
}
}
@@ -1113,7 +1113,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
result = ieee80211_init_cipher_suites(local);
if (result < 0)
- goto fail_wiphy_register;
+ goto fail_workqueue;
if (!local->ops->remain_on_channel)
local->hw.wiphy->max_remain_on_channel_duration = 5000;
@@ -1139,10 +1139,6 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
local->hw.wiphy->max_num_csa_counters = IEEE80211_MAX_CSA_COUNTERS_NUM;
- result = wiphy_register(local->hw.wiphy);
- if (result < 0)
- goto fail_wiphy_register;
-
/*
* We use the number of queues for feature tests (QoS, HT) internally
* so restrict them appropriately.
@@ -1207,6 +1203,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
goto fail_rate;
}
+ rtnl_unlock();
+
if (local->rate_ctrl) {
clear_bit(IEEE80211_HW_SUPPORTS_VHT_EXT_NSS_BW, hw->flags);
if (local->rate_ctrl->ops->capa & RATE_CTRL_CAPA_VHT_EXT_NSS_BW)
@@ -1254,6 +1252,12 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
local->sband_allocated |= BIT(band);
}
+ result = wiphy_register(local->hw.wiphy);
+ if (result < 0)
+ goto fail_wiphy_register;
+
+ rtnl_lock();
+
/* add one default STA interface if supported */
if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_STATION) &&
!ieee80211_hw_check(hw, NO_AUTO_VIF)) {
@@ -1293,6 +1297,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
#if defined(CONFIG_INET) || defined(CONFIG_IPV6)
fail_ifa:
#endif
+ wiphy_unregister(local->hw.wiphy);
+ fail_wiphy_register:
rtnl_lock();
rate_control_deinitialize(local);
ieee80211_remove_interfaces(local);
@@ -1302,8 +1308,6 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
ieee80211_led_exit(local);
destroy_workqueue(local->workqueue);
fail_workqueue:
- wiphy_unregister(local->hw.wiphy);
- fail_wiphy_register:
if (local->wiphy_ciphers_allocated)
kfree(local->hw.wiphy->cipher_suites);
kfree(local->int_scan_req);
@@ -1353,8 +1357,8 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
skb_queue_purge(&local->skb_queue_unreliable);
skb_queue_purge(&local->skb_queue_tdls_chsw);
- destroy_workqueue(local->workqueue);
wiphy_unregister(local->hw.wiphy);
+ destroy_workqueue(local->workqueue);
ieee80211_led_exit(local);
kfree(local->int_scan_req);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mac80211: fix race in ieee80211_register_hw()
2020-04-07 10:10 [PATCH v2] mac80211: fix race in ieee80211_register_hw() Sumit Garg
@ 2020-04-07 19:25 ` Krishna Chaitanya
2020-04-09 4:42 ` Sumit Garg
0 siblings, 1 reply; 8+ messages in thread
From: Krishna Chaitanya @ 2020-04-07 19:25 UTC (permalink / raw)
To: Sumit Garg
Cc: linux-wireless, Johannes Berg, David S. Miller, kuba, Kalle Valo,
netdev, Linux Kernel Mailing List, Matthias-Peter Schöpfer,
Berg Philipp (HAU-EDS), Weitner Michael (HAU-EDS),
Daniel Thompson, Loic Poulain, stable
On Tue, Apr 7, 2020 at 3:41 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> A race condition leading to a kernel crash is observed during invocation
> of ieee80211_register_hw() on a dragonboard410c device having wcn36xx
> driver built as a loadable module along with a wifi manager in user-space
> waiting for a wifi device (wlanX) to be active.
>
> Sequence diagram for a particular kernel crash scenario:
>
> user-space ieee80211_register_hw() ieee80211_tasklet_handler()
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> | | |
> |<---phy0----wiphy_register() |
> |-----iwd if_add---->| |
just a nitpick, a better one would be (iwd: if_add + ap_start) since
we need to have 'iwctl ap start'
to trigger the interrupts.
> | |<---IRQ----(RX packet)
> | Kernel crash |
> | due to unallocated |
> | workqueue. |
> | | |
> | alloc_ordered_workqueue() |
> | | |
> | Misc wiphy init. |
> | | |
> | ieee80211_if_add() |
> | | |
>
> As evident from above sequence diagram, this race condition isn't specific
> to a particular wifi driver but rather the initialization sequence in
> ieee80211_register_hw() needs to be fixed. So re-order the initialization
> sequence and the updated sequence diagram would look like:
>
> user-space ieee80211_register_hw() ieee80211_tasklet_handler()
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> | | |
> | alloc_ordered_workqueue() |
> | | |
> | Misc wiphy init. |
> | | |
> |<---phy0----wiphy_register() |
> |-----iwd if_add---->| |
same as above.
> | |<---IRQ----(RX packet)
> | | |
> | ieee80211_if_add() |
> | | |
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>
> Changes in v2:
> - Move rtnl_unlock() just after ieee80211_init_rate_ctrl_alg().
> - Update sequence diagrams in commit message for more clarification.
>
> net/mac80211/main.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index 4c2b5ba..d497129 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -1051,7 +1051,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> local->hw.wiphy->signal_type = CFG80211_SIGNAL_TYPE_UNSPEC;
> if (hw->max_signal <= 0) {
> result = -EINVAL;
> - goto fail_wiphy_register;
> + goto fail_workqueue;
> }
> }
>
> @@ -1113,7 +1113,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
>
> result = ieee80211_init_cipher_suites(local);
> if (result < 0)
> - goto fail_wiphy_register;
> + goto fail_workqueue;
>
> if (!local->ops->remain_on_channel)
> local->hw.wiphy->max_remain_on_channel_duration = 5000;
> @@ -1139,10 +1139,6 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
>
> local->hw.wiphy->max_num_csa_counters = IEEE80211_MAX_CSA_COUNTERS_NUM;
>
> - result = wiphy_register(local->hw.wiphy);
> - if (result < 0)
> - goto fail_wiphy_register;
> -
> /*
> * We use the number of queues for feature tests (QoS, HT) internally
> * so restrict them appropriately.
> @@ -1207,6 +1203,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> goto fail_rate;
> }
>
> + rtnl_unlock();
> +
> if (local->rate_ctrl) {
> clear_bit(IEEE80211_HW_SUPPORTS_VHT_EXT_NSS_BW, hw->flags);
> if (local->rate_ctrl->ops->capa & RATE_CTRL_CAPA_VHT_EXT_NSS_BW)
> @@ -1254,6 +1252,12 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> local->sband_allocated |= BIT(band);
> }
>
> + result = wiphy_register(local->hw.wiphy);
> + if (result < 0)
> + goto fail_wiphy_register;
> +
> + rtnl_lock();
> +
> /* add one default STA interface if supported */
> if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_STATION) &&
> !ieee80211_hw_check(hw, NO_AUTO_VIF)) {
> @@ -1293,6 +1297,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> #if defined(CONFIG_INET) || defined(CONFIG_IPV6)
> fail_ifa:
> #endif
> + wiphy_unregister(local->hw.wiphy);
> + fail_wiphy_register:
> rtnl_lock();
> rate_control_deinitialize(local);
> ieee80211_remove_interfaces(local);
> @@ -1302,8 +1308,6 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> ieee80211_led_exit(local);
> destroy_workqueue(local->workqueue);
> fail_workqueue:
> - wiphy_unregister(local->hw.wiphy);
> - fail_wiphy_register:
> if (local->wiphy_ciphers_allocated)
> kfree(local->hw.wiphy->cipher_suites);
> kfree(local->int_scan_req);
> @@ -1353,8 +1357,8 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
> skb_queue_purge(&local->skb_queue_unreliable);
> skb_queue_purge(&local->skb_queue_tdls_chsw);
>
> - destroy_workqueue(local->workqueue);
> wiphy_unregister(local->hw.wiphy);
> + destroy_workqueue(local->workqueue);
> ieee80211_led_exit(local);
> kfree(local->int_scan_req);
> }
> --
> 2.7.4
>
--
Thanks,
Regards,
Chaitanya T K.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mac80211: fix race in ieee80211_register_hw()
2020-04-07 19:25 ` Krishna Chaitanya
@ 2020-04-09 4:42 ` Sumit Garg
2020-04-09 4:44 ` Sedat Dilek
2020-04-15 11:35 ` Sumit Garg
0 siblings, 2 replies; 8+ messages in thread
From: Sumit Garg @ 2020-04-09 4:42 UTC (permalink / raw)
To: Johannes Berg
Cc: linux-wireless, Krishna Chaitanya, David S. Miller, kuba,
Kalle Valo, netdev, Linux Kernel Mailing List,
Matthias-Peter Schöpfer, Berg Philipp (HAU-EDS),
Weitner Michael (HAU-EDS),
Daniel Thompson, Loic Poulain, stable
Hi Johannes,
On Wed, 8 Apr 2020 at 00:55, Krishna Chaitanya <chaitanya.mgit@gmail.com> wrote:
>
> On Tue, Apr 7, 2020 at 3:41 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > A race condition leading to a kernel crash is observed during invocation
> > of ieee80211_register_hw() on a dragonboard410c device having wcn36xx
> > driver built as a loadable module along with a wifi manager in user-space
> > waiting for a wifi device (wlanX) to be active.
> >
> > Sequence diagram for a particular kernel crash scenario:
> >
> > user-space ieee80211_register_hw() ieee80211_tasklet_handler()
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > | | |
> > |<---phy0----wiphy_register() |
> > |-----iwd if_add---->| |
> just a nitpick, a better one would be (iwd: if_add + ap_start) since
> we need to have 'iwctl ap start'
> to trigger the interrupts.
> > | |<---IRQ----(RX packet)
> > | Kernel crash |
> > | due to unallocated |
> > | workqueue. |
> > | | |
> > | alloc_ordered_workqueue() |
> > | | |
> > | Misc wiphy init. |
> > | | |
> > | ieee80211_if_add() |
> > | | |
> >
> > As evident from above sequence diagram, this race condition isn't specific
> > to a particular wifi driver but rather the initialization sequence in
> > ieee80211_register_hw() needs to be fixed. So re-order the initialization
> > sequence and the updated sequence diagram would look like:
> >
> > user-space ieee80211_register_hw() ieee80211_tasklet_handler()
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > | | |
> > | alloc_ordered_workqueue() |
> > | | |
> > | Misc wiphy init. |
> > | | |
> > |<---phy0----wiphy_register() |
> > |-----iwd if_add---->| |
> same as above.
> > | |<---IRQ----(RX packet)
> > | | |
> > | ieee80211_if_add() |
> > | | |
> >
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >
In case we don't have any further comments, could you fix this nitpick
from Chaitanya while applying or would you like me to respin and send
v3?
-Sumit
> > Changes in v2:
> > - Move rtnl_unlock() just after ieee80211_init_rate_ctrl_alg().
> > - Update sequence diagrams in commit message for more clarification.
> >
> > net/mac80211/main.c | 22 +++++++++++++---------
> > 1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> > index 4c2b5ba..d497129 100644
> > --- a/net/mac80211/main.c
> > +++ b/net/mac80211/main.c
> > @@ -1051,7 +1051,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > local->hw.wiphy->signal_type = CFG80211_SIGNAL_TYPE_UNSPEC;
> > if (hw->max_signal <= 0) {
> > result = -EINVAL;
> > - goto fail_wiphy_register;
> > + goto fail_workqueue;
> > }
> > }
> >
> > @@ -1113,7 +1113,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> >
> > result = ieee80211_init_cipher_suites(local);
> > if (result < 0)
> > - goto fail_wiphy_register;
> > + goto fail_workqueue;
> >
> > if (!local->ops->remain_on_channel)
> > local->hw.wiphy->max_remain_on_channel_duration = 5000;
> > @@ -1139,10 +1139,6 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> >
> > local->hw.wiphy->max_num_csa_counters = IEEE80211_MAX_CSA_COUNTERS_NUM;
> >
> > - result = wiphy_register(local->hw.wiphy);
> > - if (result < 0)
> > - goto fail_wiphy_register;
> > -
> > /*
> > * We use the number of queues for feature tests (QoS, HT) internally
> > * so restrict them appropriately.
> > @@ -1207,6 +1203,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > goto fail_rate;
> > }
> >
> > + rtnl_unlock();
> > +
> > if (local->rate_ctrl) {
> > clear_bit(IEEE80211_HW_SUPPORTS_VHT_EXT_NSS_BW, hw->flags);
> > if (local->rate_ctrl->ops->capa & RATE_CTRL_CAPA_VHT_EXT_NSS_BW)
> > @@ -1254,6 +1252,12 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > local->sband_allocated |= BIT(band);
> > }
> >
> > + result = wiphy_register(local->hw.wiphy);
> > + if (result < 0)
> > + goto fail_wiphy_register;
> > +
> > + rtnl_lock();
> > +
> > /* add one default STA interface if supported */
> > if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_STATION) &&
> > !ieee80211_hw_check(hw, NO_AUTO_VIF)) {
> > @@ -1293,6 +1297,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > #if defined(CONFIG_INET) || defined(CONFIG_IPV6)
> > fail_ifa:
> > #endif
> > + wiphy_unregister(local->hw.wiphy);
> > + fail_wiphy_register:
> > rtnl_lock();
> > rate_control_deinitialize(local);
> > ieee80211_remove_interfaces(local);
> > @@ -1302,8 +1308,6 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > ieee80211_led_exit(local);
> > destroy_workqueue(local->workqueue);
> > fail_workqueue:
> > - wiphy_unregister(local->hw.wiphy);
> > - fail_wiphy_register:
> > if (local->wiphy_ciphers_allocated)
> > kfree(local->hw.wiphy->cipher_suites);
> > kfree(local->int_scan_req);
> > @@ -1353,8 +1357,8 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
> > skb_queue_purge(&local->skb_queue_unreliable);
> > skb_queue_purge(&local->skb_queue_tdls_chsw);
> >
> > - destroy_workqueue(local->workqueue);
> > wiphy_unregister(local->hw.wiphy);
> > + destroy_workqueue(local->workqueue);
> > ieee80211_led_exit(local);
> > kfree(local->int_scan_req);
> > }
> > --
> > 2.7.4
> >
>
>
> --
> Thanks,
> Regards,
> Chaitanya T K.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mac80211: fix race in ieee80211_register_hw()
2020-04-09 4:42 ` Sumit Garg
@ 2020-04-09 4:44 ` Sedat Dilek
2020-04-15 11:35 ` Sumit Garg
1 sibling, 0 replies; 8+ messages in thread
From: Sedat Dilek @ 2020-04-09 4:44 UTC (permalink / raw)
To: Sumit Garg
Cc: Johannes Berg, linux-wireless, Krishna Chaitanya,
David S. Miller, kuba, Kalle Valo, netdev,
Linux Kernel Mailing List, Matthias-Peter Schöpfer,
Berg Philipp (HAU-EDS), Weitner Michael (HAU-EDS),
Daniel Thompson, Loic Poulain, stable
On Thu, Apr 9, 2020 at 6:43 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Johannes,
>
> On Wed, 8 Apr 2020 at 00:55, Krishna Chaitanya <chaitanya.mgit@gmail.com> wrote:
> >
> > On Tue, Apr 7, 2020 at 3:41 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > A race condition leading to a kernel crash is observed during invocation
> > > of ieee80211_register_hw() on a dragonboard410c device having wcn36xx
> > > driver built as a loadable module along with a wifi manager in user-space
> > > waiting for a wifi device (wlanX) to be active.
> > >
> > > Sequence diagram for a particular kernel crash scenario:
> > >
> > > user-space ieee80211_register_hw() ieee80211_tasklet_handler()
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > | | |
> > > |<---phy0----wiphy_register() |
> > > |-----iwd if_add---->| |
> > just a nitpick, a better one would be (iwd: if_add + ap_start) since
> > we need to have 'iwctl ap start'
> > to trigger the interrupts.
> > > | |<---IRQ----(RX packet)
> > > | Kernel crash |
> > > | due to unallocated |
> > > | workqueue. |
> > > | | |
> > > | alloc_ordered_workqueue() |
> > > | | |
> > > | Misc wiphy init. |
> > > | | |
> > > | ieee80211_if_add() |
> > > | | |
> > >
> > > As evident from above sequence diagram, this race condition isn't specific
> > > to a particular wifi driver but rather the initialization sequence in
> > > ieee80211_register_hw() needs to be fixed. So re-order the initialization
> > > sequence and the updated sequence diagram would look like:
> > >
> > > user-space ieee80211_register_hw() ieee80211_tasklet_handler()
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > | | |
> > > | alloc_ordered_workqueue() |
> > > | | |
> > > | Misc wiphy init. |
> > > | | |
> > > |<---phy0----wiphy_register() |
> > > |-----iwd if_add---->| |
> > same as above.
> > > | |<---IRQ----(RX packet)
> > > | | |
> > > | ieee80211_if_add() |
> > > | | |
> > >
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > ---
> > >
>
> In case we don't have any further comments, could you fix this nitpick
> from Chaitanya while applying or would you like me to respin and send
> v3?
>
Hi Sumit,
when you send out a v3, feel free to add...
Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
Thanks.
Regards,
- Sedat -
> -Sumit
>
> > > Changes in v2:
> > > - Move rtnl_unlock() just after ieee80211_init_rate_ctrl_alg().
> > > - Update sequence diagrams in commit message for more clarification.
> > >
> > > net/mac80211/main.c | 22 +++++++++++++---------
> > > 1 file changed, 13 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> > > index 4c2b5ba..d497129 100644
> > > --- a/net/mac80211/main.c
> > > +++ b/net/mac80211/main.c
> > > @@ -1051,7 +1051,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > > local->hw.wiphy->signal_type = CFG80211_SIGNAL_TYPE_UNSPEC;
> > > if (hw->max_signal <= 0) {
> > > result = -EINVAL;
> > > - goto fail_wiphy_register;
> > > + goto fail_workqueue;
> > > }
> > > }
> > >
> > > @@ -1113,7 +1113,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > >
> > > result = ieee80211_init_cipher_suites(local);
> > > if (result < 0)
> > > - goto fail_wiphy_register;
> > > + goto fail_workqueue;
> > >
> > > if (!local->ops->remain_on_channel)
> > > local->hw.wiphy->max_remain_on_channel_duration = 5000;
> > > @@ -1139,10 +1139,6 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > >
> > > local->hw.wiphy->max_num_csa_counters = IEEE80211_MAX_CSA_COUNTERS_NUM;
> > >
> > > - result = wiphy_register(local->hw.wiphy);
> > > - if (result < 0)
> > > - goto fail_wiphy_register;
> > > -
> > > /*
> > > * We use the number of queues for feature tests (QoS, HT) internally
> > > * so restrict them appropriately.
> > > @@ -1207,6 +1203,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > > goto fail_rate;
> > > }
> > >
> > > + rtnl_unlock();
> > > +
> > > if (local->rate_ctrl) {
> > > clear_bit(IEEE80211_HW_SUPPORTS_VHT_EXT_NSS_BW, hw->flags);
> > > if (local->rate_ctrl->ops->capa & RATE_CTRL_CAPA_VHT_EXT_NSS_BW)
> > > @@ -1254,6 +1252,12 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > > local->sband_allocated |= BIT(band);
> > > }
> > >
> > > + result = wiphy_register(local->hw.wiphy);
> > > + if (result < 0)
> > > + goto fail_wiphy_register;
> > > +
> > > + rtnl_lock();
> > > +
> > > /* add one default STA interface if supported */
> > > if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_STATION) &&
> > > !ieee80211_hw_check(hw, NO_AUTO_VIF)) {
> > > @@ -1293,6 +1297,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > > #if defined(CONFIG_INET) || defined(CONFIG_IPV6)
> > > fail_ifa:
> > > #endif
> > > + wiphy_unregister(local->hw.wiphy);
> > > + fail_wiphy_register:
> > > rtnl_lock();
> > > rate_control_deinitialize(local);
> > > ieee80211_remove_interfaces(local);
> > > @@ -1302,8 +1308,6 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > > ieee80211_led_exit(local);
> > > destroy_workqueue(local->workqueue);
> > > fail_workqueue:
> > > - wiphy_unregister(local->hw.wiphy);
> > > - fail_wiphy_register:
> > > if (local->wiphy_ciphers_allocated)
> > > kfree(local->hw.wiphy->cipher_suites);
> > > kfree(local->int_scan_req);
> > > @@ -1353,8 +1357,8 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
> > > skb_queue_purge(&local->skb_queue_unreliable);
> > > skb_queue_purge(&local->skb_queue_tdls_chsw);
> > >
> > > - destroy_workqueue(local->workqueue);
> > > wiphy_unregister(local->hw.wiphy);
> > > + destroy_workqueue(local->workqueue);
> > > ieee80211_led_exit(local);
> > > kfree(local->int_scan_req);
> > > }
> > > --
> > > 2.7.4
> > >
> >
> >
> > --
> > Thanks,
> > Regards,
> > Chaitanya T K.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mac80211: fix race in ieee80211_register_hw()
2020-04-09 4:42 ` Sumit Garg
2020-04-09 4:44 ` Sedat Dilek
@ 2020-04-15 11:35 ` Sumit Garg
2020-04-15 13:19 ` Sedat Dilek
1 sibling, 1 reply; 8+ messages in thread
From: Sumit Garg @ 2020-04-15 11:35 UTC (permalink / raw)
To: Johannes Berg
Cc: linux-wireless, Krishna Chaitanya, David S. Miller, kuba,
Kalle Valo, netdev, Linux Kernel Mailing List,
Matthias-Peter Schöpfer, Berg Philipp (HAU-EDS),
Weitner Michael (HAU-EDS),
Daniel Thompson, Loic Poulain, stable
On Thu, 9 Apr 2020 at 10:12, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Johannes,
>
> On Wed, 8 Apr 2020 at 00:55, Krishna Chaitanya <chaitanya.mgit@gmail.com> wrote:
> >
> > On Tue, Apr 7, 2020 at 3:41 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > A race condition leading to a kernel crash is observed during invocation
> > > of ieee80211_register_hw() on a dragonboard410c device having wcn36xx
> > > driver built as a loadable module along with a wifi manager in user-space
> > > waiting for a wifi device (wlanX) to be active.
> > >
> > > Sequence diagram for a particular kernel crash scenario:
> > >
> > > user-space ieee80211_register_hw() ieee80211_tasklet_handler()
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > | | |
> > > |<---phy0----wiphy_register() |
> > > |-----iwd if_add---->| |
> > just a nitpick, a better one would be (iwd: if_add + ap_start) since
> > we need to have 'iwctl ap start'
> > to trigger the interrupts.
> > > | |<---IRQ----(RX packet)
> > > | Kernel crash |
> > > | due to unallocated |
> > > | workqueue. |
> > > | | |
> > > | alloc_ordered_workqueue() |
> > > | | |
> > > | Misc wiphy init. |
> > > | | |
> > > | ieee80211_if_add() |
> > > | | |
> > >
> > > As evident from above sequence diagram, this race condition isn't specific
> > > to a particular wifi driver but rather the initialization sequence in
> > > ieee80211_register_hw() needs to be fixed. So re-order the initialization
> > > sequence and the updated sequence diagram would look like:
> > >
> > > user-space ieee80211_register_hw() ieee80211_tasklet_handler()
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > | | |
> > > | alloc_ordered_workqueue() |
> > > | | |
> > > | Misc wiphy init. |
> > > | | |
> > > |<---phy0----wiphy_register() |
> > > |-----iwd if_add---->| |
> > same as above.
> > > | |<---IRQ----(RX packet)
> > > | | |
> > > | ieee80211_if_add() |
> > > | | |
> > >
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > ---
> > >
>
> In case we don't have any further comments, could you fix this nitpick
> from Chaitanya while applying or would you like me to respin and send
> v3?
A gentle ping. Is this patch a good candidate for 5.7-rc2?
-Sumit
>
> -Sumit
>
> > > Changes in v2:
> > > - Move rtnl_unlock() just after ieee80211_init_rate_ctrl_alg().
> > > - Update sequence diagrams in commit message for more clarification.
> > >
> > > net/mac80211/main.c | 22 +++++++++++++---------
> > > 1 file changed, 13 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> > > index 4c2b5ba..d497129 100644
> > > --- a/net/mac80211/main.c
> > > +++ b/net/mac80211/main.c
> > > @@ -1051,7 +1051,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > > local->hw.wiphy->signal_type = CFG80211_SIGNAL_TYPE_UNSPEC;
> > > if (hw->max_signal <= 0) {
> > > result = -EINVAL;
> > > - goto fail_wiphy_register;
> > > + goto fail_workqueue;
> > > }
> > > }
> > >
> > > @@ -1113,7 +1113,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > >
> > > result = ieee80211_init_cipher_suites(local);
> > > if (result < 0)
> > > - goto fail_wiphy_register;
> > > + goto fail_workqueue;
> > >
> > > if (!local->ops->remain_on_channel)
> > > local->hw.wiphy->max_remain_on_channel_duration = 5000;
> > > @@ -1139,10 +1139,6 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > >
> > > local->hw.wiphy->max_num_csa_counters = IEEE80211_MAX_CSA_COUNTERS_NUM;
> > >
> > > - result = wiphy_register(local->hw.wiphy);
> > > - if (result < 0)
> > > - goto fail_wiphy_register;
> > > -
> > > /*
> > > * We use the number of queues for feature tests (QoS, HT) internally
> > > * so restrict them appropriately.
> > > @@ -1207,6 +1203,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > > goto fail_rate;
> > > }
> > >
> > > + rtnl_unlock();
> > > +
> > > if (local->rate_ctrl) {
> > > clear_bit(IEEE80211_HW_SUPPORTS_VHT_EXT_NSS_BW, hw->flags);
> > > if (local->rate_ctrl->ops->capa & RATE_CTRL_CAPA_VHT_EXT_NSS_BW)
> > > @@ -1254,6 +1252,12 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > > local->sband_allocated |= BIT(band);
> > > }
> > >
> > > + result = wiphy_register(local->hw.wiphy);
> > > + if (result < 0)
> > > + goto fail_wiphy_register;
> > > +
> > > + rtnl_lock();
> > > +
> > > /* add one default STA interface if supported */
> > > if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_STATION) &&
> > > !ieee80211_hw_check(hw, NO_AUTO_VIF)) {
> > > @@ -1293,6 +1297,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > > #if defined(CONFIG_INET) || defined(CONFIG_IPV6)
> > > fail_ifa:
> > > #endif
> > > + wiphy_unregister(local->hw.wiphy);
> > > + fail_wiphy_register:
> > > rtnl_lock();
> > > rate_control_deinitialize(local);
> > > ieee80211_remove_interfaces(local);
> > > @@ -1302,8 +1308,6 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
> > > ieee80211_led_exit(local);
> > > destroy_workqueue(local->workqueue);
> > > fail_workqueue:
> > > - wiphy_unregister(local->hw.wiphy);
> > > - fail_wiphy_register:
> > > if (local->wiphy_ciphers_allocated)
> > > kfree(local->hw.wiphy->cipher_suites);
> > > kfree(local->int_scan_req);
> > > @@ -1353,8 +1357,8 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
> > > skb_queue_purge(&local->skb_queue_unreliable);
> > > skb_queue_purge(&local->skb_queue_tdls_chsw);
> > >
> > > - destroy_workqueue(local->workqueue);
> > > wiphy_unregister(local->hw.wiphy);
> > > + destroy_workqueue(local->workqueue);
> > > ieee80211_led_exit(local);
> > > kfree(local->int_scan_req);
> > > }
> > > --
> > > 2.7.4
> > >
> >
> >
> > --
> > Thanks,
> > Regards,
> > Chaitanya T K.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mac80211: fix race in ieee80211_register_hw()
2020-04-15 11:35 ` Sumit Garg
@ 2020-04-15 13:19 ` Sedat Dilek
2020-04-15 13:39 ` Sumit Garg
0 siblings, 1 reply; 8+ messages in thread
From: Sedat Dilek @ 2020-04-15 13:19 UTC (permalink / raw)
To: Sumit Garg
Cc: Johannes Berg, linux-wireless, Krishna Chaitanya,
David S. Miller, kuba, Kalle Valo, netdev,
Linux Kernel Mailing List, Matthias-Peter Schöpfer,
Berg Philipp (HAU-EDS), Weitner Michael (HAU-EDS),
Daniel Thompson, Loic Poulain, stable
On Wed, Apr 15, 2020 at 3:10 PM Sumit Garg <sumit.garg@linaro.org> wrote:
[.. ]
> > In case we don't have any further comments, could you fix this nitpick
> > from Chaitanya while applying or would you like me to respin and send
> > v3?
>
> A gentle ping. Is this patch a good candidate for 5.7-rc2?
>
Hi Sumit,
it's in [1] (see [2]) with slightly mods by Johannes but not in Linus tree.
Johannes requested a pull-request means will be merged in a next step
in net.git and then hopefully land in Linus tree after Dave M.
requested a pull-request.
Thanks for your patch.
Regards,
- Sedat -
[1] https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git/tag/?h=mac80211-for-net-2020-04-15
[2] https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git/commit/?h=mac80211-for-net-2020-04-15&id=52e04b4ce5d03775b6a78f3ed1097480faacc9fd
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mac80211: fix race in ieee80211_register_hw()
2020-04-15 13:19 ` Sedat Dilek
@ 2020-04-15 13:39 ` Sumit Garg
2020-04-15 13:45 ` Sedat Dilek
0 siblings, 1 reply; 8+ messages in thread
From: Sumit Garg @ 2020-04-15 13:39 UTC (permalink / raw)
To: sedat.dilek, Johannes Berg
Cc: linux-wireless, Krishna Chaitanya, David S. Miller, kuba,
Kalle Valo, netdev, Linux Kernel Mailing List,
Matthias-Peter Schöpfer, Berg Philipp (HAU-EDS),
Weitner Michael (HAU-EDS),
Daniel Thompson, Loic Poulain, stable
On Wed, 15 Apr 2020 at 18:49, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> On Wed, Apr 15, 2020 at 3:10 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> [.. ]
>
> > > In case we don't have any further comments, could you fix this nitpick
> > > from Chaitanya while applying or would you like me to respin and send
> > > v3?
> >
> > A gentle ping. Is this patch a good candidate for 5.7-rc2?
> >
>
> Hi Sumit,
>
> it's in [1] (see [2]) with slightly mods by Johannes but not in Linus tree.
>
Thanks Sedat for this information.
> Johannes requested a pull-request means will be merged in a next step
> in net.git and then hopefully land in Linus tree after Dave M.
> requested a pull-request.
I didn't get this PR notification as currently I am not subscribed to
linux-wireless ML. So apologies for the noise here.
BTW, thanks Johannes for picking up this patch.
-Sumit
>
> Thanks for your patch.
>
> Regards,
> - Sedat -
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git/tag/?h=mac80211-for-net-2020-04-15
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git/commit/?h=mac80211-for-net-2020-04-15&id=52e04b4ce5d03775b6a78f3ed1097480faacc9fd
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mac80211: fix race in ieee80211_register_hw()
2020-04-15 13:39 ` Sumit Garg
@ 2020-04-15 13:45 ` Sedat Dilek
0 siblings, 0 replies; 8+ messages in thread
From: Sedat Dilek @ 2020-04-15 13:45 UTC (permalink / raw)
To: Sumit Garg
Cc: Johannes Berg, linux-wireless, Krishna Chaitanya,
David S. Miller, kuba, Kalle Valo, netdev,
Linux Kernel Mailing List, Matthias-Peter Schöpfer,
Berg Philipp (HAU-EDS), Weitner Michael (HAU-EDS),
Daniel Thompson, Loic Poulain, stable
On Wed, Apr 15, 2020 at 3:39 PM Sumit Garg <sumit.garg@linaro.org> wrote:
[ ... ]
> I didn't get this PR notification as currently I am not subscribed to
> linux-wireless ML. So apologies for the noise here.
>
There is/are a pr-tracker(s) and bots (for example tip tree) around
which inform people automatically.
But I have never dealt with that topic and thus do not know if there
exists something for net/wireless/mac80211 around.
- Sedat -
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-04-15 13:46 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-07 10:10 [PATCH v2] mac80211: fix race in ieee80211_register_hw() Sumit Garg
2020-04-07 19:25 ` Krishna Chaitanya
2020-04-09 4:42 ` Sumit Garg
2020-04-09 4:44 ` Sedat Dilek
2020-04-15 11:35 ` Sumit Garg
2020-04-15 13:19 ` Sedat Dilek
2020-04-15 13:39 ` Sumit Garg
2020-04-15 13:45 ` Sedat Dilek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).