Re: Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

Re: Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[Jean Tourrilhes]

"John W. Linville" wrote :
> On Mon, Jul 09, 2007 at 07:54:39PM +0900, Masakazu Mokuno wrote:
> > This patch removes some duplicated wireless ioctl entries in the array
> > 'struct ioctl_trans ioctl_start[]' of fs/compat_ioctl.c
> > 
> > These entries are registered twice like:
> > 
> > 	COMPATIBLE_IOCTL(SIOCGIWPRIV)
> > 
> > and
> > 
> > 	HANDLE_IOCTL(SIOCGIWPRIV, do_wireless_ioctl)
> > 
> > 
> > Signed-off-by: Masakazu Mokuno <mokuno@sm.sony.co.jp>
> > ---
> >  fs/compat_ioctl.c |    3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > --- a/fs/compat_ioctl.c
> > +++ b/fs/compat_ioctl.c
> > @@ -3156,12 +3156,9 @@ COMPATIBLE_IOCTL(SIOCSIWSENS)
> >  COMPATIBLE_IOCTL(SIOCGIWSENS)
> >  COMPATIBLE_IOCTL(SIOCSIWRANGE)
> >  COMPATIBLE_IOCTL(SIOCSIWPRIV)
> > -COMPATIBLE_IOCTL(SIOCGIWPRIV)
> >  COMPATIBLE_IOCTL(SIOCSIWSTATS)
> > -COMPATIBLE_IOCTL(SIOCGIWSTATS)
> >  COMPATIBLE_IOCTL(SIOCSIWAP)
> >  COMPATIBLE_IOCTL(SIOCGIWAP)
> > -COMPATIBLE_IOCTL(SIOCSIWSCAN)
> >  COMPATIBLE_IOCTL(SIOCSIWRATE)
> >  COMPATIBLE_IOCTL(SIOCGIWRATE)
> >  COMPATIBLE_IOCTL(SIOCSIWRTS)
> 
> As I read the code in compat_ioctl.c, it looks to me like the
> COMPATIBLE_IOCTL definitions are the ones that are actually being
> used today.  Do you agree?

	Actually, you are wrong, and Masakazu is right. All those
ioctls contains a pointer and should go through the pointer
conversion.
	The reason why Masakazu sent that patch is that he actually
stumbled on the problem and tested it.

> Given the...stability...of the wireless extensions API, if we are going
> to remove one or the other of these not-quite-duplicate definitions,
> shouldn't we remove the HANDLE_IOCTL defintions instead?

	I don't understand, you are in favor of breaking the API ?

	Regards,

	Jean

Re: Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[John W. Linville]

On Fri, Jul 27, 2007 at 10:13:01AM -0700, Jean Tourrilhes wrote:
> "John W. Linville" wrote :
> > On Mon, Jul 09, 2007 at 07:54:39PM +0900, Masakazu Mokuno wrote:
> > > This patch removes some duplicated wireless ioctl entries in the array
> > > 'struct ioctl_trans ioctl_start[]' of fs/compat_ioctl.c
> > > 
> > > These entries are registered twice like:
> > > 
> > > 	COMPATIBLE_IOCTL(SIOCGIWPRIV)
> > > 
> > > and
> > > 
> > > 	HANDLE_IOCTL(SIOCGIWPRIV, do_wireless_ioctl)
> > > 
> > > 
> > > Signed-off-by: Masakazu Mokuno <mokuno@sm.sony.co.jp>

> > As I read the code in compat_ioctl.c, it looks to me like the
> > COMPATIBLE_IOCTL definitions are the ones that are actually being
> > used today.  Do you agree?
> 
> 	Actually, you are wrong, and Masakazu is right. All those
> ioctls contains a pointer and should go through the pointer
> conversion.

Masakazu replied in agreement that the COMPATIBLE_IOCTL entries are
the effective ones i.e. the code currently uses those entries and
the others are currently just wasting space.

Perhaps the HANDLE_IOCTL entries are indeed the correct and intended ones.  You seem to be indicating so.

> 	The reason why Masakazu sent that patch is that he actually
> stumbled on the problem and tested it.

The only problem stated is the not-quite-duplicate entries.  If there
is an actual problem (and not just sloppy code) then that makes the
situation more clear.

What is the manifestation of the problem?

> > Given the...stability...of the wireless extensions API, if we are going
> > to remove one or the other of these not-quite-duplicate definitions,
> > shouldn't we remove the HANDLE_IOCTL defintions instead?
> 
> 	I don't understand, you are in favor of breaking the API ?

I'm not sure how an honest reading could imply that.

