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

mac80211: fix TX a-MPDU locking

During my quest to make mac80211 not have any RCU
warnings from sparse, I came across the a-MPDU code
again and it wasn't quite clear why it isn't racy.
So instead of assigning the tid_tx array with just
the spinlock held in ieee80211_start_tx_ba_session
use a separate temporary array protected only by
the spinlock and protect all assignments to the
"live" array by both the spinlock and the mutex so
that other code is easily verified to be correct.

Due to pointer assignment atomicity I don't think
this is a real issue, but I'm not sure, especially
on Alpha the current code might be problematic.
Signed-off-by: default avatarJohannes Berg <johannes.berg@intel.com>
Signed-off-by: default avatarJohn W. Linville <linville@tuxdriver.com>
parent 7527a782
...@@ -136,6 +136,14 @@ void ieee80211_send_bar(struct ieee80211_sub_if_data *sdata, u8 *ra, u16 tid, u1 ...@@ -136,6 +136,14 @@ void ieee80211_send_bar(struct ieee80211_sub_if_data *sdata, u8 *ra, u16 tid, u1
ieee80211_tx_skb(sdata, skb); ieee80211_tx_skb(sdata, skb);
} }
void ieee80211_assign_tid_tx(struct sta_info *sta, int tid,
struct tid_ampdu_tx *tid_tx)
{
lockdep_assert_held(&sta->ampdu_mlme.mtx);
lockdep_assert_held(&sta->lock);
rcu_assign_pointer(sta->ampdu_mlme.tid_tx[tid], tid_tx);
}
static void kfree_tid_tx(struct rcu_head *rcu_head) static void kfree_tid_tx(struct rcu_head *rcu_head)
{ {
struct tid_ampdu_tx *tid_tx = struct tid_ampdu_tx *tid_tx =
...@@ -161,7 +169,7 @@ int ___ieee80211_stop_tx_ba_session(struct sta_info *sta, u16 tid, ...@@ -161,7 +169,7 @@ int ___ieee80211_stop_tx_ba_session(struct sta_info *sta, u16 tid,
if (test_bit(HT_AGG_STATE_WANT_START, &tid_tx->state)) { if (test_bit(HT_AGG_STATE_WANT_START, &tid_tx->state)) {
/* not even started yet! */ /* not even started yet! */
rcu_assign_pointer(sta->ampdu_mlme.tid_tx[tid], NULL); ieee80211_assign_tid_tx(sta, tid, NULL);
spin_unlock_bh(&sta->lock); spin_unlock_bh(&sta->lock);
call_rcu(&tid_tx->rcu_head, kfree_tid_tx); call_rcu(&tid_tx->rcu_head, kfree_tid_tx);
return 0; return 0;
...@@ -318,7 +326,7 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid) ...@@ -318,7 +326,7 @@ void ieee80211_tx_ba_session_handle_start(struct sta_info *sta, int tid)
" tid %d\n", tid); " tid %d\n", tid);
#endif #endif
spin_lock_bh(&sta->lock); spin_lock_bh(&sta->lock);
rcu_assign_pointer(sta->ampdu_mlme.tid_tx[tid], NULL); ieee80211_assign_tid_tx(sta, tid, NULL);
spin_unlock_bh(&sta->lock); spin_unlock_bh(&sta->lock);
ieee80211_wake_queue_agg(local, tid); ieee80211_wake_queue_agg(local, tid);
...@@ -398,7 +406,7 @@ int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid, ...@@ -398,7 +406,7 @@ int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid,
tid_tx = sta->ampdu_mlme.tid_tx[tid]; tid_tx = sta->ampdu_mlme.tid_tx[tid];
/* check if the TID is not in aggregation flow already */ /* check if the TID is not in aggregation flow already */
if (tid_tx) { if (tid_tx || sta->ampdu_mlme.tid_start_tx[tid]) {
#ifdef CONFIG_MAC80211_HT_DEBUG #ifdef CONFIG_MAC80211_HT_DEBUG
printk(KERN_DEBUG "BA request denied - session is not " printk(KERN_DEBUG "BA request denied - session is not "
"idle on tid %u\n", tid); "idle on tid %u\n", tid);
...@@ -433,8 +441,11 @@ int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid, ...@@ -433,8 +441,11 @@ int ieee80211_start_tx_ba_session(struct ieee80211_sta *pubsta, u16 tid,
sta->ampdu_mlme.dialog_token_allocator++; sta->ampdu_mlme.dialog_token_allocator++;
tid_tx->dialog_token = sta->ampdu_mlme.dialog_token_allocator; tid_tx->dialog_token = sta->ampdu_mlme.dialog_token_allocator;
/* finally, assign it to the array */ /*
rcu_assign_pointer(sta->ampdu_mlme.tid_tx[tid], tid_tx); * Finally, assign it to the start array; the work item will
* collect it and move it to the normal array.
*/
sta->ampdu_mlme.tid_start_tx[tid] = tid_tx;
ieee80211_queue_work(&local->hw, &sta->ampdu_mlme.work); ieee80211_queue_work(&local->hw, &sta->ampdu_mlme.work);
...@@ -697,7 +708,7 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u8 tid) ...@@ -697,7 +708,7 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_vif *vif, u8 *ra, u8 tid)
ieee80211_agg_splice_packets(local, tid_tx, tid); ieee80211_agg_splice_packets(local, tid_tx, tid);
/* future packets must not find the tid_tx struct any more */ /* future packets must not find the tid_tx struct any more */
rcu_assign_pointer(sta->ampdu_mlme.tid_tx[tid], NULL); ieee80211_assign_tid_tx(sta, tid, NULL);
ieee80211_agg_splice_finish(local, tid); ieee80211_agg_splice_finish(local, tid);
......
...@@ -140,13 +140,28 @@ void ieee80211_ba_session_work(struct work_struct *work) ...@@ -140,13 +140,28 @@ void ieee80211_ba_session_work(struct work_struct *work)
sta, tid, WLAN_BACK_RECIPIENT, sta, tid, WLAN_BACK_RECIPIENT,
WLAN_REASON_QSTA_TIMEOUT, true); WLAN_REASON_QSTA_TIMEOUT, true);
tid_tx = sta->ampdu_mlme.tid_tx[tid]; tid_tx = sta->ampdu_mlme.tid_start_tx[tid];
if (!tid_tx) if (tid_tx) {
continue; /*
* Assign it over to the normal tid_tx array
* where it "goes live".
*/
spin_lock_bh(&sta->lock);
sta->ampdu_mlme.tid_start_tx[tid] = NULL;
/* could there be a race? */
if (sta->ampdu_mlme.tid_tx[tid])
kfree(tid_tx);
else
ieee80211_assign_tid_tx(sta, tid, tid_tx);
spin_unlock_bh(&sta->lock);
if (test_bit(HT_AGG_STATE_WANT_START, &tid_tx->state))
ieee80211_tx_ba_session_handle_start(sta, tid); ieee80211_tx_ba_session_handle_start(sta, tid);
else if (test_and_clear_bit(HT_AGG_STATE_WANT_STOP, continue;
}
tid_tx = sta->ampdu_mlme.tid_tx[tid];
if (tid_tx && test_and_clear_bit(HT_AGG_STATE_WANT_STOP,
&tid_tx->state)) &tid_tx->state))
___ieee80211_stop_tx_ba_session(sta, tid, ___ieee80211_stop_tx_ba_session(sta, tid,
WLAN_BACK_INITIATOR, WLAN_BACK_INITIATOR,
......
...@@ -152,6 +152,7 @@ struct tid_ampdu_rx { ...@@ -152,6 +152,7 @@ struct tid_ampdu_rx {
* *
* @tid_rx: aggregation info for Rx per TID -- RCU protected * @tid_rx: aggregation info for Rx per TID -- RCU protected
* @tid_tx: aggregation info for Tx per TID * @tid_tx: aggregation info for Tx per TID
* @tid_start_tx: sessions where start was requested
* @addba_req_num: number of times addBA request has been sent. * @addba_req_num: number of times addBA request has been sent.
* @dialog_token_allocator: dialog token enumerator for each new session; * @dialog_token_allocator: dialog token enumerator for each new session;
* @work: work struct for starting/stopping aggregation * @work: work struct for starting/stopping aggregation
...@@ -168,6 +169,7 @@ struct sta_ampdu_mlme { ...@@ -168,6 +169,7 @@ struct sta_ampdu_mlme {
/* tx */ /* tx */
struct work_struct work; struct work_struct work;
struct tid_ampdu_tx *tid_tx[STA_TID_NUM]; struct tid_ampdu_tx *tid_tx[STA_TID_NUM];
struct tid_ampdu_tx *tid_start_tx[STA_TID_NUM];
u8 addba_req_num[STA_TID_NUM]; u8 addba_req_num[STA_TID_NUM];
u8 dialog_token_allocator; u8 dialog_token_allocator;
}; };
...@@ -398,6 +400,8 @@ static inline u32 get_sta_flags(struct sta_info *sta) ...@@ -398,6 +400,8 @@ static inline u32 get_sta_flags(struct sta_info *sta)
return ret; return ret;
} }
void ieee80211_assign_tid_tx(struct sta_info *sta, int tid,
struct tid_ampdu_tx *tid_tx);
#define STA_HASH_SIZE 256 #define STA_HASH_SIZE 256
......
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