[PATCH 1/1] mac80211: fix deadlock in sta->lock

[PATCH 1/1] mac80211: fix deadlock in sta->lock

[Tomas Winkler]

From: Ron Rindjunsky <ron.rindjunsky@intel.com>

This patch fixes a deadlock of sta->lock use, occuring while changing
tx aggregation states, as dev_queue_xmit end up in new function
test_and_clear_sta_flags that uses that lock thus leading to deadlock

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 net/mac80211/main.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index b0fddb7..5a9030c 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -662,6 +662,8 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
 		goto start_ba_err;
 	}
 
+	spin_unlock_bh(&sta->lock);
+
 	/* Will put all the packets in the new SW queue */
 	ieee80211_requeue(local, ieee802_1d_to_ac[tid]);
 	spin_unlock_bh(&local->mdev->queue_lock);
@@ -688,9 +690,9 @@ start_ba_err:
 	kfree(sta->ampdu_mlme.tid_tx[tid]);
 	sta->ampdu_mlme.tid_tx[tid] = NULL;
 	spin_unlock_bh(&local->mdev->queue_lock);
+	spin_unlock_bh(&sta->lock);
 	ret = -EBUSY;
 start_ba_exit:
-	spin_unlock_bh(&sta->lock);
 	rcu_read_unlock();
 	return ret;
 }
@@ -829,10 +831,11 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid)
 	}
 	state = &sta->ampdu_mlme.tid_state_tx[tid];
 
-	spin_lock_bh(&sta->lock);
+	/* no need to use sta->lock in this state check, as
+	 * ieee80211_stop_tx_ba_session will let only
+	 * one stop call to pass through per sta/tid */
 	if ((*state & HT_AGG_STATE_REQ_STOP_BA_MSK) == 0) {
 		printk(KERN_DEBUG "unexpected callback to A-MPDU stop\n");
-		spin_unlock_bh(&sta->lock);
 		rcu_read_unlock();
 		return;
 	}
@@ -855,6 +858,7 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid)
 	 * ieee80211_wake_queue is not used here as this queue is not
 	 * necessarily stopped */
 	netif_schedule(local->mdev);
+	spin_lock_bh(&sta->lock);
 	*state = HT_AGG_STATE_IDLE;
 	sta->ampdu_mlme.addba_req_num[tid] = 0;
 	kfree(sta->ampdu_mlme.tid_tx[tid]);
-- 
1.5.4.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 1/1] mac80211: fix deadlock in sta->lock

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2433 bytes --]

On Mon, 2008-05-26 at 17:09 +0300, Tomas Winkler wrote:
> From: Ron Rindjunsky <ron.rindjunsky@intel.com>
> 
> This patch fixes a deadlock of sta->lock use, occuring while changing
> tx aggregation states, as dev_queue_xmit end up in new function
> test_and_clear_sta_flags that uses that lock thus leading to deadlock

Oh, good catch, thanks. Reminds me to debug the other deadlock I saw
(not mac80211 related though)

> Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

Acked-by: Johannes Berg <johannes@sipsolutions.net>

> ---
>  net/mac80211/main.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index b0fddb7..5a9030c 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -662,6 +662,8 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
>  		goto start_ba_err;
>  	}
>  
> +	spin_unlock_bh(&sta->lock);
> +
>  	/* Will put all the packets in the new SW queue */
>  	ieee80211_requeue(local, ieee802_1d_to_ac[tid]);
>  	spin_unlock_bh(&local->mdev->queue_lock);
> @@ -688,9 +690,9 @@ start_ba_err:
>  	kfree(sta->ampdu_mlme.tid_tx[tid]);
>  	sta->ampdu_mlme.tid_tx[tid] = NULL;
>  	spin_unlock_bh(&local->mdev->queue_lock);
> +	spin_unlock_bh(&sta->lock);
>  	ret = -EBUSY;
>  start_ba_exit:
> -	spin_unlock_bh(&sta->lock);
>  	rcu_read_unlock();
>  	return ret;
>  }
> @@ -829,10 +831,11 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid)
>  	}
>  	state = &sta->ampdu_mlme.tid_state_tx[tid];
>  
> -	spin_lock_bh(&sta->lock);
> +	/* no need to use sta->lock in this state check, as
> +	 * ieee80211_stop_tx_ba_session will let only
> +	 * one stop call to pass through per sta/tid */
>  	if ((*state & HT_AGG_STATE_REQ_STOP_BA_MSK) == 0) {
>  		printk(KERN_DEBUG "unexpected callback to A-MPDU stop\n");
> -		spin_unlock_bh(&sta->lock);
>  		rcu_read_unlock();
>  		return;
>  	}
> @@ -855,6 +858,7 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid)
>  	 * ieee80211_wake_queue is not used here as this queue is not
>  	 * necessarily stopped */
>  	netif_schedule(local->mdev);
> +	spin_lock_bh(&sta->lock);
>  	*state = HT_AGG_STATE_IDLE;
>  	sta->ampdu_mlme.addba_req_num[tid] = 0;
>  	kfree(sta->ampdu_mlme.tid_tx[tid]);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/1] mac80211: fix deadlock in sta->lock

[Tomas Winkler]

On Mon, May 26, 2008 at 5:19 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2008-05-26 at 17:09 +0300, Tomas Winkler wrote:
>> From: Ron Rindjunsky <ron.rindjunsky@intel.com>
>>
>> This patch fixes a deadlock of sta->lock use, occuring while changing
>> tx aggregation states, as dev_queue_xmit end up in new function
>> test_and_clear_sta_flags that uses that lock thus leading to deadlock
>
> Oh, good catch, thanks. Reminds me to debug the other deadlock I saw
> (not mac80211 related though)
>
very encouraging :) . Can you send me details I also experience some
lockups on rc2
Thanks
Tomas