If this fixes a bug, then fine.  If we are trading the one "duplicate"
entry we have been using for one that hasn't been in use, it doesn't
make much sense.

John
-- 
John W. Linville
linville@tuxdriver.com

Re: Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[Jean Tourrilhes]

On Fri, Jul 27, 2007 at 03:10:54PM -0400, John W. Linville wrote:
> > 
> > 	Actually, you are wrong, and Masakazu is right. All those
> > ioctls contains a pointer and should go through the pointer
> > conversion.
> 
> Masakazu replied in agreement that the COMPATIBLE_IOCTL entries are
> the effective ones i.e. the code currently uses those entries and
> the others are currently just wasting space.

	No, he did not. This is what he said :
-----------------------------------------------
We can keep COMPATIBLE_IOCTL entries *if* the userland apps could work
around iw_point.pointer issue for these ioctls.
-----------------------------------------------
	The second part of the sentence is the most important one. We
don't want the apps to have to deal with this issue, therefore the
COMPATIBLE_IOCTL entries are wrong.

> >       The reason why Masakazu sent that patch is that he actually
> > stumbled on the problem and tested it.
> 
> The only problem stated is the not-quite-duplicate entries.

	You have to take account that Masakazu is not American. He did
not report a bug, he sent you a patch fixing the issue. And I want to
thank him for spending the time to track don this issue and report it.

> Perhaps the HANDLE_IOCTL entries are indeed the correct and intended
> ones.  You seem to be indicating so.

	Yes. We had a discussion about it a few month ago with
Johannes Berg which title was "wireless extensions vs. 64-bit
architectures". If you go back to that discussion, you will realise
that Johannes was clearly saying that the HANDLE_IOCTL entries are the
correct one.
		http://marc.info/?l=linux-wireless&m=117449937110479&w=2
	It seems that the fix we did at that point was not complete,
i.e. we added the missing HANDLE_IOCTL entries but forgot to remove
the corresponding COMPATIBLE_IOCTL entries. It seems that different
compiler do different things when there are duplicate.

> If this fixes a bug, then fine.  If we are trading the one "duplicate"
> entry we have been using for one that hasn't been in use, it doesn't
> make much sense.

	As I said earlier, if you want Wireless Extensions to keep
working, please cc me.

	Jean

Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[Masakazu Mokuno]

	Hi

I'd like to talk some about the patch.


I've noticed that 32bit compiled wireless tools on 64 bit kernel (PS3 runs
with powerpc 64 kernel) sometimes did not work properly than I expected,
especially 'iwlist eth0 scanning' would show messed results.

Although I also noticed there was no problem found if I'd used 64bit
wireless tools, I thought it was good that I first investigated this
problem some before I started to work on rewriting PS3 wireless support.

After some investigation, I found there was a mistake (which I believed)
in the table for handlers to deal with 32bit ioctls.  Two handlers were
defined simultaneously for one wireless ioctl as you can see.  After
brief look into these handlers, I thought the HANDLE_IOCTL(SIOCGIWPRIV,
do_wireless_ioctl) seemed to be better.  Then I made the patch that kept
the latter (do_wireless_ioctl) and submitted it.

And after that, I noticed that at least one of the causes of this iwlist
problem was that the size and alignment of iw_point structure differed
between 32bit and 64bit.  So I sent a mail that asked how we should deal
with it.

Then I got a reply that people in this ml had already knew it and worked
around in userland applications.  So I tried to use wirelesstools 29
pre22 and found that its iwlist showed reasonable scan results and I did
no further test nor investigation.

With these experiences, I thought that wireless experts here had already
agreement that it should be fixed basically by working around in
userland apps, and that I'd just brought up the settled issue again.

Some days later, John gave me the mail that referred about my patch.  I
understood that he asked me that only the former COMPATIBLE_IOCTL one
was used, but the latter HANDLE_IOCTL never used due to the nature of
compat_sys_ioctl().  That was exactly what I'd found, so I replied yes.
I meant yes in that sense.

In second half of the mail, I understood the he asked how about keeping
used one (COMPATIBLE_IOCTL) instead of HANDLE_IOCTL one, considering
compatibilities.  As I can esteem the idea that sometimes the compatibilities
are more important than changing something and I didn't think my patch
could address all problems of the issue and I had much time to test more,
I replied with the intention that I didn't stick to the patch if all
userland application could work around perfectly.

I should search much closer whether similar argument or reports existed
before sending my patch/mail, when I think of it now.  Sorry for my not
supplying suffcient information for my patch and/or concern.

--
Masakazu MOKUNO

Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[John W. Linville]

On Mon, Jul 30, 2007 at 09:02:56PM +0900, Masakazu Mokuno wrote:

