Skip to contentSkip to conversation

mac80211 fixes

PATCH New Johannes Berg 1 participant 8 messages Apr 6, 2010

Subsystems (2)
mac80211 802.11 (including cfg80211/NL80211)
lore
Share & tools
lore

Johannes Berg

author maintainer
#0 lore raw

cover mac80211 fixes

Here are a few mac80211 fixes, I'll let you decide which trees to add them to. The first one is only relevant for -34 and should probably be applied there to avoid all the warnings.

The other ones in theory apply to some previous versions, but nobody seems to have run into any of the problems, and they are quite unlikely to actually happen due to the way the code is otherwise structured and the amount of code in there. I was reluctant to add stable tags to all of them for that reason since they're quite big.

johannes

author maintainer
#1 lore raw

PATCH 1/7 mac80211: annotate station rcu dereferences

The new RCU lockdep support warns about these in some contexts -- make it aware of the locks used to protect all this. Different locks are used in different contexts which unfortunately means we can't get perfect checking.

Also remove rcu_dereference() from two places that don't actually dereference the pointers.

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/main.c     |    4 ++--
 net/mac80211/sta_info.c |   20 ++++++++++++++++----
 2 files changed, 18 insertions(+), 6 deletions(-)
net/mac80211/sta_info.c +16 -4
@@ -93,12 +93,18 @@ struct sta_info *sta_info_get(struct iee
 	struct ieee80211_local *local = sdata->local;
 	struct sta_info *sta;
 
-	sta = rcu_dereference(local->sta_hash[STA_HASH(addr)]);
+	sta = rcu_dereference_check(local->sta_hash[STA_HASH(addr)],
+				    rcu_read_lock_held() ||
+				    lockdep_is_held(&local->sta_lock) ||
+				    lockdep_is_held(&local->sta_mtx));
 	while (sta) {
 		if (sta->sdata == sdata &&
 		    memcmp(sta->sta.addr, addr, ETH_ALEN) == 0)
 			break;
-		sta = rcu_dereference(sta->hnext);
+		sta = rcu_dereference_check(sta->hnext,
+					    rcu_read_lock_held() ||
+					    lockdep_is_held(&local->sta_lock) ||
+					    lockdep_is_held(&local->sta_mtx));
 	}
 	return sta;
 }

@@ -113,13 +119,19 @@ struct sta_info *sta_info_get_bss(struct
 	struct ieee80211_local *local = sdata->local;
 	struct sta_info *sta;
 
-	sta = rcu_dereference(local->sta_hash[STA_HASH(addr)]);
+	sta = rcu_dereference_check(local->sta_hash[STA_HASH(addr)],
+				    rcu_read_lock_held() ||
+				    lockdep_is_held(&local->sta_lock) ||
+				    lockdep_is_held(&local->sta_mtx));
 	while (sta) {
 		if ((sta->sdata == sdata ||
 		     sta->sdata->bss == sdata->bss) &&
 		    memcmp(sta->sta.addr, addr, ETH_ALEN) == 0)
 			break;
-		sta = rcu_dereference(sta->hnext);
+		sta = rcu_dereference_check(sta->hnext,
+					    rcu_read_lock_held() ||
+					    lockdep_is_held(&local->sta_lock) ||
+					    lockdep_is_held(&local->sta_mtx));
 	}
 	return sta;
 }
