[RFC] mac80211: fix possible tid_rx->reorder_timer use after free

[RFC] mac80211: fix possible tid_rx->reorder_timer use after free

[Stanislaw Gruszka]

Is possible that we arm tid_rx->reorder_timer after del_timer_sync(). To
fix: first wait for RCU grace period finish and then delete timer. Timer
will not be armed again as rcu_dereference(sta->ampdu_mlme.tid_rx[tid])
will return NULL.

Debug object detected problem with the following warning:
ODEBUG: free active (active state 0) object type: timer_list hint: sta_rx_agg_reorder_timer_expired+0x0/0xf0 [mac80211]

Bug report (with full warning):
https://bugzilla.redhat.com/show_bug.cgi?id=804007

Reported-by: "jan p. springer" <jsd@igroup.org>
Cc: stable@vger.kernel.org
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 net/mac80211/agg-rx.c   |    8 ++++----
 net/mac80211/sta_info.h |    1 -
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 1068f66..2c1223e 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -43,10 +43,8 @@
 #include "ieee80211_i.h"
 #include "driver-ops.h"
 
-static void ieee80211_free_tid_rx(struct rcu_head *h)
+static void ieee80211_free_tid_rx(struct tid_ampdu_rx *tid_rx)
 {
-	struct tid_ampdu_rx *tid_rx =
-		container_of(h, struct tid_ampdu_rx, rcu_head);
 	int i;
 
 	for (i = 0; i < tid_rx->buf_size; i++)
@@ -90,10 +88,12 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
 		ieee80211_send_delba(sta->sdata, sta->sta.addr,
 				     tid, WLAN_BACK_RECIPIENT, reason);
 
+	synchronize_rcu();
+
 	del_timer_sync(&tid_rx->session_timer);
 	del_timer_sync(&tid_rx->reorder_timer);
 
-	call_rcu(&tid_rx->rcu_head, ieee80211_free_tid_rx);
+	ieee80211_free_tid_rx(tid_rx);
 }
 
 void __ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index ab05768..d73e05d 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -157,7 +157,6 @@ struct tid_ampdu_tx {
  * dialog token being used only for debugging).
  */
 struct tid_ampdu_rx {
-	struct rcu_head rcu_head;
 	spinlock_t reorder_lock;
 	struct sk_buff **reorder_buf;
 	unsigned long *reorder_time;
-- 
1.7.1

Re: [RFC] mac80211: fix possible tid_rx->reorder_timer use after free

[Johannes Berg]

On Mon, 2012-03-19 at 13:50 +0100, Stanislaw Gruszka wrote:
> Is possible that we arm tid_rx->reorder_timer after del_timer_sync(). To
> fix: first wait for RCU grace period finish and then delete timer. Timer
> will not be armed again as rcu_dereference(sta->ampdu_mlme.tid_rx[tid])
> will return NULL.
> 
> Debug object detected problem with the following warning:
> ODEBUG: free active (active state 0) object type: timer_list hint: sta_rx_agg_reorder_timer_expired+0x0/0xf0 [mac80211]
> 
> Bug report (with full warning):
> https://bugzilla.redhat.com/show_bug.cgi?id=804007
> 
> Reported-by: "jan p. springer" <jsd@igroup.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  net/mac80211/agg-rx.c   |    8 ++++----
>  net/mac80211/sta_info.h |    1 -
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
> index 1068f66..2c1223e 100644
> --- a/net/mac80211/agg-rx.c
> +++ b/net/mac80211/agg-rx.c
> @@ -43,10 +43,8 @@
>  #include "ieee80211_i.h"
>  #include "driver-ops.h"
>  
> -static void ieee80211_free_tid_rx(struct rcu_head *h)
> +static void ieee80211_free_tid_rx(struct tid_ampdu_rx *tid_rx)
>  {
> -	struct tid_ampdu_rx *tid_rx =
> -		container_of(h, struct tid_ampdu_rx, rcu_head);
>  	int i;
>  
>  	for (i = 0; i < tid_rx->buf_size; i++)
> @@ -90,10 +88,12 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
>  		ieee80211_send_delba(sta->sdata, sta->sta.addr,
>  				     tid, WLAN_BACK_RECIPIENT, reason);
>  
> +	synchronize_rcu();
> +
>  	del_timer_sync(&tid_rx->session_timer);
>  	del_timer_sync(&tid_rx->reorder_timer);
>  
> -	call_rcu(&tid_rx->rcu_head, ieee80211_free_tid_rx);
> +	ieee80211_free_tid_rx(tid_rx);

Hmmm. That synchronize_rcu() could become rather expensive. I've been
trying to reduce our use of synchronize_rcu() now.

I was checking if we could move the timer deletions into
ieee80211_free_tid_rx since call_rcu runs from another softirq, but I'm
not really sure -- the timer softirq could be running on another CPU?

johannes

Re: [RFC] mac80211: fix possible tid_rx->reorder_timer use after free

[Johannes Berg]

On Mon, 2012-03-19 at 14:03 +0100, Johannes Berg wrote:
> On Mon, 2012-03-19 at 13:50 +0100, Stanislaw Gruszka wrote:
> > Is possible that we arm tid_rx->reorder_timer after del_timer_sync(). To
> > fix: first wait for RCU grace period finish and then delete timer. Timer
> > will not be armed again as rcu_dereference(sta->ampdu_mlme.tid_rx[tid])
> > will return NULL.
> > 
> > Debug object detected problem with the following warning:
> > ODEBUG: free active (active state 0) object type: timer_list hint: sta_rx_agg_reorder_timer_expired+0x0/0xf0 [mac80211]
> > 
> > Bug report (with full warning):
> > https://bugzilla.redhat.com/show_bug.cgi?id=804007
> > 
> > Reported-by: "jan p. springer" <jsd@igroup.org>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > ---
> >  net/mac80211/agg-rx.c   |    8 ++++----
> >  net/mac80211/sta_info.h |    1 -
> >  2 files changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
> > index 1068f66..2c1223e 100644
> > --- a/net/mac80211/agg-rx.c
> > +++ b/net/mac80211/agg-rx.c
> > @@ -43,10 +43,8 @@
> >  #include "ieee80211_i.h"
> >  #include "driver-ops.h"
> >  
> > -static void ieee80211_free_tid_rx(struct rcu_head *h)
> > +static void ieee80211_free_tid_rx(struct tid_ampdu_rx *tid_rx)
> >  {
> > -	struct tid_ampdu_rx *tid_rx =
> > -		container_of(h, struct tid_ampdu_rx, rcu_head);
> >  	int i;
> >  
> >  	for (i = 0; i < tid_rx->buf_size; i++)
> > @@ -90,10 +88,12 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
> >  		ieee80211_send_delba(sta->sdata, sta->sta.addr,
> >  				     tid, WLAN_BACK_RECIPIENT, reason);
> >  
> > +	synchronize_rcu();
> > +
> >  	del_timer_sync(&tid_rx->session_timer);
> >  	del_timer_sync(&tid_rx->reorder_timer);
> >  
> > -	call_rcu(&tid_rx->rcu_head, ieee80211_free_tid_rx);
> > +	ieee80211_free_tid_rx(tid_rx);
> 
> Hmmm. That synchronize_rcu() could become rather expensive. I've been
> trying to reduce our use of synchronize_rcu() now.
> 
> I was checking if we could move the timer deletions into
> ieee80211_free_tid_rx since call_rcu runs from another softirq, but I'm
> not really sure -- the timer softirq could be running on another CPU?

I was actually thinking of using just del_timer(), but now that I think
about it, should anything prevent us from using del_timer_sync() inside
ieee80211_free_tid_rx?

johannes

Re: [RFC] mac80211: fix possible tid_rx->reorder_timer use after free

[Stanislaw Gruszka]

On Mon, Mar 19, 2012 at 02:07:57PM +0100, Johannes Berg wrote:
> I was actually thinking of using just del_timer(), but now that I think
> about it, should anything prevent us from using del_timer_sync() inside
> ieee80211_free_tid_rx?

Yes, call_rcu() callback can not sleep. Depending of RCU implementation
callback can run with bottom half disabled.

Stanislaw

Re: [RFC] mac80211: fix possible tid_rx->reorder_timer use after free

[Johannes Berg]

On Mon, 2012-03-19 at 14:43 +0100, Stanislaw Gruszka wrote:
> On Mon, Mar 19, 2012 at 02:07:57PM +0100, Johannes Berg wrote:
> > I was actually thinking of using just del_timer(), but now that I think
> > about it, should anything prevent us from using del_timer_sync() inside
> > ieee80211_free_tid_rx?
> 
> Yes, call_rcu() callback can not sleep. Depending of RCU implementation
> callback can run with bottom half disabled.

del_timer_sync() doesn't really *sleep* though, even if it waits, so it
should be fine?

johannes

Re: [RFC] mac80211: fix possible tid_rx->reorder_timer use after free

[Stanislaw Gruszka]

On Mon, Mar 19, 2012 at 02:49:54PM +0100, Johannes Berg wrote:
> On Mon, 2012-03-19 at 14:43 +0100, Stanislaw Gruszka wrote:
> > On Mon, Mar 19, 2012 at 02:07:57PM +0100, Johannes Berg wrote:
> > > I was actually thinking of using just del_timer(), but now that I think
> > > about it, should anything prevent us from using del_timer_sync() inside
> > > ieee80211_free_tid_rx?
> > 
> > Yes, call_rcu() callback can not sleep. Depending of RCU implementation
> > callback can run with bottom half disabled.
> 
> del_timer_sync() doesn't really *sleep* though, even if it waits, so it
> should be fine?

Indeed, I'll change the patch and post officially.

Thanks
Stanislaw

Re: [RFC] mac80211: fix possible tid_rx->reorder_timer use after free

[Stanislaw Gruszka]

On Mon, Mar 19, 2012 at 02:03:08PM +0100, Johannes Berg wrote:
> On Mon, 2012-03-19 at 13:50 +0100, Stanislaw Gruszka wrote:
> > Is possible that we arm tid_rx->reorder_timer after del_timer_sync(). To
> > fix: first wait for RCU grace period finish and then delete timer. Timer
> > will not be armed again as rcu_dereference(sta->ampdu_mlme.tid_rx[tid])
> > will return NULL.
> > 
> > Debug object detected problem with the following warning:
> > ODEBUG: free active (active state 0) object type: timer_list hint: sta_rx_agg_reorder_timer_expired+0x0/0xf0 [mac80211]
> > 
> > Bug report (with full warning):
> > https://bugzilla.redhat.com/show_bug.cgi?id=804007
> > 
> > Reported-by: "jan p. springer" <jsd@igroup.org>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > ---
> >  net/mac80211/agg-rx.c   |    8 ++++----
> >  net/mac80211/sta_info.h |    1 -
> >  2 files changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
> > index 1068f66..2c1223e 100644
> > --- a/net/mac80211/agg-rx.c
> > +++ b/net/mac80211/agg-rx.c
> > @@ -43,10 +43,8 @@
> >  #include "ieee80211_i.h"
> >  #include "driver-ops.h"
> >  
> > -static void ieee80211_free_tid_rx(struct rcu_head *h)
> > +static void ieee80211_free_tid_rx(struct tid_ampdu_rx *tid_rx)
> >  {
> > -	struct tid_ampdu_rx *tid_rx =
> > -		container_of(h, struct tid_ampdu_rx, rcu_head);
> >  	int i;
> >  
> >  	for (i = 0; i < tid_rx->buf_size; i++)
> > @@ -90,10 +88,12 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
> >  		ieee80211_send_delba(sta->sdata, sta->sta.addr,
> >  				     tid, WLAN_BACK_RECIPIENT, reason);
> >  
> > +	synchronize_rcu();
> > +
> >  	del_timer_sync(&tid_rx->session_timer);
> >  	del_timer_sync(&tid_rx->reorder_timer);
> >  
> > -	call_rcu(&tid_rx->rcu_head, ieee80211_free_tid_rx);
> > +	ieee80211_free_tid_rx(tid_rx);
> 
> Hmmm. That synchronize_rcu() could become rather expensive. I've been
> trying to reduce our use of synchronize_rcu() now.

Expensive in what sense? Since we are in process context we are allowed
to sleep, and some other processes will run while we sleep.

> I was checking if we could move the timer deletions into
> ieee80211_free_tid_rx since call_rcu runs from another softirq, but I'm
> not really sure -- the timer softirq could be running on another CPU?

Yes. I considered using async del_timer() from rcu callback, but that
would be racy too. 

We could do rcu_dereference (and check if it is not null) just
before mod_timer(). But that will still be racy as long spin_lock
will not be used.

Stanislaw

Re: [RFC] mac80211: fix possible tid_rx->reorder_timer use after free

[Johannes Berg]

On Mon, 2012-03-19 at 14:40 +0100, Stanislaw Gruszka wrote:

> > > +static void ieee80211_free_tid_rx(struct tid_ampdu_rx *tid_rx)
> > >  {
> > > -	struct tid_ampdu_rx *tid_rx =
> > > -		container_of(h, struct tid_ampdu_rx, rcu_head);
> > >  	int i;
> > >  
> > >  	for (i = 0; i < tid_rx->buf_size; i++)
> > > @@ -90,10 +88,12 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
> > >  		ieee80211_send_delba(sta->sdata, sta->sta.addr,
> > >  				     tid, WLAN_BACK_RECIPIENT, reason);
> > >  
> > > +	synchronize_rcu();
> > > +
> > >  	del_timer_sync(&tid_rx->session_timer);
> > >  	del_timer_sync(&tid_rx->reorder_timer);
> > >  
> > > -	call_rcu(&tid_rx->rcu_head, ieee80211_free_tid_rx);
> > > +	ieee80211_free_tid_rx(tid_rx);
> > 
> > Hmmm. That synchronize_rcu() could become rather expensive. I've been
> > trying to reduce our use of synchronize_rcu() now.
> 
> Expensive in what sense? Since we are in process context we are allowed
> to sleep, and some other processes will run while we sleep.

It takes a long time, and we have to wait for it. So for example, it
could delay roaming by another 100ms or however long it takes (yes, it
can take a very long time, we've seen up to 200ms I think)

johannes