> In second half of the mail, I understood the he asked how about keeping
> used one (COMPATIBLE_IOCTL) instead of HANDLE_IOCTL one, considering
> compatibilities.  As I can esteem the idea that sometimes the compatibilities
> are more important than changing something and I didn't think my patch
> could address all problems of the issue and I had much time to test more,
> I replied with the intention that I didn't stick to the patch if all
> userland application could work around perfectly.

Masakazu-san,

Thank you very much for your thorough explanation of the situation.
I apologize for having been a bit slow to grasp what you were doing.

I agree that the HANDLE_IOCTL version seems correct.  As you correctly
understood, my concern was that we not break anything new in the
process of fixing this bug.

Have you tested the both the old and the new wireless-tools on
a kernel with your patch applied?  Both 32- and 64-bit versions?
Do they all work equally well?

Thanks,

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[Jean Tourrilhes]

On Mon, Jul 30, 2007 at 10:47:26AM -0400, John W. Linville wrote:
> On Mon, Jul 30, 2007 at 09:02:56PM +0900, Masakazu Mokuno wrote:
> 
> > In second half of the mail, I understood the he asked how about keeping
> > used one (COMPATIBLE_IOCTL) instead of HANDLE_IOCTL one, considering
> > compatibilities.  As I can esteem the idea that sometimes the compatibilities
> > are more important than changing something and I didn't think my patch
> > could address all problems of the issue and I had much time to test more,
> > I replied with the intention that I didn't stick to the patch if all
> > userland application could work around perfectly.
> 
> Masakazu-san,
> 
> Thank you very much for your thorough explanation of the situation.
> I apologize for having been a bit slow to grasp what you were doing.
> 
> I agree that the HANDLE_IOCTL version seems correct.  As you correctly
> understood, my concern was that we not break anything new in the
> process of fixing this bug.
> 
> Have you tested the both the old and the new wireless-tools on
> a kernel with your patch applied?  Both 32- and 64-bit versions?
> Do they all work equally well?
> 
> Thanks,
> 
> John

	Using SIOCSIWSCAN is probably a bad idea to test, because
there is the extra problem of alignement *within* the data, on top of
the pointer problem.
	Testing with SIOCGIWSTATS or SIOCGIWPRIV would narrow down to
only the pointer problem, and the kernel fix should work with both old
and new version of the Wireless Tools. It's simple to test, either you
are getting valid data or garbage, the only issue is that some drivers
may be returning garbage for those calls.
	Johannes told me (in the thread I linked) that the
HANDLE_IOCTL was improving things (and that how we discovered the
alignement problem). But, Johannes did not do exhaustive testing and I
would welcome more testing, for sure.
	Have fun...

	Jean

Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[Masakazu Mokuno]

On Mon, 30 Jul 2007 11:37:29 -0700
Jean Tourrilhes <jt@hpl.hp.com> wrote:

> 	Testing with SIOCGIWSTATS or SIOCGIWPRIV would narrow down to
> only the pointer problem, and the kernel fix should work with both old
> and new version of the Wireless Tools. It's simple to test, either you
> are getting valid data or garbage, the only issue is that some drivers
> may be returning garbage for those calls.

As I'm outside of the firewall of the company this week,  I'll test in
the next week.

I think it is sufficient to see these ioctls work, using 'iwpriv
<interface>' for SIOCGIWPRIV and 'iwconfig <interface>' for SIOCGIWSTAT.
Is that right?  Anything to add more?

For the 'old version' of wirelesstools,  which version should I use?
wirelesstools 28?

regards

--
Masakazu MOKUNO

Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[Jean Tourrilhes]

On Tue, Jul 31, 2007 at 01:45:06PM +0900, Masakazu Mokuno wrote:
> 
> On Mon, 30 Jul 2007 11:37:29 -0700
> Jean Tourrilhes <jt@hpl.hp.com> wrote:
> 
> > 	Testing with SIOCGIWSTATS or SIOCGIWPRIV would narrow down to
> > only the pointer problem, and the kernel fix should work with both old
> > and new version of the Wireless Tools. It's simple to test, either you
> > are getting valid data or garbage, the only issue is that some drivers
> > may be returning garbage for those calls.
> 
> As I'm outside of the firewall of the company this week,  I'll test in
> the next week.
> 
> I think it is sufficient to see these ioctls work, using 'iwpriv
> <interface>' for SIOCGIWPRIV and 'iwconfig <interface>' for SIOCGIWSTAT.
> Is that right?

	Correct. You should check what to expect to make sure the
returned data is valid.

