mac80211: aggregation: Convert timers to use timer_setup()

PATCH In review Kees Cook 2 participants 8 messages Oct 17, 2017 – Oct 18, 2017

Subsystems (3)

Networking Network drivers mac80211

lore

Share & tools

lore

Kees Cook

author

2017-10-17 20:25 UTC #0 lore raw

PATCH 1/1 mac80211: aggregation: Convert timers to use timer_setup()

In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly.

This removes the tid mapping array and expands the tid structures to add a pointer back to the station, along with the tid index itself.

Cc Johannes Berg <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
Resend, with linux-wireless in Cc (no idea how it got missed before)

---
 net/mac80211/agg-rx.c   | 41 +++++++++++++++++------------------------
 net/mac80211/agg-tx.c   | 42 ++++++++++++++++--------------------------
 net/mac80211/sta_info.c |  8 --------
 net/mac80211/sta_info.h | 12 ++++++++++--
 4 files changed, 43 insertions(+), 60 deletions(-)

net/mac80211/agg-rx.c +17 ~~-24~~

index 88cc1ae935ea..63aba6dbc92a 100644
@@ -151,21 +151,17 @@ EXPORT_SYMBOL(ieee80211_stop_rx_ba_session);
  * After accepting the AddBA Request we activated a timer,
  * resetting it after each frame that arrives from the originator.
  */
-static void sta_rx_agg_session_timer_expired(unsigned long data)
+static void sta_rx_agg_session_timer_expired(struct timer_list *t)
 {
-	/* not an elegant detour, but there is no choice as the timer passes
-	 * only one argument, and various sta_info are needed here, so init
-	 * flow in sta_info_create gives the TID as data, while the timer_to_id
-	 * array gives the sta through container_of */
-	u8 *ptid = (u8 *)data;
-	u8 *timer_to_id = ptid - *ptid;
-	struct sta_info *sta = container_of(timer_to_id, struct sta_info,
-					 timer_to_tid[0]);
+	struct tid_ampdu_rx *tid_rx_timer =
+		from_timer(tid_rx_timer, t, session_timer);
+	struct sta_info *sta = tid_rx_timer->sta;
+	u16 tid = tid_rx_timer->tid;
 	struct tid_ampdu_rx *tid_rx;
 	unsigned long timeout;
 
 	rcu_read_lock();
-	tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[*ptid]);
+	tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[tid]);
 	if (!tid_rx) {
 		rcu_read_unlock();
 		return;

@@ -180,21 +176,18 @@ static void sta_rx_agg_session_timer_expired(unsigned long data)
 	rcu_read_unlock();
 
 	ht_dbg(sta->sdata, "RX session timer expired on %pM tid %d\n",
-	       sta->sta.addr, (u16)*ptid);
+	       sta->sta.addr, tid);
 
-	set_bit(*ptid, sta->ampdu_mlme.tid_rx_timer_expired);
+	set_bit(tid, sta->ampdu_mlme.tid_rx_timer_expired);
 	ieee80211_queue_work(&sta->local->hw, &sta->ampdu_mlme.work);
 }
 
-static void sta_rx_agg_reorder_timer_expired(unsigned long data)
+static void sta_rx_agg_reorder_timer_expired(struct timer_list *t)
 {
-	u8 *ptid = (u8 *)data;
-	u8 *timer_to_id = ptid - *ptid;
-	struct sta_info *sta = container_of(timer_to_id, struct sta_info,
-			timer_to_tid[0]);
+	struct tid_ampdu_rx *tid_rx = from_timer(tid_rx, t, reorder_timer);
 
 	rcu_read_lock();
-	ieee80211_release_reorder_timeout(sta, *ptid);
+	ieee80211_release_reorder_timeout(tid_rx->sta, tid_rx->tid);
 	rcu_read_unlock();
 }

@@ -356,14 +349,12 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
 	spin_lock_init(&tid_agg_rx->reorder_lock);
 
 	/* rx timer */
-	setup_deferrable_timer(&tid_agg_rx->session_timer,
-			       sta_rx_agg_session_timer_expired,
-			       (unsigned long)&sta->timer_to_tid[tid]);
+	timer_setup(&tid_agg_rx->session_timer,
+		    sta_rx_agg_session_timer_expired, TIMER_DEFERRABLE);
 
 	/* rx reorder timer */
