Re: hung task in mac80211

I'll look in a bit - but

+			mutex_unlock(&sta->ampdu_mlme.mtx);
 			___ieee80211_stop_rx_ba_session(
 				sta, tid, WLAN_BACK_RECIPIENT,
 				WLAN_REASON_QSTA_TIMEOUT, true);

This already has three underscores so shouldn't drop.

+			mutex_unlock(&sta->ampdu_mlme.mtx);
 			___ieee80211_stop_rx_ba_session(

ditto

+			mutex_unlock(&sta->ampdu_mlme.mtx);
 			__ieee80211_start_rx_ba_session(sta, 0, 0,
0, 1, tid,

maybe this one needs a ___ version then?

+			mutex_unlock(&sta->ampdu_mlme.mtx);
 			___ieee80211_stop_rx_ba_session(

already ___

I'm surprised nobody saw this before - though perhaps Sebastian's
useless report is the same.

johannes

On Wed, 06 Sep 2017 14:48:35 +0200
Johannes Berg [off-list ref] wrote:

I'll look in a bit - but

quoted

+			mutex_unlock(&sta->ampdu_mlme.mtx);
 			___ieee80211_stop_rx_ba_session(
 				sta, tid, WLAN_BACK_RECIPIENT,
 				WLAN_REASON_QSTA_TIMEOUT, true);  

This already has three underscores so shouldn't drop.

Right, of course.

[...]

quoted

+			mutex_unlock(&sta->ampdu_mlme.mtx);
 			__ieee80211_start_rx_ba_session(sta, 0, 0,
0, 1, tid,  

maybe this one needs a ___ version then?

Either that, or as it's a single call, perhaps just the following?
Matter of taste I guess...

diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index c92df492e898..377dd3c233d3 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c

@@ -332,10 +332,13 @@ void ieee80211_ba_session_work(struct work_struct *work)
 				WLAN_REASON_UNSPECIFIED, true);
 
 		if (test_and_clear_bit(tid,
-				       sta->ampdu_mlme.tid_rx_manage_offl))
+				       sta->ampdu_mlme.tid_rx_manage_offl)) {
+			mutex_unlock(&sta->ampdu_mlme.mtx);
 			__ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
 							IEEE80211_MAX_AMPDU_BUF,
 							false, true);
+			mutex_lock(&sta->ampdu_mlme.mtx);
+		}
 
 		if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
 				       sta->ampdu_mlme.tid_rx_manage_offl))

-- 
Stefano

On Wed, 2017-09-06 at 15:19 +0200, Stefano Brivio wrote:

On Wed, 06 Sep 2017 14:48:35 +0200
Johannes Berg [off-list ref] wrote:

quoted

I'll look in a bit - but

quoted

+			mutex_unlock(&sta->ampdu_mlme.mtx);
 			___ieee80211_stop_rx_ba_session(
 				sta, tid, WLAN_BACK_RECIPIENT,
 				WLAN_REASON_QSTA_TIMEOUT,
true);  

This already has three underscores so shouldn't drop.

Right, of course.

quoted

[...]

quoted

+			mutex_unlock(&sta->ampdu_mlme.mtx);
 			__ieee80211_start_rx_ba_session(sta, 0,
0,
0, 1, tid,  

maybe this one needs a ___ version then?

Either that, or as it's a single call, perhaps just the following?
Matter of taste I guess...

I don't think it's a matter of taste - for me, in principle, dropping
locks for small sections of code where the larger section holds it is a
bug waiting to happen. It may (may, I don't even know) be OK here, but
in general it's something to avoid.

johannes

On Wed, 06 Sep 2017 15:21:00 +0200
Johannes Berg [off-list ref] wrote:

On Wed, 2017-09-06 at 15:19 +0200, Stefano Brivio wrote:

quoted

On Wed, 06 Sep 2017 14:48:35 +0200
Johannes Berg [off-list ref] wrote:
  

quoted

I'll look in a bit - but
  

quoted

+			mutex_unlock(&sta->ampdu_mlme.mtx);
 			___ieee80211_stop_rx_ba_session(
 				sta, tid, WLAN_BACK_RECIPIENT,
 				WLAN_REASON_QSTA_TIMEOUT,
true);    

This already has three underscores so shouldn't drop.  

Right, of course.
  

quoted

[...]  

quoted

+			mutex_unlock(&sta->ampdu_mlme.mtx);
 			__ieee80211_start_rx_ba_session(sta, 0,
0,
0, 1, tid,    

maybe this one needs a ___ version then?  

Either that, or as it's a single call, perhaps just the following?
Matter of taste I guess...  

I don't think it's a matter of taste - for me, in principle, dropping
locks for small sections of code where the larger section holds it is a
bug waiting to happen. It may (may, I don't even know) be OK here, but
in general it's something to avoid.

Yes, that was based on the assumption that the initial part of
__ieee80211_start_rx_ba_session() can't really affect the AMPDU
state-machine in any way.

But sure, one small change there in the future and the assumption
doesn't hold anymore.


--
Stefano

On Wed, 2017-09-06 at 15:27 +0200, Stefano Brivio wrote:

Yes, that was based on the assumption that the initial part of
__ieee80211_start_rx_ba_session() can't really affect the AMPDU
state-machine in any way.

That's not really the point, if that changes that function would have
to move the locking around, and nothing else.

The point is more that code in ieee80211_ba_session_work() could assume
the lock is held across the entire loop, since that's the way it's
written and looks like even with your patch.

So for example replacing the loop of tid = 0..NUM_TIDS-1 with a
list_for_each_entry() would already be unsafe with the dropping if the
list were to require the mutex for locking.

johannes

On Wed, 06 Sep 2017 15:30:10 +0200
Johannes Berg [off-list ref] wrote:

So for example replacing the loop of tid = 0..NUM_TIDS-1 with a
list_for_each_entry() would already be unsafe with the dropping if the
list were to require the mutex for locking.

Sure. Still, it would need another code change to break, but in general
I do agree indeed. :)


--
Stefano