Skip to content

Commit 7a7c0a6

Browse files
jmberg-inteljmberg-intel
committed
mac80211: fix TX aggregation start/stop callback race
When starting or stopping an aggregation session, one of the steps is that the driver calls back to mac80211 that the start/stop can proceed. This is handled by queueing up a fake SKB and processing it from the normal iface/sdata work. Since this isn't flushed when disassociating, the following race is possible: * associate * start aggregation session * driver callback * disassociate * associate again to the same AP * callback processing runs, leading to a WARN_ON() that the TID hadn't requested aggregation If the second association isn't to the same AP, there would only be a message printed ("Could not find station: <addr>"), but the same race could happen. Fix this by not going the whole detour with a fake SKB etc. but simply looking up the aggregation session in the driver callback, marking it with a START_CB/STOP_CB bit and then scheduling the regular aggregation work that will now process these bits as well. This also simplifies the code and gets rid of the whole problem with allocation failures of said skb, which could have left the session in limbo. Reported-by: Jouni Malinen <[email protected]> Signed-off-by: Johannes Berg <[email protected]>
1 parent 029c581 commit 7a7c0a6

File tree

5 files changed

+71
-100
lines changed

5 files changed

+71
-100
lines changed

‎net/mac80211/agg-tx.c

Lines changed: 52 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Copyright 2006-2007 Jiri Benc <[email protected]>
88
* Copyright 2007, Michael Wu <[email protected]>
99
* Copyright 2007-2010, Intel Corporation
10-
* Copyright(c) 2015 Intel Deutschland GmbH
10+
* Copyright(c) 2015-2017 Intel Deutschland GmbH
1111
*
1212
* This program is free software; you can redistribute it and/or modify
1313
* it under the terms of the GNU General Public License version 2 as
@@ -741,66 +741,64 @@ static void ieee80211_agg_tx_operational(struct ieee80211_local *local,
741741
ieee80211_agg_start_txq(sta, tid, true);
742742
}
743743

744-
void ieee80211_start_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u16 tid)
744+
void ieee80211_start_tx_ba_cb(struct sta_info *sta, int tid,
745+
struct tid_ampdu_tx *tid_tx)
745746
{
746-
struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
747+
struct ieee80211_sub_if_data *sdata = sta->sdata;
747748
struct ieee80211_local *local = sdata->local;
748-
struct sta_info *sta;
749-
struct tid_ampdu_tx *tid_tx;
750749

751-
trace_api_start_tx_ba_cb(sdata, ra, tid);
750+
if (WARN_ON(test_and_set_bit(HT_AGG_STATE_DRV_READY, &tid_tx->state)))
751+
return;
752+
753+
if (test_bit(HT_AGG_STATE_RESPONSE_RECEIVED, &tid_tx->state))
754+
ieee80211_agg_tx_operational(local, sta, tid);
755+
}
756+
757+
static struct tid_ampdu_tx *
758+
ieee80211_lookup_tid_tx(struct ieee80211_sub_if_data *sdata,
759+
const u8 *ra, u16 tid, struct sta_info **sta)
760+
{
761+
struct tid_ampdu_tx *tid_tx;
752762

753763
if (tid >= IEEE80211_NUM_TIDS) {
754764
ht_dbg(sdata, "Bad TID value: tid = %d (>= %d)\n",
755765
tid, IEEE80211_NUM_TIDS);
756-
return;
766+
return NULL;
757767
}
758768

759-
mutex_lock(&local->sta_mtx);
760-
sta = sta_info_get_bss(sdata, ra);
761-
if (!sta) {
762-
mutex_unlock(&local->sta_mtx);
769+
*sta = sta_info_get_bss(sdata, ra);
770+
if (!*sta) {
763771
ht_dbg(sdata, "Could not find station: %pM\n", ra);
764-
return;
772+
return NULL;
765773
}
766774

767-
mutex_lock(&sta->ampdu_mlme.mtx);
768-
tid_tx = rcu_dereference_protected_tid_tx(sta, tid);
775+
tid_tx = rcu_dereference((*sta)->ampdu_mlme.tid_tx[tid]);
769776

770-
if (WARN_ON(!tid_tx)) {
777+
if (WARN_ON(!tid_tx))
771778
ht_dbg(sdata, "addBA was not requested!\n");
772-
goto unlock;
773-
}
774779

775-
if (WARN_ON(test_and_set_bit(HT_AGG_STATE_DRV_READY, &tid_tx->state)))
776-
goto unlock;
777-
778-
if (test_bit(HT_AGG_STATE_RESPONSE_RECEIVED, &tid_tx->state))
779-
ieee80211_agg_tx_operational(local, sta, tid);
780-
781-
unlock:
782-
mutex_unlock(&sta->ampdu_mlme.mtx);
783-
mutex_unlock(&local->sta_mtx);
780+
return tid_tx;
784781
}
785782

786783
void ieee80211_start_tx_ba_cb_irqsafe(struct ieee80211_vif *vif,
787784
const u8 *ra, u16 tid)
788785
{
789786
struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
790787
struct ieee80211_local *local = sdata->local;
791-
struct ieee80211_ra_tid *ra_tid;
792-
struct sk_buff *skb = dev_alloc_skb(0);
788+
struct sta_info *sta;
789+
struct tid_ampdu_tx *tid_tx;
793790

794-
if (unlikely(!skb))
795-
return;
791+
trace_api_start_tx_ba_cb(sdata, ra, tid);
796792

797-
ra_tid = (struct ieee80211_ra_tid *) &skb->cb;
798-
memcpy(&ra_tid->ra, ra, ETH_ALEN);
799-
ra_tid->tid = tid;
793+
rcu_read_lock();
794+
tid_tx = ieee80211_lookup_tid_tx(sdata, ra, tid, &sta);
795+
if (!tid_tx)
796+
goto out;
800797

801-
skb->pkt_type = IEEE80211_SDATA_QUEUE_AGG_START;
802-
skb_queue_tail(&sdata->skb_queue, skb);
803-
ieee80211_queue_work(&local->hw, &sdata->work);
798+
set_bit(HT_AGG_STATE_START_CB, &tid_tx->state);
799+
ieee80211_queue_work(&local->hw, &sta->ampdu_mlme.work);
800+
out:
801+
rcu_read_unlock();
804802
}
805803
EXPORT_SYMBOL(ieee80211_start_tx_ba_cb_irqsafe);
806804

@@ -860,37 +858,18 @@ int ieee80211_stop_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid)
860858
}
861859
EXPORT_SYMBOL(ieee80211_stop_tx_ba_session);
862860

