[PATCH] ar9170usb: fix panic triggered by undersized rxstream buffer

[PATCH] ar9170usb: fix panic triggered by undersized rxstream buffer

[Christian Lamparter]

While ar9170's USB transport packet size is currently set to 8KiB,
the PHY is capable of receiving AMPDUs with up to 64KiB.
Such a large frame will be split over several rx URBs and
exceed the previously allocated space for rx stream reconstruction.

This patch increases the buffer size to 64KiB which is
in fact the phy & rx stream designed size limit.

Cc: stable@kernel.org
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=15591
Reported-by: Christian Mehlis <mehlis@inf.fu-berlin.de>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
diff --git a/drivers/net/wireless/ath/ar9170/hw.h b/drivers/net/wireless/ath/ar9170/hw.h
index 0a1d4c2..06f1f3c 100644
--- a/drivers/net/wireless/ath/ar9170/hw.h
+++ b/drivers/net/wireless/ath/ar9170/hw.h
@@ -425,5 +425,6 @@ enum ar9170_txq {
 
 #define AR9170_TXQ_DEPTH	32
 #define AR9170_TX_MAX_PENDING	128
+#define AR9170_RX_STREAM_MAX_SIZE 65535
 
 #endif /* __AR9170_HW_H */
diff --git a/drivers/net/wireless/ath/ar9170/main.c b/drivers/net/wireless/ath/ar9170/main.c
index 257c734..e017119 100644
--- a/drivers/net/wireless/ath/ar9170/main.c
+++ b/drivers/net/wireless/ath/ar9170/main.c
@@ -2515,7 +2515,7 @@ void *ar9170_alloc(size_t priv_size)
 	 * tends to split the streams into separate rx descriptors.
 	 */
 
-	skb = __dev_alloc_skb(AR9170_MAX_RX_BUFFER_SIZE, GFP_KERNEL);
+	skb = __dev_alloc_skb(AR9170_RX_STREAM_MAX_SIZE, GFP_KERNEL);
 	if (!skb)
 		goto err_nomem;
 

Re: [PATCH] ar9170usb: fix panic triggered by undersized rxstream buffer

[Johannes Berg]

On Tue, 2010-03-23 at 21:51 +0100, Christian Lamparter wrote:
> While ar9170's USB transport packet size is currently set to 8KiB,
> the PHY is capable of receiving AMPDUs with up to 64KiB.
> Such a large frame will be split over several rx URBs and
> exceed the previously allocated space for rx stream reconstruction.
> 
> This patch increases the buffer size to 64KiB which is
> in fact the phy & rx stream designed size limit.

That's a pretty high order allocation, you may want to paged allocation
-- you'll end up doing a order 5 allocation here!

johannes

Re: [PATCH] ar9170usb: fix panic triggered by undersized rxstream buffer

[Zhu Yi]

On Wed, 2010-03-24 at 09:59 +0800, Johannes Berg wrote:
> On Tue, 2010-03-23 at 21:51 +0100, Christian Lamparter wrote:
> > While ar9170's USB transport packet size is currently set to 8KiB,
> > the PHY is capable of receiving AMPDUs with up to 64KiB.
> > Such a large frame will be split over several rx URBs and
> > exceed the previously allocated space for rx stream reconstruction.
> > 
> > This patch increases the buffer size to 64KiB which is
> > in fact the phy & rx stream designed size limit.
> 
> That's a pretty high order allocation, you may want to paged allocation
> -- you'll end up doing a order 5 allocation here!

Yup, order-5 given the struct skb_shared_info overhead in __alloc_skb().
If the URBs are split over, you probably don't need to allocate such a
big chunk of memory in one go. You just need to connect them into a
paged skb later before pushing to mac80211. BTW, I've moved the
skb_linearize() from iwlwifi to mac80211. Will submit the patches today.

Thanks,
-yi

Re: [PATCH] ar9170usb: fix panic triggered by undersized rxstream buffer

[Christian Lamparter]

On Wednesday 24 March 2010 03:20:18 Zhu Yi wrote:
> On Wed, 2010-03-24 at 09:59 +0800, Johannes Berg wrote:
> > On Tue, 2010-03-23 at 21:51 +0100, Christian Lamparter wrote:
> > > While ar9170's USB transport packet size is currently set to 8KiB,
> > > the PHY is capable of receiving AMPDUs with up to 64KiB.
> > > Such a large frame will be split over several rx URBs and
> > > exceed the previously allocated space for rx stream reconstruction.
> > > 
> > > This patch increases the buffer size to 64KiB which is
> > > in fact the phy & rx stream designed size limit.
> > 
> > That's a pretty high order allocation, you may want to paged allocation
> > -- you'll end up doing a order 5 allocation here!
> Yup, order-5 given the struct skb_shared_info overhead in __alloc_skb().
> If the URBs are split over, you probably don't need to allocate such a
> big chunk of memory in one go. 

I know, I know, but unfortunately I do need a continuous address space.
The reason is that usually one URB can have up to 5 data frames
(each with ~1600 Octets). That's why the driver memcpys everything
from the rxstream skbs into newly allocated ones for each individual frame.
(The reconstruction  simply adds another memcpy to a temporary buffer,
 until we have everything...).

as far as I can tell, there's only one other options:
 * mix vmalloc with paged skbs API ;-)

   This looks kind of funny. The API abuse is highly questionable and
   probably not something for "stable".

 * early drop, if len > 8KiB

   This has the downside that we lose the data and the rx stream state.
   (E.g.: signal quality & phy data, mac error codes, the lot...)

 * something I don't know?

   please tell me about it!

> You just need to connect them into a paged skb later before pushing to mac80211.
> BTW, I've moved the skb_linearize() from iwlwifi to mac80211. Will submit the patches today.
AFAIK Atheros resolved this issue with AR9271. At least that's what I
heard from Luis about the _new_ scatter/gather IO implementation.

Regards,
	Chr