[PATCH] mac80211: Fix swapped parameters to ieee80211_set_channel()

[PATCH] mac80211: Fix swapped parameters to ieee80211_set_channel()

[Andrew Lunn]

Hi Folks

I found a silly typo in
net/mac80211/ieee80211_ioctl.c:ieee80211_ioctl_siwfreq().

The parameters passed to ieee80211_set_channel() should be 
(local, channel, freq), but in one instance it passes 
(local, freq, channel) which results in an EINVAL.

        Andrew

Signed-off-by: Andrew Lunn, <andrew@lunn.ch>
---
diff --git a/net/mac80211/ieee80211_ioctl.c b/net/mac80211/ieee80211_ioctl.c
index 7027eed..d4a032e 100644
--- a/net/mac80211/ieee80211_ioctl.c
+++ b/net/mac80211/ieee80211_ioctl.c
@@ -344,7 +344,7 @@ static int ieee80211_ioctl_siwfreq(struct net_device *dev,
                                        IEEE80211_STA_AUTO_CHANNEL_SEL;
                        return 0;
                } else
-                       return ieee80211_set_channel(local, freq->m, -1);
+                        return ieee80211_set_channel(local, -1, freq->m);
        } else {
                int i, div = 1000000;
                for (i = 0; i < freq->e; i++)

Re: [PATCH] mac80211: Fix swapped parameters to ieee80211_set_channel()

[Johannes Berg]

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


>                 } else
> -                       return ieee80211_set_channel(local, freq->m, -1);
> +                        return ieee80211_set_channel(local, -1, freq->m);

Good catch, but please resubmit with tabs as indentation and CC John as
he gets to apply patches. Or I can send it on if you wish.

johannes

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

Re: [PATCH] try 2: mac80211: Fix swapped parameters to ieee80211_set_channel()

[Andrew Lunn]

> Good catch, but please resubmit with tabs as indentation and CC John as
> he gets to apply patches. Or I can send it on if you wish.

OK. Here it is with tabs, and with CC to John Linville

    Andrew

Signed-off-by Andrew lunn <andrew@lunn.ch>
---


diff --git a/net/mac80211/ieee80211_ioctl.c b/net/mac80211/ieee80211_ioctl.c
index 7027eed..50d4a46 100644
--- a/net/mac80211/ieee80211_ioctl.c
+++ b/net/mac80211/ieee80211_ioctl.c
@@ -344,7 +344,7 @@ static int ieee80211_ioctl_siwfreq(struct net_device *dev,
                                        IEEE80211_STA_AUTO_CHANNEL_SEL;
                        return 0;
                } else
-                       return ieee80211_set_channel(local, freq->m, -1);
+                       return ieee80211_set_channel(local, -1, freq->m);
        } else {
                int i, div = 1000000;
                for (i = 0; i < freq->e; i++)

Re: [PATCH] try 2: mac80211: Fix swapped parameters to ieee80211_set_channel()

[Johannes Berg]

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

On Wed, 2007-12-12 at 19:02 +0100, Andrew Lunn wrote:
> > Good catch, but please resubmit with tabs as indentation and CC John
> as
> > he gets to apply patches. Or I can send it on if you wish.
> 
> OK. Here it is with tabs, and with CC to John Linville

Except now you have a bad patch description. Keep in mind that the patch
will most likely be applied with automatic tools :)

I'll leave it up to John whether he wants a new one.

> Signed-off-by Andrew lunn <andrew@lunn.ch>

Acked-by: Johannes Berg <johannes@sipsolutions.net>
> ---
> 
> 
> diff --git a/net/mac80211/ieee80211_ioctl.c b/net/mac80211/ieee80211_ioctl.c
> index 7027eed..50d4a46 100644
> --- a/net/mac80211/ieee80211_ioctl.c
> +++ b/net/mac80211/ieee80211_ioctl.c
> @@ -344,7 +344,7 @@ static int ieee80211_ioctl_siwfreq(struct net_device *dev,
>                                         IEEE80211_STA_AUTO_CHANNEL_SEL;
>                         return 0;
>                 } else
> -                       return ieee80211_set_channel(local, freq->m, -1);
> +                       return ieee80211_set_channel(local, -1, freq->m);
>         } else {
>                 int i, div = 1000000;
>                 for (i = 0; i < freq->e; i++)
> 

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

Re: [PATCH] try 2: mac80211: Fix swapped parameters to ieee80211_set_channel()

[Johannes Berg]

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

> Acked-by: Johannes Berg <johannes@sipsolutions.net>

Actually, I take that back.

The code is correct, as per the comment above it. Stupid wext :/

        /* freq->e == 0: freq->m = channel; otherwise freq = m * 10^e */
        if (freq->e == 0) {
                if (freq->m < 0) {
                        if (sdata->type == IEEE80211_IF_TYPE_STA)
                                sdata->u.sta.flags |=
                                        IEEE80211_STA_AUTO_CHANNEL_SEL;
                        return 0;
                } else  
                        return ieee80211_set_channel(local, freq->m, -1);

johannes

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

Re: [PATCH] try 2: mac80211: Fix swapped parameters to ieee80211_set_channel()

[Andrew Lunn]

On Thu, Dec 13, 2007 at 01:16:14AM +0100, Johannes Berg wrote:
> > Acked-by: Johannes Berg <johannes@sipsolutions.net>
> 
> Actually, I take that back.
> 
> The code is correct, as per the comment above it. Stupid wext :/
> 
>         /* freq->e == 0: freq->m = channel; otherwise freq = m * 10^e */
>         if (freq->e == 0) {
>                 if (freq->m < 0) {
>                         if (sdata->type == IEEE80211_IF_TYPE_STA)
>                                 sdata->u.sta.flags |=
>                                         IEEE80211_STA_AUTO_CHANNEL_SEL;
>                         return 0;
>                 } else  
>                         return ieee80211_set_channel(local, freq->m, -1);
> 

Probably a stupid question, but are you sure the comment is correct?

I added a printk for freq->e and freq->m. When i do

iwconfig wlan0 freq 5240

i see that e = 0, m = 5240 and that with my patch in place everything
is happy. Without the patch, it passes 5240 as a channel, not a
frequency and i get an EINVAL. 

So either the comment is wrong, or the comment is correct but
something is filling in freq wrongly. 

Any ideas?

    Thanks
        Andrew

Re: [PATCH] try 2: mac80211: Fix swapped parameters to ieee80211_set_channel()

[Johannes Berg]

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


> iwconfig wlan0 freq 5240

man iwconfig(8)

       freq/channel
              Set the operating frequency or channel in the  device.  A  value
              below 1000 indicates a channel number, a value greater than 1000
              is a frequency in Hz. You may append the suffix k, M or G to the
              value  (for  example,  "2.46G"  for  2.46 GHz frequency), or add
              enough ’0’.

johannes

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

Re: [PATCH] try 2: mac80211: Fix swapped parameters to ieee80211_set_channel()

[Andrew Lunn]

On Thu, Dec 13, 2007 at 01:30:25PM +0100, Johannes Berg wrote:
> 
> > iwconfig wlan0 freq 5240
> 
> man iwconfig(8)
> 
>        freq/channel
>               Set the operating frequency or channel in the  device.  A  value
>               below 1000 indicates a channel number, a value greater than 1000
>               is a frequency in Hz. You may append the suffix k, M or G to the
>               value  (for  example,  "2.46G"  for  2.46 GHz frequency), or add
>               enough ???0???.

Yup, see my last email. 

However this description does not match the implementation. The
implementation is that its a channel if e = 0. My "frequency" is > 1000,  
but it was still interpreted as a channel. Looking at the
implementation of iw_float2freq(), the description should be that a
value less than 1e9 is a channel. 

So there should really be a check in the user space tools which says
that values between 1000 and 1e9 are invalid. This would catch errors
like the one i made for both 2.4GHz and 5GHz.

     Andrew

Re: [PATCH] try 2: mac80211: Fix swapped parameters to ieee80211_set_channel()

[Johannes Berg]

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


> However this description does not match the implementation. The
> implementation is that its a channel if e = 0. My "frequency" is > 1000,  
> but it was still interpreted as a channel. Looking at the
> implementation of iw_float2freq(), the description should be that a
> value less than 1e9 is a channel. 
> 
> So there should really be a check in the user space tools which says
> that values between 1000 and 1e9 are invalid. This would catch errors
> like the one i made for both 2.4GHz and 5GHz.

Yeah, well, we all know wext sucks. You are of course right, but I think
that nobody's motivated any more to fix things.

johannes

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

[PATCH] wirelesstools: More user space validation of channel/frequency

[Andrew Lunn]

On Thu, Dec 13, 2007 at 01:53:18PM +0100, Johannes Berg wrote:
> 
> > However this description does not match the implementation. The
> > implementation is that its a channel if e = 0. My "frequency" is > 1000,  
> > but it was still interpreted as a channel. Looking at the
> > implementation of iw_float2freq(), the description should be that a
> > value less than 1e9 is a channel. 
> > 
> > So there should really be a check in the user space tools which says
> > that values between 1000 and 1e9 are invalid. This would catch errors
> > like the one i made for both 2.4GHz and 5GHz.
> 
> Yeah, well, we all know wext sucks. You are of course right, but I think
> that nobody's motivated any more to fix things.

Well, here is a patch suggestion. If it got me, it will get others,
and i think this little patch will help. 

    Andrew

Signed-off-by Andrew Lunn <andrew@lunn.ch>
---
--- wireless_tools.30-clean/iwconfig.c	2007-09-18 01:38:08.000000000 +0200
+++ wireless_tools.30/iwconfig.c	2007-12-13 13:57:16.000000000 +0100
@@ -791,6 +791,13 @@
 	      if(unit[0] == 'k') freq *= KILO;
 	    }
 
+	  if (freq >= 1000.0 && freq < 1e9) {
+	    fprintf(stderr, "Invalid channel/frequency. \n");
+	    fprintf(stderr, "Channels must be < 1000. \n");
+	    fprintf(stderr, "Frequencies must be > 1GHz\n");
+	    return (IWERR_ARG_TYPE);
+	  }
+
 	  iw_float2freq(freq, &(wrq.u.freq));
 
 	  wrq.u.freq.flags = IW_FREQ_FIXED;

Re: [PATCH] try 2: mac80211: Fix swapped parameters to ieee80211_set_channel()

[Andrew Lunn]

On Thu, Dec 13, 2007 at 01:09:04PM +0100, Andrew Lunn wrote:
> On Thu, Dec 13, 2007 at 01:16:14AM +0100, Johannes Berg wrote:
> > > Acked-by: Johannes Berg <johannes@sipsolutions.net>
> > 
> > Actually, I take that back.
> > 
> > The code is correct, as per the comment above it. Stupid wext :/
> > 
> >         /* freq->e == 0: freq->m = channel; otherwise freq = m * 10^e */
> >         if (freq->e == 0) {
> >                 if (freq->m < 0) {
> >                         if (sdata->type == IEEE80211_IF_TYPE_STA)
> >                                 sdata->u.sta.flags |=
> >                                         IEEE80211_STA_AUTO_CHANNEL_SEL;
> >                         return 0;
> >                 } else  
> >                         return ieee80211_set_channel(local, freq->m, -1);
> > 
> 
> Probably a stupid question, but are you sure the comment is correct?
> 
> I added a printk for freq->e and freq->m. When i do
> 
> iwconfig wlan0 freq 5240

O.K. I'm stupid!

That command line should be 
iwconfig wlan0 freq 5240M

Otherwise it tries to use 5240 HZ, which results in e == 0!

Maybe i should add a helpful hint to iwconfig. if setting the frequency
fails, and the frequency is < 2GHz, give the user a hit that maybe
they are missing postfix?

Patch withdraw, brain upgrade requested instead.

        Andrew