[PATCH] zd1211rw-mac80211: Fix for monitor mode bug

[PATCH] zd1211rw-mac80211: Fix for monitor mode bug

[Daniel Drake]

From: Ulrich Kunitz <kune@deine-taler.de>

The ZD1211 supports the delivery of packets with a wrong CRC value
to the host. We switched that feature on in monitor mode, so that
incomplete packets were delivered.

This problem has been reported to bugzilla.kernel.org as bug 8152.
This patch fixes it for the mac80211 stack.

Signed-off-by: Ulrich Kunitz <kune@deine-taler.de>
Signed-off-by: Daniel Drake <dsd@gentoo.org>
---
 drivers/net/wireless/mac80211/zd1211rw/zd_mac.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/mac80211/zd1211rw/zd_mac.c b/drivers/net/wireless/mac80211/zd1211rw/zd_mac.c
index 097f614..65eabfa 100644
--- a/drivers/net/wireless/mac80211/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/mac80211/zd1211rw/zd_mac.c
@@ -91,17 +91,9 @@ void zd_mac_clear(struct zd_mac *mac)
 
 static int reset_mode(struct zd_mac *mac)
 {
-	struct zd_ioreq32 ioreqs[] = {
-		{ CR_RX_FILTER, STA_RX_FILTER },
-		{ CR_SNIFFER_ON, 0U },
-	};
-
-	if (mac->mode == IEEE80211_IF_TYPE_MNTR) {
-		ioreqs[0].value = 0xffffffff;
-		ioreqs[1].value = 0x1;
-	}
+	u32 filter = mac->mode == IEEE80211_IF_TYPE_MNTR ? ~0 : STA_RX_FILTER;
 
-	return zd_iowrite32a(&mac->chip, ioreqs, ARRAY_SIZE(ioreqs));
+	return zd_iowrite32(&mac->chip, CR_RX_FILTER, filter);
 }
 
 static int zd_mac_open(struct ieee80211_hw *dev)
-- 
1.5.0.5

Re: [PATCH] zd1211rw-mac80211: Fix for monitor mode bug

[Jiri Benc]

[removing netdev]

On Mon, 26 Mar 2007 00:18:17 +0100 (BST), Daniel Drake wrote:
> The ZD1211 supports the delivery of packets with a wrong CRC value
> to the host. We switched that feature on in monitor mode, so that
> incomplete packets were delivered.
> 
> This problem has been reported to bugzilla.kernel.org as bug 8152.
> This patch fixes it for the mac80211 stack.

ACK.

But this feature of zd1211 looks interesting. Johannes, shouldn't we
add a parameter to cfg80211 allowing such packets to be received by
monitor iterfaces?

Daniel, does zd1211 send information about failed CRC check with the
packet? Or do we need to recheck CRC in the stack if the feature
mentioned above is implemented and there are both regular and monitor
interface operating?

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [PATCH] zd1211rw-mac80211: Fix for monitor mode bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

On Mon, 2007-03-26 at 11:54 +0200, Jiri Benc wrote:

> But this feature of zd1211 looks interesting. Johannes, shouldn't we
> add a parameter to cfg80211 allowing such packets to be received by
> monitor iterfaces?

Dunno. bcm43xx has this feature too, it even allows you to bypass the
second CRC check in the PLCP header. Picks up your microwave with
that ;)

Maybe take this into account for the driver API when implementing
IFF_PROMISC as we talked about?

> Daniel, does zd1211 send information about failed CRC check with the
> packet? Or do we need to recheck CRC in the stack if the feature
> mentioned above is implemented and there are both regular and monitor
> interface operating?

bcm43xx doesn't tell you when the CRC failed, nor does it tell you when
the PLCP CRC failed (if you want to see those frames). Hence, we'd have
to recheck the CRC(s).

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] zd1211rw-mac80211: Fix for monitor mode bug

[Andy Green]

Johannes Berg wrote:

> bcm43xx doesn't tell you when the CRC failed, nor does it tell you when
> the PLCP CRC failed (if you want to see those frames). Hence, we'd have
> to recheck the CRC(s).

FWIW I can definitely use the broken frames despite the overhead and 
would welcome being able to get them via a config option.

-Andy

Re: [PATCH] zd1211rw-mac80211: Fix for monitor mode bug

[Michael Buesch]

On Monday 26 March 2007 12:14, Andy Green wrote:
> Johannes Berg wrote:
> 
> > bcm43xx doesn't tell you when the CRC failed, nor does it tell you when
> > the PLCP CRC failed (if you want to see those frames). Hence, we'd have
> > to recheck the CRC(s).
> 
> FWIW I can definitely use the broken frames despite the overhead and 
> would welcome being able to get them via a config option.


We currently have two module parameters in bcm43xx-mac80211 for this:

static int modparam_mon_keep_bad;
module_param_named(mon_keep_bad, modparam_mon_keep_bad, int, 0444);
MODULE_PARM_DESC(mon_keep_bad, "Keep bad frames in monitor mode");

static int modparam_mon_keep_badplcp;
module_param_named(mon_keep_badplcp, modparam_mon_keep_bad, int, 0444);
MODULE_PARM_DESC(mon_keep_badplcp, "Keep frames with bad PLCP in monitor mode");

Of course, it might be desired to have that knob in mac80211 and cfg80211, somehow.

-- 
Greetings Michael.

Re: [PATCH] zd1211rw-mac80211: Fix for monitor mode bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 738 bytes --]

On Mon, 2007-03-26 at 13:28 +0200, Michael Buesch wrote:

> We currently have two module parameters in bcm43xx-mac80211 for this:
> 
> static int modparam_mon_keep_bad;
> module_param_named(mon_keep_bad, modparam_mon_keep_bad, int, 0444);
> MODULE_PARM_DESC(mon_keep_bad, "Keep bad frames in monitor mode");
> 
> static int modparam_mon_keep_badplcp;
> module_param_named(mon_keep_badplcp, modparam_mon_keep_bad, int, 0444);
> MODULE_PARM_DESC(mon_keep_badplcp, "Keep frames with bad PLCP in monitor mode");

Out of curiosity, doesn't that break things when you have both a monitor
and a non-monitor interface [1]? Or do you just disable it then?

johannes

[1] I think that mac80211 assumes CRC checks have passed, no?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] zd1211rw-mac80211: Fix for monitor mode bug

[Michael Buesch]

On Monday 26 March 2007 13:31, Johannes Berg wrote:
> On Mon, 2007-03-26 at 13:28 +0200, Michael Buesch wrote:
> 
> > We currently have two module parameters in bcm43xx-mac80211 for this:
> > 
> > static int modparam_mon_keep_bad;
> > module_param_named(mon_keep_bad, modparam_mon_keep_bad, int, 0444);
> > MODULE_PARM_DESC(mon_keep_bad, "Keep bad frames in monitor mode");
> > 
> > static int modparam_mon_keep_badplcp;
> > module_param_named(mon_keep_badplcp, modparam_mon_keep_bad, int, 0444);
> > MODULE_PARM_DESC(mon_keep_badplcp, "Keep frames with bad PLCP in monitor mode");
> 
> Out of curiosity, doesn't that break things when you have both a monitor
> and a non-monitor interface [1]? Or do you just disable it then?
> 
> johannes
> 
> [1] I think that mac80211 assumes CRC checks have passed, no?

Well, yeah. It does of course. If you have a STA and a MON interface,
the STA also receives the packs with bad CRCs. But that's how
mac80211 designed the virt-interface stuff.
Same goes for promisc. If you have a STA and a MON you can't have
the STA in non-promisc mode, so you also receive promisc packets on
the STA. That's how it's supposed to be. (I think mac80211 filters
them in the STA code somewhere).

The modparams actually are just a temporary hack and were never meant
to stay there. I wanted to have the knob in cfg80211, sooner or later.
So when the knob is there, mac80211 should take care that the STA
doesn't receive the bad packets.

-- 
Greetings Michael.

Re: [PATCH] zd1211rw-mac80211: Fix for monitor mode bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]

On Mon, 2007-03-26 at 22:39 +0200, Michael Buesch wrote:

> Well, yeah. It does of course. If you have a STA and a MON interface,
> the STA also receives the packs with bad CRCs. But that's how
> mac80211 designed the virt-interface stuff.
> Same goes for promisc. If you have a STA and a MON you can't have
> the STA in non-promisc mode, so you also receive promisc packets on
> the STA. That's how it's supposed to be. (I think mac80211 filters
> them in the STA code somewhere).