-	setup_timer(&tid_agg_rx->reorder_timer,
-		    sta_rx_agg_reorder_timer_expired,
-		    (unsigned long)&sta->timer_to_tid[tid]);
+	timer_setup(&tid_agg_rx->reorder_timer,
+		    sta_rx_agg_reorder_timer_expired, 0);
 
 	/* prepare reordering buffer */
 	tid_agg_rx->reorder_buf =

@@ -399,6 +390,8 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
 	tid_agg_rx->auto_seq = auto_seq;
 	tid_agg_rx->started = false;
 	tid_agg_rx->reorder_buf_filtered = 0;
+	tid_agg_rx->tid = tid;
+	tid_agg_rx->sta = sta;
 	status = WLAN_STATUS_SUCCESS;
 
 	/* activate it for RX */

net/mac80211/agg-tx.c +16 ~~-26~~

75 lines not on this page · raw message

net/mac80211/sta_info.c +0 -8

index 877d35796776..b5add1464aeb 100644
@@ -379,14 +379,6 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
 	if (sta_prepare_rate_control(local, sta, gfp))
 		goto free_txq;
 
-	for (i = 0; i < IEEE80211_NUM_TIDS; i++) {
-		/*
-		 * timer_to_tid must be initialized with identity mapping
-		 * to enable session_timer's data differentiation. See
-		 * sta_rx_agg_session_timer_expired for usage.
-		 */
-		sta->timer_to_tid[i] = i;
-	}
 	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
 		skb_queue_head_init(&sta->ps_tx_buf[i]);
 		skb_queue_head_init(&sta->tx_filtered[i]);

net/mac80211/sta_info.h +10 -2

index 5c54acd10562..1b9c1e81495d 100644
@@ -126,6 +126,8 @@ enum ieee80211_agg_stop_reason {
 	AGG_STOP_DESTROY_STA,
 };
 
+struct sta_info;
+
 /**
  * struct tid_ampdu_tx - TID aggregation information (Tx).
  *

@@ -133,8 +135,10 @@ enum ieee80211_agg_stop_reason {
  * @session_timer: check if we keep Tx-ing on the TID (by timeout value)
  * @addba_resp_timer: timer for peer's response to addba request
  * @pending: pending frames queue -- use sta's spinlock to protect
+ * @sta: station we are attached to
  * @dialog_token: dialog token for aggregation session
  * @timeout: session timeout value to be filled in ADDBA requests
+ * @tid: index in station tid list
  * @state: session state (see above)
  * @last_tx: jiffies of last tx activity
  * @stop_initiator: initiator of a session stop

@@ -158,9 +162,11 @@ struct tid_ampdu_tx {
 	struct timer_list session_timer;
 	struct timer_list addba_resp_timer;
 	struct sk_buff_head pending;
+	struct sta_info *sta;
 	unsigned long state;
 	unsigned long last_tx;
 	u16 timeout;
+	u16 tid;
 	u8 dialog_token;
 	u8 stop_initiator;
 	bool tx_stop;

@@ -181,12 +187,14 @@ struct tid_ampdu_tx {
  * @reorder_time: jiffies when skb was added
  * @session_timer: check if peer keeps Tx-ing on the TID (by timeout value)
  * @reorder_timer: releases expired frames from the reorder buffer.
+ * @sta: station we are attached to
  * @last_rx: jiffies of last rx activity
  * @head_seq_num: head sequence number in reordering buffer.
  * @stored_mpdu_num: number of MPDUs in reordering buffer
  * @ssn: Starting Sequence Number expected to be aggregated.
  * @buf_size: buffer size for incoming A-MPDUs
  * @timeout: reset timer value (in TUs).
+ * @tid: index in station tid list
  * @rcu_head: RCU head used for freeing this struct
  * @reorder_lock: serializes access to reorder buffer, see below.
  * @auto_seq: used for offloaded BA sessions to automatically pick head_seq_and

@@ -208,6 +216,7 @@ struct tid_ampdu_rx {
 	u64 reorder_buf_filtered;
 	struct sk_buff_head *reorder_buf;
 	unsigned long *reorder_time;
+	struct sta_info *sta;
 	struct timer_list session_timer;
 	struct timer_list reorder_timer;
 	unsigned long last_rx;

@@ -216,6 +225,7 @@ struct tid_ampdu_rx {
 	u16 ssn;
 	u16 buf_size;
 	u16 timeout;
+	u16 tid;
 	u8 auto_seq:1,
 	   removed:1,
 	   started:1;

@@ -447,7 +457,6 @@ struct ieee80211_sta_rx_stats {
  *	plus one for non-QoS frames)
  * @tid_seq: per-TID sequence numbers for sending to this STA
  * @ampdu_mlme: A-MPDU state machine state
- * @timer_to_tid: identity mapping to ID timers
  * @mesh: mesh STA information
  * @debugfs_dir: debug filesystem directory dentry
  * @dead: set to true when sta is unlinked

@@ -554,7 +563,6 @@ struct sta_info {
 	 * Aggregation information, locked with lock.
 	 */
 	struct sta_ampdu_mlme ampdu_mlme;
