| Message ID | 1361326267-16847-1-git-send-email-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Ben, On Wed, Feb 20, 2013 at 1:11 PM, <greearb@candelatech.com> wrote: > From: Ben Greear <greearb@candelatech.com> > > The monitor_work and beacon_connection_loss_work items were > not being canceled on disassociation (and not on deletion > either). This leads to work-items trying to run after memory > has been deleted. > > I could not find a cleaner way to do this because the > cancel_work_sync for these items must be done outside > of the ifmgd->mtx. > > In addition, re-order the quiesce code so that timers are > always stopped before work-items are flushed. This was > not the problem I saw, but I think it may still be more > correct. > > This fixes the crashes we see in 3.7.9+. The crash stack > trace itself isn't so helpful, but this warning gives > more useful info: [snip] > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > > NOTE: Please do not apply this until it is reviewed by Johannes, > at least. > > net/mac80211/mlme.c | 68 +++++++++++++++++++++++++++++++++++++++++++++----- > 1 files changed, 61 insertions(+), 7 deletions(-) > > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c > index 164ecf0..5a65144 100644 > --- a/net/mac80211/mlme.c > +++ b/net/mac80211/mlme.c > @@ -2954,19 +2978,15 @@ void ieee80211_sta_quiesce(struct ieee80211_sub_if_data *sdata) > > cancel_work_sync(&ifmgd->request_smps_work); > > + sdata_err(sdata, "Cancel-work-sync monitor_work, sta_quiesce.\n"); Debug code? > cancel_work_sync(&ifmgd->monitor_work); > cancel_work_sync(&ifmgd->beacon_connection_loss_work); > cancel_work_sync(&ifmgd->csa_connection_drop_work); > if (del_timer_sync(&ifmgd->timer)) > set_bit(TMR_RUNNING_TIMER, &ifmgd->timers_running); > > - cancel_work_sync(&ifmgd->chswitch_work); > if (del_timer_sync(&ifmgd->chswitch_timer)) > set_bit(TMR_RUNNING_CHANSW, &ifmgd->timers_running); > - > - /* these will just be re-established on connection */ > - del_timer_sync(&ifmgd->conn_mon_timer); > - del_timer_sync(&ifmgd->bcn_mon_timer); > } > > void ieee80211_sta_restart(struct ieee80211_sub_if_data *sdata) > @@ -3262,6 +3282,8 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata, > struct ieee80211_mgd_auth_data *auth_data; > u16 auth_alg; > int err; > + bool maybe_cancel_work = false; > + bool hit_err_clear = false; You could replace these with a single variable. > > /* prepare auth data structure */ > > @@ -3319,8 +3341,10 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata, > /* prep auth_data so we don't go into idle on disassoc */ > ifmgd->auth_data = auth_data; > > - if (ifmgd->associated) > + if (ifmgd->associated) { > + maybe_cancel_work = true; > ieee80211_set_disassoc(sdata, 0, 0, false, NULL); > + } > > sdata_info(sdata, "authenticate with %pM\n", req->bss->bssid); > > @@ -3340,6 +3364,7 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata, > goto out_unlock; > > err_clear: > + hit_err_clear = true; > memset(ifmgd->bssid, 0, ETH_ALEN); > ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BSSID); > ifmgd->auth_data = NULL; > @@ -3348,6 +3373,12 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata, > out_unlock: > mutex_unlock(&ifmgd->mtx); > > + if (maybe_cancel_work && hit_err_clear) { > + /* Have to do this outside the ifmgd->mtx lock. */ > + cancel_work_sync(&ifmgd->monitor_work); > + cancel_work_sync(&ifmgd->beacon_connection_loss_work); > + } > + > return err; > } > > @@ -3361,6 +3392,8 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata, > struct ieee80211_supported_band *sband; > const u8 *ssidie, *ht_ie; > int i, err; > + bool maybe_cancel_work = false; > + bool hit_err_state = false; ditto. > > ssidie = ieee80211_bss_get_ie(req->bss, WLAN_EID_SSID); > if (!ssidie) > @@ -3372,8 +3405,10 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata, > > mutex_lock(&ifmgd->mtx); > > - if (ifmgd->associated) > + if (ifmgd->associated) { > + maybe_cancel_work = true; > ieee80211_set_disassoc(sdata, 0, 0, false, NULL); > + } > > if (ifmgd->auth_data && !ifmgd->auth_data->done) { > err = -EBUSY; > @@ -3555,9 +3590,16 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata, > ifmgd->assoc_data = NULL; > err_free: > kfree(assoc_data); > + hit_err_state = true; > out: > mutex_unlock(&ifmgd->mtx); > > + if (maybe_cancel_work && hit_err_state) { > + /* Have to do this outside the ifmgd->mtx lock. */ > + cancel_work_sync(&ifmgd->monitor_work); > + cancel_work_sync(&ifmgd->beacon_connection_loss_work); > + } > + > return err; > } > Thanks,
> In addition, re-order the quiesce code so that timers are > always stopped before work-items are flushed. This was > not the problem I saw, but I think it may still be more > correct. I'd prefer this to be a separate patch, and then might not apply that one for 3.9 > NOTE: Please do not apply this until it is reviewed by Johannes, > at least. No worries - I have my own mac80211 tree so nobody else will apply it anyway :) > @@ -2618,6 +2625,12 @@ void ieee80211_sta_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata, > } > mutex_unlock(&ifmgd->mtx); > > + if (cancel_work) { > + /* Have to do this outside the ifmgd->mtx lock. */ > + cancel_work_sync(&ifmgd->monitor_work); > + cancel_work_sync(&ifmgd->beacon_connection_loss_work); > + } It might make more sense to move this into the disassoc/deauth case below? > switch (rma) { > case RX_MGMT_NONE: > /* no action */ > @@ -2668,6 +2681,10 @@ static void ieee80211_sta_connection_lost(struct ieee80211_sub_if_data *sdata, > false, frame_buf); > mutex_unlock(&ifmgd->mtx); > > + /* Have to do this outside the ifmgd->mtx lock. */ > + cancel_work_sync(&ifmgd->monitor_work); > + cancel_work_sync(&ifmgd->beacon_connection_loss_work); OTOH, you do this many many times, and that doesn't seem necessary... If the work structs run when we disconnected, that's ok, they just musn't run after we destroy the interface, so I think it'd be much better to just put the two lines into ieee80211_mgd_stop() instead of all the other places. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/19/2013 10:23 PM, Julian Calaby wrote: > Hi Ben, > > On Wed, Feb 20, 2013 at 1:11 PM, <greearb@candelatech.com> wrote: >> From: Ben Greear <greearb@candelatech.com> >> >> The monitor_work and beacon_connection_loss_work items were >> not being canceled on disassociation (and not on deletion >> either). This leads to work-items trying to run after memory >> has been deleted. >> >> I could not find a cleaner way to do this because the >> cancel_work_sync for these items must be done outside >> of the ifmgd->mtx. >> >> In addition, re-order the quiesce code so that timers are >> always stopped before work-items are flushed. This was >> not the problem I saw, but I think it may still be more >> correct. >> >> This fixes the crashes we see in 3.7.9+. The crash stack >> trace itself isn't so helpful, but this warning gives >> more useful info: > > [snip] > >> Signed-off-by: Ben Greear <greearb@candelatech.com> >> --- >> >> NOTE: Please do not apply this until it is reviewed by Johannes, >> at least. >> >> net/mac80211/mlme.c | 68 +++++++++++++++++++++++++++++++++++++++++++++----- >> 1 files changed, 61 insertions(+), 7 deletions(-) >> >> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c >> index 164ecf0..5a65144 100644 >> --- a/net/mac80211/mlme.c >> +++ b/net/mac80211/mlme.c >> @@ -2954,19 +2978,15 @@ void ieee80211_sta_quiesce(struct ieee80211_sub_if_data *sdata) >> >> cancel_work_sync(&ifmgd->request_smps_work); >> >> + sdata_err(sdata, "Cancel-work-sync monitor_work, sta_quiesce.\n"); > > Debug code? Err, yes..will remove. >> void ieee80211_sta_restart(struct ieee80211_sub_if_data *sdata) >> @@ -3262,6 +3282,8 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata, >> struct ieee80211_mgd_auth_data *auth_data; >> u16 auth_alg; >> int err; >> + bool maybe_cancel_work = false; >> + bool hit_err_clear = false; > > You could replace these with a single variable. I don't see how. It seems that we should only cancel the work items if we called set_disassoc AND if the subsequent attempts to (re)associate failed. Maybe I missed something? Thanks, Ben
On 02/20/2013 01:59 AM, Johannes Berg wrote: > >> In addition, re-order the quiesce code so that timers are >> always stopped before work-items are flushed. This was >> not the problem I saw, but I think it may still be more >> correct. > > I'd prefer this to be a separate patch, and then might not apply that > one for 3.9 >> @@ -2668,6 +2681,10 @@ static void ieee80211_sta_connection_lost(struct ieee80211_sub_if_data *sdata, >> false, frame_buf); >> mutex_unlock(&ifmgd->mtx); >> >> + /* Have to do this outside the ifmgd->mtx lock. */ >> + cancel_work_sync(&ifmgd->monitor_work); >> + cancel_work_sync(&ifmgd->beacon_connection_loss_work); > > OTOH, you do this many many times, and that doesn't seem necessary... > > If the work structs run when we disconnected, that's ok, they just > musn't run after we destroy the interface, so I think it'd be much > better to just put the two lines into ieee80211_mgd_stop() instead of > all the other places. Ok, that sounds promising to me. For that matter, could we just call the sta_quiesce method in mgd_stop? It would be nice to have all of the final timer and work-queue cleanup in a single place. And, maybe add some checks in the work-callback methods to bail early if the station isn't connected? Thanks, Ben > > johannes >
On Wed, 2013-02-20 at 06:09 -0800, Ben Greear wrote: > >> @@ -2668,6 +2681,10 @@ static void ieee80211_sta_connection_lost(struct ieee80211_sub_if_data *sdata, > >> false, frame_buf); > >> mutex_unlock(&ifmgd->mtx); > >> > >> + /* Have to do this outside the ifmgd->mtx lock. */ > >> + cancel_work_sync(&ifmgd->monitor_work); > >> + cancel_work_sync(&ifmgd->beacon_connection_loss_work); > > > > OTOH, you do this many many times, and that doesn't seem necessary... > > > > If the work structs run when we disconnected, that's ok, they just > > musn't run after we destroy the interface, so I think it'd be much > > better to just put the two lines into ieee80211_mgd_stop() instead of > > all the other places. > > Ok, that sounds promising to me. For that matter, could we just call the sta_quiesce > method in mgd_stop? It would be nice to have all of the final timer and work-queue cleanup > in a single place. That might be interesting, but I think we'd have to clear the timer_pending thing afterwards? > And, maybe add some checks in the work-callback methods to bail early if the station > isn't connected? They already do :) johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/20/2013 06:13 AM, Johannes Berg wrote: > On Wed, 2013-02-20 at 06:09 -0800, Ben Greear wrote: > >>>> @@ -2668,6 +2681,10 @@ static void ieee80211_sta_connection_lost(struct ieee80211_sub_if_data *sdata, >>>> false, frame_buf); >>>> mutex_unlock(&ifmgd->mtx); >>>> >>>> + /* Have to do this outside the ifmgd->mtx lock. */ >>>> + cancel_work_sync(&ifmgd->monitor_work); >>>> + cancel_work_sync(&ifmgd->beacon_connection_loss_work); >>> >>> OTOH, you do this many many times, and that doesn't seem necessary... >>> >>> If the work structs run when we disconnected, that's ok, they just >>> musn't run after we destroy the interface, so I think it'd be much >>> better to just put the two lines into ieee80211_mgd_stop() instead of >>> all the other places. >> >> Ok, that sounds promising to me. For that matter, could we just call the sta_quiesce >> method in mgd_stop? It would be nice to have all of the final timer and work-queue cleanup >> in a single place. > > That might be interesting, but I think we'd have to clear the > timer_pending thing afterwards? Are you talking about this code? if (del_timer_sync(&ifmgd->timer)) set_bit(TMR_RUNNING_TIMER, &ifmgd->timers_running); if (del_timer_sync(&ifmgd->chswitch_timer)) set_bit(TMR_RUNNING_CHANSW, &ifmgd->timers_running); If so, maybe make a helper method that does all of the first part of the sta_quiesce() logic, call that from both mgd_stop and the new sta_quiesce() method, and then have sta_quiesce look like: sta_cleanup_timers_and_work(); if (del_timer_sync(&ifmgd->timer)) set_bit(TMR_RUNNING_TIMER, &ifmgd->timers_running); if (del_timer_sync(&ifmgd->chswitch_timer)) set_bit(TMR_RUNNING_CHANSW, &ifmgd->timers_running); ? > >> And, maybe add some checks in the work-callback methods to bail early if the station >> isn't connected? > > They already do :) Ok, thanks. Ben
On Wed, 2013-02-20 at 06:24 -0800, Ben Greear wrote: > On 02/20/2013 06:13 AM, Johannes Berg wrote: > > On Wed, 2013-02-20 at 06:09 -0800, Ben Greear wrote: > > > >>>> @@ -2668,6 +2681,10 @@ static void ieee80211_sta_connection_lost(struct ieee80211_sub_if_data *sdata, > >>>> false, frame_buf); > >>>> mutex_unlock(&ifmgd->mtx); > >>>> > >>>> + /* Have to do this outside the ifmgd->mtx lock. */ > >>>> + cancel_work_sync(&ifmgd->monitor_work); > >>>> + cancel_work_sync(&ifmgd->beacon_connection_loss_work); > >>> > >>> OTOH, you do this many many times, and that doesn't seem necessary... > >>> > >>> If the work structs run when we disconnected, that's ok, they just > >>> musn't run after we destroy the interface, so I think it'd be much > >>> better to just put the two lines into ieee80211_mgd_stop() instead of > >>> all the other places. > >> > >> Ok, that sounds promising to me. For that matter, could we just call the sta_quiesce > >> method in mgd_stop? It would be nice to have all of the final timer and work-queue cleanup > >> in a single place. > > > > That might be interesting, but I think we'd have to clear the > > timer_pending thing afterwards? > > Are you talking about this code? > > if (del_timer_sync(&ifmgd->timer)) > set_bit(TMR_RUNNING_TIMER, &ifmgd->timers_running); > > if (del_timer_sync(&ifmgd->chswitch_timer)) > set_bit(TMR_RUNNING_CHANSW, &ifmgd->timers_running); > > If so, maybe make a helper method that does all of the first part of the sta_quiesce() > logic, call that from both mgd_stop and the new sta_quiesce() method, and then have > sta_quiesce look like: > > sta_cleanup_timers_and_work(); > if (del_timer_sync(&ifmgd->timer)) > set_bit(TMR_RUNNING_TIMER, &ifmgd->timers_running); > > if (del_timer_sync(&ifmgd->chswitch_timer)) > set_bit(TMR_RUNNING_CHANSW, &ifmgd->timers_running); What part of that would then be "cleanup_timers"? No, I think you should just look at it and set timers_running = 0 after calling the function, or so. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ben, On Thu, Feb 21, 2013 at 1:04 AM, Ben Greear <greearb@candelatech.com> wrote: > On 02/19/2013 10:23 PM, Julian Calaby wrote: >> >> Hi Ben, >> >> On Wed, Feb 20, 2013 at 1:11 PM, <greearb@candelatech.com> wrote: >>> >>> From: Ben Greear <greearb@candelatech.com> >>> >>> The monitor_work and beacon_connection_loss_work items were >>> not being canceled on disassociation (and not on deletion >>> either). This leads to work-items trying to run after memory >>> has been deleted. >>> >>> I could not find a cleaner way to do this because the >>> cancel_work_sync for these items must be done outside >>> of the ifmgd->mtx. >>> >>> In addition, re-order the quiesce code so that timers are >>> always stopped before work-items are flushed. This was >>> not the problem I saw, but I think it may still be more >>> correct. >>> >>> This fixes the crashes we see in 3.7.9+. The crash stack >>> trace itself isn't so helpful, but this warning gives >>> more useful info: >> >> >> [snip] >> >>> Signed-off-by: Ben Greear <greearb@candelatech.com> >>> --- >>> >>> NOTE: Please do not apply this until it is reviewed by Johannes, >>> at least. >>> >>> net/mac80211/mlme.c | 68 >>> +++++++++++++++++++++++++++++++++++++++++++++----- >>> 1 files changed, 61 insertions(+), 7 deletions(-) >>> >>> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c >>> index 164ecf0..5a65144 100644 >>> --- a/net/mac80211/mlme.c >>> +++ b/net/mac80211/mlme.c >>> @@ -3262,6 +3282,8 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data >>> *sdata, >>> struct ieee80211_mgd_auth_data *auth_data; >>> u16 auth_alg; >>> int err; >>> + bool maybe_cancel_work = false; >>> + bool hit_err_clear = false; >> >> >> You could replace these with a single variable. > > > I don't see how. It seems that we should only cancel the work items > if we called set_disassoc AND if the subsequent attempts to (re)associate > failed. > > Maybe I missed something? No, I misread the code. Sorry for the noise. Thanks,
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c index 164ecf0..5a65144 100644 --- a/net/mac80211/mlme.c +++ b/net/mac80211/mlme.c @@ -1725,6 +1725,10 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata, ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED; mutex_unlock(&ifmgd->mtx); + /* Have to do this outside the ifmgd->mtx lock. */ + cancel_work_sync(&ifmgd->monitor_work); + cancel_work_sync(&ifmgd->beacon_connection_loss_work); + /* * must be outside lock due to cfg80211, * but that's not a problem. @@ -2579,6 +2583,7 @@ void ieee80211_sta_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata, struct cfg80211_bss *bss = NULL; enum rx_mgmt_action rma = RX_MGMT_NONE; u16 fc; + bool cancel_work = false; rx_status = (struct ieee80211_rx_status *) skb->cb; mgmt = (struct ieee80211_mgmt *) skb->data; @@ -2598,9 +2603,11 @@ void ieee80211_sta_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata, break; case IEEE80211_STYPE_DEAUTH: rma = ieee80211_rx_mgmt_deauth(sdata, mgmt, skb->len); + cancel_work = true; break; case IEEE80211_STYPE_DISASSOC: rma = ieee80211_rx_mgmt_disassoc(sdata, mgmt, skb->len); + cancel_work = true; break; case IEEE80211_STYPE_ASSOC_RESP: case IEEE80211_STYPE_REASSOC_RESP: @@ -2618,6 +2625,12 @@ void ieee80211_sta_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata, } mutex_unlock(&ifmgd->mtx); + if (cancel_work) { + /* Have to do this outside the ifmgd->mtx lock. */ + cancel_work_sync(&ifmgd->monitor_work); + cancel_work_sync(&ifmgd->beacon_connection_loss_work); + } + switch (rma) { case RX_MGMT_NONE: /* no action */ @@ -2668,6 +2681,10 @@ static void ieee80211_sta_connection_lost(struct ieee80211_sub_if_data *sdata, false, frame_buf); mutex_unlock(&ifmgd->mtx); + /* Have to do this outside the ifmgd->mtx lock. */ + cancel_work_sync(&ifmgd->monitor_work); + cancel_work_sync(&ifmgd->beacon_connection_loss_work); + /* * must be outside lock due to cfg80211, * but that's not a problem. @@ -2946,6 +2963,13 @@ void ieee80211_sta_quiesce(struct ieee80211_sub_if_data *sdata) { struct ieee80211_if_managed *ifmgd = &sdata->u.mgd; + /* Stop timers before deleting work items, as timers could + * race and re-add the work-items. + * These will be re-established on connection. + */ + del_timer_sync(&ifmgd->conn_mon_timer); + del_timer_sync(&ifmgd->bcn_mon_timer); + /* * we need to use atomic bitops for the running bits * only because both timers might fire at the same @@ -2954,19 +2978,15 @@ void ieee80211_sta_quiesce(struct ieee80211_sub_if_data *sdata) cancel_work_sync(&ifmgd->request_smps_work); + sdata_err(sdata, "Cancel-work-sync monitor_work, sta_quiesce.\n"); cancel_work_sync(&ifmgd->monitor_work); cancel_work_sync(&ifmgd->beacon_connection_loss_work); cancel_work_sync(&ifmgd->csa_connection_drop_work); if (del_timer_sync(&ifmgd->timer)) set_bit(TMR_RUNNING_TIMER, &ifmgd->timers_running); - cancel_work_sync(&ifmgd->chswitch_work); if (del_timer_sync(&ifmgd->chswitch_timer)) set_bit(TMR_RUNNING_CHANSW, &ifmgd->timers_running); - - /* these will just be re-established on connection */ - del_timer_sync(&ifmgd->conn_mon_timer); - del_timer_sync(&ifmgd->bcn_mon_timer); } void ieee80211_sta_restart(struct ieee80211_sub_if_data *sdata) @@ -3262,6 +3282,8 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata, struct ieee80211_mgd_auth_data *auth_data; u16 auth_alg; int err; + bool maybe_cancel_work = false; + bool hit_err_clear = false; /* prepare auth data structure */ @@ -3319,8 +3341,10 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata, /* prep auth_data so we don't go into idle on disassoc */ ifmgd->auth_data = auth_data; - if (ifmgd->associated) + if (ifmgd->associated) { + maybe_cancel_work = true; ieee80211_set_disassoc(sdata, 0, 0, false, NULL); + } sdata_info(sdata, "authenticate with %pM\n", req->bss->bssid); @@ -3340,6 +3364,7 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata, goto out_unlock; err_clear: + hit_err_clear = true; memset(ifmgd->bssid, 0, ETH_ALEN); ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BSSID); ifmgd->auth_data = NULL; @@ -3348,6 +3373,12 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata, out_unlock: mutex_unlock(&ifmgd->mtx); + if (maybe_cancel_work && hit_err_clear) { + /* Have to do this outside the ifmgd->mtx lock. */ + cancel_work_sync(&ifmgd->monitor_work); + cancel_work_sync(&ifmgd->beacon_connection_loss_work); + } + return err; } @@ -3361,6 +3392,8 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata, struct ieee80211_supported_band *sband; const u8 *ssidie, *ht_ie; int i, err; + bool maybe_cancel_work = false; + bool hit_err_state = false; ssidie = ieee80211_bss_get_ie(req->bss, WLAN_EID_SSID); if (!ssidie) @@ -3372,8 +3405,10 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata, mutex_lock(&ifmgd->mtx); - if (ifmgd->associated) + if (ifmgd->associated) { + maybe_cancel_work = true; ieee80211_set_disassoc(sdata, 0, 0, false, NULL); + } if (ifmgd->auth_data && !ifmgd->auth_data->done) { err = -EBUSY; @@ -3555,9 +3590,16 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata, ifmgd->assoc_data = NULL; err_free: kfree(assoc_data); + hit_err_state = true; out: mutex_unlock(&ifmgd->mtx); + if (maybe_cancel_work && hit_err_state) { + /* Have to do this outside the ifmgd->mtx lock. */ + cancel_work_sync(&ifmgd->monitor_work); + cancel_work_sync(&ifmgd->beacon_connection_loss_work); + } + return err; } @@ -3567,6 +3609,7 @@ int ieee80211_mgd_deauth(struct ieee80211_sub_if_data *sdata, struct ieee80211_if_managed *ifmgd = &sdata->u.mgd; u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN]; bool tx = !req->local_state_change; + bool cancel_work = false; mutex_lock(&ifmgd->mtx); @@ -3584,6 +3627,7 @@ int ieee80211_mgd_deauth(struct ieee80211_sub_if_data *sdata, ether_addr_equal(ifmgd->associated->bssid, req->bssid)) { ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH, req->reason_code, tx, frame_buf); + cancel_work = true; } else { drv_mgd_prepare_tx(sdata->local, sdata); ieee80211_send_deauth_disassoc(sdata, req->bssid, @@ -3594,6 +3638,12 @@ int ieee80211_mgd_deauth(struct ieee80211_sub_if_data *sdata, mutex_unlock(&ifmgd->mtx); + if (cancel_work) { + /* Have to do this outside the ifmgd->mtx lock. */ + cancel_work_sync(&ifmgd->monitor_work); + cancel_work_sync(&ifmgd->beacon_connection_loss_work); + } + __cfg80211_send_deauth(sdata->dev, frame_buf, IEEE80211_DEAUTH_FRAME_LEN); @@ -3634,6 +3684,10 @@ int ieee80211_mgd_disassoc(struct ieee80211_sub_if_data *sdata, frame_buf); mutex_unlock(&ifmgd->mtx); + /* Have to do this outside the ifmgd->mtx lock. */ + cancel_work_sync(&ifmgd->monitor_work); + cancel_work_sync(&ifmgd->beacon_connection_loss_work); + __cfg80211_send_disassoc(sdata->dev, frame_buf, IEEE80211_DEAUTH_FRAME_LEN);