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 Tuesday 23 March 2010 03:12:44 you wrote:
> On Sun, 2010-03-21 at 11:01 +0900, Bruno Randolf wrote:
> > On Sunday 21 March 2010 05:40:05 Johannes Berg wrote:
> > > On Sat, 2010-03-20 at 21:02 +0100, Lorenzo Bianconi wrote:
> > > > >> 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.
> >
> > 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.
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...
bruno
On Sunday 21 March 2010 05:40:05 Johannes Berg wrote:
> On Sat, 2010-03-20 at 21:02 +0100, Lorenzo Bianconi wrote:
> > >> 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.
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.
bruno
On Sun, 2010-03-21 at 11:01 +0900, Bruno Randolf wrote:
> On Sunday 21 March 2010 05:40:05 Johannes Berg wrote:
> > On Sat, 2010-03-20 at 21:02 +0100, Lorenzo Bianconi wrote:
> > > >> 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.
>
> 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.
johannes
On Sat, 2010-03-20 at 21:02 +0100, Lorenzo Bianconi wrote:
> >> 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.
johannes
On Wed, 2010-03-31 at 17:12 +0900, Bruno Randolf wrote:
> On Tuesday 23 March 2010 03:12:44 you wrote:
> > On Sun, 2010-03-21 at 11:01 +0900, Bruno Randolf wrote:
> > > On Sunday 21 March 2010 05:40:05 Johannes Berg wrote:
> > > > On Sat, 2010-03-20 at 21:02 +0100, Lorenzo Bianconi wrote:
> > > > > >> 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.
> > >
> > > 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.
>
> 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...
I don't think there's any way this can be a driver bug, assuming it
stops the queues properly, which it has to unless it comes up with its
own queueing scheme...
johannes
> On Saturday 20 March 2010 11:44:24 Bruno Randolf wrote:
>> 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.
>
> forgot to add: [1] and [2] are connected by ethernet to [WL1/2]...
> (obviously?)
>
> bruno
>
Hi,
I tested the same network configuration used by bruno:
PC1<--ethernet cable-->Device1<--wireless link-->Device2<--ethernet cable-->PC2
but I injected packets using a VAP in monitor mode on both wireless devices.
I used UDP iperf traffic (50Mbps) from PC1 to PC2 as data stream. I
agree with bruno,
I think the main aspect of the test is to transmit a traffic steam
that the wireless link is not
able to handle. In order to trigger the crash, if the device is EDCA
capable, I suggest to use the
Best Effort hardware queue (skb->priority=3, AC_queue=2 in the
ieee80211_select_queue()) for all
data packets.
Regards
Lorenzo
On Sun, 2010-03-21 at 11:01 +0900, Bruno Randolf wrote:
> On Sunday 21 March 2010 05:40:05 Johannes Berg wrote:
> > On Sat, 2010-03-20 at 21:02 +0100, Lorenzo Bianconi wrote:
> > > >> 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.
>
> 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.
Yeah, but since we stop the netdev queues it should already be happening
there.
johannes
On Saturday 20 March 2010 11:44:24 Bruno Randolf wrote:
> 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.
forgot to add: [1] and [2] are connected by ethernet to [WL1/2]...
(obviously?)
bruno