* pending queue depth in ieee80211_local data structure
@ 2010-03-18 10:12 Lorenzo Bianconi
2010-03-18 10:44 ` Bruno Randolf
0 siblings, 1 reply; 4+ messages in thread
From: Lorenzo Bianconi @ 2010-03-18 10:12 UTC (permalink / raw)
To: linux-wireless
Hi all,
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 test I carried out this happen 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 wrote this patch in order to fix the issue.
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
--- 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
@@ -1399,13 +1399,15 @@
skb = tx.skb;
spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
-
+
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
*/
+ if (skb_queue_len(&local->pending[queue]) >= PENDING_BUF)
+ goto drop;
do {
next = skb->next;
skb->next = NULL;
@@ -2028,8 +2030,12 @@
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: pending queue depth in ieee80211_local data structure 2010-03-18 10:12 pending queue depth in ieee80211_local data structure Lorenzo Bianconi @ 2010-03-18 10:44 ` Bruno Randolf 2010-03-18 11:35 ` Lorenzo Bianconi 0 siblings, 1 reply; 4+ messages in thread From: Bruno Randolf @ 2010-03-18 10:44 UTC (permalink / raw) To: Lorenzo Bianconi; +Cc: linux-wireless On Thursday 18 March 2010 19:12:32 Lorenzo Bianconi wrote: > Hi all, > > 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 test I carried out this happen 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 wrote this patch in order to fix the issue. i think, i noticed the same issue: sending a UDP stream which is higher than the possible bandwidth will eventually cause an out of memory panic. bruno > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> > > --- 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 > @@ -1399,13 +1399,15 @@ > skb = tx.skb; > > spin_lock_irqsave(&local->queue_stop_reason_lock, flags); > - > + > 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 > */ > + if (skb_queue_len(&local->pending[queue]) >= PENDING_BUF) > + goto drop; > do { > next = skb->next; > skb->next = NULL; > @@ -2028,8 +2030,12 @@ > 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 > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" > in the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: pending queue depth in ieee80211_local data structure 2010-03-18 10:44 ` Bruno Randolf @ 2010-03-18 11:35 ` Lorenzo Bianconi 2010-03-18 12:56 ` Larry Finger 0 siblings, 1 reply; 4+ messages in thread From: Lorenzo Bianconi @ 2010-03-18 11:35 UTC (permalink / raw) To: br1, ht6100; +Cc: linux-wireless > On Thursday 18 March 2010 19:12:32 Lorenzo Bianconi wrote: >> Hi all, >> >> 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 test I carried out this happen 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 wrote this patch in order to fix the issue. > > i think, i noticed the same issue: sending a UDP stream which is higher than > the possible bandwidth will eventually cause an out of memory panic. > > bruno > >> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> >> >> --- 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 >> @@ -1399,13 +1399,15 @@ >> skb = tx.skb; >> >> spin_lock_irqsave(&local->queue_stop_reason_lock, flags); >> - >> + >> 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 >> */ >> + if (skb_queue_len(&local->pending[queue]) >= > PENDING_BUF) >> + goto drop; >> do { >> next = skb->next; >> skb->next = NULL; >> @@ -2028,8 +2030,12 @@ >> 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 >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-wireless" >> in the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > Hi all, I pasted the first version of the patch where I missed to unlock the spinlock in the ieee80211_tx(). This is the last version of the patch. Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -708,6 +708,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 @@ -1449,14 +1449,18 @@ skb = tx.skb; spin_lock_irqsave(&local->queue_stop_reason_lock, flags); - + 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)) @@ -2074,8 +2078,12 @@ 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: pending queue depth in ieee80211_local data structure 2010-03-18 11:35 ` Lorenzo Bianconi @ 2010-03-18 12:56 ` Larry Finger 0 siblings, 0 replies; 4+ messages in thread From: Larry Finger @ 2010-03-18 12:56 UTC (permalink / raw) To: Lorenzo Bianconi; +Cc: br1, ht6100, linux-wireless On 03/18/2010 06:35 AM, Lorenzo Bianconi wrote: > Hi all, > > I pasted the first version of the patch where I missed to unlock the > spinlock in the ieee80211_tx(). > This is the last version of the patch. Probably not. > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> > > --- a/net/mac80211/ieee80211_i.h > +++ b/net/mac80211/ieee80211_i.h > @@ -708,6 +708,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 > @@ -1449,14 +1449,18 @@ > skb = tx.skb; > > spin_lock_irqsave(&local->queue_stop_reason_lock, flags); > - > + The new line here has trailing white space. I wondered why you were changing one blank line for another. You should use scripts/checkpatch to verify your patch. That script would have caught this. > 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)) > @@ -2074,8 +2078,12 @@ > 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++) John Linville's efforts as the wireless maintainer are made easier when everyone follows the guidelines in Documentation/SubmittingPatches. For instance, this patch should have been submitted with the subject "[PATCH V2] mac80211: Revise pending queue depth in ieee80211_local data structure", or some such title. At the beginning of the submission, you should describe the problem following the guidelines mentioned above. This section is followed by the "Signed-off-by:" line with a line consisting of "---". Everything above this line becomes part of the official record if/when the patch is accepted. In this case, the quoting of previous emails and the inclusion of the previous patch is inappropriate. Below the ---, you can include additional information such as how this version differs from previous submissions, and any instructions to John. I have not reviewed the content of this patch - only the problem with the white space caught my eye. Larry ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-03-18 12:56 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-03-18 10:12 pending queue depth in ieee80211_local data structure Lorenzo Bianconi 2010-03-18 10:44 ` Bruno Randolf 2010-03-18 11:35 ` Lorenzo Bianconi 2010-03-18 12:56 ` Larry Finger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).