mac80211: Revise,pending queue depth in ieee80211_local data structure

Hi all,

I resend the patch in order to fix style violations that Larry suggested me.

I noticed a possible issue in the pending queue management of the ieee80211_local data structure. In particular, there is no control of the queue depth and this could cause a memory overflow. In the tests I carried out I obtain a memory overflow when I use a low priority queue (e.g. Backgreound queue) and I transmit a data stream that exceeds the channel capacity (e.g. 50Mbps@MCS 3, 800ns GI and 20MHz channel width). I tested the patch below on the last compat-wireless (2010-03-03) on an AR9280 chipset (Ubiquiti Rocket M with the latest version of OpenWrt trunk).

Please try this.

johannes

---
 net/mac80211/tx.c   |    6 ++++++
 net/mac80211/util.c |    6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

net/mac80211/tx.c +6 -0

@@ -1991,6 +1991,7 @@ static bool ieee80211_tx_pending_skb(str
 void ieee80211_tx_pending(unsigned long data)
 {
 	struct ieee80211_local *local = (struct ieee80211_local *)data;
+	struct ieee80211_sub_if_data *sdata;
 	unsigned long flags;
 	int i;
 	bool txok;

@@ -2029,6 +2030,11 @@ void ieee80211_tx_pending(unsigned long
 			if (!txok)
 				break;
 		}
+
+		if (skb_queue_empty(&local->pending[i]))
+			list_for_each_entry_rcu(sdata, &local->interfaces, list)
+				netif_tx_wake_queue(
+					netdev_get_tx_queue(sdata->dev, i));
 	}
 	spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
 

net/mac80211/util.c +0 -1

@@ -268,7 +268,6 @@ static void __ieee80211_wake_queue(struc
 				   enum queue_stop_reason reason)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
-	struct ieee80211_sub_if_data *sdata;
 
 	if (WARN_ON(queue >= hw->queues))
 		return;

@@ -281,11 +280,6 @@ static void __ieee80211_wake_queue(struc
 

net/mac80211/util.c static void __ieee80211_wake_queue(struc · line 288

Quoted 7 lines

net/mac80211/util.c +0 -5

 	if (!skb_queue_empty(&local->pending[queue]))
 		tasklet_schedule(&local->tx_pending_tasklet);
-
-	rcu_read_lock();
-	list_for_each_entry_rcu(sdata, &local->interfaces, list)
-		netif_tx_wake_queue(netdev_get_tx_queue(sdata->dev, queue));
-	rcu_read_unlock();

I tested the patch on kernel 2.6.32.7 with compat-wireless-2010-03-03

it seems that the problem is not solved. If I set the lowest priority

(Backgreound), the system will crash for an out of memory panic. During

tests I carried out, I transmit 50Mbps UDP traffic.

On Fri, 19 Mar 2010 10:33:47 +0100, "[email protected]"

Quoted 9 lines

net/mac80211/util.c static void __ieee80211_wake_queue(struc · line 288

Quoted 9 lines

net/mac80211/util.c +0 -5

 	if (!skb_queue_empty(&local->pending[queue]))
 
 		tasklet_schedule(&local->tx_pending_tasklet);
 
-
-	rcu_read_lock();
-	list_for_each_entry_rcu(sdata, &local->interfaces, list)
-		netif_tx_wake_queue(netdev_get_tx_queue(sdata->dev, queue));
-	rcu_read_unlock();

That's obviously wrong, need to move the code to the else branch of the above if instead.

Quoted 1 line

I tested the patch on kernel 2.6.32.7 with compat-wireless-2010-03-03

but

Quoted 1 line

it seems that the problem is not solved. If I set the lowest priority

queue

Quoted 1 line

(Backgreound), the system will crash for an out of memory panic. During

the

Quoted 1 line

tests I carried out, I transmit 50Mbps UDP traffic.

How are you generating traffic? I just dumped like 2Gbps traffic at it and everything works just fine. I verified queues are stopped and started properly.

i think i'm hitting the same bug, but not sure (i will try to check next week). this is easy to reproduce, but you need 4 (at least 3) machines:

setup 2 wireless cards, talking to each other (i use IBSS) on different machines. then setup routing like this:

[1] - [WL1] - [WL2] - [2]

now send more UDP traffic than the wireless link can handle (iperf -u -b 50M for example) between [1] and [2]. watch the free memory get less and less at [WL1] until it chrashes.

Quoted 8 lines

i think i'm hitting the same bug, but not sure (i will try to check next week). this is easy to reproduce, but you need 4 (at least 3) machines:

setup 2 wireless cards, talking to each other (i use IBSS) on different machines. then setup routing like this:

[1] - [WL1] - [WL2] - [2]

Quoted 3 lines

now send more UDP traffic than the wireless link can handle (iperf -u -b 50M for example) between [1] and [2]. watch the free memory get less and less at [WL1] until it chrashes.

forgot to add: [1] and [2] are connected by ethernet to [WL1/2]... (obviously?)

bruno

Quoted 2 lines

Quoted 2 lines

setup 2 wireless cards, talking to each other (i use IBSS) on different machines. then setup routing like this:

PC1<--ethernet cable-->Device1<--wireless link-->Device2<--ethernet cable-->PC2

Quoted 6 lines · Lorenzo Bianconi

Quoted 4 lines

Quoted 4 lines

setup 2 wireless cards, talking to each other (i use IBSS) on different

machines. then setup routing like this:

PC1<--ethernet cable-->Device1<--wireless link-->Device2<--ethernet cable-->PC2

I'm not really sure how this scenario differs from injecting traffic locally, but I'll try to reproduce.

Quoted 8 lines · On Sunday 21 March 2010 05:40:05 Johannes Berg wrote:

Quoted 6 lines · Lorenzo Bianconi

Quoted 4 lines

Quoted 4 lines

setup 2 wireless cards, talking to each other (i use IBSS) on different

machines. then setup routing like this:

PC1<--ethernet cable-->Device1<--wireless link-->Device2<--ethernet cable-->PC2

Quoted 2 lines

I'm not really sure how this scenario differs from injecting traffic locally, but I'll try to reproduce.

if you generate traffic locally it's enough to stop the queues, this will cause userspace to stop generating traffic. in the forwarding case (which we are talking about) stopping the queues will not stop the sender to send traffic, thus we have to start dropping packets at some point.

Quoted 8 lines · On Sunday 21 March 2010 05:40:05 Johannes Berg wrote:

Quoted 6 lines · Lorenzo Bianconi

Quoted 4 lines

Quoted 4 lines

setup 2 wireless cards, talking to each other (i use IBSS) on different

machines. then setup routing like this:

PC1<--ethernet cable-->Device1<--wireless link-->Device2<--ethernet cable-->PC2

Quoted 2 lines

I'm not really sure how this scenario differs from injecting traffic locally, but I'll try to reproduce.

if you generate traffic locally it's enough to stop the queues, this will cause userspace to stop generating traffic. in the forwarding case (which we are talking about) stopping the queues will not stop the sender to send traffic, thus we have to start dropping packets at some point.

Quoted 9 lines · Bruno Randolf

Quoted 8 lines · On Sunday 21 March 2010 05:40:05 Johannes Berg wrote:

Quoted 6 lines · Lorenzo Bianconi

Quoted 4 lines

Quoted 4 lines

setup 2 wireless cards, talking to each other (i use IBSS) on different

machines. then setup routing like this:

PC1<--ethernet cable-->Device1<--wireless link-->Device2<--ethernet cable-->PC2

Quoted 7 lines

Quoted 2 lines

I'm not really sure how this scenario differs from injecting traffic locally, but I'll try to reproduce.

if you generate traffic locally it's enough to stop the queues, this will cause userspace to stop generating traffic. in the forwarding case (which we are talking about) stopping the queues will not stop the sender to send traffic, thus we have to start dropping packets at some point.

So I tried this scenario, and I see the queues stopping, and traffic being dropped quite as you'd want it. Even the race condition that I noticed didn't ever trigger.

Please make sure you're using a kernel recent enough to include the fixes, maybe try compat-wireless.

Quoted 22 lines · On Tuesday 23 March 2010 03:12:44 you wrote:

Quoted 9 lines · Bruno Randolf

Quoted 8 lines · On Sunday 21 March 2010 05:40:05 Johannes Berg wrote:

Quoted 6 lines · Lorenzo Bianconi

Quoted 4 lines

Quoted 4 lines

setup 2 wireless cards, talking to each other (i use IBSS) on different

machines. then setup routing like this:

PC1<--ethernet cable-->Device1<--wireless link-->Device2<--ethernet cable-->PC2

Quoted 7 lines

Quoted 2 lines

I'm not really sure how this scenario differs from injecting traffic locally, but I'll try to reproduce.

if you generate traffic locally it's enough to stop the queues, this will cause userspace to stop generating traffic. in the forwarding case (which we are talking about) stopping the queues will not stop the sender to send traffic, thus we have to start dropping packets at some point.

So I tried this scenario, and I see the queues stopping, and traffic being dropped quite as you'd want it. Even the race condition that I noticed didn't ever trigger.

Quoted 2 lines

Please make sure you're using a kernel recent enough to include the fixes, maybe try compat-wireless.

so it must be an ath5k bug then... lorenzo's description just resembled my case so much, that i thought it could be the same bug. sorry to have bothered you with this...