[PATCH 3.3 v2] rt2x00: fix random stalls

[PATCH 3.3 v2] rt2x00: fix random stalls

[Stanislaw Gruszka]

Is possible that we stop queue and then do not wake up it again,
especially when packets are transmitted fast. That can be easily
reproduced with modified tx queue entry_num to some small value e.g. 16.

If mac80211 already hold local->queue_stop_reason_lock, then we can wait
on that lock in both rt2x00queue_pause_queue() and
rt2x00queue_unpause_queue(). After drooping ->queue_stop_reason_lock
is possible that __ieee80211_wake_queue() will be performed before
__ieee80211_stop_queue(), hence we stop queue and newer wake up it
again.

Another race condition is possible when between rt2x00queue_threshold()
check and rt2x00queue_pause_queue() we will process all pending tx
buffers on different cpu. This might happen if for example intterrupt
will be triggered on cpu performing rt2x00mac_tx().

To prevent race conditions serialize pause/unpause by queue->tx_lock.

Cc: stable@vger.kernel.org
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
v1 -> v2:
 - use spin_lock_bh
 - serialize also rt2x00queue_threshold()

 drivers/net/wireless/rt2x00/rt2x00dev.c |    6 +++++-
 drivers/net/wireless/rt2x00/rt2x00mac.c |    7 +++++++
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
index 49a51b4..61a5042 100644
--- a/drivers/net/wireless/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
@@ -430,10 +430,14 @@ void rt2x00lib_txdone(struct queue_entry *entry,
 	/*
 	 * If the data queue was below the threshold before the txdone
 	 * handler we must make sure the packet queue in the mac80211 stack
-	 * is reenabled when the txdone handler has finished.
+	 * is reenabled when the txdone handler has finished. This has to be
+	 * serialized with rt2x00mac_tx(), otherwise we can wake up queue
+	 * before it was stopped.
 	 */
+	spin_lock_bh(&entry->queue->tx_lock);
 	if (!rt2x00queue_threshold(entry->queue))
 		rt2x00queue_unpause_queue(entry->queue);
+	spin_unlock_bh(&entry->queue->tx_lock);
 }
 EXPORT_SYMBOL_GPL(rt2x00lib_txdone);
 
diff --git a/drivers/net/wireless/rt2x00/rt2x00mac.c b/drivers/net/wireless/rt2x00/rt2x00mac.c
index ede3c58..923702c 100644
--- a/drivers/net/wireless/rt2x00/rt2x00mac.c
+++ b/drivers/net/wireless/rt2x00/rt2x00mac.c
@@ -152,13 +152,20 @@ void rt2x00mac_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
 	if (unlikely(rt2x00queue_write_tx_frame(queue, skb, false)))
 		goto exit_fail;
 
+	/*
+	 * Pausing queue has to be serialized with rt2x00lib_txdone() .
+	 */
+	spin_lock(&queue->tx_lock);
 	if (rt2x00queue_threshold(queue))
 		rt2x00queue_pause_queue(queue);
+	spin_unlock(&queue->tx_lock);
 
 	return;
 
  exit_fail:
+	spin_lock(&queue->tx_lock);
 	rt2x00queue_pause_queue(queue);
+	spin_unlock(&queue->tx_lock);
  exit_free_skb:
 	ieee80211_free_txskb(hw, skb);
 }
-- 
1.7.1

Re: [rt2x00-users] [PATCH 3.3 v2] rt2x00: fix random stalls

[Gertjan van Wingerde]

Hi Stanislaw,