Yeah, it filters those later by BSSID, that's intended, but it wasn't
ever programmed to check the CRC. We'll need to introduce a new rx
status flag "was crc checked".

> The modparams actually are just a temporary hack and were never meant
> to stay there. I wanted to have the knob in cfg80211, sooner or later.
> So when the knob is there, mac80211 should take care that the STA
> doesn't receive the bad packets.

Right.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] zd1211rw-mac80211: Fix for monitor mode bug

[Michael Buesch]

On Wednesday 28 March 2007 11:19, Johannes Berg wrote:
> On Mon, 2007-03-26 at 22:39 +0200, Michael Buesch wrote:
> 
> > Well, yeah. It does of course. If you have a STA and a MON interface,
> > the STA also receives the packs with bad CRCs. But that's how
> > mac80211 designed the virt-interface stuff.
> > Same goes for promisc. If you have a STA and a MON you can't have
> > the STA in non-promisc mode, so you also receive promisc packets on
> > the STA. That's how it's supposed to be. (I think mac80211 filters
> > them in the STA code somewhere).
> 
> Yeah, it filters those later by BSSID, that's intended, but it wasn't
> ever programmed to check the CRC. We'll need to introduce a new rx
> status flag "was crc checked".
> 
> > The modparams actually are just a temporary hack and were never meant
> > to stay there. I wanted to have the knob in cfg80211, sooner or later.
> > So when the knob is there, mac80211 should take care that the STA
> > doesn't receive the bad packets.

I think we need at least two "stages" for this config.
One that says: Get all packets with bad FCS
And one that says: Get all packets with bad FCS and everything else
that looks like a signal, which is probably your microwave oven, though.

So, when these packets are passed up the stack the PLCP is stripped.
I think we should make the stack aware of the PLCP (and add an RX flag
PLCP_available) so that it is able to check the PLCP checksum.
Otherwise the only way to detect your microwave oven from the packet
stream would be failed FCS. Which would also most likely failed then, but
I think it's probably useful for the user to see the PLCP, too, on
a mon interface.

So we need RX flags "PLCP_available" and "Not_checksummed".

-- 
Greetings Michael.

PLCP header information (was: Re: [PATCH] zd1211rw-mac80211: Fix for monitor mode bug)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On Wed, 2007-03-28 at 11:55 +0200, Michael Buesch wrote:

> I think we need at least two "stages" for this config.
> One that says: Get all packets with bad FCS
> And one that says: Get all packets with bad FCS and everything else
> that looks like a signal, which is probably your microwave oven, though.

I agree.

> So, when these packets are passed up the stack the PLCP is stripped.
> I think we should make the stack aware of the PLCP (and add an RX flag
> PLCP_available) so that it is able to check the PLCP checksum.
> Otherwise the only way to detect your microwave oven from the packet
> stream would be failed FCS. Which would also most likely failed then, but
> I think it's probably useful for the user to see the PLCP, too, on
> a mon interface.
> 
> So we need RX flags "PLCP_available" and "Not_checksummed".

Also "PLCP checksummed" or such. And we need to think about how we
represent the PLCP in radiotap.

The PLCP header has different lengths depending on the type of PHY used,
for Clause 17 (A) it is 40 bits, for the others (I think) it is 48 bits.
For N I think the whole notion was changed a bit with the stuff being
defined in terms of microseconds and not bits.

It'd be useful to make this part of radiotap but it'd need a length byte
too. I propose always padding it to a multiple of 8 bytes with a length
byte as the first byte.

For mac80211, we'd add a new item s16 plcp offset that tells us where in
the radiotap header that the driver generated the plcp header can be
found so that in the RX path we don't need to parse the radiotap header
to find the PLCP if necessary.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] zd1211rw-mac80211: Fix for monitor mode bug

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 388 bytes --]

On Mon, 2007-03-26 at 11:59 +0200, Johannes Berg wrote:

> bcm43xx doesn't tell you when the CRC failed, nor does it tell you when
> the PLCP CRC failed (if you want to see those frames). Hence, we'd have
> to recheck the CRC(s).

Then again, it would be trivial to patch the firmware to include bits
for this, so we may want to be able to use them too if they exist.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]