Patch prevents call set_wep_key() with zero key length. That fix long
standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
"airo: clean up WEP key operations". Additionally print call trace when
someone will try to use improper parameters, and remove key.len = 0
assignment, because it is in not possible code path.
Reported-and-bisected-by: Chris Siebenmann <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: <[email protected]>
Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/airo.c | 34 ++++++++++++++++++----------------
1 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index 4331d67..2a9f029 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -5254,11 +5254,7 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
WepKeyRid wkr;
int rc;
- if (keylen == 0) {
- airo_print_err(ai->dev->name, "%s: key length to set was zero",
- __func__);
- return -1;
- }
+ WARN_ON(keylen == 0);
memset(&wkr, 0, sizeof(wkr));
wkr.len = cpu_to_le16(sizeof(wkr));
@@ -6405,11 +6401,7 @@ static int airo_set_encode(struct net_device *dev,
if (dwrq->length > MIN_KEY_SIZE)
key.len = MAX_KEY_SIZE;
else
- if (dwrq->length > 0)
- key.len = MIN_KEY_SIZE;
- else
- /* Disable the key */
- key.len = 0;
+ key.len = MIN_KEY_SIZE;
/* Check if the key is not marked as invalid */
if(!(dwrq->flags & IW_ENCODE_NOKEY)) {
/* Cleanup */
@@ -6590,12 +6582,22 @@ static int airo_set_encodeext(struct net_device *dev,
default:
return -EINVAL;
}
- /* Send the key to the card */
- rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
- if (rc < 0) {
- airo_print_err(local->dev->name, "failed to set WEP key"
- " at index %d: %d.", idx, rc);
- return rc;
+ if (key.len == 0) {
+ rc = set_wep_tx_idx(local, idx, perm, 1);
+ if (rc < 0) {
+ airo_print_err(local->dev->name,
+ "failed to set WEP transmit index to %d: %d.",
+ idx, rc);
+ return rc;
+ }
+ } else {
+ rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
+ if (rc < 0) {
+ airo_print_err(local->dev->name,
+ "failed to set WEP key at index %d: %d.",
+ idx, rc);
+ return rc;
+ }
}
}
--
1.6.2.5
On Wed, 2010-02-03 at 16:31 -0500, Chris Siebenmann wrote:
> | > Patch prevents call set_wep_key() with zero key
> | > length. That fix long standing regression since commit
> | > c0380693520b1a1e4f756799a0edc379378b462a "airo: clean up WEP key
> | > operations". [...]
> |
> | What problem/regression does this actually fix? [...]
>
> That mentioned commit broke wireless on a Thinkpad T42 (running Fedora
> 11); from that commit onwards, the wireless system appeared to see no
> networks. The patch makes things work again.
>
> More details are in the Fedora bugzilla:
> https://bugzilla.redhat.com/show_bug.cgi?id=522530
You mean with iwlist? That's quite odd because setting WEP keys doesn't
have anything to do with the scanning stuff, even in the firmware. Note
that Airo has always been somewhat tempermental for scans and you may or
may not see any or all APs in any given scan.
Dan
On Thu, 2010-02-04 at 17:10 -0500, Chris Siebenmann wrote:
> | > That mentioned commit broke wireless on a Thinkpad T42 (running Fedora
> | > 11); from that commit onwards, the wireless system appeared to see no
> | > networks. The patch makes things work again.
> | >
> | > More details are in the Fedora bugzilla:
> | > https://bugzilla.redhat.com/show_bug.cgi?id=522530
> |
> | You mean with iwlist? That's quite odd because setting WEP keys
> | doesn't have anything to do with the scanning stuff, even in the
> | firmware. Note that Airo has always been somewhat tempermental for
> | scans and you may or may not see any or all APs in any given scan.
>
> All I know is that the failure is total; in all situations I've
> tried where a good kernel sees one or more networks and APs, a bad
> kernel sees none at all (and 'iwlist wifi0 scanning' reports 'No
> scan results'). A good kernel sometimes reports different numbers of
> networks and sometimes has quality problems, but a bad kernel never sees
> anything.
>
> (Well, has never seen anything within a relatively modest amount of
> time. I have not let it sit for, say, an hour in order to see if
> something magically gets better; I have always rebooted into a kernel
> with working wireless.)
When are you attempting to scan? Is anything like wpa_supplicant or
NetworkManager running in the background when you do this?
Dan
On Thu, Feb 04, 2010 at 03:56:13PM -0800, Dan Williams wrote:
> On Thu, 2010-02-04 at 13:07 +0100, Stanislaw Gruszka wrote:
> > Patch prevents call set_wep_key() with zero key length. That fix long
> > standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
> > "airo: clean up WEP key operations". Additionally print call trace when
> > someone will try to use improper parameters, and remove key.len = 0
> > assignment, because it is in not possible code path.
>
> Could you send the hunk that touches airo_set_encode() separately
> please? That's obviously correct and an easy ack, but isn't really
> related to the other hunks here.
I'm confused by your further patch, I'm deferring fixing this to you :)
Stanislaw
On Mon, 2010-02-08 at 15:55 -0500, John W. Linville wrote:
> On Thu, Feb 04, 2010 at 10:59:38PM -0800, Dan Williams wrote:
> > John, can we revert the patch you applied, and apply the following patch
> > instead (in the next mail)? I'd rather use the one I'm about to send
> > because it makes it clearer *why* setting the TX key index is required,
> > and it limits that case to only the time when it's necessary. Pending
> > the testing by Chris, I'd rather have the revised version committed
> > instead.
>
> Do you still want this don, even after Chris's testing says your
> patch doesn't work? I wasn't sure from your response.
Not yet, no. I'd like to work out the issue first, if we can.
Thanks,
Dan
| > I don't get quite this behavior, although I'm not doing exactly these
| > steps; instead, I do:
| > - boot machine
| > - use 'iwlist scan' and see multiple APs
| > - use NetworkManager to connect to our WEP-enabled AP
| > - 'iwlist scan' returns only the single AP
| > - turn things off with:
| > iwconfig eth1 essid ANY key off
| > - 'iwconfig eth1' and NetworkManager both show no connected AP
| > - 'iwlist scan' returns only the ex-connected AP
|
| These are the results you get on the older kernel without the commit
| we're all talking about here?
I have been testing on the kernel with working wireless,
specifically Fedora 11's 2.6.29.6-217.2.8.fc11 kernel.
I have to amend my testing results, however. I have now managed to get
the network live with iwconfig, not NetworkManager, and when I do this
and do 'iwconfig eth1 essid ANY key off', I actually see *multiple*
APs with 'iwlist scan' if I catch it at the right time. (In fact, I
sometimes associate with an unprotected network in range.)
(There is only one good network in range, which is ours, but I'm on
the edge of a number that sometimes show up in normal scan output and
sometimes don't.)
So this doesn't match the behavior you were seeing with your old kernel
and machine.
- cks
| When are you attempting to scan? Is anything like wpa_supplicant or
| NetworkManager running in the background when you do this?
My primary results are from Fedora 11's GNOME GUI (boot machine, log
in, attempt to connect to one of our wireless networks in various
places); I believe that this uses NetworkManager. I tried with iwlist
now and got the same results, but it looks like wpa_supplicant and
NetworkManager were both running at the time based on pstree output.
- cks
| > In the GUI, no wireless networks show up/are listed in the usual
| > dropdown menu; this means that I can't go as far as even trying to
| > connect to anything. From the command line, 'iwlist wifi0 scanning'
| > lists nothing.
|
| Wait, wifi0? You need to be using 'eth0' or 'eth1', whichever eth
| device that 'iwconfig' reports. wifi0 is a historical airo anomaly
| that's only used for packet capture and sniffing of the 802.11 frames.
| It should not be used for anything other than Wireshark basically.
|
| Do you get the same behavior if you use the normal ethX interface of
| the airo device?
Yes; on a 'bad' kernel, 'iwlist eth1 scanning' lists nothing, and on a
good kernel it lists the networks that I expect (with some variability;
where I have the machine at the moment is on the edge of visibility for
some networks, and they appear and disappear periodically).
| Note that just because wifi0 may return scan results doesn't mean it
| should be used with iwconfig...
|
| But also, you're doing the iwlist scan as root, correct?
Yes (on both the good and the bad kernels).
I haven't been trying to use iwconfig for anything; all of my actual
use of the wireless networks has been done through the Fedora 11 GUI.
- cks
Even if keylen == 0 is a bug and should not really happen, better avoid
possibility of passing bad value to firmware.
Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/airo.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index 2a9f029..01bffc2 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -5254,7 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
WepKeyRid wkr;
int rc;
- WARN_ON(keylen == 0);
+ if (WARN_ON(keylen == 0))
+ return -1;
memset(&wkr, 0, sizeof(wkr));
wkr.len = cpu_to_le16(sizeof(wkr));
--
1.6.2.5
On Thu, 2010-02-04 at 17:42 -0500, Chris Siebenmann wrote:
> | > | When are you attempting to scan? Is anything like wpa_supplicant
> | > | or NetworkManager running in the background when you do this?
> | >
> | > My primary results are from Fedora 11's GNOME GUI (boot machine,
> | > log in, attempt to connect to one of our wireless networks in
> | > various places); I believe that this uses NetworkManager. I tried
> | > with iwlist now and got the same results, but it looks like
> | > wpa_supplicant and NetworkManager were both running at the time
> | > based on pstree output.
> |
> | Ok, that should be fine since wpa_supplicant is the only thing hitting
> | the airo up for scans and whatnot. I'm grabbing a kernel now to do
> | some testing with my 350 (PCMCIA). I assume you have an internal
> | minicard?
>
> I'm not sure what the machine's exact configuration is because I
> inherited it (the usual long cycle of passing older and older hardware
> around at work), but I believe it must; I certainly don't have anything
> plugged into any external slots or sockets.
And just to confirm, what are the exact symptoms that you have with
"broken" kernels? Just that connections can't be made, the card can't
find access points, etc? or?
Dan
On Thu, 2010-02-04 at 18:36 -0500, Chris Siebenmann wrote:
> | > In the GUI, no wireless networks show up/are listed in the usual
> | > dropdown menu; this means that I can't go as far as even trying to
> | > connect to anything. From the command line, 'iwlist wifi0 scanning'
> | > lists nothing.
> |
> | Wait, wifi0? You need to be using 'eth0' or 'eth1', whichever eth
> | device that 'iwconfig' reports. wifi0 is a historical airo anomaly
> | that's only used for packet capture and sniffing of the 802.11 frames.
> | It should not be used for anything other than Wireshark basically.
> |
> | Do you get the same behavior if you use the normal ethX interface of
> | the airo device?
>
> Yes; on a 'bad' kernel, 'iwlist eth1 scanning' lists nothing, and on a
> good kernel it lists the networks that I expect (with some variability;
> where I have the machine at the moment is on the edge of visibility for
> some networks, and they appear and disappear periodically).
>
> | Note that just because wifi0 may return scan results doesn't mean it
> | should be used with iwconfig...
> |
> | But also, you're doing the iwlist scan as root, correct?
>
> Yes (on both the good and the bad kernels).
>
> I haven't been trying to use iwconfig for anything; all of my actual
> use of the wireless networks has been done through the Fedora 11 GUI.
I've been able to reproduce the issue with my 350. Note that even with
the original patch, the 350 will never return a scan result *other* than
the associated AP. If you turn off the AP, the airo will not return any
scan results even though before it connected to the AP it would return a
bunch.
Airo firmware is, well, old and crappy, and nobody ever got enough
documentation out of Cisco to get the driver working extremely well.
But oddly enough, the patch does fix the issue, though I have a slightly
modified version that I'd rather be applied (see other mails).
Dan
On Tue, 2010-02-02 at 15:34 +0100, Stanislaw Gruszka wrote:
> Patch prevents call set_wep_key() with zero key length. That fix long
> standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
> "airo: clean up WEP key operations". Additionally print call trace when
> someone will try to use improper parameters, and remove key.len = 0
> assignment, because it is in not possible code path.
What problem/regression does this actually fix? set_wep_key() should
never be called with zero-length key, so we should actually be returning
from that function instead of using WARN_ON() which would allow a
zero-length key to be set (which isn't really valid).
To disable a WEP key, the WEXT caller should be setting
IW_ENCODE_DISABLED in the flags, and *not* sending WEP keys. Or if they
do for some reason send WEP keys, they need to use the key flag
IW_ENCODE_NOKEY to ensure they key is marked as "invalid" and ignored by
the driver.
Dan
| The firmware apparently needs the WEP key RID touched when turning
| WEP off, otherwise it gets angry and refuses to return scan results.
| The firmware doesn't handle zero-length WEP keys so quite the warning
| message that was inadvertently printed when disabling WEP, which
| wpa_supplicant could trigger repeatedly.
|
| Signed-off-by: Dan Williams <[email protected]>
This patch does *not* work for me; with it applied to the 2.6.33-rc6
from-git that I have been using as the testing basis, the resulting
kernel does not see any wireless networks. dmesg has multiple reports
of:
airo(eth1): cmd:103 status:7f03 rsp0:0 rsp1:ff10 rsp2:c0f0
airo(eth1): cmd:103 status:7f03 rsp0:0 rsp1:ff10 rsp2:c0f0
- cks
On Fri, 2010-02-26 at 15:10 +0100, Stanislaw Gruszka wrote:
> Even if keylen == 0 is a bug and should not really happen, better avoid
> possibility of passing bad value to firmware.
>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
I actually told John to just apply your first patch via IRC, but perhaps
he hasn't done that yet. I cannot reproduce the issue that Chris
encountered with my PCMCIA 350 using 5.60 firmware, and I bricked my
card loading 5.40 firmware (same as Chris) onto it.
I couldn't find any failures your original patch on my 5.60 device, my
objections were simply code flow and semantics. So I'll ack the
original patch.
Dan
> ---
> drivers/net/wireless/airo.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
> index 2a9f029..01bffc2 100644
> --- a/drivers/net/wireless/airo.c
> +++ b/drivers/net/wireless/airo.c
> @@ -5254,7 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
> WepKeyRid wkr;
> int rc;
>
> - WARN_ON(keylen == 0);
> + if (WARN_ON(keylen == 0))
> + return -1;
>
> memset(&wkr, 0, sizeof(wkr));
> wkr.len = cpu_to_le16(sizeof(wkr));
On Fri, 2010-02-05 at 18:12 -0500, Chris Siebenmann wrote:
> | Yes, I know it may have worked in earlier kernels. And we should find
> | out why that is, and hopefully fix the regression if any. But
> | understand that airo has always been problematic and has never worked as
> | well as other fullmac cards like orinoco.
> |
> | I booted up RHEL5 today, which uses a 2.6.18 kernel and of course does
> | not have the patch-in-question. That behavior is:
> |
> | 1) stop NetworkManager and wpa_supplicant
> | 2) insert airo card
> | 3) iwlist scan returns many results
> | 4) connect to WEP-enabled AP
> | 5) iwlist scan returns *only* the connected ap
> | 6) iwconfig eth1 essid any key off
> | 7) iwlist scan returns no results
> |
> | remember, that's without any of the patches we're talking about, using a
> | kernel from 2007. Which is about the same behavior you get, correct?
>
> I don't get quite this behavior, although I'm not doing exactly these
> steps; instead, I do:
> - boot machine
> - use 'iwlist scan' and see multiple APs
> - use NetworkManager to connect to our WEP-enabled AP
> - 'iwlist scan' returns only the single AP
> - turn things off with:
> iwconfig eth1 essid ANY key off
> - 'iwconfig eth1' and NetworkManager both show no connected AP
> - 'iwlist scan' returns only the ex-connected AP
These are the results you get on the older kernel without the commit
we're all talking about here?
> (I do not know the right commands to connect to our WEP-enabled AP and
> verify that everything is working right without using NetworkManager,
> so I used it for the connection phase.)
iwconfig ethX essid 'foobar' key <your hex wep key>
Then run 'iwconfig ethX' over and over until you see a valid AP BSSID
listed in the result, and then you can run 'dhclient -1 -v -d ethX' to
try and DHCP.
Also, what firmware version do you have?
Dan
On Thu, 2010-02-04 at 13:07 +0100, Stanislaw Gruszka wrote:
> Patch prevents call set_wep_key() with zero key length. That fix long
> standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
> "airo: clean up WEP key operations". Additionally print call trace when
> someone will try to use improper parameters, and remove key.len = 0
> assignment, because it is in not possible code path.
Could you send the hunk that touches airo_set_encode() separately
please? That's obviously correct and an easy ack, but isn't really
related to the other hunks here.
Thanks,
Dan
> v1->v2
> Return instantly from set_wep_key() when keylen == 0.
>
> Reported-and-bisected-by: Chris Siebenmann <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> ---
> drivers/net/wireless/airo.c | 33 ++++++++++++++++++---------------
> 1 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
> index 4331d67..01bffc2 100644
> --- a/drivers/net/wireless/airo.c
> +++ b/drivers/net/wireless/airo.c
> @@ -5254,11 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
> WepKeyRid wkr;
> int rc;
>
> - if (keylen == 0) {
> - airo_print_err(ai->dev->name, "%s: key length to set was zero",
> - __func__);
> + if (WARN_ON(keylen == 0))
> return -1;
> - }
>
> memset(&wkr, 0, sizeof(wkr));
> wkr.len = cpu_to_le16(sizeof(wkr));
> @@ -6405,11 +6402,7 @@ static int airo_set_encode(struct net_device *dev,
> if (dwrq->length > MIN_KEY_SIZE)
> key.len = MAX_KEY_SIZE;
> else
> - if (dwrq->length > 0)
> - key.len = MIN_KEY_SIZE;
> - else
> - /* Disable the key */
> - key.len = 0;
> + key.len = MIN_KEY_SIZE;
> /* Check if the key is not marked as invalid */
> if(!(dwrq->flags & IW_ENCODE_NOKEY)) {
> /* Cleanup */
> @@ -6590,12 +6583,22 @@ static int airo_set_encodeext(struct net_device *dev,
> default:
> return -EINVAL;
> }
> - /* Send the key to the card */
> - rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
> - if (rc < 0) {
> - airo_print_err(local->dev->name, "failed to set WEP key"
> - " at index %d: %d.", idx, rc);
> - return rc;
> + if (key.len == 0) {
> + rc = set_wep_tx_idx(local, idx, perm, 1);
> + if (rc < 0) {
> + airo_print_err(local->dev->name,
> + "failed to set WEP transmit index to %d: %d.",
> + idx, rc);
> + return rc;
> + }
> + } else {
> + rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
> + if (rc < 0) {
> + airo_print_err(local->dev->name,
> + "failed to set WEP key at index %d: %d.",
> + idx, rc);
> + return rc;
> + }
> }
> }
>
| > Patch prevents call set_wep_key() with zero key
| > length. That fix long standing regression since commit
| > c0380693520b1a1e4f756799a0edc379378b462a "airo: clean up WEP key
| > operations". [...]
|
| What problem/regression does this actually fix? [...]
That mentioned commit broke wireless on a Thinkpad T42 (running Fedora
11); from that commit onwards, the wireless system appeared to see no
networks. The patch makes things work again.
More details are in the Fedora bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=522530
- cks
On Thu, Feb 04, 2010 at 10:59:38PM -0800, Dan Williams wrote:
> John, can we revert the patch you applied, and apply the following patch
> instead (in the next mail)? I'd rather use the one I'm about to send
> because it makes it clearer *why* setting the TX key index is required,
> and it limits that case to only the time when it's necessary. Pending
> the testing by Chris, I'd rather have the revised version committed
> instead.
Do you still want this don, even after Chris's testing says your
patch doesn't work? I wasn't sure from your response.
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
| > I'm not sure what the machine's exact configuration is because
| > I inherited it (the usual long cycle of passing older and older
| > hardware around at work), but I believe it must; I certainly don't
| > have anything plugged into any external slots or sockets.
|
| And just to confirm, what are the exact symptoms that you have with
| "broken" kernels? Just that connections can't be made, the card can't
| find access points, etc? or?
In the GUI, no wireless networks show up/are listed in the usual
dropdown menu; this means that I can't go as far as even trying to
connect to anything. From the command line, 'iwlist wifi0 scanning'
lists nothing.
- cks
Patch prevents call set_wep_key() with zero key length. That fix long
standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
"airo: clean up WEP key operations". Additionally print call trace when
someone will try to use improper parameters, and remove key.len = 0
assignment, because it is in not possible code path.
v1->v2
Return instantly from set_wep_key() when keylen == 0.
Reported-and-bisected-by: Chris Siebenmann <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: <[email protected]>
Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/airo.c | 33 ++++++++++++++++++---------------
1 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index 4331d67..01bffc2 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -5254,11 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
WepKeyRid wkr;
int rc;
- if (keylen == 0) {
- airo_print_err(ai->dev->name, "%s: key length to set was zero",
- __func__);
+ if (WARN_ON(keylen == 0))
return -1;
- }
memset(&wkr, 0, sizeof(wkr));
wkr.len = cpu_to_le16(sizeof(wkr));
@@ -6405,11 +6402,7 @@ static int airo_set_encode(struct net_device *dev,
if (dwrq->length > MIN_KEY_SIZE)
key.len = MAX_KEY_SIZE;
else
- if (dwrq->length > 0)
- key.len = MIN_KEY_SIZE;
- else
- /* Disable the key */
- key.len = 0;
+ key.len = MIN_KEY_SIZE;
/* Check if the key is not marked as invalid */
if(!(dwrq->flags & IW_ENCODE_NOKEY)) {
/* Cleanup */
@@ -6590,12 +6583,22 @@ static int airo_set_encodeext(struct net_device *dev,
default:
return -EINVAL;
}
- /* Send the key to the card */
- rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
- if (rc < 0) {
- airo_print_err(local->dev->name, "failed to set WEP key"
- " at index %d: %d.", idx, rc);
- return rc;
+ if (key.len == 0) {
+ rc = set_wep_tx_idx(local, idx, perm, 1);
+ if (rc < 0) {
+ airo_print_err(local->dev->name,
+ "failed to set WEP transmit index to %d: %d.",
+ idx, rc);
+ return rc;
+ }
+ } else {
+ rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
+ if (rc < 0) {
+ airo_print_err(local->dev->name,
+ "failed to set WEP key at index %d: %d.",
+ idx, rc);
+ return rc;
+ }
}
}
--
1.6.2.5
John, can we revert the patch you applied, and apply the following patch
instead (in the next mail)? I'd rather use the one I'm about to send
because it makes it clearer *why* setting the TX key index is required,
and it limits that case to only the time when it's necessary. Pending
the testing by Chris, I'd rather have the revised version committed
instead.
Thanks,
Dan
Even if keylen == 0 is a bug and should not really happen, better avoid
possibility of passing bad value to firmware.
Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/airo.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index 2a9f029..01bffc2 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -5254,7 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
WepKeyRid wkr;
int rc;
- WARN_ON(keylen == 0);
+ if (WARN_ON(keylen == 0))
+ return -1;
memset(&wkr, 0, sizeof(wkr));
wkr.len = cpu_to_le16(sizeof(wkr));
--
1.6.2.5
On Thu, 2010-02-04 at 13:07 +0100, Stanislaw Gruszka wrote:
> Patch prevents call set_wep_key() with zero key length. That fix long
> standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
> "airo: clean up WEP key operations". Additionally print call trace when
> someone will try to use improper parameters, and remove key.len = 0
> assignment, because it is in not possible code path.
>
> v1->v2
> Return instantly from set_wep_key() when keylen == 0.
>
> Reported-and-bisected-by: Chris Siebenmann <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
Acked-by: Dan Williams <[email protected]>
> ---
> drivers/net/wireless/airo.c | 33 ++++++++++++++++++---------------
> 1 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
> index 4331d67..01bffc2 100644
> --- a/drivers/net/wireless/airo.c
> +++ b/drivers/net/wireless/airo.c
> @@ -5254,11 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
> WepKeyRid wkr;
> int rc;
>
> - if (keylen == 0) {
> - airo_print_err(ai->dev->name, "%s: key length to set was zero",
> - __func__);
> + if (WARN_ON(keylen == 0))
> return -1;
> - }
>
> memset(&wkr, 0, sizeof(wkr));
> wkr.len = cpu_to_le16(sizeof(wkr));
> @@ -6405,11 +6402,7 @@ static int airo_set_encode(struct net_device *dev,
> if (dwrq->length > MIN_KEY_SIZE)
> key.len = MAX_KEY_SIZE;
> else
> - if (dwrq->length > 0)
> - key.len = MIN_KEY_SIZE;
> - else
> - /* Disable the key */
> - key.len = 0;
> + key.len = MIN_KEY_SIZE;
> /* Check if the key is not marked as invalid */
> if(!(dwrq->flags & IW_ENCODE_NOKEY)) {
> /* Cleanup */
> @@ -6590,12 +6583,22 @@ static int airo_set_encodeext(struct net_device *dev,
> default:
> return -EINVAL;
> }
> - /* Send the key to the card */
> - rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
> - if (rc < 0) {
> - airo_print_err(local->dev->name, "failed to set WEP key"
> - " at index %d: %d.", idx, rc);
> - return rc;
> + if (key.len == 0) {
> + rc = set_wep_tx_idx(local, idx, perm, 1);
> + if (rc < 0) {
> + airo_print_err(local->dev->name,
> + "failed to set WEP transmit index to %d: %d.",
> + idx, rc);
> + return rc;
> + }
> + } else {
> + rc = set_wep_key(local, idx, key.key, key.len, perm, 1);
> + if (rc < 0) {
> + airo_print_err(local->dev->name,
> + "failed to set WEP key at index %d: %d.",
> + idx, rc);
> + return rc;
> + }
> }
> }
>
On Thu, Feb 04, 2010 at 01:07:13PM +0100, Stanislaw Gruszka wrote:
> Patch prevents call set_wep_key() with zero key length. That fix long
> standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
> "airo: clean up WEP key operations". Additionally print call trace when
> someone will try to use improper parameters, and remove key.len = 0
> assignment, because it is in not possible code path.
>
> v1->v2
> Return instantly from set_wep_key() when keylen == 0.
>
> Reported-and-bisected-by: Chris Siebenmann <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
Stanislaw, your original patch was already committed in
wireless-next-2.6. Could you send a second patch with just your
v1->v2 change?
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Thu, 2010-02-04 at 17:26 -0500, Chris Siebenmann wrote:
> | When are you attempting to scan? Is anything like wpa_supplicant or
> | NetworkManager running in the background when you do this?
>
> My primary results are from Fedora 11's GNOME GUI (boot machine, log
> in, attempt to connect to one of our wireless networks in various
> places); I believe that this uses NetworkManager. I tried with iwlist
> now and got the same results, but it looks like wpa_supplicant and
> NetworkManager were both running at the time based on pstree output.
Ok, that should be fine since wpa_supplicant is the only thing hitting
the airo up for scans and whatnot. I'm grabbing a kernel now to do some
testing with my 350 (PCMCIA). I assume you have an internal minicard?
Dan
| > That mentioned commit broke wireless on a Thinkpad T42 (running Fedora
| > 11); from that commit onwards, the wireless system appeared to see no
| > networks. The patch makes things work again.
| >
| > More details are in the Fedora bugzilla:
| > https://bugzilla.redhat.com/show_bug.cgi?id=522530
|
| You mean with iwlist? That's quite odd because setting WEP keys
| doesn't have anything to do with the scanning stuff, even in the
| firmware. Note that Airo has always been somewhat tempermental for
| scans and you may or may not see any or all APs in any given scan.
All I know is that the failure is total; in all situations I've
tried where a good kernel sees one or more networks and APs, a bad
kernel sees none at all (and 'iwlist wifi0 scanning' reports 'No
scan results'). A good kernel sometimes reports different numbers of
networks and sometimes has quality problems, but a bad kernel never sees
anything.
(Well, has never seen anything within a relatively modest amount of
time. I have not let it sit for, say, an hour in order to see if
something magically gets better; I have always rebooted into a kernel
with working wireless.)
- cks
| > | When are you attempting to scan? Is anything like wpa_supplicant
| > | or NetworkManager running in the background when you do this?
| >
| > My primary results are from Fedora 11's GNOME GUI (boot machine,
| > log in, attempt to connect to one of our wireless networks in
| > various places); I believe that this uses NetworkManager. I tried
| > with iwlist now and got the same results, but it looks like
| > wpa_supplicant and NetworkManager were both running at the time
| > based on pstree output.
|
| Ok, that should be fine since wpa_supplicant is the only thing hitting
| the airo up for scans and whatnot. I'm grabbing a kernel now to do
| some testing with my 350 (PCMCIA). I assume you have an internal
| minicard?
I'm not sure what the machine's exact configuration is because I
inherited it (the usual long cycle of passing older and older hardware
around at work), but I believe it must; I certainly don't have anything
plugged into any external slots or sockets.
- cks
| Patch prevents call set_wep_key() with zero key length. That fix long
| standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
| "airo: clean up WEP key operations". Additionally print call trace when
| someone will try to use improper parameters, and remove key.len = 0
| assignment, because it is in not possible code path.
|
| v1->v2
| Return instantly from set_wep_key() when keylen == 0.
|
| Reported-and-bisected-by: Chris Siebenmann <[email protected]>
| Cc: Dan Williams <[email protected]>
| Cc: <[email protected]>
| Signed-off-by: Stanislaw Gruszka <[email protected]>
Tested-by: Chris Siebenmann <[email protected]>
(Tested on a Thinkpad T42, using current git tip + this patch; HEAD is
ab658321f32770b903a4426e2a6fae0392757755. This is the same machine I
bisected the problem on.)
- cks
| Patch prevents call set_wep_key() with zero key length. That fix long
| standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
| "airo: clean up WEP key operations". Additionally print call trace when
| someone will try to use improper parameters, and remove key.len = 0
| assignment, because it is in not possible code path.
|
| Reported-and-bisected-by: Chris Siebenmann <[email protected]>
| Cc: Dan Williams <[email protected]>
| Cc: <[email protected]>
| Signed-off-by: Stanislaw Gruszka <[email protected]>
Tested-by: Chris Siebenmann <[email protected]>
(Tested on a Thinkpad T42, using current git tip + this patch; HEAD is
ab658321f32770b903a4426e2a6fae0392757755. This is the same machine I
bisected the problem on.)
- cks
On Fri, 2010-02-05 at 11:26 -0500, Chris Siebenmann wrote:
> | The firmware apparently needs the WEP key RID touched when turning
> | WEP off, otherwise it gets angry and refuses to return scan results.
> | The firmware doesn't handle zero-length WEP keys so quite the warning
> | message that was inadvertently printed when disabling WEP, which
> | wpa_supplicant could trigger repeatedly.
> |
> | Signed-off-by: Dan Williams <[email protected]>
>
> This patch does *not* work for me; with it applied to the 2.6.33-rc6
> from-git that I have been using as the testing basis, the resulting
> kernel does not see any wireless networks. dmesg has multiple reports
Yes, I know it may have worked in earlier kernels. And we should find
out why that is, and hopefully fix the regression if any. But
understand that airo has always been problematic and has never worked as
well as other fullmac cards like orinoco.
I booted up RHEL5 today, which uses a 2.6.18 kernel and of course does
not have the patch-in-question. That behavior is:
1) stop NetworkManager and wpa_supplicant
2) insert airo card
3) iwlist scan returns many results
4) connect to WEP-enabled AP
5) iwlist scan returns *only* the connected ap
6) iwconfig eth1 essid any key off
7) iwlist scan returns no results
remember, that's without any of the patches we're talking about, using a
kernel from 2007. Which is about the same behavior you get, correct?
> of:
>
> airo(eth1): cmd:103 status:7f03 rsp0:0 rsp1:ff10 rsp2:c0f0
> airo(eth1): cmd:103 status:7f03 rsp0:0 rsp1:ff10 rsp2:c0f0
CMD 0x0103 is the LISTBSS command, which is sent to trigger a scan.
What firmware version do you have? It should get printed out in 'dmesg'
when loading the driver.
Dan
On Thu, 2010-02-04 at 15:26 -0800, Dan Williams wrote:
> On Thu, 2010-02-04 at 18:03 -0500, Chris Siebenmann wrote:
> > | > I'm not sure what the machine's exact configuration is because
> > | > I inherited it (the usual long cycle of passing older and older
> > | > hardware around at work), but I believe it must; I certainly don't
> > | > have anything plugged into any external slots or sockets.
> > |
> > | And just to confirm, what are the exact symptoms that you have with
> > | "broken" kernels? Just that connections can't be made, the card can't
> > | find access points, etc? or?
> >
> > In the GUI, no wireless networks show up/are listed in the usual
> > dropdown menu; this means that I can't go as far as even trying to
> > connect to anything. From the command line, 'iwlist wifi0 scanning'
> > lists nothing.
>
> Wait, wifi0? You need to be using 'eth0' or 'eth1', whichever eth
> device that 'iwconfig' reports. wifi0 is a historical airo anomaly
> that's only used for packet capture and sniffing of the 802.11 frames.
> It should not be used for anything other than Wireshark basically.
>
> Do you get the same behavior if you use the normal ethX interface of the
> airo device?
Note that just because wifi0 may return scan results doesn't mean it
should be used with iwconfig...
But also, you're doing the iwlist scan as root, correct?
Dan
On Wed, Feb 03, 2010 at 12:43:44PM -0800, Dan Williams wrote:
> On Tue, 2010-02-02 at 15:34 +0100, Stanislaw Gruszka wrote:
> > Patch prevents call set_wep_key() with zero key length. That fix long
> > standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
> > "airo: clean up WEP key operations". Additionally print call trace when
> > someone will try to use improper parameters, and remove key.len = 0
> > assignment, because it is in not possible code path.
>
> What problem/regression does this actually fix? set_wep_key() should
> never be called with zero-length key,
There was one case where set_wep_key() was called instead of
set_wep_tx_idx().
> so we should actually be returning
> from that function instead of using WARN_ON() which would allow a
> zero-length key to be set (which isn't really valid).
Ok, changing that in v2 patch.
Cheers
Stanislaw
| Yes, I know it may have worked in earlier kernels. And we should find
| out why that is, and hopefully fix the regression if any. But
| understand that airo has always been problematic and has never worked as
| well as other fullmac cards like orinoco.
|
| I booted up RHEL5 today, which uses a 2.6.18 kernel and of course does
| not have the patch-in-question. That behavior is:
|
| 1) stop NetworkManager and wpa_supplicant
| 2) insert airo card
| 3) iwlist scan returns many results
| 4) connect to WEP-enabled AP
| 5) iwlist scan returns *only* the connected ap
| 6) iwconfig eth1 essid any key off
| 7) iwlist scan returns no results
|
| remember, that's without any of the patches we're talking about, using a
| kernel from 2007. Which is about the same behavior you get, correct?
I don't get quite this behavior, although I'm not doing exactly these
steps; instead, I do:
- boot machine
- use 'iwlist scan' and see multiple APs
- use NetworkManager to connect to our WEP-enabled AP
- 'iwlist scan' returns only the single AP
- turn things off with:
iwconfig eth1 essid ANY key off
- 'iwconfig eth1' and NetworkManager both show no connected AP
- 'iwlist scan' returns only the ex-connected AP
(I do not know the right commands to connect to our WEP-enabled AP and
verify that everything is working right without using NetworkManager,
so I used it for the connection phase.)
- cks
So that I don't forget this again:
| Also, what firmware version do you have?
The driver reports:
airo(): Probing for PCI adapters
airo 0000:02:02.0: PCI INT A -> Link[LNKC] -> GSI 11 (level, low) -> IRQ 11
airo(): Found an MPI350 card
airo(eth1): Firmware version 5.41.00
airo(eth1): WPA supported.
airo(eth1): MAC enabled 00:0e:9b:90:af:29
- cks
The firmware apparently needs the WEP key RID touched when turning WEP
off, otherwise it gets angry and refuses to return scan results. The
firmware doesn't handle zero-length WEP keys so quite the warning
message that was inadvertently printed when disabling WEP, which
wpa_supplicant could trigger repeatedly.
Signed-off-by: Dan Williams <[email protected]>
---
diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index 4331d67..38902c7 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -5254,11 +5254,8 @@ static int set_wep_key(struct airo_info *ai, u16 index, const char *key,
WepKeyRid wkr;
int rc;
- if (keylen == 0) {
- airo_print_err(ai->dev->name, "%s: key length to set was zero",
- __func__);
+ if (WARN_ON (keylen == 0))
return -1;
- }
memset(&wkr, 0, sizeof(wkr));
wkr.len = cpu_to_le16(sizeof(wkr));
@@ -6532,7 +6529,7 @@ static int airo_set_encodeext(struct net_device *dev,
struct iw_encode_ext *ext = (struct iw_encode_ext *)extra;
int perm = ( encoding->flags & IW_ENCODE_TEMP ? 0 : 1 );
__le16 currentAuthType = local->config.authType;
- int idx, key_len, alg = ext->alg, set_key = 1, rc;
+ int idx, key_len, alg = ext->alg, rc;
wep_key_t key;
if (!local->wep_capable)
@@ -6566,10 +6563,9 @@ static int airo_set_encodeext(struct net_device *dev,
idx, rc);
return rc;
}
- set_key = ext->key_len > 0 ? 1 : 0;
}
- if (set_key) {
+ if (ext->key_len > 0) {
/* Set the requested key first */
memset(key.key, 0, MAX_KEY_SIZE);
switch (alg) {
@@ -6600,12 +6596,20 @@ static int airo_set_encodeext(struct net_device *dev,
}
/* Read the flags */
- if(encoding->flags & IW_ENCODE_DISABLED)
+ if (encoding->flags & IW_ENCODE_DISABLED) {
+ /* The firmware seems to need the WEP key RID touched when
+ * setting WEP disabled; resetting the transmit key index to 0
+ * is good enough. Otherwise it gets confused and stops
+ * delivering scan results (!).
+ */
+ if (!(ext->ext_flags & IW_ENCODE_EXT_SET_TX_KEY))
+ set_wep_tx_idx(local, 0, 0, 1);
local->config.authType = AUTH_OPEN; // disable encryption
- if(encoding->flags & IW_ENCODE_RESTRICTED)
+ } else if (encoding->flags & IW_ENCODE_RESTRICTED)
local->config.authType = AUTH_SHAREDKEY; // Only Both
- if(encoding->flags & IW_ENCODE_OPEN)
+ else if (encoding->flags & IW_ENCODE_OPEN)
local->config.authType = AUTH_ENCRYPT; // Only Wep
+
/* Commit the changes to flags if needed */
if (local->config.authType != currentAuthType)
set_bit (FLAG_COMMIT, &local->flags);
On Thu, 2010-02-04 at 18:03 -0500, Chris Siebenmann wrote:
> | > I'm not sure what the machine's exact configuration is because
> | > I inherited it (the usual long cycle of passing older and older
> | > hardware around at work), but I believe it must; I certainly don't
> | > have anything plugged into any external slots or sockets.
> |
> | And just to confirm, what are the exact symptoms that you have with
> | "broken" kernels? Just that connections can't be made, the card can't
> | find access points, etc? or?
>
> In the GUI, no wireless networks show up/are listed in the usual
> dropdown menu; this means that I can't go as far as even trying to
> connect to anything. From the command line, 'iwlist wifi0 scanning'
> lists nothing.
Wait, wifi0? You need to be using 'eth0' or 'eth1', whichever eth
device that 'iwconfig' reports. wifi0 is a historical airo anomaly
that's only used for packet capture and sniffing of the 802.11 frames.
It should not be used for anything other than Wireshark basically.
Do you get the same behavior if you use the normal ethX interface of the
airo device?
Dan
On Fri, Feb 26, 2010 at 03:09:29PM -0800, Dan Williams wrote:
> On Fri, 2010-02-26 at 15:10 +0100, Stanislaw Gruszka wrote:
> > Even if keylen == 0 is a bug and should not really happen, better avoid
> > possibility of passing bad value to firmware.
> >
> > Signed-off-by: Stanislaw Gruszka <[email protected]>
>
> I actually told John to just apply your first patch via IRC, but perhaps
> he hasn't done that yet. I cannot reproduce the issue that Chris
> encountered with my PCMCIA 350 using 5.60 firmware, and I bricked my
> card loading 5.40 firmware (same as Chris) onto it.
>
> I couldn't find any failures your original patch on my 5.60 device, my
> objections were simply code flow and semantics. So I'll ack the
> original patch.
commit f09c256375c7cf1e112b8ef6306cdd313490d7c0
Author: Stanislaw Gruszka <[email protected]>
Date: Tue Feb 2 15:34:50 2010 +0100
airo: fix setting zero length WEP key
Patch prevents call set_wep_key() with zero key length. That fix long
standing regression since commit c0380693520b1a1e4f756799a0edc379378b462a
"airo: clean up WEP key operations". Additionally print call trace when
someone will try to use improper parameters, and remove key.len = 0
assignment, because it is in not possible code path.
Reported-by: Chris Siebenmann <[email protected]>
Bisected-by: Chris Siebenmann <[email protected]>
Tested-by: Chris Siebenmann <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: <[email protected]>
Signed-off-by: Stanislaw Gruszka <[email protected]>
Signed-off-by: John W. Linville <[email protected]>
Hth...
John
--
John W. Linville ? ? ? ? ? ? ? ?Someday the world will need a hero, and you
[email protected] ? ? ? ? ? ? ? ? ?might be all we have. ?Be ready.
On Fri, Feb 26, 2010 at 08:26:17PM -0500, John W. Linville wrote:
> On Fri, Feb 26, 2010 at 03:09:29PM -0800, Dan Williams wrote:
> > On Fri, 2010-02-26 at 15:10 +0100, Stanislaw Gruszka wrote:
> > > Even if keylen == 0 is a bug and should not really happen, better avoid
> > > possibility of passing bad value to firmware.
> > >
> > > Signed-off-by: Stanislaw Gruszka <[email protected]>
This is small v1->v2 patch on top of below commit. You asked for it :)
> commit f09c256375c7cf1e112b8ef6306cdd313490d7c0
> Author: Stanislaw Gruszka <[email protected]>
> Date: Tue Feb 2 15:34:50 2010 +0100
>
> airo: fix setting zero length WEP key
Stanislaw