>  Anything to add more?
> 
> For the 'old version' of wirelesstools,  which version should I use?
> wirelesstools 28?

	Yes, v28 is great, unless John has a specific version in mind.

> regards
> 
> --
> Masakazu MOKUNO

	Jean

Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[Masakazu Mokuno]

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

	Hi

On Mon, 30 Jul 2007 10:47:26 -0400
"John W. Linville" <linville@tuxdriver.com> wrote:

> Have you tested the both the old and the new wireless-tools on
> a kernel with your patch applied?  Both 32- and 64-bit versions?
> Do they all work equally well?

Sorry for my late reply.

OK, I tested the following this week:
  - kernel 2.6.23-rc2 (geoff's PS3 tree)
     - the patch applied(*1) and unpatched
      (*1) another patch also applied.  See later section
      
  - wirlesstools version 28 and 29 pre22
     - 64bit and 32bit compiled
     - iwpriv and iwconfig
  
Test 
  - boot each kernel of two versions (patched and unpatched) on PS3
  - associate zd1211rw to an access point with wpa_supplicant (64bit
    compiled)
  - run 'iwconfig eth1' and 'iwpriv eth1'
    four versions 
       wirelesstools v28/32bit, v28/64bit
       wirelesstools v29/32bit, v29/64bit
  - compare the outputs
  
As I expected, 32bit iwconfig and iwpriv did not show proper result on
unpatched kernel, like:

> [root@localhost wireless_tools.28.32]# ./iwpriv eth1
> eth1      no private ioctls.
> 
> [root@localhost wireless_tools.28.32]# ./iwconfig eth1
> Warning: Driver for device eth1 has been compiled with version 22
> of Wireless Extension, while this program supports up to version 20.
> Some things may be broken...
> 
> eth1      IEEE 802.11b/g  ESSID:off/any  Nickname:"zd1211"
>           Mode:Managed  Frequency:2.462 GHz  Access Point: 00:16:01:3A:F5:FD   
>           Bit Rate:24 Mb/s   
>           Encryption key:<too big>
> [root@localhost wireless_tools.29.32]# ./iwpriv eth1
> eth1      no private ioctls.
> 
> [root@localhost wireless_tools.29.32]# ./iwconfig eth1
> eth1      IEEE 802.11b/g  ESSID:off/any  Nickname:"zd1211"
>           Mode:Managed  Frequency:2.462 GHz  Access Point: 00:16:01:3A:F5:FD   
>           Bit Rate=24 Mb/s   
>           Encryption key:<too big>


On the patched kernel, 32bit iwconfig and iwpriv showed appropriate
outputs and were same as 64bit's output:

> [root@localhost wireless_tools.28.32]# ./iwpriv eth1
> eth1      Available private ioctls :
>           set_regdomain    (8BE0) : set   1 byte  & get   0      
>           get_regdomain    (8BE1) : set   0       & get   1 byte 
> 
> [root@localhost wireless_tools.28.32]# ./iwconfig eth1
> Warning: Driver for device eth1 has been compiled with version 22
> of Wireless Extension, while this program supports up to version 20.
> Some things may be broken...
> 
> eth1      IEEE 802.11b/g  ESSID:"planexuser"  Nickname:"zd1211"
>           Mode:Managed  Frequency:2.462 GHz  Access Point: 00:16:01:3A:F5:FD   
>           Bit Rate:24 Mb/s   
>           Encryption key:9741-5654-B44F-7C71-2B3A-F918-1908-431F   Security mode:open
>           Link Quality=81/100  Signal level=45/100  
>           Rx invalid nwid:0  Rx invalid crypt:0  Rx invalid frag:0
>           Tx excessive retries:0  Invalid misc:0   Missed beacon:0
> [root@localhost wireless_tools.28.64]# ./iwpriv eth1
> eth1      Available private ioctls :
>           set_regdomain    (8BE0) : set   1 byte  & get   0      
>           get_regdomain    (8BE1) : set   0       & get   1 byte 
> 
> [root@localhost wireless_tools.28.64]# ./iwconfig eth1
> Warning: Driver for device eth1 has been compiled with version 22
> of Wireless Extension, while this program supports up to version 20.
> Some things may be broken...
> 
> eth1      IEEE 802.11b/g  ESSID:"planexuser"  Nickname:"zd1211"
>           Mode:Managed  Frequency:2.462 GHz  Access Point: 00:16:01:3A:F5:FD   
>           Bit Rate=24 Mb/s   
>           Encryption key:9741-5654-B44F-7C71-2B3A-F918-1908-431F   Security mode:open
>           Link Quality=82/100  Signal level=45/100  
>           Rx invalid nwid:0  Rx invalid crypt:0  Rx invalid frag:0
>           Tx excessive retries:0  Invalid misc:0   Missed beacon:0

So I think SIOCGIWPRIV and SOICGIWSTAT issues were fixed without
regressions.


While this testing, I've found another problem for iocompat32. 

fs/compat_ioctl.c:do_wireless_ioctl() copies the contents of the
argument of the 32bit ioctls into allocated area.  But it does not copy
back the contents on the return from 64bit ioctl calls.  So the caller
would get the unmodified argument even if the ioctl handler modified it.
For example, SIOCGIWENCODE could not return proper key length to the
caller, then iwconfig would show the strange result:

>           Encryption key:<too big>

The applied patch fixes the issue.

As I mentioned early in this mail, I did testing showed above with this
patch and found no obvious issue, but I did not have time/test-suite to
do comprehensive testing.  Please someone do review this new patch and
test more.

regards

--
Masakazu MOKUNO

[-- Attachment #2: fix_do_wirless_ioctl.diff --]
[-- Type: application/octet-stream, Size: 1712 bytes --]

As struct iw_point is bi-directional payload, we should copy back the content
on return from ioctl calls

Signed-off-by: Masakazu Mokuno <mokuno@sm.sony.co.jp>
---
 fs/compat_ioctl.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -2311,8 +2311,10 @@ static int do_wireless_ioctl(unsigned in
 	struct iwreq __user *iwr_u;
 	struct iw_point __user *iwp;
 	struct compat_iw_point __user *iwp_u;
-	compat_caddr_t pointer;
+	compat_caddr_t pointer_u;
+	void __user *pointer;
 	__u16 length, flags;
+	int ret;
 
 	iwr_u = compat_ptr(arg);
 	iwp_u = (struct compat_iw_point __user *) &iwr_u->u.data;
@@ -2330,17 +2332,29 @@ static int do_wireless_ioctl(unsigned in
 			   sizeof(iwr->ifr_ifrn.ifrn_name)))
 		return -EFAULT;
 
-	if (__get_user(pointer, &iwp_u->pointer) ||
+	if (__get_user(pointer_u, &iwp_u->pointer) ||
 	    __get_user(length, &iwp_u->length) ||
 	    __get_user(flags, &iwp_u->flags))
 		return -EFAULT;
 
-	if (__put_user(compat_ptr(pointer), &iwp->pointer) ||
+	if (__put_user(compat_ptr(pointer_u), &iwp->pointer) ||
 	    __put_user(length, &iwp->length) ||
 	    __put_user(flags, &iwp->flags))
 		return -EFAULT;
 
-	return sys_ioctl(fd, cmd, (unsigned long) iwr);
+	ret = sys_ioctl(fd, cmd, (unsigned long) iwr);
+
+	if (__get_user(pointer, &iwp->pointer) ||
+	    __get_user(length, &iwp->length) ||
+	    __get_user(flags, &iwp->flags))
+		return -EFAULT;
+
+	if (__put_user(ptr_to_compat(pointer), &iwp_u->pointer) ||
+	    __put_user(length, &iwp_u->length) ||
+	    __put_user(flags, &iwp_u->flags))
+		return -EFAULT;
+
+	return ret;
 }
 
 /* Since old style bridge ioctl's endup using SIOCDEVPRIVATE

Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[Jean Tourrilhes]

On Tue, Aug 07, 2007 at 04:41:00PM +0900, Masakazu Mokuno wrote:
> 	Hi
> 
> On Mon, 30 Jul 2007 10:47:26 -0400
> "John W. Linville" <linville@tuxdriver.com> wrote:
> 
> > Have you tested the both the old and the new wireless-tools on
> > a kernel with your patch applied?  Both 32- and 64-bit versions?
> > Do they all work equally well?
> 
> Sorry for my late reply.

	Don't be sorry, we prefer to be correct than to be quick. You
did a great job, thanks very much for the detailed bug report.

> OK, I tested the following this week:
>   - kernel 2.6.23-rc2 (geoff's PS3 tree)
>      - the patch applied(*1) and unpatched
>       (*1) another patch also applied.  See later section
>       
>   - wirlesstools version 28 and 29 pre22
>      - 64bit and 32bit compiled
>      - iwpriv and iwconfig
>   
> Test 
>   - boot each kernel of two versions (patched and unpatched) on PS3
>   - associate zd1211rw to an access point with wpa_supplicant (64bit
>     compiled)
>   - run 'iwconfig eth1' and 'iwpriv eth1'
>     four versions 
>        wirelesstools v28/32bit, v28/64bit
>        wirelesstools v29/32bit, v29/64bit
>   - compare the outputs

	That's pretty complete.

> As I expected, 32bit iwconfig and iwpriv did not show proper result on
> unpatched kernel, like:
> 
> > [root@localhost wireless_tools.28.32]# ./iwpriv eth1
> > eth1      no private ioctls.
> > 
> > [root@localhost wireless_tools.28.32]# ./iwconfig eth1
> > Warning: Driver for device eth1 has been compiled with version 22
> > of Wireless Extension, while this program supports up to version 20.
> > Some things may be broken...
> > 
> > eth1      IEEE 802.11b/g  ESSID:off/any  Nickname:"zd1211"
> >           Mode:Managed  Frequency:2.462 GHz  Access Point: 00:16:01:3A:F5:FD   
> >           Bit Rate:24 Mb/s   
> >           Encryption key:<too big>
> > [root@localhost wireless_tools.29.32]# ./iwpriv eth1
> > eth1      no private ioctls.
> > 
> > [root@localhost wireless_tools.29.32]# ./iwconfig eth1
> > eth1      IEEE 802.11b/g  ESSID:off/any  Nickname:"zd1211"
> >           Mode:Managed  Frequency:2.462 GHz  Access Point: 00:16:01:3A:F5:FD   
> >           Bit Rate=24 Mb/s   
> >           Encryption key:<too big>

	Ok, that was expected.

> On the patched kernel, 32bit iwconfig and iwpriv showed appropriate
> outputs and were same as 64bit's output:
> 
> > [root@localhost wireless_tools.28.32]# ./iwpriv eth1
> > eth1      Available private ioctls :
> >           set_regdomain    (8BE0) : set   1 byte  & get   0      
> >           get_regdomain    (8BE1) : set   0       & get   1 byte 
> > 
> > [root@localhost wireless_tools.28.32]# ./iwconfig eth1
> > Warning: Driver for device eth1 has been compiled with version 22
> > of Wireless Extension, while this program supports up to version 20.
> > Some things may be broken...
> > 
> > eth1      IEEE 802.11b/g  ESSID:"planexuser"  Nickname:"zd1211"
> >           Mode:Managed  Frequency:2.462 GHz  Access Point: 00:16:01:3A:F5:FD   
> >           Bit Rate:24 Mb/s   
> >           Encryption key:9741-5654-B44F-7C71-2B3A-F918-1908-431F   Security mode:open
> >           Link Quality=81/100  Signal level=45/100  
> >           Rx invalid nwid:0  Rx invalid crypt:0  Rx invalid frag:0
> >           Tx excessive retries:0  Invalid misc:0   Missed beacon:0
> > [root@localhost wireless_tools.28.64]# ./iwpriv eth1
> > eth1      Available private ioctls :
> >           set_regdomain    (8BE0) : set   1 byte  & get   0      
> >           get_regdomain    (8BE1) : set   0       & get   1 byte 
> > 
> > [root@localhost wireless_tools.28.64]# ./iwconfig eth1
> > Warning: Driver for device eth1 has been compiled with version 22
> > of Wireless Extension, while this program supports up to version 20.
> > Some things may be broken...
> > 
> > eth1      IEEE 802.11b/g  ESSID:"planexuser"  Nickname:"zd1211"
> >           Mode:Managed  Frequency:2.462 GHz  Access Point: 00:16:01:3A:F5:FD   
> >           Bit Rate=24 Mb/s   
> >           Encryption key:9741-5654-B44F-7C71-2B3A-F918-1908-431F   Security mode:open
> >           Link Quality=82/100  Signal level=45/100  
> >           Rx invalid nwid:0  Rx invalid crypt:0  Rx invalid frag:0
> >           Tx excessive retries:0  Invalid misc:0   Missed beacon:0
> 
> So I think SIOCGIWPRIV and SOICGIWSTAT issues were fixed without
> regressions.

	This is pretty much what I was expecting.

> While this testing, I've found another problem for iocompat32. 
> 
> fs/compat_ioctl.c:do_wireless_ioctl() copies the contents of the
> argument of the 32bit ioctls into allocated area.  But it does not copy
> back the contents on the return from 64bit ioctl calls.  So the caller
> would get the unmodified argument even if the ioctl handler modified it.
> For example, SIOCGIWENCODE could not return proper key length to the
> caller, then iwconfig would show the strange result:
> 
> >           Encryption key:<too big>

	Good catch ! I was not aware of that problem.
	That problem would also apply to a few other calls, such as
SIOCGIWRANGE, SIOCGIWSPY, SIOCGIWSCAN, SIOCGIWESSID, SIOCGIWNICKN and
SIOCGIWGENIE. However, for all these calls the problem would not show
up because userspace put the length as the maximum permissible length
and then parsing of the result would stop properly because the
returned data is NUL terminated. You would also miss the flags, but
most people probably did not pay attention to that.
	In any case, this problem need to be fixed.

> The applied patch fixes the issue.
> 
> As I mentioned early in this mail, I did testing showed above with this
> patch and found no obvious issue, but I did not have time/test-suite to
> do comprehensive testing.  Please someone do review this new patch and
> test more.

	The testing you did is pretty much all that's needed. All
ioctls will look the same. SIOCGIWENCODE was a good candidate as you
tested both the length, the pointer and the flags (Security mode).
	As far as I can see, the patch looks great. But, I'm not
familiar with the 64/32 bit conversions, so I may have overlooked
something.

Acked-by: Jean Tourrilhes <jt@hpl.hp.com>

> regards
> 
> --
> Masakazu MOKUNO

	Thanks a lot !

	Jean

Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[Masakazu Mokuno]

Hi John,

Could you please apply the patch that I has submitted in 8th Aug, for
2.6.24?

http://marc.info/?l=linux-wireless&m=118647249314475

This patch does not seem to be included into the wireless tree.


-- 
Masakazu MOKUNO

Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[John W. Linville]

On Wed, Sep 12, 2007 at 01:36:16PM +0900, Masakazu Mokuno wrote:
> Hi John,
> 
> Could you please apply the patch that I has submitted in 8th Aug, for
> 2.6.24?
> 
> http://marc.info/?l=linux-wireless&m=118647249314475
> 
> This patch does not seem to be included into the wireless tree.

Well, thanks for pointing that out.  The reason is that the patch
was lost, probably due to the fact that it wasn't posted under a new
subject with something like "[PATCH] fix compat_ioctl blah blah blah".
I try to read every email and find all the patches, but this list does
get a lot of traffic.  If you could mark new patches more clearly it
would help me a lot.  Plus, it is probably easier than tracking the
trees and having to send reminders. :-)

Thanks,

John
-- 
John W. Linville
linville@tuxdriver.com

[PATCH] remove duplicated ioctl entries in compat_ioctl.c

[Masakazu Mokuno]

This patch removes some duplicated wireless ioctl entries in the array
'struct ioctl_trans ioctl_start[]' of fs/compat_ioctl.c

These entries are registered twice like:

	COMPATIBLE_IOCTL(SIOCGIWPRIV)

and

	HANDLE_IOCTL(SIOCGIWPRIV, do_wireless_ioctl)


Signed-off-by: Masakazu Mokuno <mokuno@sm.sony.co.jp>
---
 fs/compat_ioctl.c |    3 ---
 1 file changed, 3 deletions(-)

--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -3156,12 +3156,9 @@ COMPATIBLE_IOCTL(SIOCSIWSENS)
 COMPATIBLE_IOCTL(SIOCGIWSENS)
 COMPATIBLE_IOCTL(SIOCSIWRANGE)
 COMPATIBLE_IOCTL(SIOCSIWPRIV)
-COMPATIBLE_IOCTL(SIOCGIWPRIV)
 COMPATIBLE_IOCTL(SIOCSIWSTATS)
-COMPATIBLE_IOCTL(SIOCGIWSTATS)
 COMPATIBLE_IOCTL(SIOCSIWAP)
 COMPATIBLE_IOCTL(SIOCGIWAP)
-COMPATIBLE_IOCTL(SIOCSIWSCAN)
 COMPATIBLE_IOCTL(SIOCSIWRATE)
 COMPATIBLE_IOCTL(SIOCGIWRATE)
 COMPATIBLE_IOCTL(SIOCSIWRTS)


--
Masakazu MOKUNO

Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[Holger Schurig]

> This patch removes some duplicated wireless ioctl entries in
> the array 'struct ioctl_trans ioctl_start[]' of
> fs/compat_ioctl.c
> -COMPATIBLE_IOCTL(SIOCSIWSCAN)
>  COMPATIBLE_IOCTL(SIOCSIWRATE)
>  COMPATIBLE_IOCTL(SIOCGIWRATE)

Maybe you remove the double COMPATIBLE_IOCTL(SIOCGIWRATE) as 
well?

Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[Holger Schurig]

Forget my mail, I wasn' seening the tiny C/G difference

Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[John W. Linville]

On Mon, Jul 09, 2007 at 07:54:39PM +0900, Masakazu Mokuno wrote:
> This patch removes some duplicated wireless ioctl entries in the array
> 'struct ioctl_trans ioctl_start[]' of fs/compat_ioctl.c
> 
> These entries are registered twice like:
> 
> 	COMPATIBLE_IOCTL(SIOCGIWPRIV)
> 
> and
> 
> 	HANDLE_IOCTL(SIOCGIWPRIV, do_wireless_ioctl)
> 
> 
> Signed-off-by: Masakazu Mokuno <mokuno@sm.sony.co.jp>
> ---
>  fs/compat_ioctl.c |    3 ---
>  1 file changed, 3 deletions(-)
> 
> --- a/fs/compat_ioctl.c
> +++ b/fs/compat_ioctl.c
> @@ -3156,12 +3156,9 @@ COMPATIBLE_IOCTL(SIOCSIWSENS)
>  COMPATIBLE_IOCTL(SIOCGIWSENS)
>  COMPATIBLE_IOCTL(SIOCSIWRANGE)
>  COMPATIBLE_IOCTL(SIOCSIWPRIV)
> -COMPATIBLE_IOCTL(SIOCGIWPRIV)
>  COMPATIBLE_IOCTL(SIOCSIWSTATS)
> -COMPATIBLE_IOCTL(SIOCGIWSTATS)
>  COMPATIBLE_IOCTL(SIOCSIWAP)
>  COMPATIBLE_IOCTL(SIOCGIWAP)
> -COMPATIBLE_IOCTL(SIOCSIWSCAN)
>  COMPATIBLE_IOCTL(SIOCSIWRATE)
>  COMPATIBLE_IOCTL(SIOCGIWRATE)
>  COMPATIBLE_IOCTL(SIOCSIWRTS)

As I read the code in compat_ioctl.c, it looks to me like the
COMPATIBLE_IOCTL definitions are the ones that are actually being
used today.  Do you agree?

Given the...stability...of the wireless extensions API, if we are going
to remove one or the other of these not-quite-duplicate definitions,
shouldn't we remove the HANDLE_IOCTL defintions instead?

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [PATCH] remove duplicated ioctl entries in compat_ioctl.c

[Masakazu Mokuno]

On Thu, 26 Jul 2007 14:50:29 -0400
"John W. Linville" <linville@tuxdriver.com> wrote:

> On Mon, Jul 09, 2007 at 07:54:39PM +0900, Masakazu Mokuno wrote:
> > This patch removes some duplicated wireless ioctl entries in the array
> > 'struct ioctl_trans ioctl_start[]' of fs/compat_ioctl.c
> > 
> > These entries are registered twice like:
> > 
> > 	COMPATIBLE_IOCTL(SIOCGIWPRIV)
> > 
> > and
> > 
> > 	HANDLE_IOCTL(SIOCGIWPRIV, do_wireless_ioctl)
> > 
> > 
> > Signed-off-by: Masakazu Mokuno <mokuno@sm.sony.co.jp>
> > ---
> >  fs/compat_ioctl.c |    3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > --- a/fs/compat_ioctl.c
> > +++ b/fs/compat_ioctl.c
> > @@ -3156,12 +3156,9 @@ COMPATIBLE_IOCTL(SIOCSIWSENS)
> >  COMPATIBLE_IOCTL(SIOCGIWSENS)
> >  COMPATIBLE_IOCTL(SIOCSIWRANGE)
> >  COMPATIBLE_IOCTL(SIOCSIWPRIV)
> > -COMPATIBLE_IOCTL(SIOCGIWPRIV)
> >  COMPATIBLE_IOCTL(SIOCSIWSTATS)
> > -COMPATIBLE_IOCTL(SIOCGIWSTATS)
> >  COMPATIBLE_IOCTL(SIOCSIWAP)
> >  COMPATIBLE_IOCTL(SIOCGIWAP)
> > -COMPATIBLE_IOCTL(SIOCSIWSCAN)
> >  COMPATIBLE_IOCTL(SIOCSIWRATE)
> >  COMPATIBLE_IOCTL(SIOCGIWRATE)
> >  COMPATIBLE_IOCTL(SIOCSIWRTS)
> 
> As I read the code in compat_ioctl.c, it looks to me like the
> COMPATIBLE_IOCTL definitions are the ones that are actually being
> used today.  Do you agree?

Yes. The latter one in the array is silently ignored. In our case, 
HANDLE_IOCTL(SIOCGIWPRIV, do_wireless_ioctl) ignored.

> Given the...stability...of the wireless extensions API, if we are going
> to remove one or the other of these not-quite-duplicate definitions,
> shouldn't we remove the HANDLE_IOCTL defintions instead?

I'm not sure which is better to keep. 
We can keep COMPATIBLE_IOCTL entries if the userland apps could work
around iw_point.pointer issue for these ioctls.

--
Masakazu MOKUNO