Commit 23e6a7ea authored by Johannes Berg's avatar Johannes Berg Committed by John W. Linville

mac80211: fix race in TX aggregation

When disabling TX aggregation because it was rejected or from
the timer (it was not accepted), there is a window where we
first set the state to operation, unlock, and then undo the
whole thing. Avoid that by splitting up the stop function.
Also get rid of the pointless sta_info indirection in the timer.
Signed-off-by: default avatarJohannes Berg <johannes@sipsolutions.net>
Signed-off-by: default avatarJohn W. Linville <linville@tuxdriver.com>
parent 86ab6c5a
...@@ -123,6 +123,34 @@ void ieee80211_send_bar(struct ieee80211_sub_if_data *sdata, u8 *ra, u16 tid, u1 ...@@ -123,6 +123,34 @@ void ieee80211_send_bar(struct ieee80211_sub_if_data *sdata, u8 *ra, u16 tid, u1
ieee80211_tx_skb(sdata, skb, 0); ieee80211_tx_skb(sdata, skb, 0);
} }
static int __ieee80211_stop_tx_ba_session(struct ieee80211_local *local,
struct sta_info *sta, u16 tid,
enum ieee80211_back_parties initiator)
{
int ret;
u8 *state;
state = &sta->ampdu_mlme.tid_state_tx[tid];
if (local->hw.ampdu_queues)
ieee80211_stop_queue(&local->hw, sta->tid_to_tx_q[tid]);
*state = HT_AGG_STATE_REQ_STOP_BA_MSK |
(initiator << HT_AGG_STATE_INITIATOR_SHIFT);
ret = local->ops->ampdu_action(&local->hw, IEEE80211_AMPDU_TX_STOP,
&sta->sta, tid, NULL);
/* HW shall not deny going back to legacy */
if (WARN_ON(ret)) {
*state = HT_AGG_STATE_OPERATIONAL;
if (local->hw.ampdu_queues)
ieee80211_wake_queue(&local->hw, sta->tid_to_tx_q[tid]);
}
return ret;
}
/* /*
* After sending add Block Ack request we activated a timer until * After sending add Block Ack request we activated a timer until
* add Block Ack response will arrive from the recipient. * add Block Ack response will arrive from the recipient.
...@@ -135,23 +163,13 @@ static void sta_addba_resp_timer_expired(unsigned long data) ...@@ -135,23 +163,13 @@ static void sta_addba_resp_timer_expired(unsigned long data)
* flow in sta_info_create gives the TID as data, while the timer_to_id * flow in sta_info_create gives the TID as data, while the timer_to_id
* array gives the sta through container_of */ * array gives the sta through container_of */
u16 tid = *(u8 *)data; u16 tid = *(u8 *)data;
struct sta_info *temp_sta = container_of((void *)data, struct sta_info *sta = container_of((void *)data,
struct sta_info, timer_to_tid[tid]); struct sta_info, timer_to_tid[tid]);
struct ieee80211_local *local = sta->local;
struct ieee80211_local *local = temp_sta->local;
struct ieee80211_hw *hw = &local->hw;
struct sta_info *sta;
u8 *state; u8 *state;
rcu_read_lock();
sta = sta_info_get(local, temp_sta->sta.addr);
if (!sta) {
rcu_read_unlock();
return;
}
state = &sta->ampdu_mlme.tid_state_tx[tid]; state = &sta->ampdu_mlme.tid_state_tx[tid];
/* check if the TID waits for addBA response */ /* check if the TID waits for addBA response */
spin_lock_bh(&sta->lock); spin_lock_bh(&sta->lock);
if (!(*state & HT_ADDBA_REQUESTED_MSK)) { if (!(*state & HT_ADDBA_REQUESTED_MSK)) {
...@@ -161,21 +179,15 @@ static void sta_addba_resp_timer_expired(unsigned long data) ...@@ -161,21 +179,15 @@ static void sta_addba_resp_timer_expired(unsigned long data)
printk(KERN_DEBUG "timer expired on tid %d but we are not " printk(KERN_DEBUG "timer expired on tid %d but we are not "
"expecting addBA response there", tid); "expecting addBA response there", tid);
#endif #endif
goto timer_expired_exit; return;
} }
#ifdef CONFIG_MAC80211_HT_DEBUG #ifdef CONFIG_MAC80211_HT_DEBUG
printk(KERN_DEBUG "addBA response timer expired on tid %d\n", tid); printk(KERN_DEBUG "addBA response timer expired on tid %d\n", tid);
#endif #endif
/* go through the state check in stop_BA_session */ __ieee80211_stop_tx_ba_session(local, sta, tid, WLAN_BACK_INITIATOR);
*state = HT_AGG_STATE_OPERATIONAL;
spin_unlock_bh(&sta->lock); spin_unlock_bh(&sta->lock);
ieee80211_stop_tx_ba_session(hw, temp_sta->sta.addr, tid,
WLAN_BACK_INITIATOR);
timer_expired_exit:
rcu_read_unlock();
} }
int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid) int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
...@@ -187,6 +199,9 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid) ...@@ -187,6 +199,9 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
u8 *state; u8 *state;
int ret = 0; int ret = 0;
if (WARN_ON(!local->ops->ampdu_action))
return -EINVAL;
if ((tid >= STA_TID_NUM) || !(hw->flags & IEEE80211_HW_AMPDU_AGGREGATION)) if ((tid >= STA_TID_NUM) || !(hw->flags & IEEE80211_HW_AMPDU_AGGREGATION))
return -EINVAL; return -EINVAL;
...@@ -280,9 +295,8 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid) ...@@ -280,9 +295,8 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
/* This is slightly racy because the queue isn't stopped */ /* This is slightly racy because the queue isn't stopped */
start_seq_num = sta->tid_seq[tid]; start_seq_num = sta->tid_seq[tid];
if (local->ops->ampdu_action) ret = local->ops->ampdu_action(hw, IEEE80211_AMPDU_TX_START,
ret = local->ops->ampdu_action(hw, IEEE80211_AMPDU_TX_START, &sta->sta, tid, &start_seq_num);
&sta->sta, tid, &start_seq_num);
if (ret) { if (ret) {
/* No need to requeue the packets in the agg queue, since we /* No need to requeue the packets in the agg queue, since we
...@@ -423,6 +437,9 @@ int ieee80211_stop_tx_ba_session(struct ieee80211_hw *hw, ...@@ -423,6 +437,9 @@ int ieee80211_stop_tx_ba_session(struct ieee80211_hw *hw,
u8 *state; u8 *state;
int ret = 0; int ret = 0;
if (WARN_ON(!local->ops->ampdu_action))
return -EINVAL;
if (tid >= STA_TID_NUM) if (tid >= STA_TID_NUM)
return -EINVAL; return -EINVAL;
...@@ -439,7 +456,7 @@ int ieee80211_stop_tx_ba_session(struct ieee80211_hw *hw, ...@@ -439,7 +456,7 @@ int ieee80211_stop_tx_ba_session(struct ieee80211_hw *hw,
if (*state != HT_AGG_STATE_OPERATIONAL) { if (*state != HT_AGG_STATE_OPERATIONAL) {
ret = -ENOENT; ret = -ENOENT;
goto stop_BA_exit; goto unlock;
} }
#ifdef CONFIG_MAC80211_HT_DEBUG #ifdef CONFIG_MAC80211_HT_DEBUG
...@@ -447,27 +464,13 @@ int ieee80211_stop_tx_ba_session(struct ieee80211_hw *hw, ...@@ -447,27 +464,13 @@ int ieee80211_stop_tx_ba_session(struct ieee80211_hw *hw,
ra, tid); ra, tid);
#endif /* CONFIG_MAC80211_HT_DEBUG */ #endif /* CONFIG_MAC80211_HT_DEBUG */
if (hw->ampdu_queues) ret = __ieee80211_stop_tx_ba_session(local, sta, tid, initiator);
ieee80211_stop_queue(hw, sta->tid_to_tx_q[tid]);
*state = HT_AGG_STATE_REQ_STOP_BA_MSK |
(initiator << HT_AGG_STATE_INITIATOR_SHIFT);
if (local->ops->ampdu_action) unlock:
ret = local->ops->ampdu_action(hw, IEEE80211_AMPDU_TX_STOP,
&sta->sta, tid, NULL);
/* HW shall not deny going back to legacy */
if (WARN_ON(ret)) {
*state = HT_AGG_STATE_OPERATIONAL;
if (hw->ampdu_queues)
ieee80211_wake_queue(hw, sta->tid_to_tx_q[tid]);
goto stop_BA_exit;
}
stop_BA_exit:
spin_unlock_bh(&sta->lock); spin_unlock_bh(&sta->lock);
rcu_read_unlock(); rcu_read_unlock();
return ret; return ret;
} }
EXPORT_SYMBOL(ieee80211_stop_tx_ba_session); EXPORT_SYMBOL(ieee80211_stop_tx_ba_session);
...@@ -623,10 +626,8 @@ void ieee80211_process_addba_resp(struct ieee80211_local *local, ...@@ -623,10 +626,8 @@ void ieee80211_process_addba_resp(struct ieee80211_local *local,
spin_unlock_bh(&sta->lock); spin_unlock_bh(&sta->lock);
} else { } else {
sta->ampdu_mlme.addba_req_num[tid]++; sta->ampdu_mlme.addba_req_num[tid]++;
/* this will allow the state check in stop_BA_session */ __ieee80211_stop_tx_ba_session(local, sta, tid,
*state = HT_AGG_STATE_OPERATIONAL; WLAN_BACK_INITIATOR);
spin_unlock_bh(&sta->lock); spin_unlock_bh(&sta->lock);
ieee80211_stop_tx_ba_session(hw, sta->sta.addr, tid,
WLAN_BACK_INITIATOR);
} }
} }
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment