[PATCH 1/3] rt2x00: zero-out rx_status

[PATCH 1/3] rt2x00: zero-out rx_status

[Gabor Juhos]

In commit 'mac80211: support radiotap vendor namespace RX data'
new fields were added to 'struct ieee80211_rx_status'.
The rt2x00 driver does not initializes those fields and
this can cause unexpected behaviour.

The rt2x00 driver from the compat-wireless-2012-12-01
tarball caused the following warning:

  WARNING: at
  /devel/ramips/build_dir/target-mipsel_r2_uClibc-0.9.33.2/linux-ramips_rt305x/
  compat-wireless-2012-12-01/net/mac80211/rx.c:115 ieee80211_rx_irqsafe+0x274/0xbcc
  [mac80211]()
  Modules linked in: dwc_otg ledtrig_usbdev nf_nat_irc
  nf_nat_ftp nf_conntrack_irc nf_conntrack_ftp ipt_MASQUERADE
  iptable_nat nf_nat pppoe xt_conntrack xt_CT xt_NOTRACK iptable_raw
  xt_state nf_conntrack_ipv4 nf_defrag_ipv4 nf_conntrack pppox
  ipt_REJECT xt_TCPMSS xt_comment xt_multiport xt_mac xt_limit
  iptable_mangle iptable_filter ip_tables xt_tcpudp x_tables ppp_async
  ppp_generic slhc rt2800pci(O) rt2800lib(O) rt2x00soc(O) rt2x00pci(O)
  rt2x00lib(O) mac80211(O) usbcore usb_common nls_base crc_itu_t
  crc_ccitt eeprom_93cx6 cfg80211(O) compat(O) arc4 aes_generic
  crypto_blkcipher cryptomgr aead crypto_hash crypto_algapi leds_gpio
  button_hotplug(O) gpio_keys_polled input_polldev input_core
  Call Trace:
  [<801e96b4>] dump_stack+0x8/0x34
  [<80010a9c>] warn_slowpath_common+0x78/0xa4
  [<80010ae0>] warn_slowpath_null+0x18/0x24
  [<80a9710c>] ieee80211_rx_irqsafe+0x274/0xbcc [mac80211]

The patch ensures that each field gets initialized with
zeroes.

Cc: <users@rt2x00.serialmonkey.com>
Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
---
 drivers/net/wireless/rt2x00/rt2x00dev.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c
index 3248b42..be90277 100644
--- a/drivers/net/wireless/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/rt2x00/rt2x00dev.c
@@ -686,6 +686,8 @@ void rt2x00lib_rxdone(struct queue_entry *entry, gfp_t gfp)
 	 * to mac80211.
 	 */
 	rx_status = IEEE80211_SKB_RXCB(entry->skb);
+	memset(rx_status, 0, sizeof(*rx_status));
+
 	rx_status->mactime = rxdesc.timestamp;
 	rx_status->band = rt2x00dev->curr_band;
 	rx_status->freq = rt2x00dev->curr_freq;
-- 
1.7.10

[PATCH 2/3] ath5k: zero-out rx_status

[Gabor Juhos]

In commit 'mac80211: support radiotap vendor namespace RX data'
new fields were added to 'struct ieee80211_rx_status'.
The ath5k driver does not initializes those fields and
this can cause unexpected behaviour. The patch ensures
that each field gets initialized with zeroes.

Cc: <ath5k-devel@lists.ath5k.org>
Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
---
Compile tested only.
---
 drivers/net/wireless/ath/ath5k/base.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 4ad40cf..8adf9c3 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -1323,8 +1323,8 @@ ath5k_receive_frame(struct ath5k_hw *ah, struct sk_buff *skb,
 	ath5k_remove_padding(skb);
 
 	rxs = IEEE80211_SKB_RXCB(skb);
+	memset(rxs, 0, sizeof(*rxs));
 
-	rxs->flag = 0;
 	if (unlikely(rs->rs_status & AR5K_RXERR_MIC))
 		rxs->flag |= RX_FLAG_MMIC_ERROR;
 
-- 
1.7.10

[PATCH 3/3] p54: zero-out rx_status

[Gabor Juhos]

In commit 'mac80211: support radiotap vendor namespace RX data'
new fields were added to 'struct ieee80211_rx_status'.
The ath5k driver does not initializes those fields and
this can cause unexpected behaviour. The patch ensures
that each field gets initialized with zeroes.

Cc: Christian Lamparter <chunkeey@googlemail.com>
Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
---
Compile tested only.
---
 drivers/net/wireless/p54/txrx.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/p54/txrx.c b/drivers/net/wireless/p54/txrx.c
index 12f0a34..be2552e 100644
--- a/drivers/net/wireless/p54/txrx.c
+++ b/drivers/net/wireless/p54/txrx.c
@@ -345,6 +345,8 @@ static int p54_rx_data(struct p54_common *priv, struct sk_buff *skb)
 	if (!(hdr->flags & cpu_to_le16(P54_HDR_FLAG_DATA_IN_FCS_GOOD)))
 		return 0;
 
+	memset(rx_status, 0, sizeof(*rx_status));
+
 	if (hdr->decrypt_status == P54_DECRYPT_OK)
 		rx_status->flag |= RX_FLAG_DECRYPTED;
 	if ((hdr->decrypt_status == P54_DECRYPT_FAIL_MICHAEL) ||
-- 
1.7.10

Re: [PATCH 3/3] p54: zero-out rx_status

[Christian Lamparter]

On Tuesday, December 11, 2012 02:55:05 PM Gabor Juhos wrote:
> In commit 'mac80211: support radiotap vendor namespace RX data'
> new fields were added to 'struct ieee80211_rx_status'.
> The ath5k driver does not initializes those fields and
      ^^^^^ p54?!

> this can cause unexpected behaviour. The patch ensures
> that each field gets initialized with zeroes.

Actually, when the skb is alloced/initialized by
__alloc_skb, the skb->cb is already zeroed (which
is where the ieee80211_rx_status will be stored).

And while p54 recycles command response skbs, the 
driver does not touch the skb->cb of 802.11 skbs,
until the frame is destined for ieee80211_rx_irqsave.

If this issue just popped up now, I suspect that 
something else is silently corrupting our SKBs 
[or can anybody see how rt2x00 hit this issue?]

> Cc: Christian Lamparter <chunkeey@googlemail.com>
> Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
> ---
> Compile tested only.
> ---
>  drivers/net/wireless/p54/txrx.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/p54/txrx.c b/drivers/net/wireless/p54/txrx.c
> index 12f0a34..be2552e 100644
> --- a/drivers/net/wireless/p54/txrx.c
> +++ b/drivers/net/wireless/p54/txrx.c
> @@ -345,6 +345,8 @@ static int p54_rx_data(struct p54_common *priv, struct sk_buff *skb)
>  	if (!(hdr->flags & cpu_to_le16(P54_HDR_FLAG_DATA_IN_FCS_GOOD)))
>  		return 0;
>  
> + 	memset(rx_status, 0, sizeof(*rx_status));
> +
>  	if (hdr->decrypt_status == P54_DECRYPT_OK)
>  		rx_status->flag |= RX_FLAG_DECRYPTED;
>  	if ((hdr->decrypt_status == P54_DECRYPT_FAIL_MICHAEL) ||
> 

Re: [PATCH 3/3] p54: zero-out rx_status

[Johannes Berg]

On Tue, 2012-12-11 at 16:08 +0100, Christian Lamparter wrote:
> On Tuesday, December 11, 2012 02:55:05 PM Gabor Juhos wrote:
> > In commit 'mac80211: support radiotap vendor namespace RX data'
> > new fields were added to 'struct ieee80211_rx_status'.
> > The ath5k driver does not initializes those fields and
>       ^^^^^ p54?!
> 
> > this can cause unexpected behaviour. The patch ensures
> > that each field gets initialized with zeroes.
> 
> Actually, when the skb is alloced/initialized by
> __alloc_skb, the skb->cb is already zeroed (which
> is where the ieee80211_rx_status will be stored).
> 
> And while p54 recycles command response skbs, the 
> driver does not touch the skb->cb of 802.11 skbs,
> until the frame is destined for ieee80211_rx_irqsave.
> 
> If this issue just popped up now, I suspect that 
> something else is silently corrupting our SKBs 
> [or can anybody see how rt2x00 hit this issue?]

When I reviewed the drivers, I didn't think there was a problem in any
of them but iwlwifi & iwlegacy which I fixed, for the reasons you
mention above. So I'm just as confused as you are, I guess :)

johannes

Re: [PATCH 3/3] p54: zero-out rx_status

[Johannes Berg]

On Tue, 2012-12-11 at 16:25 +0100, Johannes Berg wrote:

> > If this issue just popped up now, I suspect that 
> > something else is silently corrupting our SKBs 
> > [or can anybody see how rt2x00 hit this issue?]

Oh, ok I assumed that rt2x00 RX SKBs are cleanly allocated, but now that
I trace the code further I see that while the SKBs are on the queue
inside rt2x00 they have something called "struct skb_frame_desc" on the
skb->cb, so this is what's causing the rt2x00 issue. This should be
mentioned in the rt2x00 commit, and all other drivers should be fine.

johannes

Re: [PATCH 3/3] p54: zero-out rx_status

[Christian Lamparter]

On Tuesday, December 11, 2012 04:30:13 PM Johannes Berg wrote:
> On Tue, 2012-12-11 at 16:25 +0100, Johannes Berg wrote:
> 
> > > If this issue just popped up now, I suspect that 
> > > something else is silently corrupting our SKBs 
> > > [or can anybody see how rt2x00 hit this issue?]
> 
> Oh, ok I assumed that rt2x00 RX SKBs are cleanly allocated, but now that
> I trace the code further I see that while the SKBs are on the queue
> inside rt2x00 they have something called "struct skb_frame_desc" on the
> skb->cb, so this is what's causing the rt2x00 issue. This should be
> mentioned in the rt2x00 commit, and all other drivers should be fine.
Ah, you are right. I assumed that rt2x00_queue stuff was only important
for TX. But it seems rt2x00 pci devices use them as well for RX.

Regards,
	Chr

Re: [PATCH 3/3] p54: zero-out rx_status

[Gabor Juhos]

2012.12.11. 16:08 keltezéssel, Christian Lamparter írta:
> On Tuesday, December 11, 2012 02:55:05 PM Gabor Juhos wrote:
>> In commit 'mac80211: support radiotap vendor namespace RX data'
>> new fields were added to 'struct ieee80211_rx_status'.
>> The ath5k driver does not initializes those fields and
>       ^^^^^ p54?!

Of course.

>> this can cause unexpected behaviour. The patch ensures
>> that each field gets initialized with zeroes.
> 
> Actually, when the skb is alloced/initialized by
> __alloc_skb, the skb->cb is already zeroed

Thank you for the explanation, I was not aware of this.

> (which is where the ieee80211_rx_status will be stored).
> 
> And while p54 recycles command response skbs, the 
> driver does not touch the skb->cb of 802.11 skbs,
> until the frame is destined for ieee80211_rx_irqsave.

Ok.

> If this issue just popped up now, I suspect that 
> something else is silently corrupting our SKBs 
> [or can anybody see how rt2x00 hit this issue?]

It is popped up now because we have switched to compat-wireless-2012-12-06 in
OpenWrt a few days ago. Previously we were using compat-wireless-2012-09-07.

-Gabor

Re: [PATCH 3/3] p54: zero-out rx_status

[Gabor Juhos]

2012.12.11. 16:30 keltezéssel, Johannes Berg írta:
> On Tue, 2012-12-11 at 16:25 +0100, Johannes Berg wrote:
> 
>>> If this issue just popped up now, I suspect that 
>>> something else is silently corrupting our SKBs 
>>> [or can anybody see how rt2x00 hit this issue?]
> 
> Oh, ok I assumed that rt2x00 RX SKBs are cleanly allocated, but now that
> I trace the code further I see that while the SKBs are on the queue
> inside rt2x00 they have something called "struct skb_frame_desc" on the
> skb->cb, so this is what's causing the rt2x00 issue. This should be
> mentioned in the rt2x00 commit, and all other drivers should be fine.

Ok, I will update the commit message and will resend the rt2x00 patch only.
Thank you for the review!

-Gabor