mac80211: Fix swapped parameters to ieee80211_set_channel()

PATCH In review Andrew Lunn 2 participants 11 messages Dec 12, 2007 – Dec 13, 2007 mac80211

lore

Share & tools

lore

Andrew Lunn

author

2007-12-12 16:08 UTC #0 lore raw

PATCH 1/1 mac80211: Fix swapped parameters to ieee80211_set_channel()

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, <[email protected]>

---

net/mac80211/ieee80211_ioctl.c +1 -1

index 7027eed..d4a032e 100644
@@ -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++)

Johannes Berg

maintainer

2007-12-12 17:44 UTC #1 lore raw

net/mac80211/ieee80211_ioctl.c static int ieee80211_ioctl_siwfreq(struct net_device *dev, · line 347

Quoted 4 lines

                } else

net/mac80211/ieee80211_ioctl.c +1 -1

-                       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

Andrew Lunn

author

2007-12-12 18:02 UTC #2 lore raw

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

Quoted 2 lines

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 <[email protected]>
---

net/mac80211/ieee80211_ioctl.c +1 -1

index 7027eed..50d4a46 100644
@@ -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++)

Johannes Berg

maintainer Acked-by

2007-12-12 18:09 UTC #3 lore raw

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

Quoted 5 lines · Andrew Lunn

Quoted 1 line
Good catch, but please resubmit with tabs as indentation and CC John
as
Quoted 1 line
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.

Quoted 1 line

Signed-off-by Andrew lunn <[email protected]>

Acked-by Johannes Berg <[email protected]>

Quoted 17 lines

---

net/mac80211/ieee80211_ioctl.c +1 -1

index 7027eed..50d4a46 100644
@@ -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++)

Andrew Lunn

author

2007-12-13 12:09 UTC #5 lore raw

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

Quoted 6 lines · Johannes Berg

Quoted 1 line
Acked-by Johannes Berg <[email protected]>
Actually, I take that back.
The code is correct, as per the comment above it. Stupid wext :/

Quoted 10 lines

        /* 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

Johannes Berg

maintainer

2007-12-13 12:32 UTC #6 lore raw

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

Quoted 2 lines

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

Andrew Lunn

author

2007-12-13 12:50 UTC #7 lore raw

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

Quoted 11 lines · Johannes Berg

Quoted 2 lines
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

Johannes Berg

maintainer

2007-12-13 12:53 UTC #8 lore raw

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

Quoted 10 lines

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

Andrew Lunn

author

2007-12-13 13:14 UTC #9 lore raw

PATCH 1/1 wirelesstools: More user space validation of channel/frequency

Quoted 7 lines · Johannes Berg

Quoted 7 lines
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.

Quoted 6 lines

Quoted 3 lines
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 <[email protected]>
---

iwconfig.c +7 -0

@@ -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;

Andrew Lunn

author

2007-12-13 12:38 UTC #10 lore raw

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

In reply to Andrew Lunn

Quoted 18 lines · Andrew Lunn

Quoted 6 lines · Johannes Berg

Quoted 1 line
Acked-by Johannes Berg <[email protected]>
Actually, I take that back.
The code is correct, as per the comment above it. Stupid wext :/

Quoted 10 lines

        /* 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);

Quoted 5 lines

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