863-
void ieee80211_stop_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u8 tid)
861+
void ieee80211_stop_tx_ba_cb(struct sta_info *sta, int tid,
862+
struct tid_ampdu_tx *tid_tx)
864863
{
865-
struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
866-
struct ieee80211_local *local = sdata->local;
867-
struct sta_info *sta;
868-
struct tid_ampdu_tx *tid_tx;
864+
struct ieee80211_sub_if_data *sdata = sta->sdata;
869865
bool send_delba = false;
870866

871-
trace_api_stop_tx_ba_cb(sdata, ra, tid);
872-
873-
if (tid >= IEEE80211_NUM_TIDS) {
874-
ht_dbg(sdata, "Bad TID value: tid = %d (>= %d)\n",
875-
tid, IEEE80211_NUM_TIDS);
876-
return;
877-
}
878-
879-
ht_dbg(sdata, "Stopping Tx BA session for %pM tid %d\n", ra, tid);
880-
881-
mutex_lock(&local->sta_mtx);
882-
883-
sta = sta_info_get_bss(sdata, ra);
884-
if (!sta) {
885-
ht_dbg(sdata, "Could not find station: %pM\n", ra);
886-
goto unlock;
887-
}
867+
ht_dbg(sdata, "Stopping Tx BA session for %pM tid %d\n",
868+
sta->sta.addr, tid);
888869

889-
mutex_lock(&sta->ampdu_mlme.mtx);
890870
spin_lock_bh(&sta->lock);
891-
tid_tx = rcu_dereference_protected_tid_tx(sta, tid);
892871

893-
if (!tid_tx || !test_bit(HT_AGG_STATE_STOPPING, &tid_tx->state)) {
872+
if (!test_bit(HT_AGG_STATE_STOPPING, &tid_tx->state)) {
894873
ht_dbg(sdata,
895874
"unexpected callback to A-MPDU stop for %pM tid %d\n",
896875
sta->sta.addr, tid);
@@ -906,32 +885,29 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u8 tid)
906885
spin_unlock_bh(&sta->lock);
907886

908887
if (send_delba)
909-
ieee80211_send_delba(sdata, ra, tid,
888+
ieee80211_send_delba(sdata, sta->sta.addr, tid,
910889
WLAN_BACK_INITIATOR, WLAN_REASON_QSTA_NOT_USE);
911-
912-
mutex_unlock(&sta->ampdu_mlme.mtx);
913-
unlock:
914-
mutex_unlock(&local->sta_mtx);
915890
}
916891

917892
void ieee80211_stop_tx_ba_cb_irqsafe(struct ieee80211_vif *vif,
918893
const u8 *ra, u16 tid)
919894
{
920895
struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
921896
struct ieee80211_local *local = sdata->local;
922-
struct ieee80211_ra_tid *ra_tid;
923-
struct sk_buff *skb = dev_alloc_skb(0);
897+
struct sta_info *sta;
898+
struct tid_ampdu_tx *tid_tx;
924899

925-
if (unlikely(!skb))
926-
return;
900+
trace_api_stop_tx_ba_cb(sdata, ra, tid);
927901

928-
ra_tid = (struct ieee80211_ra_tid *) &skb->cb;
929-
memcpy(&ra_tid->ra, ra, ETH_ALEN);
930-
ra_tid->tid = tid;
902+
rcu_read_lock();
903+
tid_tx = ieee80211_lookup_tid_tx(sdata, ra, tid, &sta);
904+
if (!tid_tx)
905+
goto out;
931906

932-
skb->pkt_type = IEEE80211_SDATA_QUEUE_AGG_STOP;
933-
skb_queue_tail(&sdata->skb_queue, skb);
934-
ieee80211_queue_work(&local->hw, &sdata->work);
907+
set_bit(HT_AGG_STATE_STOP_CB, &tid_tx->state);
908+
ieee80211_queue_work(&local->hw, &sta->ampdu_mlme.work);
909+
out:
910+
rcu_read_unlock();
935911
}
936912
EXPORT_SYMBOL(ieee80211_stop_tx_ba_cb_irqsafe);
937913

‎net/mac80211/ht.c

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
* Copyright 2006-2007 Jiri Benc <[email protected]>
88
* Copyright 2007, Michael Wu <[email protected]>
99
* Copyright 2007-2010, Intel Corporation
10+
* Copyright 2017 Intel Deutschland GmbH
1011
*
1112
* This program is free software; you can redistribute it and/or modify
1213
* it under the terms of the GNU General Public License version 2 as
@@ -289,15 +290,16 @@ void ieee80211_sta_tear_down_BA_sessions(struct sta_info *sta,
289290
{
290291
int i;
291292

292-
cancel_work_sync(&sta->ampdu_mlme.work);
293-
294293
for (i = 0; i < IEEE80211_NUM_TIDS; i++) {
295294
__ieee80211_stop_tx_ba_session(sta, i, reason);
296295
__ieee80211_stop_rx_ba_session(sta, i, WLAN_BACK_RECIPIENT,
297296
WLAN_REASON_QSTA_LEAVE_QBSS,
298297
reason != AGG_STOP_DESTROY_STA &&
299298
reason != AGG_STOP_PEER_REQUEST);
300299
}
300+
301+
/* stopping might queue the work again - so cancel only afterwards */
302+
cancel_work_sync(&sta->ampdu_mlme.work);
301303
}
302304

303305
void ieee80211_ba_session_work(struct work_struct *work)
@@ -352,10 +354,16 @@ void ieee80211_ba_session_work(struct work_struct *work)
352354
spin_unlock_bh(&sta->lock);
353355

354356
tid_tx = rcu_dereference_protected_tid_tx(sta, tid);
355-
if (tid_tx && test_and_clear_bit(HT_AGG_STATE_WANT_STOP,
356-
&tid_tx->state))
357+
if (!tid_tx)
358+
continue;
359+
360+
if (test_and_clear_bit(HT_AGG_STATE_START_CB, &tid_tx->state))
361+
ieee80211_start_tx_ba_cb(sta, tid, tid_tx);
362+
if (test_and_clear_bit(HT_AGG_STATE_WANT_STOP, &tid_tx->state))
357363
___ieee80211_stop_tx_ba_session(sta, tid,
358364
AGG_STOP_LOCAL_REQUEST);
365+
if (test_and_clear_bit(HT_AGG_STATE_STOP_CB, &tid_tx->state))
366+
ieee80211_stop_tx_ba_cb(sta, tid, tid_tx);
359367
}
360368
mutex_unlock(&sta->ampdu_mlme.mtx);
361369
}

‎net/mac80211/ieee80211_i.h

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1036,8 +1036,6 @@ struct ieee80211_rx_agg {
10361036

10371037
enum sdata_queue_type {
10381038
IEEE80211_SDATA_QUEUE_TYPE_FRAME = 0,
1039-
IEEE80211_SDATA_QUEUE_AGG_START = 1,
1040-
IEEE80211_SDATA_QUEUE_AGG_STOP = 2,
10411039
IEEE80211_SDATA_QUEUE_RX_AGG_START = 3,
10421040
IEEE80211_SDATA_QUEUE_RX_AGG_STOP = 4,
10431041
};
@@ -1427,12 +1425,6 @@ ieee80211_get_sband(struct ieee80211_sub_if_data *sdata)
14271425
return local->hw.wiphy->bands[band];
14281426
}
14291427

1430-
/* this struct represents 802.11n's RA/TID combination */
1431-
struct ieee80211_ra_tid {
1432-
u8 ra[ETH_ALEN];
1433-
u16 tid;
1434-
};
1435-
14361428
/* this struct holds the value parsing from channel switch IE */
14371429
struct ieee80211_csa_ie {
14381430
struct cfg80211_chan_def chandef;
@@ -1794,8 +1786,10 @@ int __ieee80211_stop_tx_ba_session(struct sta_info *sta, u16 tid,
17941786
enum ieee80211_agg_stop_reason reason);
17951787
int ___ieee80211_stop_tx_ba_session(struct sta_info *sta, u16 tid,
17961788
enum ieee80211_agg_stop_reason reason);
1797-
void ieee80211_start_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u16 tid);
1798-
void ieee80211_stop_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u8 tid);
1789+
void ieee80211_start_tx_ba_cb(struct sta_info *sta, int tid,
1790+
struct tid_ampdu_tx *tid_tx);
1791+
void ieee80211_stop_tx_ba_cb(struct sta_info *sta, int tid,
1792+
struct tid_ampdu_tx *tid_tx);
17991793
void ieee80211_ba_session_work(struct work_struct *work);
18001794
void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid);
18011795
void ieee80211_release_reorder_timeout(struct sta_info *sta, int tid);