Re: [PATCH 1/1] mac80211: fix deadlock in sta->lock

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 967 bytes --]

On Mon, 2008-05-26 at 17:32 +0300, Tomas Winkler wrote:
> On Mon, May 26, 2008 at 5:19 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Mon, 2008-05-26 at 17:09 +0300, Tomas Winkler wrote:
> >> From: Ron Rindjunsky <ron.rindjunsky@intel.com>
> >>
> >> This patch fixes a deadlock of sta->lock use, occuring while changing
> >> tx aggregation states, as dev_queue_xmit end up in new function
> >> test_and_clear_sta_flags that uses that lock thus leading to deadlock
> >
> > Oh, good catch, thanks. Reminds me to debug the other deadlock I saw
> > (not mac80211 related though)
> >
> very encouraging :) . Can you send me details I also experience some
> lockups on rc2

Well when I resume my machine it locks up right after printing "eth0:
resuming" or something like that, *iff* eth0 was down when suspending. I
fixed a bug like that (NAPI related) a while ago, but w/o sungem changes
it has re-appeared, or so it seems.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/1] mac80211: fix deadlock in sta->lock

[Tomas Winkler]

On Mon, May 26, 2008 at 5:40 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2008-05-26 at 17:32 +0300, Tomas Winkler wrote:
>> On Mon, May 26, 2008 at 5:19 PM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Mon, 2008-05-26 at 17:09 +0300, Tomas Winkler wrote:
>> >> From: Ron Rindjunsky <ron.rindjunsky@intel.com>
>> >>
>> >> This patch fixes a deadlock of sta->lock use, occuring while changing
>> >> tx aggregation states, as dev_queue_xmit end up in new function
>> >> test_and_clear_sta_flags that uses that lock thus leading to deadlock
>> >
>> > Oh, good catch, thanks. Reminds me to debug the other deadlock I saw
>> > (not mac80211 related though)
>> >
>> very encouraging :) . Can you send me details I also experience some
>> lockups on rc2
>
> Well when I resume my machine it locks up right after printing "eth0:
> resuming" or something like that, *iff* eth0 was down when suspending. I
> fixed a bug like that (NAPI related) a while ago, but w/o sungem changes
> it has re-appeared, or so it seems.
>
Thanks, doesn't look like what I see.
Tomas

Re: [PATCH 1/1] mac80211: fix deadlock in sta->lock

[Tomas Winkler]

On Mon, May 26, 2008 at 5:19 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2008-05-26 at 17:09 +0300, Tomas Winkler wrote:
>> From: Ron Rindjunsky <ron.rindjunsky@intel.com>
>>
>> This patch fixes a deadlock of sta->lock use, occuring while changing
>> tx aggregation states, as dev_queue_xmit end up in new function
>> test_and_clear_sta_flags that uses that lock thus leading to deadlock
>
> Oh, good catch, thanks. Reminds me to debug the other deadlock I saw
> (not mac80211 related though)
>
>> Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
>> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>
> Acked-by: Johannes Berg <johannes@sipsolutions.net>
>
>> ---
>>  net/mac80211/main.c |   10 +++++++---
>>  1 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>> index b0fddb7..5a9030c 100644
>> --- a/net/mac80211/main.c
>> +++ b/net/mac80211/main.c
>> @@ -662,6 +662,8 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
>>               goto start_ba_err;
>>       }
>>
>> +     spin_unlock_bh(&sta->lock);
>> +
>>       /* Will put all the packets in the new SW queue */
>>       ieee80211_requeue(local, ieee802_1d_to_ac[tid]);
>>       spin_unlock_bh(&local->mdev->queue_lock);
>> @@ -688,9 +690,9 @@ start_ba_err:
>>       kfree(sta->ampdu_mlme.tid_tx[tid]);
>>       sta->ampdu_mlme.tid_tx[tid] = NULL;
>>       spin_unlock_bh(&local->mdev->queue_lock);
>> +     spin_unlock_bh(&sta->lock);
>>       ret = -EBUSY;
>>  start_ba_exit:
>> -     spin_unlock_bh(&sta->lock);
>>       rcu_read_unlock();
>>       return ret;
>>  }
>> @@ -829,10 +831,11 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid)
>>       }
>>       state = &sta->ampdu_mlme.tid_state_tx[tid];
>>
>> -     spin_lock_bh(&sta->lock);
>> +     /* no need to use sta->lock in this state check, as
>> +      * ieee80211_stop_tx_ba_session will let only
>> +      * one stop call to pass through per sta/tid */
>>       if ((*state & HT_AGG_STATE_REQ_STOP_BA_MSK) == 0) {
>>               printk(KERN_DEBUG "unexpected callback to A-MPDU stop\n");
>> -             spin_unlock_bh(&sta->lock);
>>               rcu_read_unlock();
>>               return;
>>       }
>> @@ -855,6 +858,7 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid)
>>        * ieee80211_wake_queue is not used here as this queue is not
>>        * necessarily stopped */
>>       netif_schedule(local->mdev);
>> +     spin_lock_bh(&sta->lock);
>>       *state = HT_AGG_STATE_IDLE;
>>       sta->ampdu_mlme.addba_req_num[tid] = 0;
>>       kfree(sta->ampdu_mlme.tid_tx[tid]);
>

This patch is wrong, it breaks error path.locking.Will resubmit
Tomas

Re: [PATCH 1/1] mac80211: fix deadlock in sta->lock

[Ron Rindjunsky]

>
> This patch is wrong, it breaks error path.locking.Will resubmit
> Tomas
>

agree, the error path should be fixed, thanks.