[PATCH] ath5k: consistently use rx_bufsize for RX DMA

[PATCH] ath5k: consistently use rx_bufsize for RX DMA

[Bruno Randolf]

We should use the same buffer size we set up for DMA also in the hardware
descriptor. Previously we used common->rx_bufsize for setting up the DMA
mapping, but skb_tailroom(skb) for the size we tell to the hardware in the
descriptor itself. The problem is that skb_tailroom(skb) can give us a larger
value than the size we set up for DMA before: In my case rx_bufsize is 2528,
and we allocated an skb of 2559 bytes length, including padding for cache
alignment, but sbk_tailroom() was 2592. Just consistently use rx_bufsize for
all RX DMA memory sizes.

Also check the return value of setup function.

Signed-off-by: Bruno Randolf <br1@einfach.org>
---

PS: I'm not sure if this is enough to fix bug 15861, but it's definetly one
possibility where the hardware could write out of bounds. But the bug report
looks too similar to the ath9k one, so i suspect a common bug between ath5k and
ath9k. (?)

---
 drivers/net/wireless/ath/ath5k/base.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 9f42af7..5f24e4a 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -1217,6 +1217,7 @@ ath5k_rxbuf_setup(struct ath5k_softc *sc, struct ath5k_buf *bf)
 	struct ath5k_hw *ah = sc->ah;
 	struct sk_buff *skb = bf->skb;
 	struct ath5k_desc *ds;
+	int ret;
 
 	if (!skb) {
 		skb = ath5k_rx_skb_alloc(sc, &bf->skbaddr);
@@ -1243,9 +1244,9 @@ ath5k_rxbuf_setup(struct ath5k_softc *sc, struct ath5k_buf *bf)
 	ds = bf->desc;
 	ds->ds_link = bf->daddr;	/* link to self */
 	ds->ds_data = bf->skbaddr;
-	ah->ah_setup_rx_desc(ah, ds,
-		skb_tailroom(skb),	/* buffer size */
-		0);
+	ret = ah->ah_setup_rx_desc(ah, ds, ah->common.rx_bufsize, 0);
+	if (ret)
+		return ret;
 
 	if (sc->rxlink != NULL)
 		*sc->rxlink = bf->daddr;

Re: [PATCH] ath5k: consistently use rx_bufsize for RX DMA

[Luis R. Rodriguez]

On Fri, May 14, 2010 at 12:50 AM, Bruno Randolf <br1@einfach.org> wrote:
> We should use the same buffer size we set up for DMA also in the hardware
> descriptor. Previously we used common->rx_bufsize for setting up the DMA
> mapping, but skb_tailroom(skb) for the size we tell to the hardware in the
> descriptor itself. The problem is that skb_tailroom(skb) can give us a larger
> value than the size we set up for DMA before: In my case rx_bufsize is 2528,
> and we allocated an skb of 2559 bytes length, including padding for cache
> alignment, but sbk_tailroom() was 2592. Just consistently use rx_bufsize for
> all RX DMA memory sizes.
>
> Also check the return value of setup function.
>
> Signed-off-by: Bruno Randolf <br1@einfach.org>

Cc: stable?

Is that other bug reproducible, can the user test this to cure it?

  Luis

Re: [PATCH] ath5k: consistently use rx_bufsize for RX DMA

[Bruno Randolf]

On Saturday 15 May 2010 01:04:34 Luis R. Rodriguez wrote:
> On Fri, May 14, 2010 at 12:50 AM, Bruno Randolf <br1@einfach.org> wrote:
> > We should use the same buffer size we set up for DMA also in the hardware
> > descriptor. Previously we used common->rx_bufsize for setting up the DMA
> > mapping, but skb_tailroom(skb) for the size we tell to the hardware in
> > the descriptor itself. The problem is that skb_tailroom(skb) can give us
> > a larger value than the size we set up for DMA before: In my case
> > rx_bufsize is 2528, and we allocated an skb of 2559 bytes length,
> > including padding for cache alignment, but sbk_tailroom() was 2592. Just
> > consistently use rx_bufsize for all RX DMA memory sizes.
> > 
> > Also check the return value of setup function.
> > 
> > Signed-off-by: Bruno Randolf <br1@einfach.org>
> 
> Cc: stable?

might be useful. i just would like some review before that.

> Is that other bug reproducible, can the user test this to cure it?

not sure. seems like he can with running kismet for a few hours (i'm doing the 
same over the weekend). i doubt that this is "the" bug though... a) because 
the bug on ath5k and ath9k are so similar and b) because we see the beginning 
of packets. rather looks like the next pointer of the descriptor is pointing 
to somewhere it shouldnt, sometimes? (guessing)

bruno

Re: [PATCH] ath5k: consistently use rx_bufsize for RX DMA

[Luis R. Rodriguez]

On Fri, May 14, 2010 at 03:05:22PM -0700, Bruno Randolf wrote:
> On Saturday 15 May 2010 01:04:34 Luis R. Rodriguez wrote:
> > On Fri, May 14, 2010 at 12:50 AM, Bruno Randolf <br1@einfach.org> wrote:
> > > We should use the same buffer size we set up for DMA also in the hardware
> > > descriptor. Previously we used common->rx_bufsize for setting up the DMA
> > > mapping, but skb_tailroom(skb) for the size we tell to the hardware in
> > > the descriptor itself. The problem is that skb_tailroom(skb) can give us
> > > a larger value than the size we set up for DMA before: In my case
> > > rx_bufsize is 2528, and we allocated an skb of 2559 bytes length,
> > > including padding for cache alignment, but sbk_tailroom() was 2592. Just
> > > consistently use rx_bufsize for all RX DMA memory sizes.
> > > 
> > > Also check the return value of setup function.
> > > 
> > > Signed-off-by: Bruno Randolf <br1@einfach.org>
> > 
> > Cc: stable?
> 
> might be useful. i just would like some review before that.

FWIW

Reviewed-by: Luis R. Rodriguez <lrodriguez@atheros.com>

> > Is that other bug reproducible, can the user test this to cure it?
> 
> not sure. seems like he can with running kismet for a few hours (i'm doing the 
> same over the weekend). i doubt that this is "the" bug though... 

Oh I'm not assuming it is, but it still looks like a valid stable bug fix to me.

  Luis