‎net/mac80211/iface.c

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1237,7 +1237,6 @@ static void ieee80211_iface_work(struct work_struct *work)
12371237
struct ieee80211_local *local = sdata->local;
12381238
struct sk_buff *skb;
12391239
struct sta_info *sta;
1240-
struct ieee80211_ra_tid *ra_tid;
12411240
struct ieee80211_rx_agg *rx_agg;
12421241

12431242
if (!ieee80211_sdata_running(sdata))
@@ -1253,15 +1252,7 @@ static void ieee80211_iface_work(struct work_struct *work)
12531252
while ((skb = skb_dequeue(&sdata->skb_queue))) {
12541253
struct ieee80211_mgmt *mgmt = (void *)skb->data;
12551254

1256-
if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_START) {
1257-
ra_tid = (void *)&skb->cb;
1258-
ieee80211_start_tx_ba_cb(&sdata->vif, ra_tid->ra,
1259-
ra_tid->tid);
1260-
} else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_STOP) {
1261-
ra_tid = (void *)&skb->cb;
1262-
ieee80211_stop_tx_ba_cb(&sdata->vif, ra_tid->ra,
1263-
ra_tid->tid);
1264-
} else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_START) {
1255+
if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_START) {
12651256
rx_agg = (void *)&skb->cb;
12661257
mutex_lock(&local->sta_mtx);
12671258
sta = sta_info_get_bss(sdata, rx_agg->addr);

‎net/mac80211/sta_info.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,8 @@ enum ieee80211_sta_info_flags {
116116
#define HT_AGG_STATE_STOPPING 3
117117
#define HT_AGG_STATE_WANT_START 4
118118
#define HT_AGG_STATE_WANT_STOP 5
119+
#define HT_AGG_STATE_START_CB 6
120+
#define HT_AGG_STATE_STOP_CB 7
119121

120122
enum ieee80211_agg_stop_reason {
121123
AGG_STOP_DECLINED,

0 commit comments

Comments
 (0)