On 03/07/12 19:31, Stanislaw Gruszka wrote:
> Is possible that we stop queue and then do not wake up it again,
> especially when packets are transmitted fast. That can be easily
> reproduced with modified tx queue entry_num to some small value e.g. 16.
> 
> If mac80211 already hold local->queue_stop_reason_lock, then we can wait
> on that lock in both rt2x00queue_pause_queue() and
> rt2x00queue_unpause_queue(). After drooping ->queue_stop_reason_lock
> is possible that __ieee80211_wake_queue() will be performed before
> __ieee80211_stop_queue(), hence we stop queue and newer wake up it
> again.
> 
> Another race condition is possible when between rt2x00queue_threshold()
> check and rt2x00queue_pause_queue() we will process all pending tx
> buffers on different cpu. This might happen if for example intterrupt
> will be triggered on cpu performing rt2x00mac_tx().
> 
> To prevent race conditions serialize pause/unpause by queue->tx_lock.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> v1 -> v2:
>  - use spin_lock_bh
>  - serialize also rt2x00queue_threshold()
> 
>  drivers/net/wireless/rt2x00/rt2x00dev.c |    6 +++++-
>  drivers/net/wireless/rt2x00/rt2x00mac.c |    7 +++++++
>  2 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
> index 49a51b4..61a5042 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00dev.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
> @@ -430,10 +430,14 @@ void rt2x00lib_txdone(struct queue_entry *entry,
>  	/*
>  	 * If the data queue was below the threshold before the txdone
>  	 * handler we must make sure the packet queue in the mac80211 stack
> -	 * is reenabled when the txdone handler has finished.
> +	 * is reenabled when the txdone handler has finished. This has to be
> +	 * serialized with rt2x00mac_tx(), otherwise we can wake up queue
> +	 * before it was stopped.
>  	 */
> +	spin_lock_bh(&entry->queue->tx_lock);
>  	if (!rt2x00queue_threshold(entry->queue))
>  		rt2x00queue_unpause_queue(entry->queue);
> +	spin_unlock_bh(&entry->queue->tx_lock);
>  }
>  EXPORT_SYMBOL_GPL(rt2x00lib_txdone);
>  
> diff --git a/drivers/net/wireless/rt2x00/rt2x00mac.c b/drivers/net/wireless/rt2x00/rt2x00mac.c
> index ede3c58..923702c 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00mac.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00mac.c
> @@ -152,13 +152,20 @@ void rt2x00mac_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
>  	if (unlikely(rt2x00queue_write_tx_frame(queue, skb, false)))
>  		goto exit_fail;
>  
> +	/*
> +	 * Pausing queue has to be serialized with rt2x00lib_txdone() .
> +	 */
> +	spin_lock(&queue->tx_lock);
>  	if (rt2x00queue_threshold(queue))
>  		rt2x00queue_pause_queue(queue);
> +	spin_unlock(&queue->tx_lock);
>  
>  	return;
>  
>   exit_fail:
> +	spin_lock(&queue->tx_lock);
>  	rt2x00queue_pause_queue(queue);
> +	spin_unlock(&queue->tx_lock);
>   exit_free_skb:
>  	ieee80211_free_txskb(hw, skb);
>  }

I'm sorry, but I'm still not convinced that we can use spin_lock_bh in
one place of the code and then spin_lock in another place of the code,
using the *same* spinlock.
I always use the cheat sheet shown in:
http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c214.html

which to me shows that by definition we should be using spin_lock_bh in
all cases now, the new ones and the existing cases where we lock tx_lock.

---
Gertjan

Re: [rt2x00-users] [PATCH 3.3 v2] rt2x00: fix random stalls

[Stanislaw Gruszka]

Hi Gertjan

