Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()

[Kees Cook]

On Wed, Oct 18, 2017 at 1:50 PM, Johannes Berg
 wrote:
> On Wed, 2017-10-18 at 07:19 -0700, Kees Cook wrote:
>> On Wed, Oct 18, 2017 at 3:29 AM, Johannes Berg
>>  wrote:
>> > > 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.
>
> 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

-- 
Kees Cook
Pixel Security

Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()

[Johannes Berg]

On Wed, 2017-10-18 at 07:19 -0700, Kees Cook wrote:
> On Wed, Oct 18, 2017 at 3:29 AM, Johannes Berg
>  wrote:
> > > 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.

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

Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()

[Kees Cook]

On Wed, Oct 18, 2017 at 3:29 AM, Johannes Berg
 wrote:
>> 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

-- 
Kees Cook
Pixel Security

Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()

[Kees Cook]

On Wed, Oct 18, 2017 at 4:37 AM, Johannes Berg
 wrote:
> On Wed, 2017-10-18 at 13:31 +0200, Johannes Berg wrote:
>> On Wed, 2017-10-18 at 12:29 +0200, Johannes Berg wrote:
>>
>> > 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.

>> 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

-- 
Kees Cook
Pixel Security

Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()

[Johannes Berg]

On Wed, 2017-10-18 at 13:31 +0200, Johannes Berg wrote:
> On Wed, 2017-10-18 at 12:29 +0200, Johannes Berg wrote:
> 
> > 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

Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()

[Johannes Berg]

On Wed, 2017-10-18 at 12:29 +0200, Johannes Berg wrote:

> 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

Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()

[Johannes Berg]

Hi,

[quoting your other email:]

> 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.

> -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();

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

[PATCH] mac80211: aggregation: Convert timers to use timer_setup()

[Kees Cook]

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 
Cc: "David S. Miller" 
Cc: linux-wireless@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook 
---
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(-)

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 88cc1ae935ea..63aba6dbc92a 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -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(>local->hw, >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(_agg_rx->reorder_lock);
 
/* rx timer */
-   setup_deferrable_timer(_agg_rx->session_timer,
-  sta_rx_agg_session_timer_expired,
-  (unsigned long)>timer_to_tid[tid]);
+   timer_setup(_agg_rx->session_timer,
+   sta_rx_agg_session_timer_expired, TIMER_DEFERRABLE);
 
/* rx reorder timer */
-   setup_timer(_agg_rx->reorder_timer,
-   sta_rx_agg_reorder_timer_expired,
-   (unsigned long)>timer_to_tid[tid]);
+   timer_setup(_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 */
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index bef516ec47f9..dedbb1fb10e7 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -422,15 +422,12 @@ int ___ieee80211_stop_tx_ba_session(struct sta_info *sta, 
u16 tid,
  * add Block Ack response will arrive from the recipient.
  * If this timer expires sta_addba_resp_timer_expired will be executed.
  */
-static void