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).
Signed-off-by: Lorenzo Bianconi <[email protected]>
---
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -703,6 +703,8 @@
struct work_struct sta_finish_work;
int sta_generation;
+/* Pending buffer dimension */
+#define PENDING_BUF 512
struct sk_buff_head pending[IEEE80211_MAX_QUEUES];
struct tasklet_struct tx_pending_tasklet;
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1403,10 +1403,17 @@
if (local->queue_stop_reasons[queue] ||
!skb_queue_empty(&local->pending[queue])) {
/*
- * if queue is stopped, queue up frames for later
- * transmission from the tasklet
+ * if queue is stopped and there is enough space
+ * in the queue, queue up frames for later transmission
+ * from the tasklet
*/
- do {
+ if (skb_queue_len(&local->pending[queue])
+ >= PENDING_BUF) {
+ spin_unlock_irqrestore(
+ &local->queue_stop_reason_lock,
+ flags);
+ goto drop;
+ } do {
next = skb->next;
skb->next = NULL;
if (unlikely(txpending))
@@ -2028,8 +2035,14 @@
flags);
txok = ieee80211_tx_pending_skb(local, skb);
- if (!txok)
- __skb_queue_head(&local->pending[i], skb);
+ if (!txok) {
+ if (skb_queue_len(&local->pending[i])
+ < PENDING_BUF)
+ __skb_queue_head(&local->pending[i],
+ skb);
+ else
+ kfree_skb(skb);
+ }
spin_lock_irqsave(&local->queue_stop_reason_lock,
flags);
if (!txok)
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -383,7 +383,10 @@
spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
__ieee80211_stop_queue(hw, queue, IEEE80211_QUEUE_STOP_REASON_SKB_ADD);
- __skb_queue_tail(&local->pending[queue], skb);
+ if (skb_queue_len(&local->pending[queue]) < PENDING_BUF)
+ __skb_queue_tail(&local->pending[queue], skb);
+ else
+ kfree_skb(skb);
__ieee80211_wake_queue(hw, queue, IEEE80211_QUEUE_STOP_REASON_SKB_ADD);
spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
}
@@ -409,9 +412,12 @@
continue;
}
- ret++;
queue = skb_get_queue_mapping(skb);
- __skb_queue_tail(&local->pending[queue], skb);
+ if (skb_queue_len(&local->pending[queue]) < PENDING_BUF) {
+ ret++;
+ __skb_queue_tail(&local->pending[queue], skb);
+ } else
+ kfree_skb(skb);
}
for (i = 0; i < hw->queues; i++)
--
Regards
Lorenzo
On Fri, 19 Mar 2010 10:33:47 +0100, "[email protected]"
>> 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.
> I tested the patch on kernel 2.6.32.7 with compat-wireless-2010-03-03
but
> it seems that the problem is not solved. If I set the lowest priority
queue
> (Backgreound), the system will crash for an out of memory panic. During
the
> 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.
johannes
Ok, I think I see the problem, but this is not the right thing to do.
The problem would seem to be synchronisation issues, I'll try to come up
with a fix.
johannes
Please try this.
johannes
---
net/mac80211/tx.c | 6 ++++++
net/mac80211/util.c | 6 ------
2 files changed, 6 insertions(+), 6 deletions(-)
--- wireless-testing.orig/net/mac80211/tx.c 2010-03-18 11:17:43.000000000 -0700
+++ wireless-testing/net/mac80211/tx.c 2010-03-18 11:19:16.000000000 -0700
@@ -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);
--- wireless-testing.orig/net/mac80211/util.c 2010-03-18 11:17:08.000000000 -0700
+++ wireless-testing/net/mac80211/util.c 2010-03-18 11:19:28.000000000 -0700
@@ -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
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();
}
void ieee80211_wake_queue_by_reason(struct ieee80211_hw *hw, int queue,
On Thu, 2010-03-18 at 16:43 +0100, [email protected] wrote:
> 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).
What kernel are you testing this on? I would have thought
cf0277e714a0db302a8f80e1b85fd61c32cf00b3 fixed this.
johannes
> Please try this.
>
> johannes
>
> ---
> net/mac80211/tx.c | 6 ++++++
> net/mac80211/util.c | 6 ------
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> --- wireless-testing.orig/net/mac80211/tx.c 2010-03-18 11:17:43.000000000 -0700
> +++ wireless-testing/net/mac80211/tx.c 2010-03-18 11:19:16.000000000 -0700
> @@ -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);
>
> --- wireless-testing.orig/net/mac80211/util.c 2010-03-18 11:17:08.000000000 -0700
> +++ wireless-testing/net/mac80211/util.c 2010-03-18 11:19:28.000000000 -0700
> @@ -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
>
> 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();
> }
>
> void ieee80211_wake_queue_by_reason(struct ieee80211_hw *hw, int queue,
>
>
Hi Johannes,
I tested the patch on kernel 2.6.32.7 with compat-wireless-2010-03-03 but
it seems that the problem is not solved. If I set the lowest priority queue
(Backgreound), the system will crash for an out of memory panic. During the
tests I carried out, I transmit 50Mbps UDP traffic.
Regards
Lorenzo
On Saturday 20 March 2010 03:49:27 Johannes Berg wrote:
> On Fri, 19 Mar 2010 10:33:47 +0100, "[email protected]"
>
> >> 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.
>
> > I tested the patch on kernel 2.6.32.7 with compat-wireless-2010-03-03
>
> but
>
> > it seems that the problem is not solved. If I set the lowest priority
>
> queue
>
> > (Backgreound), the system will crash for an out of memory panic. During
>
> the
>
> > 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.
bruno