On Thu, Mar 08, 2012 at 10:40:43PM +0100, Gertjan van Wingerde wrote:
> > @@ -152,13 +152,20 @@ void rt2x00mac_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
> >  	if (unlikely(rt2x00queue_write_tx_frame(queue, skb, false)))
> >  		goto exit_fail;
> >  
> > +	/*
> > +	 * Pausing queue has to be serialized with rt2x00lib_txdone() .
> > +	 */
> > +	spin_lock(&queue->tx_lock);
> >  	if (rt2x00queue_threshold(queue))
> >  		rt2x00queue_pause_queue(queue);
> > +	spin_unlock(&queue->tx_lock);
> >  
> >  	return;
> >  
> >   exit_fail:
> > +	spin_lock(&queue->tx_lock);
> >  	rt2x00queue_pause_queue(queue);
> > +	spin_unlock(&queue->tx_lock);
> >   exit_free_skb:
> >  	ieee80211_free_txskb(hw, skb);
> >  }
> 
> I'm sorry, but I'm still not convinced that we can use spin_lock_bh in
> one place of the code and then spin_lock in another place of the code,
> using the *same* spinlock.
> I always use the cheat sheet shown in:
> http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c214.html
> 
> which to me shows that by definition we should be using spin_lock_bh in
> all cases now, the new ones and the existing cases where we lock tx_lock.

We have bh disabled here since ieee80211_xmit is always called with bh
disabled (early on dev_queue_xmit() or in ieee80211_tx_skb_tid()). I can
add comment about that.

Additionally I ran patch with CONFIG_LOCKDEP which is capable to detect
locking context errors and no warning was printed.

Thanks
Stanislaw

Re: [rt2x00-users] [PATCH 3.3 v2] rt2x00: fix random stalls

[Johannes Berg]

On Fri, 2012-03-09 at 09:22 +0100, Stanislaw Gruszka wrote:

> > > +	spin_lock(&queue->tx_lock);
> > >  	rt2x00queue_pause_queue(queue);
> > > +	spin_unlock(&queue->tx_lock);
> > >   exit_free_skb:
> > >  	ieee80211_free_txskb(hw, skb);
> > >  }
> > 
> > I'm sorry, but I'm still not convinced that we can use spin_lock_bh in
> > one place of the code and then spin_lock in another place of the code,
> > using the *same* spinlock.
> > I always use the cheat sheet shown in:
> > http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c214.html
> > 
> > which to me shows that by definition we should be using spin_lock_bh in
> > all cases now, the new ones and the existing cases where we lock tx_lock.
> 
> We have bh disabled here since ieee80211_xmit is always called with bh
> disabled (early on dev_queue_xmit() or in ieee80211_tx_skb_tid()). I can
> add comment about that.

And in fact if you were to use spin_unlock_bh() in that kind of context
it would be a bug :-)

johannes

Re: [rt2x00-users] [PATCH 3.3 v2] rt2x00: fix random stalls

[Gertjan van Wingerde]

Hi Stanislaw, Johannes,

On Fri, Mar 9, 2012 at 9:33 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2012-03-09 at 09:22 +0100, Stanislaw Gruszka wrote:
>
>> > > + spin_lock(&queue->tx_lock);
>> > >   rt2x00queue_pause_queue(queue);
>> > > + spin_unlock(&queue->tx_lock);
>> > >   exit_free_skb:
>> > >   ieee80211_free_txskb(hw, skb);
>> > >  }
>> >
>> > I'm sorry, but I'm still not convinced that we can use spin_lock_bh in
>> > one place of the code and then spin_lock in another place of the code,
>> > using the *same* spinlock.
>> > I always use the cheat sheet shown in:
>> > http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c214.html
>> >
>> > which to me shows that by definition we should be using spin_lock_bh in
>> > all cases now, the new ones and the existing cases where we lock tx_lock.
>>
>> We have bh disabled here since ieee80211_xmit is always called with bh
>> disabled (early on dev_queue_xmit() or in ieee80211_tx_skb_tid()). I can
>> add comment about that.

OK. Thanks for the explanation. Now it is clear to me. However, I
think it would be
good to add a comment here, as we are otherwise bound to have the same
discussion
again in a couple of months time.
With that comment added you can add my:
Acked-by: Gertjan van Wingerde <gwingerde@gmail.com>

to the v3 patch submission.

>
> And in fact if you were to use spin_unlock_bh() in that kind of context
> it would be a bug :-)
>

Thanks for your explanation as well, Johannes. Everything is clear to me now :-)


-- 
---
Gertjan