* [PATCH] ath5k: Fix loop variable initializations
@ 2008-05-12 14:30 Bob Copeland
2008-05-12 20:27 ` Helge Deller
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Bob Copeland @ 2008-05-12 14:30 UTC (permalink / raw)
To: mickflemm, mcgrof, jirislaby; +Cc: linux-wireless, ath5k-devel
From: Bob Copeland <me@bobcopeland.com>
Date: Mon, 12 May 2008 09:25:33 -0400
Subject: [PATCH] ath5k: Fix loop variable initializations
In ath5k_tasklet_rx, both status structures 'rxs' and 'rs' are
initialized at the top of the tasklet, but not within the loop.
If the loop is executed multiple times in the tasklet, then the
variables may see changes from previous packets.
For TKIP, this results in 'Invalid Michael MIC' errors if two packets
are processed in the tasklet: rxs.flag gets set to RX_DECRYPTED by
mac80211 when it decrypts the first encrypted packet. The subsequent
packet will have RX_DECRYPTED set upon entry to mac80211, so mac80211
will not try to decrypt it.
We currently initialize all but two fields in the structures, so fix
the other two.
Signed-off-by: Bob Copeland <me@bobcopeland.com>
---
drivers/net/wireless/ath5k/base.c | 2 +-
drivers/net/wireless/ath5k/hw.c | 2 ++
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
index 4e5c8fc..fd8537e 100644
--- a/drivers/net/wireless/ath5k/base.c
+++ b/drivers/net/wireless/ath5k/base.c
@@ -1888,7 +1888,7 @@ accept:
* right now, so it's not too bad...
*/
rxs.mactime = ath5k_extend_tsf(sc->ah, rs.rs_tstamp);
- rxs.flag |= RX_FLAG_TSFT;
+ rxs.flag = RX_FLAG_TSFT;
rxs.freq = sc->curchan->center_freq;
rxs.band = sc->curband->band;
diff --git a/drivers/net/wireless/ath5k/hw.c b/drivers/net/wireless/ath5k/hw.c
index 5fb1ae6..8137d82 100644
--- a/drivers/net/wireless/ath5k/hw.c
+++ b/drivers/net/wireless/ath5k/hw.c
@@ -4119,6 +4119,7 @@ static int ath5k_hw_proc_5210_rx_status(struct ath5k_hw *ah,
rs->rs_tstamp = AR5K_REG_MS(rx_status->rx_status_1,
AR5K_5210_RX_DESC_STATUS1_RECEIVE_TIMESTAMP);
rs->rs_status = 0;
+ rs->rs_phyerr = 0;
/*
* Key table status
@@ -4193,6 +4194,7 @@ static int ath5k_hw_proc_5212_rx_status(struct ath5k_hw *ah,
rs->rs_tstamp = AR5K_REG_MS(rx_status->rx_status_1,
AR5K_5212_RX_DESC_STATUS1_RECEIVE_TIMESTAMP);
rs->rs_status = 0;
+ rs->rs_phyerr = 0;
/*
* Key table status
--
1.5.4.2.182.gb3092
--
Bob Copeland %% www.bobcopeland.com
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ath5k: Fix loop variable initializations 2008-05-12 14:30 [PATCH] ath5k: Fix loop variable initializations Bob Copeland @ 2008-05-12 20:27 ` Helge Deller 2008-05-12 23:18 ` Nick Kossifidis 2008-05-12 23:25 ` Nick Kossifidis 2 siblings, 0 replies; 6+ messages in thread From: Helge Deller @ 2008-05-12 20:27 UTC (permalink / raw) To: linux-wireless; +Cc: ath5k-devel Thanks Bob! This fixes http://bugzilla.kernel.org/show_bug.cgi?id=10605 Helge Bob Copeland wrote: > From: Bob Copeland <me@bobcopeland.com> > Date: Mon, 12 May 2008 09:25:33 -0400 > Subject: [PATCH] ath5k: Fix loop variable initializations > > In ath5k_tasklet_rx, both status structures 'rxs' and 'rs' are > initialized at the top of the tasklet, but not within the loop. > If the loop is executed multiple times in the tasklet, then the > variables may see changes from previous packets. > > For TKIP, this results in 'Invalid Michael MIC' errors if two packets > are processed in the tasklet: rxs.flag gets set to RX_DECRYPTED by > mac80211 when it decrypts the first encrypted packet. The subsequent > packet will have RX_DECRYPTED set upon entry to mac80211, so mac80211 > will not try to decrypt it. > > We currently initialize all but two fields in the structures, so fix > the other two. > > Signed-off-by: Bob Copeland <me@bobcopeland.com> > --- > drivers/net/wireless/ath5k/base.c | 2 +- > drivers/net/wireless/ath5k/hw.c | 2 ++ > 2 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/drivers/net/wireless/ath5k/base.c > b/drivers/net/wireless/ath5k/base.c index 4e5c8fc..fd8537e 100644 > --- a/drivers/net/wireless/ath5k/base.c > +++ b/drivers/net/wireless/ath5k/base.c > @@ -1888,7 +1888,7 @@ accept: > * right now, so it's not too bad... > */ > rxs.mactime = ath5k_extend_tsf(sc->ah, rs.rs_tstamp); > - rxs.flag |= RX_FLAG_TSFT; > + rxs.flag = RX_FLAG_TSFT; > > rxs.freq = sc->curchan->center_freq; > rxs.band = sc->curband->band; > diff --git a/drivers/net/wireless/ath5k/hw.c > b/drivers/net/wireless/ath5k/hw.c index 5fb1ae6..8137d82 100644 > --- a/drivers/net/wireless/ath5k/hw.c > +++ b/drivers/net/wireless/ath5k/hw.c > @@ -4119,6 +4119,7 @@ static int ath5k_hw_proc_5210_rx_status(struct > ath5k_hw *ah, > rs->rs_tstamp = AR5K_REG_MS(rx_status->rx_status_1, > AR5K_5210_RX_DESC_STATUS1_RECEIVE_TIMESTAMP); > rs->rs_status = 0; > + rs->rs_phyerr = 0; > > /* > * Key table status > @@ -4193,6 +4194,7 @@ static int ath5k_hw_proc_5212_rx_status(struct > ath5k_hw *ah, > rs->rs_tstamp = AR5K_REG_MS(rx_status->rx_status_1, > AR5K_5212_RX_DESC_STATUS1_RECEIVE_TIMESTAMP); > rs->rs_status = 0; > + rs->rs_phyerr = 0; > > /* > * Key table status ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ath5k: Fix loop variable initializations 2008-05-12 14:30 [PATCH] ath5k: Fix loop variable initializations Bob Copeland 2008-05-12 20:27 ` Helge Deller @ 2008-05-12 23:18 ` Nick Kossifidis 2008-05-13 0:17 ` Bob Copeland 2008-05-12 23:25 ` Nick Kossifidis 2 siblings, 1 reply; 6+ messages in thread From: Nick Kossifidis @ 2008-05-12 23:18 UTC (permalink / raw) To: Bob Copeland; +Cc: mcgrof, jirislaby, linux-wireless, ath5k-devel Have you checked this one... 1838 if (rs.rs_status & AR5K_RXERR_MIC) { 1839 rxs.flag |= RX_FLAG_MMIC_ERROR; 1840 goto accept; are you sure that doesn't happen ? Also why initialize rxs.flag here? > rxs.flag = RX_FLAG_TSFT; Why not doing a simple rxs.flag = 0 ; or a check of flags set by mac80211 at the start of the loop ? This way we'll also have a clean rxs.flag when doing rxs.flag |= RX_FLAG_MMIC_ERROR and we won't clear RX_FLAG_MMIC_ERROR when doing rxs.flag = RX_FLAG_TSFT; -- GPG ID: 0xD21DB2DB As you read this post global entropy rises. Have Fun ;-) Nick ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ath5k: Fix loop variable initializations 2008-05-12 23:18 ` Nick Kossifidis @ 2008-05-13 0:17 ` Bob Copeland 2008-05-13 1:16 ` Bob Copeland 0 siblings, 1 reply; 6+ messages in thread From: Bob Copeland @ 2008-05-13 0:17 UTC (permalink / raw) To: Nick Kossifidis; +Cc: mcgrof, jirislaby, linux-wireless, ath5k-devel On Tue, May 13, 2008 at 02:18:48AM +0300, Nick Kossifidis wrote: > Why not doing a simple > rxs.flag = 0 ; > or a check of flags set by mac80211 at the start of the loop ? Yeah that's a better idea. I'll respin the patch with this and the other suggestion. -- Bob Copeland %% www.bobcopeland.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ath5k: Fix loop variable initializations 2008-05-13 0:17 ` Bob Copeland @ 2008-05-13 1:16 ` Bob Copeland 0 siblings, 0 replies; 6+ messages in thread From: Bob Copeland @ 2008-05-13 1:16 UTC (permalink / raw) To: Nick Kossifidis; +Cc: mcgrof, jirislaby, linux-wireless, ath5k-devel On Mon, May 12, 2008 at 08:17:26PM -0400, Bob Copeland wrote: > On Tue, May 13, 2008 at 02:18:48AM +0300, Nick Kossifidis wrote: > > Why not doing a simple > > rxs.flag = 0 ; > > or a check of flags set by mac80211 at the start of the loop ? Here's an updated version: In ath5k_tasklet_rx, both status structures 'rxs' and 'rs' are initialized at the top of the tasklet, but not within the loop. If the loop is executed multiple times in the tasklet then the variables may see changes from previous packets. For TKIP, this results in 'Invalid Michael MIC' errors if two packets are processed in the tasklet: rxs.flag gets set to RX_DECRYPTED by mac80211 when it decrypts the first encrypted packet. The subsequent packet will have RX_DECRYPTED set upon entry to mac80211, so mac80211 will not try to decrypt it. We currently initialize all but two fields in the structures, so fix the other two. Signed-off-by: Bob Copeland <me@bobcopeland.com> --- drivers/net/wireless/ath5k/base.c | 2 ++ drivers/net/wireless/ath5k/hw.c | 6 ++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c index 4e5c8fc..635b9ac 100644 --- a/drivers/net/wireless/ath5k/base.c +++ b/drivers/net/wireless/ath5k/base.c @@ -1787,6 +1787,8 @@ ath5k_tasklet_rx(unsigned long data) spin_lock(&sc->rxbuflock); do { + rxs.flag = 0; + if (unlikely(list_empty(&sc->rxbuf))) { ATH5K_WARN(sc, "empty rx buf pool\n"); break; diff --git a/drivers/net/wireless/ath5k/hw.c b/drivers/net/wireless/ath5k/hw.c index 5fb1ae6..77990b5 100644 --- a/drivers/net/wireless/ath5k/hw.c +++ b/drivers/net/wireless/ath5k/hw.c @@ -4119,6 +4119,7 @@ static int ath5k_hw_proc_5210_rx_status(struct ath5k_hw *ah, rs->rs_tstamp = AR5K_REG_MS(rx_status->rx_status_1, AR5K_5210_RX_DESC_STATUS1_RECEIVE_TIMESTAMP); rs->rs_status = 0; + rs->rs_phyerr = 0; /* * Key table status @@ -4145,7 +4146,7 @@ static int ath5k_hw_proc_5210_rx_status(struct ath5k_hw *ah, if (rx_status->rx_status_1 & AR5K_5210_RX_DESC_STATUS1_PHY_ERROR) { rs->rs_status |= AR5K_RXERR_PHY; - rs->rs_phyerr = AR5K_REG_MS(rx_status->rx_status_1, + rs->rs_phyerr |= AR5K_REG_MS(rx_status->rx_status_1, AR5K_5210_RX_DESC_STATUS1_PHY_ERROR); } @@ -4193,6 +4194,7 @@ static int ath5k_hw_proc_5212_rx_status(struct ath5k_hw *ah, rs->rs_tstamp = AR5K_REG_MS(rx_status->rx_status_1, AR5K_5212_RX_DESC_STATUS1_RECEIVE_TIMESTAMP); rs->rs_status = 0; + rs->rs_phyerr = 0; /* * Key table status @@ -4215,7 +4217,7 @@ static int ath5k_hw_proc_5212_rx_status(struct ath5k_hw *ah, if (rx_status->rx_status_1 & AR5K_5212_RX_DESC_STATUS1_PHY_ERROR) { rs->rs_status |= AR5K_RXERR_PHY; - rs->rs_phyerr = AR5K_REG_MS(rx_err->rx_error_1, + rs->rs_phyerr |= AR5K_REG_MS(rx_err->rx_error_1, AR5K_RX_DESC_ERROR1_PHY_ERROR_CODE); } -- 1.5.4.2.182.gb3092 -- Bob Copeland %% www.bobcopeland.com ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ath5k: Fix loop variable initializations 2008-05-12 14:30 [PATCH] ath5k: Fix loop variable initializations Bob Copeland 2008-05-12 20:27 ` Helge Deller 2008-05-12 23:18 ` Nick Kossifidis @ 2008-05-12 23:25 ` Nick Kossifidis 2 siblings, 0 replies; 6+ messages in thread From: Nick Kossifidis @ 2008-05-12 23:25 UTC (permalink / raw) To: Bob Copeland; +Cc: mcgrof, jirislaby, linux-wireless, ath5k-devel > + rs->rs_phyerr = 0; > Do you mind also changing these to |= instead of = (just for code clarity) ? rs->rs_phyerr = AR5K_REG_MS(rx_status->rx_status_1, AR5K_5210_RX_DESC_STATUS1_PHY_ERROR); rs->rs_phyerr = AR5K_REG_MS(rx_err->rx_error_1, AR5K_RX_DESC_ERROR1_PHY_ERROR_CODE); -- GPG ID: 0xD21DB2DB As you read this post global entropy rises. Have Fun ;-) Nick ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-05-13 1:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-05-12 14:30 [PATCH] ath5k: Fix loop variable initializations Bob Copeland 2008-05-12 20:27 ` Helge Deller 2008-05-12 23:18 ` Nick Kossifidis 2008-05-13 0:17 ` Bob Copeland 2008-05-13 1:16 ` Bob Copeland 2008-05-12 23:25 ` Nick Kossifidis
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).