net/mac80211/main.c +2 -2
@@ -225,11 +225,11 @@ void ieee80211_bss_info_change_notify(st
 			switch (sdata->vif.type) {
 			case NL80211_IFTYPE_AP:
 				sdata->vif.bss_conf.enable_beacon =
-					!!rcu_dereference(sdata->u.ap.beacon);
+					!!sdata->u.ap.beacon;
 				break;
 			case NL80211_IFTYPE_ADHOC:
 				sdata->vif.bss_conf.enable_beacon =
-					!!rcu_dereference(sdata->u.ibss.presp);
+					!!sdata->u.ibss.presp;
 				break;
 			case NL80211_IFTYPE_MESH_POINT:
 				sdata->vif.bss_conf.enable_beacon = true;
author maintainer
#2 lore raw

PATCH 2/7 mac80211: fix station destruction problem

In reply to Johannes Berg

When a station w/o a key is destroyed, or when a driver submits work for a station and thereby references it again, it seems like potentially we could reference the station structure while it is being destroyed.

Wait for an RCU grace period to elapse before finishing destroying the station after we have removed the station from the driver and from the hash table etc., even in the case where no key is associated with the station.

Also, there's no point in deleting the plink timer here since it'll be properly deleted just a bit later.

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/sta_info.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
net/mac80211/sta_info.c +9 -6
@@ -645,9 +645,6 @@ static int __must_check __sta_info_destr
 		 * may mean it is removed from hardware which requires that
 		 * the key->sta pointer is still valid, so flush the key todo
 		 * list here.
-		 *
-		 * ieee80211_key_todo() will synchronize_rcu() so after this
-		 * nothing can reference this sta struct any more.
 		 */
 		ieee80211_key_todo();
 

@@ -679,11 +676,17 @@ static int __must_check __sta_info_destr
 		sdata = sta->sdata;
 	}
 
+	/*
+	 * At this point, after we wait for an RCU grace period,
+	 * neither mac80211 nor the driver can reference this
+	 * sta struct any more except by still existing timers
+	 * associated with this station that we clean up below.
+	 */
+	synchronize_rcu();
+
 #ifdef CONFIG_MAC80211_MESH
-	if (ieee80211_vif_is_mesh(&sdata->vif)) {
+	if (ieee80211_vif_is_mesh(&sdata->vif))
 		mesh_accept_plinks_update(sdata);
-		del_timer(&sta->plink_timer);
-	}
 #endif
 
 #ifdef CONFIG_MAC80211_VERBOSE_DEBUG
author maintainer
#3 lore raw

PATCH 3/7 mac80211: remove irq disabling for sta lock

In reply to Johannes Berg

All other places except one in the TX path, which has BHs disabled, and it also cannot be locked from interrupts so disabling IRQs is not necessary.

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/tx.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
net/mac80211/tx.c +2 -3
@@ -1144,13 +1144,12 @@ ieee80211_tx_prepare(struct ieee80211_su
 
 	if (tx->sta && ieee80211_is_data_qos(hdr->frame_control) &&
 	    (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION)) {
-		unsigned long flags;
 		struct tid_ampdu_tx *tid_tx;
 
 		qc = ieee80211_get_qos_ctl(hdr);
 		tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
 
-		spin_lock_irqsave(&tx->sta->lock, flags);
+		spin_lock(&tx->sta->lock);
 		/*
 		 * XXX: This spinlock could be fairly expensive, but see the
 		 *	comment in agg-tx.c:ieee80211_agg_tx_operational().

@@ -1175,7 +1174,7 @@ ieee80211_tx_prepare(struct ieee80211_su
 			info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
 			__skb_queue_tail(&tid_tx->pending, skb);
 		}
-		spin_unlock_irqrestore(&tx->sta->lock, flags);
+		spin_unlock(&tx->sta->lock);
 
 		if (unlikely(queued))
 			return TX_QUEUED;
author maintainer
#4 lore raw

PATCH 4/7 mac80211: remove ieee80211_sta_stop_rx_ba_session

In reply to Johannes Berg

All callers of ieee80211_sta_stop_rx_ba_session can just call __ieee80211_stop_rx_ba_session instead because they already have the station struct, so do that and remove ieee80211_sta_stop_rx_ba_session.

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/agg-rx.c      |   24 ++----------------------
 net/mac80211/ht.c          |    3 +--
 net/mac80211/ieee80211_i.h |    2 --
 net/mac80211/rx.c          |    4 ++--
 4 files changed, 5 insertions(+), 28 deletions(-)
net/mac80211/agg-rx.c +2 -22
@@ -79,28 +79,9 @@ void __ieee80211_stop_rx_ba_session(stru
 	spin_unlock_bh(&sta->lock);
 }
 
-void ieee80211_sta_stop_rx_ba_session(struct ieee80211_sub_if_data *sdata, u8 *ra, u16 tid,
-					u16 initiator, u16 reason)
-{
-	struct sta_info *sta;
-
-	rcu_read_lock();
-
-	sta = sta_info_get(sdata, ra);
-	if (!sta) {
-		rcu_read_unlock();
-		return;
-	}
-
-	__ieee80211_stop_rx_ba_session(sta, tid, initiator, reason);
-
-	rcu_read_unlock();
-}
-
 /*
  * After accepting the AddBA Request we activated a timer,
  * resetting it after each frame that arrives from the originator.
- * if this timer expires ieee80211_sta_stop_rx_ba_session will be executed.
  */
 static void sta_rx_agg_session_timer_expired(unsigned long data)
 {

@@ -116,9 +97,8 @@ static void sta_rx_agg_session_timer_exp
 #ifdef CONFIG_MAC80211_HT_DEBUG
 	printk(KERN_DEBUG "rx session timer expired on tid %d\n", (u16)*ptid);
 #endif
-	ieee80211_sta_stop_rx_ba_session(sta->sdata, sta->sta.addr,
-					 (u16)*ptid, WLAN_BACK_TIMER,
-					 WLAN_REASON_QSTA_TIMEOUT);
+	__ieee80211_stop_rx_ba_session(sta, *ptid, WLAN_BACK_RECIPIENT,
+				       WLAN_REASON_QSTA_TIMEOUT);
 }
 
 static void ieee80211_send_addba_resp(struct ieee80211_sub_if_data *sdata, u8 *da, u16 tid,
net/mac80211/ieee80211_i.h +0 -2
@@ -1098,8 +1098,6 @@ int ieee80211_send_smps_action(struct ie
 			       enum ieee80211_smps_mode smps, const u8 *da,
 			       const u8 *bssid);
 
-void ieee80211_sta_stop_rx_ba_session(struct ieee80211_sub_if_data *sdata, u8 *da,
-				u16 tid, u16 initiator, u16 reason);
 void __ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
 				    u16 initiator, u16 reason);
 void ieee80211_sta_tear_down_BA_sessions(struct sta_info *sta);
net/mac80211/rx.c +2 -2
@@ -739,8 +739,8 @@ static void ieee80211_rx_reorder_ampdu(s
 	/* if this mpdu is fragmented - terminate rx aggregation session */
 	sc = le16_to_cpu(hdr->seq_ctrl);
 	if (sc & IEEE80211_SCTL_FRAG) {
-		ieee80211_sta_stop_rx_ba_session(sta->sdata, sta->sta.addr,
-			tid, 0, WLAN_REASON_QSTA_REQUIRE_SETUP);
+		__ieee80211_stop_rx_ba_session(sta, tid, WLAN_BACK_RECIPIENT,
+					       WLAN_REASON_QSTA_REQUIRE_SETUP);
 		dev_kfree_skb(skb);
 		return;
 	}
net/mac80211/ht.c +1 -2
@@ -175,8 +175,7 @@ void ieee80211_process_delba(struct ieee
 #endif /* CONFIG_MAC80211_HT_DEBUG */
 
 	if (initiator == WLAN_BACK_INITIATOR)
-		ieee80211_sta_stop_rx_ba_session(sdata, sta->sta.addr, tid,
-						 WLAN_BACK_INITIATOR, 0);
+		__ieee80211_stop_rx_ba_session(sta, tid, WLAN_BACK_INITIATOR, 0);
 	else { /* WLAN_BACK_RECIPIENT */
 		spin_lock_bh(&sta->lock);
 		if (sta->ampdu_mlme.tid_state_tx[tid] & HT_ADDBA_REQUESTED_MSK)
author maintainer
#5 lore raw

PATCH 5/7 mac80211: rename WLAN_STA_SUSPEND to WLAN_STA_BLOCK_BA

In reply to Johannes Berg

I want to use it during station destruction as well so rename it to WLAN_STA_BLOCK_BA which is also the only use of it now.

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/agg-rx.c   |    2 +-
 net/mac80211/agg-tx.c   |    2 +-
 net/mac80211/pm.c       |    2 +-
 net/mac80211/sta_info.h |    6 +++---
 net/mac80211/util.c     |    2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)
net/mac80211/agg-rx.c +1 -1
@@ -173,7 +173,7 @@ void ieee80211_process_addba_request(str
 
 	status = WLAN_STATUS_REQUEST_DECLINED;
 
-	if (test_sta_flags(sta, WLAN_STA_SUSPEND)) {
+	if (test_sta_flags(sta, WLAN_STA_BLOCK_BA)) {
 #ifdef CONFIG_MAC80211_HT_DEBUG
 		printk(KERN_DEBUG "Suspend in progress. "
 		       "Denying ADDBA request\n");
net/mac80211/agg-tx.c +1 -1
@@ -245,7 +245,7 @@ int ieee80211_start_tx_ba_session(struct
 		return -EINVAL;
 	}
 
-	if (test_sta_flags(sta, WLAN_STA_SUSPEND)) {
+	if (test_sta_flags(sta, WLAN_STA_BLOCK_BA)) {
 #ifdef CONFIG_MAC80211_HT_DEBUG
 		printk(KERN_DEBUG "Suspend in progress. "
 		       "Denying BA session request\n");
net/mac80211/pm.c +1 -1
@@ -46,7 +46,7 @@ int __ieee80211_suspend(struct ieee80211
 
 	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
 		list_for_each_entry_rcu(sta, &local->sta_list, list) {
-			set_sta_flags(sta, WLAN_STA_SUSPEND);
+			set_sta_flags(sta, WLAN_STA_BLOCK_BA);
 			ieee80211_sta_tear_down_BA_sessions(sta);
 		}
 	}
net/mac80211/sta_info.h +3 -3
@@ -35,8 +35,8 @@
  *	IEEE80211_TX_CTL_CLEAR_PS_FILT control flag) when the next
  *	frame to this station is transmitted.
  * @WLAN_STA_MFP: Management frame protection is used with this STA.
- * @WLAN_STA_SUSPEND: Set/cleared during a suspend/resume cycle.
- *	Used to deny ADDBA requests (both TX and RX).
+ * @WLAN_STA_BLOCK_BA: Used to deny ADDBA requests (both TX and RX)
+ *	during suspend/resume.
  * @WLAN_STA_PS_DRIVER: driver requires keeping this station in
  *	power-save mode logically to flush frames that might still
  *	be in the queues

@@ -57,7 +57,7 @@ enum ieee80211_sta_info_flags {
 	WLAN_STA_WDS		= 1<<7,
 	WLAN_STA_CLEAR_PS_FILT	= 1<<9,
 	WLAN_STA_MFP		= 1<<10,
-	WLAN_STA_SUSPEND	= 1<<11,
+	WLAN_STA_BLOCK_BA	= 1<<11,
 	WLAN_STA_PS_DRIVER	= 1<<12,
 	WLAN_STA_PSPOLL		= 1<<13,
 	WLAN_STA_DISASSOC       = 1<<14,
net/mac80211/util.c +1 -1
@@ -1140,7 +1140,7 @@ int ieee80211_reconfig(struct ieee80211_
 
 	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
 		list_for_each_entry_rcu(sta, &local->sta_list, list) {
-			clear_sta_flags(sta, WLAN_STA_SUSPEND);
+			clear_sta_flags(sta, WLAN_STA_BLOCK_BA);
 		}
 	}
author maintainer
#6 lore raw

PATCH 6/7 mac80211: clean up/fix aggregation code

In reply to Johannes Berg

The aggregation code has a number of quirks, like inventing an unneeded WLAN_BACK_TIMER value and leaking memory under certain circumstances during station destruction. Fix these issues by using the regular aggregation session teardown code and blocking new aggregation sessions, all before the station is really destructed.

As a side effect, this gets rid of the long code block to destroy aggregation safely.

Additionally, rename tid_state_rx which can only have the values IDLE and OPERATIONAL to tid_active_rx to make it easier to understand that there is no bitwise stuff going on on the RX side -- the TX side remains because it needs to keep track of the driver and peer states.

Signed-off-by Johannes Berg <[email protected]>
---
 include/linux/ieee80211.h  |    1 
 net/mac80211/agg-rx.c      |   48 ++++++++++++++++---------------------
 net/mac80211/debugfs_sta.c |   10 +++----
 net/mac80211/rx.c          |    5 +--
 net/mac80211/sta_info.c    |   58 +++++++--------------------------------------
 net/mac80211/sta_info.h    |    6 +---
 6 files changed, 40 insertions(+), 88 deletions(-)
include/linux/ieee80211.h +0 -1
@@ -1324,7 +1324,6 @@ enum ieee80211_back_actioncode {
 enum ieee80211_back_parties {
 	WLAN_BACK_RECIPIENT = 0,
 	WLAN_BACK_INITIATOR = 1,
-	WLAN_BACK_TIMER = 2,
 };
 
 /* SA Query action */
net/mac80211/agg-rx.c +21 -27

91 lines not on this page · raw message

net/mac80211/rx.c +2 -3
@@ -720,7 +720,7 @@ static void ieee80211_rx_reorder_ampdu(s
 
 	tid = *ieee80211_get_qos_ctl(hdr) & IEEE80211_QOS_CTL_TID_MASK;
 
-	if (sta->ampdu_mlme.tid_state_rx[tid] != HT_AGG_STATE_OPERATIONAL)
+	if (!sta->ampdu_mlme.tid_active_rx[tid])
 		goto dont_reorder;
 
 	tid_agg_rx = sta->ampdu_mlme.tid_rx[tid];

@@ -1804,8 +1804,7 @@ ieee80211_rx_h_ctrl(struct ieee80211_rx_
 		if (!rx->sta)
 			return RX_DROP_MONITOR;
 		tid = le16_to_cpu(bar->control) >> 12;
-		if (rx->sta->ampdu_mlme.tid_state_rx[tid]
-					!= HT_AGG_STATE_OPERATIONAL)
+		if (!rx->sta->ampdu_mlme.tid_active_rx[tid])
 			return RX_DROP_MONITOR;
 		tid_agg_rx = rx->sta->ampdu_mlme.tid_rx[tid];
net/mac80211/sta_info.c +10 -48

82 lines not on this page · raw message

net/mac80211/sta_info.h +2 -4
@@ -36,7 +36,7 @@
  *	frame to this station is transmitted.
  * @WLAN_STA_MFP: Management frame protection is used with this STA.
  * @WLAN_STA_BLOCK_BA: Used to deny ADDBA requests (both TX and RX)
- *	during suspend/resume.
+ *	during suspend/resume and station removal.
  * @WLAN_STA_PS_DRIVER: driver requires keeping this station in
  *	power-save mode logically to flush frames that might still
  *	be in the queues

@@ -106,7 +106,6 @@ struct tid_ampdu_tx {
  * @buf_size: buffer size for incoming A-MPDUs
  * @timeout: reset timer value (in TUs).
  * @dialog_token: dialog token for aggregation session
- * @shutdown: this session is being shut down due to STA removal
  */
 struct tid_ampdu_rx {
 	struct sk_buff **reorder_buf;

@@ -118,7 +117,6 @@ struct tid_ampdu_rx {
 	u16 buf_size;
 	u16 timeout;
 	u8 dialog_token;
-	bool shutdown;
 };
 
 /**

@@ -156,7 +154,7 @@ enum plink_state {
  */
 struct sta_ampdu_mlme {
 	/* rx */
-	u8 tid_state_rx[STA_TID_NUM];
+	bool tid_active_rx[STA_TID_NUM];
 	struct tid_ampdu_rx *tid_rx[STA_TID_NUM];
 	/* tx */
 	u8 tid_state_tx[STA_TID_NUM];
net/mac80211/debugfs_sta.c +5 -5
@@ -119,7 +119,7 @@ STA_OPS(last_seq_ctrl);
 static ssize_t sta_agg_status_read(struct file *file, char __user *userbuf,
 					size_t count, loff_t *ppos)
 {
-	char buf[64 + STA_TID_NUM * 40], *p = buf;
+	char buf[71 + STA_TID_NUM * 40], *p = buf;
 	int i;
 	struct sta_info *sta = file->private_data;
 

@@ -127,16 +127,16 @@ static ssize_t sta_agg_status_read(struc
 	p += scnprintf(p, sizeof(buf) + buf - p, "next dialog_token: %#02x\n",
 			sta->ampdu_mlme.dialog_token_allocator + 1);
 	p += scnprintf(p, sizeof(buf) + buf - p,
-		       "TID\t\tRX\tDTKN\tSSN\t\tTX\tDTKN\tSSN\tpending\n");
+		       "TID\t\tRX active\tDTKN\tSSN\t\tTX\tDTKN\tSSN\tpending\n");
 	for (i = 0; i < STA_TID_NUM; i++) {
 		p += scnprintf(p, sizeof(buf) + buf - p, "%02d", i);
 		p += scnprintf(p, sizeof(buf) + buf - p, "\t\t%x",
-				sta->ampdu_mlme.tid_state_rx[i]);
+				sta->ampdu_mlme.tid_active_rx[i]);
 		p += scnprintf(p, sizeof(buf) + buf - p, "\t%#.2x",
-				sta->ampdu_mlme.tid_state_rx[i] ?
+				sta->ampdu_mlme.tid_active_rx[i] ?
 				sta->ampdu_mlme.tid_rx[i]->dialog_token : 0);
 		p += scnprintf(p, sizeof(buf) + buf - p, "\t%#.3x",
-				sta->ampdu_mlme.tid_state_rx[i] ?
+				sta->ampdu_mlme.tid_active_rx[i] ?
 				sta->ampdu_mlme.tid_rx[i]->ssn : 0);
 
 		p += scnprintf(p, sizeof(buf) + buf - p, "\t\t%x",
author maintainer
#7 lore raw

PATCH 7/7 mac80211: fix some RX aggregation locking

In reply to Johannes Berg

A few places in mac80211 do not currently acquire
the sta lock for RX aggregation, but they should.

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/rx.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)
net/mac80211/rx.c +15 -4
@@ -720,14 +720,16 @@ static void ieee80211_rx_reorder_ampdu(s
 
 	tid = *ieee80211_get_qos_ctl(hdr) & IEEE80211_QOS_CTL_TID_MASK;
 
+	spin_lock(&sta->lock);
+
 	if (!sta->ampdu_mlme.tid_active_rx[tid])
-		goto dont_reorder;
+		goto dont_reorder_unlock;
 
 	tid_agg_rx = sta->ampdu_mlme.tid_rx[tid];
 
 	/* qos null data frames are excluded */
 	if (unlikely(hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_NULLFUNC)))
-		goto dont_reorder;
+		goto dont_reorder_unlock;
 
 	/* new, potentially un-ordered, ampdu frame - process it */
 

@@ -739,15 +741,20 @@ static void ieee80211_rx_reorder_ampdu(s
 	/* if this mpdu is fragmented - terminate rx aggregation session */
 	sc = le16_to_cpu(hdr->seq_ctrl);
 	if (sc & IEEE80211_SCTL_FRAG) {
+		spin_unlock(&sta->lock);
 		__ieee80211_stop_rx_ba_session(sta, tid, WLAN_BACK_RECIPIENT,
 					       WLAN_REASON_QSTA_REQUIRE_SETUP);
 		dev_kfree_skb(skb);
 		return;
 	}
 
-	if (ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, skb, frames))
+	if (ieee80211_sta_manage_reorder_buf(hw, tid_agg_rx, skb, frames)) {
+		spin_unlock(&sta->lock);
 		return;
+	}
 
+ dont_reorder_unlock:
+	spin_unlock(&sta->lock);
  dont_reorder:
 	__skb_queue_tail(frames, skb);
 }

@@ -1803,9 +1810,12 @@ ieee80211_rx_h_ctrl(struct ieee80211_rx_
 	if (ieee80211_is_back_req(bar->frame_control)) {
 		if (!rx->sta)
 			return RX_DROP_MONITOR;
+		spin_lock(&rx->sta->lock);
 		tid = le16_to_cpu(bar->control) >> 12;
-		if (!rx->sta->ampdu_mlme.tid_active_rx[tid])
+		if (!rx->sta->ampdu_mlme.tid_active_rx[tid]) {
+			spin_unlock(&rx->sta->lock);
 			return RX_DROP_MONITOR;
+		}
 		tid_agg_rx = rx->sta->ampdu_mlme.tid_rx[tid];
 
 		start_seq_num = le16_to_cpu(bar->start_seq_num) >> 4;

@@ -1819,6 +1829,7 @@ ieee80211_rx_h_ctrl(struct ieee80211_rx_
 		ieee80211_release_reorder_frames(hw, tid_agg_rx, start_seq_num,
 						 frames);
 		kfree_skb(skb);
+		spin_unlock(&rx->sta->lock);
 		return RX_QUEUED;
 	}