-	u8 timer_to_tid[IEEE80211_NUM_TIDS];
 
 #ifdef CONFIG_MAC80211_DEBUGFS
 	struct dentry *debugfs_dir;

Signature · 6 lines

2.7.4


-- 
Kees Cook
Pixel Security

Johannes Berg

maintainer

2017-10-18 10:29 UTC #1 lore raw

Hi,

[quoting your other email:]

Quoted 4 lines

This has been the least trivial timer conversion yet. Given the use of RCU and other things I may not even know about, I'd love to get a close look at this. I *think* this is correct, as it will re-lookup the tid entries when firing the timer.

I'm not really sure why you're doing the lookup again? That seems pointless, since you already have the right structure, and already rely on it being valid. You can't really get a new struct assigned to the same TID without the old one being destroyed.

Quoted 13 lines

net/mac80211/agg-rx.c +3 -9

-static void sta_rx_agg_session_timer_expired(unsigned long data)
+static void sta_rx_agg_session_timer_expired(struct timer_list *t)
 {
-	/* not an elegant detour, but there is no choice as the timer passes
-	 * only one argument, and various sta_info are needed here, so init
-	 * flow in sta_info_create gives the TID as data, while the timer_to_id
-	 * array gives the sta through container_of */
-	u8 *ptid = (u8 *)data;
-	u8 *timer_to_id = ptid - *ptid;
-	struct sta_info *sta = container_of(timer_to_id, struct sta_info,
-					 timer_to_tid[0]);
+	struct tid_ampdu_rx *tid_rx_timer =
+		from_timer(tid_rx_timer, t, session_timer);

net/mac80211/agg-rx.c EXPORT_SYMBOL(ieee80211_stop_rx_ba_session); · line 166

Quoted 10 lines

net/mac80211/agg-rx.c +3 -1

+	struct sta_info *sta = tid_rx_timer->sta;
+	u16 tid = tid_rx_timer->tid;
 	struct tid_ampdu_rx *tid_rx;
 	unsigned long timeout;
 
 	rcu_read_lock();
-	tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[*ptid]);
+	tid_rx = rcu_dereference(sta->ampdu_mlme.tid_rx[tid]);
 	if (!tid_rx) {
 		rcu_read_unlock();

So through all of this, I'm pretty sure we can just use tid_rx_timer instead of tid_rx.

(Same for TX)

Anyway, the change here looks correct to me, so I'll apply it and then perhaps clean up more. I've only changed "u16 tid" to "u8 tid" since the valid range is 0-15 (in theory, in practice 0-7).

johannes

Johannes Berg

maintainer

2017-10-18 11:31 UTC #2 lore raw

Quoted 3 lines · Johannes Berg

Anyway, the change here looks correct to me, so I'll apply it and then perhaps clean up more. I've only changed "u16 tid" to "u8 tid" since the valid range is 0-15 (in theory, in practice 0-7).

Well, I guess I'm clearly wrong - it's crashing our test suite left and right.

I'll dig a little bit, but I don't have much time today, and will be out for a few days starting tomorrow.

johannes

Johannes Berg

maintainer

2017-10-18 11:37 UTC #3 lore raw

Quoted 11 lines · Johannes Berg

Quoted 3 lines · Johannes Berg
Anyway, the change here looks correct to me, so I'll apply it and then perhaps clean up more. I've only changed "u16 tid" to "u8 tid" since the valid range is 0-15 (in theory, in practice 0-7).
Well, I guess I'm clearly wrong - it's crashing our test suite left and right.
I'll dig a little bit, but I don't have much time today, and will be out for a few days starting tomorrow.

Ok, it's pretty obvious - you never initialize the new fields in tid_tx (sta and tid), only in tid_rx. Let's see if it passes with that fixed.

johannes

Kees Cook

author

2017-10-18 14:17 UTC #4 lore raw

Quoted 6 lines · Johannes Berg

Quoted 5 lines · Johannes Berg
Quoted 3 lines · Johannes Berg
Anyway, the change here looks correct to me, so I'll apply it and then perhaps clean up more. I've only changed "u16 tid" to "u8 tid" since the valid range is 0-15 (in theory, in practice 0-7).

I started with u8 tid, but I saw it cast to u16 and in a few other places it was u16, so I went with that ultimately.

Quoted 8 lines

Quoted 5 lines
Well, I guess I'm clearly wrong - it's crashing our test suite left and right.
I'll dig a little bit, but I don't have much time today, and will be out for a few days starting tomorrow.
Ok, it's pretty obvious - you never initialize the new fields in tid_tx (sta and tid), only in tid_rx. Let's see if it passes with that fixed.

Argh, whoops, thanks for working on this.

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Kees Cook

author

2017-10-18 14:19 UTC #5 lore raw

In reply to Johannes Berg

Quoted 9 lines · Johannes Berg

Quoted 4 lines
This has been the least trivial timer conversion yet. Given the use of RCU and other things I may not even know about, I'd love to get a close look at this. I *think* this is correct, as it will re-lookup the tid entries when firing the timer.
I'm not really sure why you're doing the lookup again? That seems pointless, since you already have the right structure, and already rely on it being valid. You can't really get a new struct assigned to the same TID without the old one being destroyed.

I couldn't tell what the lifetime expectation was, so I left the re-lookup. I assumed it was possible to have a timer fire after the structure had been removed from the station structure.

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Johannes Berg

maintainer

2017-10-18 20:50 UTC #6 lore raw

Quoted 7 lines · Kees Cook

Quoted 5 lines · Johannes Berg
Quoted 4 lines
This has been the least trivial timer conversion yet. Given the use of RCU and other things I may not even know about, I'd love to get a close look at this. I *think* this is correct, as it will re-lookup the tid entries when firing the timer.

Quoted 8 lines

Quoted 4 lines
I'm not really sure why you're doing the lookup again? That seems pointless, since you already have the right structure, and already rely on it being valid. You can't really get a new struct assigned to the same TID without the old one being destroyed.
I couldn't tell what the lifetime expectation was, so I left the re-lookup. I assumed it was possible to have a timer fire after the structure had been removed from the station structure.

Oh, right, I guess that would've been possible in theory. It's not actually possible though since the aggregation sessions can't live on without a station, so I've already made a follow-up patch to remove the indirection.

johannes

Kees Cook

author

2017-10-18 21:02 UTC #7 lore raw

Quoted 13 lines · Johannes Berg

Quoted 12 lines · Kees Cook
Quoted 9 lines · Johannes Berg
Quoted 4 lines
This has been the least trivial timer conversion yet. Given the use of RCU and other things I may not even know about, I'd love to get a close look at this. I *think* this is correct, as it will re-lookup the tid entries when firing the timer.
I'm not really sure why you're doing the lookup again? That seems pointless, since you already have the right structure, and already rely on it being valid. You can't really get a new struct assigned to the same TID without the old one being destroyed.

Quoted 8 lines

Quoted 3 lines
I couldn't tell what the lifetime expectation was, so I left the re-lookup. I assumed it was possible to have a timer fire after the structure had been removed from the station structure.
Oh, right, I guess that would've been possible in theory. It's not actually possible though since the aggregation sessions can't live on without a station, so I've already made a follow-up patch to remove the indirection.

Okay, cool, thanks.

It seems like I have a similar case in the iwlwifi driver too, but it's different enough that I'm not sure about that one either. I'll send that when I've got it ready.

-Kees

Signature · 2 lines

Kees Cook
Pixel Security