RE: [PATCH] incorrect use of sizeof() in ioctl definitions

RE: [PATCH] incorrect use of sizeof() in ioctl definitions

[Tian, Kevin]

> Andries Brouwer [mailto:aebr@win.tue.nl] wrote:
> On Tue, Sep 30, 2003 at 11:25:56PM +0100, Matthew Wilcox wrote:
> > On Tue, Sep 30, 2003 at 02:08:05PM -0700, Andrew Morton wrote:
> > > Arun Sharma <arun.sharma@intel.com> wrote:
> > > >
> > > > Some drivers seem to use macros such as _IOR/_IOW in a way that
ends
> up
> > > > calling the sizeof() operator twice. For eg:
> > > >
> > > > -#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, sizeof(__u32*))
> > > > +#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, __u32*)
> 
> But this changes the define. You want
> 
> #define FBIO_ATY128_GET_MIRROR	_IOR_BAD('@', 1, __u32*)

	Maybe I got something wrong, but could someone please help me to
understand why introduce _IOR_BAD here? Thanks first! :)

	IMO, the birth of new ioctl definition convention considers
"size" of argument as a part, so we should conform to this rule.
_IOR_BAD is no different as old one <_IOR('@', 1, sizeof(__u32*))>,
which expands as sizeof(sizeof(__32*)) and actually assume temp result
of internal sizeof as the argument. Of course this didn't reflect the
true point and we should change the definition. :)

	Also I'm confused about the modification about using size_t to
replace sizeof(). Take MATROXFB_SET_OUTPUT_MODE for example:

(old)	#define MATROXFB_SET_OUTPUT_MODE
_IOW('n',0xFA,sizeof(struct matroxioc_output_mode))
(now)	#define MATROXFB_SET_OUTPUT_MODE        _IOW('n',0xFA,size_t)

	The size of matroxioc_output_mode is 8 bytes on all platforms,
however size_t will be calculated as 4 bytes on 32bit arch and 8 bytes
on 64bit arch. So this is also like using sizeof(), which imposes the
difference between 32bit ioctl number and 64bit ioctl number. However in
standard manner, I mean:
	#define MATROXFB_SET_OUTPUT_MODE        _IOW('n',0xFA,struct
matroxioc_output_mode)
 	The value should be identical on all platforms, which save our
efforts to do useless conversion when running 32bit apps on 64bit
platform.

	The most important is: to use sizeof() or size_t here both
messed the ioctl definition, which violate the initial motivation of
_IO**, is it?

> Something else we should do is to change all occurrences of 'size'
> here into 'argtype'. All this nonsense came because of the bad choice
> of identifier.
	Agree. The inaccurate name here confused us... :)

RE: [PATCH] incorrect use of sizeof() in ioctl definitions

[Maciej Zenczykowski]

> 	Maybe I got something wrong, but could someone please help me to
> understand why introduce _IOR_BAD here? Thanks first! :)

So as not to break userspace we must still support old values, at the same 
time we want new programs to start using the new correct values - hence 
the introduction of _backward compatibility_ values.

> (old)	#define MATROXFB_SET_OUTPUT_MODE
> _IOW('n',0xFA,sizeof(struct matroxioc_output_mode))
> (now)	#define MATROXFB_SET_OUTPUT_MODE        _IOW('n',0xFA,size_t)

the old was bad since it was sizeof(sizeof(...)) - it so happens that by 
def sizeof(anything) is a size_t - thus replacing sizeof(sizeof(..)) with 
sizeof(size_t) changes nothing - just shortens the code...
Of course what we probably should really have is the above (now) code 
defined as "BAD" and the previous (old) define without the sizeof as the 
current (no BAD prefix).

> 	The size of matroxioc_output_mode is 8 bytes on all platforms,
> however size_t will be calculated as 4 bytes on 32bit arch and 8 bytes
> on 64bit arch. So this is also like using sizeof(), which imposes the
> difference between 32bit ioctl number and 64bit ioctl number. However in
> standard manner, I mean:
> 	#define MATROXFB_SET_OUTPUT_MODE        _IOW('n',0xFA,struct
> matroxioc_output_mode)
>  	The value should be identical on all platforms, which save our
> efforts to do useless conversion when running 32bit apps on 64bit
> platform.

Precisely - unfortunately due to coding errors (the erroneous double 
sizeof invocation) this isn't the way it is - and for backwards 
compatibility it can't be broken... sad, really...

> 
> 	The most important is: to use sizeof() or size_t here both
> messed the ioctl definition, which violate the initial motivation of
> _IO**, is it?

Yap, both violate.  It is a mess and there is no easy fix due to the need
to retain the old invalid ioctl's as well... and the real 'tough' mess is
on platforms where due to type sizes both the OLD and the NEW defines
result in the same IOCTL define value... that'll probably screw over
switch statements (I'm not sure - but I think you can't have the same
value twice in a case statement).  If this happens on all platforms than 
the conversion OLD==BAD to NEW can be done quietly - otherwise... we've 
got hell.

Cheers,
MaZe.

RE: [PATCH] incorrect use of sizeof() in ioctl definitions

[Tian, Kevin]

> Wrote from Maciej Zenczykowski [mailto:maze@cela.pl]
> 
> So as not to break userspace we must still support old values, at the
same
> time we want new programs to start using the new correct values -
hence
> the introduction of _backward compatibility_ values.

Thanks. :) Now I see... but are there any rules to decide which part
should be upgraded even breaking the backward compatibility? You know,
the latest 2.6 kernel will request many modules recompiled to run on it.
IMO, most ioctls defined in this bad manner seems to be not-widely used
ones, and... maybe it's worthy of some sacrifice on temporary
compatibility, thus to keep a clean and consistent environment. 

> the old was bad since it was sizeof(sizeof(...)) - it so happens that
by
> def sizeof(anything) is a size_t - thus replacing sizeof(sizeof(..))
with
> sizeof(size_t) changes nothing - just shortens the code...
> Of course what we probably should really have is the above (now) code
> defined as "BAD" and the previous (old) define without the sizeof as
the
> current (no BAD prefix).

Reasonable!

> Yap, both violate.  It is a mess and there is no easy fix due to the
need
> to retain the old invalid ioctl's as well... 

If without such need... :(

Thanks,
Kevin

Re: [PATCH] incorrect use of sizeof() in ioctl definitions

[Matthew Wilcox]

On Wed, Oct 08, 2003 at 07:42:23PM +0800, Tian, Kevin wrote:
> Thanks. :) Now I see... but are there any rules to decide which part
> should be upgraded even breaking the backward compatibility? You know,

Nothing should break.  Linux does not require userspace to be recompiled.
Binaries from the 0.99 days still run today.  An exception to this would
be ioctls that were introduced during the 2.5 cycle as they have not
appeared in a stable release yet.

Also, please remember that the size part of the ioctl is only a hint.
Very few things really care about it (which is why the breakage wasn't
discovered before).

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

RE: [PATCH] incorrect use of sizeof() in ioctl definitions

[Tian, Kevin]

Well, I may misunderstand the measurement here. By previous comment from
Matthew Wilcox, I see "Clearly it's too late to change the ioctl
definitions...". Er, so all things like IOR_BAD and size_t are just to
keep current API untouched, while warning subsequent guys right way to
populate ioctls. :) Then the last question is: is it worthy of some
efforts to modify these APIs completely? Maybe the bee just bites
once...

Re: [PATCH] incorrect use of sizeof() in ioctl definitions

[Matthew Wilcox]

On Wed, Oct 08, 2003 at 05:58:53PM +0800, Tian, Kevin wrote:
> Well, I may misunderstand the measurement here. By previous comment from
> Matthew Wilcox, I see "Clearly it's too late to change the ioctl
> definitions...". Er, so all things like IOR_BAD and size_t are just to
> keep current API untouched, while warning subsequent guys right way to
> populate ioctls. :) Then the last question is: is it worthy of some
> efforts to modify these APIs completely? Maybe the bee just bites
> once...

Already done on i386 and some other platforms.  Not on ia64 yet, I see.
The bad_ioctl stuff is part of that typechecking.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

[PATCH] incorrect use of sizeof() in ioctl definitions

[Arun Sharma]

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


Some drivers seem to use macros such as _IOR/_IOW in a way that ends up calling the sizeof() operator twice. For eg:

-#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, sizeof(__u32*))
+#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, __u32*)

from include/asm-ia64/ioctl.h (other archs are similar):

#define _IOR(type,nr,size)      _IOC(_IOC_READ,(type),(nr),sizeof(size))

While this is not a problem for native 32 bit or native 64 platforms, it is a problem for 32 bit applications running on 64 bit platforms in compatibility mode because

sizeof(sizeof(__u32*)) on a 64 bit kernel != sizeof(sizeof(__u32*)) on a 32 bit app.

The attached patch attempts to fix the instances we could find.

	-Arun


[-- Attachment #2: ioctl32-sizeof.txt --]
[-- Type: text/plain, Size: 9003 bytes --]

--- ia64-linux2.6.0/include/linux/matroxfb.h	Tue Aug 26 16:47:26 2003
+++ ia64-linux2.6.0-patch/include/linux/matroxfb.h	Fri Sep  5 16:24:58 2003
@@ -15,21 +15,21 @@
 #define MATROXFB_OUTPUT_MODE_NTSC	0x0002
 #define MATROXFB_OUTPUT_MODE_MONITOR	0x0080
 };
-#define MATROXFB_SET_OUTPUT_MODE	_IOW('n',0xFA,sizeof(struct matroxioc_output_mode))
-#define MATROXFB_GET_OUTPUT_MODE	_IOWR('n',0xFA,sizeof(struct matroxioc_output_mode))
+#define MATROXFB_SET_OUTPUT_MODE	_IOW('n',0xFA,struct matroxioc_output_mode)
+#define MATROXFB_GET_OUTPUT_MODE	_IOWR('n',0xFA,struct matroxioc_output_mode)
 
 /* bitfield */
 #define MATROXFB_OUTPUT_CONN_PRIMARY	(1 << MATROXFB_OUTPUT_PRIMARY)
 #define MATROXFB_OUTPUT_CONN_SECONDARY	(1 << MATROXFB_OUTPUT_SECONDARY)
 #define MATROXFB_OUTPUT_CONN_DFP	(1 << MATROXFB_OUTPUT_DFP)
 /* connect these outputs to this framebuffer */
-#define MATROXFB_SET_OUTPUT_CONNECTION	_IOW('n',0xF8,sizeof(__u32))
+#define MATROXFB_SET_OUTPUT_CONNECTION	_IOW('n',0xF8,__u32)
 /* which outputs are connected to this framebuffer */
-#define MATROXFB_GET_OUTPUT_CONNECTION	_IOR('n',0xF8,sizeof(__u32))
+#define MATROXFB_GET_OUTPUT_CONNECTION	_IOR('n',0xF8,__u32)
 /* which outputs are available for this framebuffer */
-#define MATROXFB_GET_AVAILABLE_OUTPUTS	_IOR('n',0xF9,sizeof(__u32))
+#define MATROXFB_GET_AVAILABLE_OUTPUTS	_IOR('n',0xF9,__u32)
 /* which outputs exist on this framebuffer */
-#define MATROXFB_GET_ALL_OUTPUTS	_IOR('n',0xFB,sizeof(__u32))
+#define MATROXFB_GET_ALL_OUTPUTS	_IOR('n',0xFB,__u32)
 
 enum matroxfb_ctrl_id {
   MATROXFB_CID_TESTOUT	 = V4L2_CID_PRIVATE_BASE,
--- ia64-linux2.6.0/drivers/video/aty/aty128fb.c	Fri Sep  5 16:10:59 2003
+++ ia64-linux2.6.0-patch/drivers/video/aty/aty128fb.c	Fri Sep  5 16:01:43 2003
@@ -2041,9 +2041,9 @@
 #define ATY_MIRROR_CRT_ON	0x00000002
 
 /* out param: u32*	backlight value: 0 to 15 */
-#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, sizeof(__u32*))
+#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, __u32*)
 /* in param: u32*	backlight value: 0 to 15 */
-#define FBIO_ATY128_SET_MIRROR	_IOW('@', 2, sizeof(__u32*))
+#define FBIO_ATY128_SET_MIRROR	_IOW('@', 2, __u32*)
 
 static int aty128fb_ioctl(struct inode *inode, struct file *file, u_int cmd,
 			  u_long arg, struct fb_info *info)
--- ia64-linux2.6.0/include/linux/coda.h	Tue Aug 26 16:47:26 2003
+++ ia64-linux2.6.0-patch/include/linux/coda.h	Fri Sep  5 16:15:54 2003
@@ -324,7 +324,7 @@
 #define VC_MAXMSGSIZE      sizeof(union inputArgs)+sizeof(union outputArgs) +\
                             VC_MAXDATASIZE  
 
-#define CIOC_KERNEL_VERSION _IOWR('c', 10, sizeof (int))
+#define CIOC_KERNEL_VERSION _IOWR('c', 10, int)
 
 #if 0
 #define CODA_KERNEL_VERSION 0 /* don't care about kernel version number */
--- ia64-linux2.6.0/drivers/net/wan/cosa.h	Tue Aug 26 16:47:25 2003
+++ ia64-linux2.6.0-patch/drivers/net/wan/cosa.h	Fri Sep  5 16:17:52 2003
@@ -73,19 +73,19 @@
 #define COSAIORSET	_IO('C',0xf0)
 
 /* Start microcode at given address */
-#define COSAIOSTRT	_IOW('C',0xf1,sizeof(int))
+#define COSAIOSTRT	_IOW('C',0xf1,int)
 
 /* Read the block from the device memory */
-#define COSAIORMEM	_IOR('C',0xf2,sizeof(struct cosa_download *))
+#define COSAIORMEM	_IOR('C',0xf2,struct cosa_download*)
 
 /* Write the block to the device memory (i.e. download the microcode) */
-#define COSAIODOWNLD	_IOW('C',0xf2,sizeof(struct cosa_download *))
+#define COSAIODOWNLD	_IOW('C',0xf2,struct cosa_download*)
 
 /* Read the device type (one of "srp", "cosa", and "cosa8" for now) */
-#define COSAIORTYPE	_IOR('C',0xf3,sizeof(char *))
+#define COSAIORTYPE	_IOR('C',0xf3,char*)
 
 /* Read the device identification string */
-#define COSAIORIDSTR	_IOR('C',0xf4,sizeof(char *))
+#define COSAIORIDSTR	_IOR('C',0xf4,char*)
 /* Maximum length of the identification string. */
 #define COSA_MAX_ID_STRING 128
 
@@ -100,7 +100,7 @@
 #define COSAIONRCHANS	_IO('C',0xf8)
 
 /* Set the driver for the bus-master operations */
-#define COSAIOBMSET	_IOW('C', 0xf9, sizeof(unsigned short))
+#define COSAIOBMSET	_IOW('C', 0xf9, unsigned short)
 
 #define COSA_BM_OFF	0	/* Bus-mastering off - use ISA DMA (default) */
 #define COSA_BM_ON	1	/* Bus-mastering on - faster but untested */
--- ia64-linux2.6.0/include/linux/fs.h	Fri Sep  5 16:14:19 2003
+++ ia64-linux2.6.0-patch/include/linux/fs.h	Fri Sep  5 16:18:57 2003
@@ -187,15 +187,15 @@
 #define BLKSSZGET  _IO(0x12,104)/* get block device sector size */
 #if 0
 #define BLKPG      _IO(0x12,105)/* See blkpg.h */
-#define BLKELVGET  _IOR(0x12,106,sizeof(blkelv_ioctl_arg_t))/* elevator get */
-#define BLKELVSET  _IOW(0x12,107,sizeof(blkelv_ioctl_arg_t))/* elevator set */
+#define BLKELVGET  _IOR(0x12,106,blkelv_ioctl_arg_t)/* elevator get */
+#define BLKELVSET  _IOW(0x12,107,blkelv_ioctl_arg_t)/* elevator set */
 /* This was here just to show that the number is taken -
    probably all these _IO(0x12,*) ioctls should be moved to blkpg.h. */
 #endif
 /* A jump here: 108-111 have been used for various private purposes. */
-#define BLKBSZGET  _IOR(0x12,112,sizeof(int))
-#define BLKBSZSET  _IOW(0x12,113,sizeof(int))
-#define BLKGETSIZE64 _IOR(0x12,114,sizeof(u64))	/* return device size in bytes (u64 *arg) */
+#define BLKBSZGET  _IOR(0x12,112,int)
+#define BLKBSZSET  _IOW(0x12,113,int)
+#define BLKGETSIZE64 _IOR(0x12,114,u64)	/* return device size in bytes (u64 *arg) */
 
 #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */
 #define FIBMAP	   _IO(0x00,1)	/* bmap access */
--- ia64-linux2.6.0/include/linux/i8k.h	Tue Aug 26 16:47:26 2003
+++ ia64-linux2.6.0-patch/include/linux/i8k.h	Mon Sep  8 09:25:39 2003
@@ -22,11 +22,11 @@
 
 #define I8K_BIOS_VERSION	_IOR ('i', 0x80, 4)
 #define I8K_MACHINE_ID		_IOR ('i', 0x81, 16)
-#define I8K_POWER_STATUS	_IOR ('i', 0x82, sizeof(int))
-#define I8K_FN_STATUS		_IOR ('i', 0x83, sizeof(int))
-#define I8K_GET_TEMP		_IOR ('i', 0x84, sizeof(int))
-#define I8K_GET_SPEED		_IOWR('i', 0x85, sizeof(int))
-#define I8K_GET_FAN		_IOWR('i', 0x86, sizeof(int))
+#define I8K_POWER_STATUS	_IOR ('i', 0x82, int)
+#define I8K_FN_STATUS		_IOR ('i', 0x83, int)
+#define I8K_GET_TEMP		_IOR ('i', 0x84, int)
+#define I8K_GET_SPEED		_IOWR('i', 0x85, int)
+#define I8K_GET_FAN		_IOWR('i', 0x86, int)
 #define I8K_SET_FAN		_IOWR('i', 0x87, sizeof(int)*2)
 
 #define I8K_FAN_LEFT		1
--- ia64-linux2.6.0/include/linux/if_pppox.h	Tue Aug 26 16:47:26 2003
+++ ia64-linux2.6.0-patch/include/linux/if_pppox.h	Fri Sep  5 16:22:06 2003
@@ -67,9 +67,9 @@
  *
  ********************************************************************/
 
-#define PPPOEIOCSFWD	_IOW(0xB1 ,0, sizeof(struct sockaddr_pppox))
+#define PPPOEIOCSFWD	_IOW(0xB1 ,0, struct sockaddr_pppox)
 #define PPPOEIOCDFWD	_IO(0xB1 ,1)
-/*#define PPPOEIOCGFWD	_IOWR(0xB1,2, sizeof(struct sockaddr_pppox))*/
+/*#define PPPOEIOCGFWD	_IOWR(0xB1,2, struct sockaddr_pppox)*/
 
 /* Codes to identify message types */
 #define PADI_CODE	0x09
--- ia64-linux2.6.0/include/linux/pmu.h	Tue Aug 26 16:47:26 2003
+++ ia64-linux2.6.0-patch/include/linux/pmu.h	Fri Sep  5 16:26:16 2003
@@ -107,15 +107,15 @@
 /* no param */
 #define PMU_IOC_SLEEP		_IO('B', 0)
 /* out param: u32*	backlight value: 0 to 15 */
-#define PMU_IOC_GET_BACKLIGHT	_IOR('B', 1, sizeof(__u32*))
+#define PMU_IOC_GET_BACKLIGHT	_IOR('B', 1, __u32*)
 /* in param: u32	backlight value: 0 to 15 */
-#define PMU_IOC_SET_BACKLIGHT	_IOW('B', 2, sizeof(__u32))
+#define PMU_IOC_SET_BACKLIGHT	_IOW('B', 2, __u32)
 /* out param: u32*	PMU model */
-#define PMU_IOC_GET_MODEL	_IOR('B', 3, sizeof(__u32*))
+#define PMU_IOC_GET_MODEL	_IOR('B', 3, __u32*)
 /* out param: u32*	has_adb: 0 or 1 */
-#define PMU_IOC_HAS_ADB		_IOR('B', 4, sizeof(__u32*)) 
+#define PMU_IOC_HAS_ADB		_IOR('B', 4, __u32*) 
 /* out param: u32*	can_sleep: 0 or 1 */
-#define PMU_IOC_CAN_SLEEP	_IOR('B', 5, sizeof(__u32*)) 
+#define PMU_IOC_CAN_SLEEP	_IOR('B', 5, __u32*) 
 /* no param */
 #define PMU_IOC_GRAB_BACKLIGHT	_IOR('B', 6, 0) 
 
--- ia64-linux2.6.0/include/linux/radeonfb.h	Tue Aug 26 16:47:26 2003
+++ ia64-linux2.6.0-patch/include/linux/radeonfb.h	Fri Sep  5 16:26:41 2003
@@ -8,8 +8,8 @@
 #define ATY_RADEON_CRT_ON	0x00000002
 
 
-#define FBIO_RADEON_GET_MIRROR	_IOR('@', 3, sizeof(__u32*))
-#define FBIO_RADEON_SET_MIRROR	_IOW('@', 4, sizeof(__u32*))
+#define FBIO_RADEON_GET_MIRROR	_IOR('@', 3, __u32*)
+#define FBIO_RADEON_SET_MIRROR	_IOW('@', 4, __u32*)
 
 #endif
 
--- ia64-linux2.6.0/include/video/sisfb.h	Tue Aug 26 16:47:26 2003
+++ ia64-linux2.6.0-patch/include/video/sisfb.h	Fri Sep  5 16:27:27 2003
@@ -145,9 +145,9 @@
 /*     If changing this, vgatypes.h must also be changed (for X driver)    */
 
 /* TW: ioctl for identifying and giving some info (esp. memory heap start) */
-#define SISFB_GET_INFO	  	_IOR('n',0xF8,sizeof(__u32))
+#define SISFB_GET_INFO	  	_IOR('n',0xF8,__u32)
 
-#define SISFB_GET_VBRSTATUS  	_IOR('n',0xF9,sizeof(__u32))
+#define SISFB_GET_VBRSTATUS  	_IOR('n',0xF9,__u32)
 
 /* TW: Structure argument for SISFB_GET_INFO ioctl  */
 typedef struct _SISFB_INFO sisfb_info, *psisfb_info;

Re: [PATCH] incorrect use of sizeof() in ioctl definitions

[Andrew Morton]

Arun Sharma <arun.sharma@intel.com> wrote:
>
> Some drivers seem to use macros such as _IOR/_IOW in a way that ends up calling the sizeof() operator twice. For eg:
> 
> -#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, sizeof(__u32*))
> +#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, __u32*)
> 
> from include/asm-ia64/ioctl.h (other archs are similar):
> 
> #define _IOR(type,nr,size)      _IOC(_IOC_READ,(type),(nr),sizeof(size))

Matthew Wilcox fixed all except one of these a while back.  What is left
over is this chunk:


diff -puN drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fix drivers/video/aty/aty128fb.c
--- 25/drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fix	Tue Sep 30 14:04:12 2003
+++ 25-akpm/drivers/video/aty/aty128fb.c	Tue Sep 30 14:04:12 2003
@@ -2041,9 +2041,9 @@ aty128fb_setcolreg(u_int regno, u_int re
 #define ATY_MIRROR_CRT_ON	0x00000002
 
 /* out param: u32*	backlight value: 0 to 15 */
-#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, sizeof(__u32*))
+#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, __u32*)
 /* in param: u32*	backlight value: 0 to 15 */
-#define FBIO_ATY128_SET_MIRROR	_IOW('@', 2, sizeof(__u32*))
+#define FBIO_ATY128_SET_MIRROR	_IOW('@', 2, __u32*)
 
 static int aty128fb_ioctl(struct inode *inode, struct file *file, u_int cmd,
 			  u_long arg, struct fb_info *info)


Matthew's conversion mainly converted things to size_t, but from the looks
of it, __u32* is the right thing to use in this case, I think?

Re: [PATCH] incorrect use of sizeof() in ioctl definitions

[Russell King]

On Tue, Sep 30, 2003 at 02:08:05PM -0700, Andrew Morton wrote:
> diff -puN drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fix drivers/video/aty/aty128fb.c
> --- 25/drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fix	Tue Sep 30 14:04:12 2003
> +++ 25-akpm/drivers/video/aty/aty128fb.c	Tue Sep 30 14:04:12 2003
> @@ -2041,9 +2041,9 @@ aty128fb_setcolreg(u_int regno, u_int re
>  #define ATY_MIRROR_CRT_ON	0x00000002
>  
>  /* out param: u32*	backlight value: 0 to 15 */
> -#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, sizeof(__u32*))
> +#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, __u32*)
>  /* in param: u32*	backlight value: 0 to 15 */
> -#define FBIO_ATY128_SET_MIRROR	_IOW('@', 2, sizeof(__u32*))
> +#define FBIO_ATY128_SET_MIRROR	_IOW('@', 2, __u32*)
>  
>  static int aty128fb_ioctl(struct inode *inode, struct file *file, u_int cmd,
>  			  u_long arg, struct fb_info *info)
> 
> 
> Matthew's conversion mainly converted things to size_t, but from the looks
> of it, __u32* is the right thing to use in this case, I think?

sizeof(__u32*) may not be sizeof(sizeof(__u32*)), so this would be an API
change...  Therefore, all these wrong entries need to change to size_t
(preferably with the real type following inside a comment so we don't
loose useful information.)

-- 
Russell King (rmk@arm.linux.org.uk)	http://www.arm.linux.org.uk/personal/
      Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
      maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                      2.6 Serial core

Re: [PATCH] incorrect use of sizeof() in ioctl definitions

[Andrew Morton]

Russell King <rmk@arm.linux.org.uk> wrote:
>
> >  /* out param: u32*	backlight value: 0 to 15 */
> > -#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, sizeof(__u32*))
> > +#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, __u32*)
> >  /* in param: u32*	backlight value: 0 to 15 */
> > -#define FBIO_ATY128_SET_MIRROR	_IOW('@', 2, sizeof(__u32*))
> > +#define FBIO_ATY128_SET_MIRROR	_IOW('@', 2, __u32*)
> >  
> >  static int aty128fb_ioctl(struct inode *inode, struct file *file, u_int cmd,
> >  			  u_long arg, struct fb_info *info)
> > 
> > 
> > Matthew's conversion mainly converted things to size_t, but from the looks
> > of it, __u32* is the right thing to use in this case, I think?
> 
> sizeof(__u32*) may not be sizeof(sizeof(__u32*)), so this would be an API
> change...  Therefore, all these wrong entries need to change to size_t
> (preferably with the real type following inside a comment so we don't
> loose useful information.)

In that case I'm going to need a bit of education as to what the whole
thing is trying to do.

If FBIO_ATY128_SET_MIRROR is really supposed to be passing a pointer into
the ioctl then we *want* the encoded ioctl number to be different for
32-bit-compiled userspace and 64-bit-compiled userspace, don't we?

Re: [PATCH] incorrect use of sizeof() in ioctl definitions

[Maciej Zenczykowski]

> > Matthew's conversion mainly converted things to size_t, but from the looks
> > of it, __u32* is the right thing to use in this case, I think?
> 
> sizeof(__u32*) may not be sizeof(sizeof(__u32*)), so this would be an API
> change...  Therefore, all these wrong entries need to change to size_t
> (preferably with the real type following inside a comment so we don't
> loose useful information.)

I hit this bug a while back - most often it's for 4byte sized objects 
(ints or pointers) on x86 arch (don't know about others).  Unfortunately 
there was a case where it was for an 8 and was wrongly declared via sizeof 
and thus 4.  I'd suggest moving all to single sizeof's and where this 
causes API changes to implement bogus/extra declarations which could then 
possibly be faded out with time.  However currently the glibc kernel 
headers are also similarly screwed.  Obviously this would bloat the kernel 
a little, OTOH for x86 arch I only noticed one case where it didn't end up 
the same whether thru double or single sizeof.  Perhaps we could be lucky 
and have almost as much luck on other archs?

MaZe.

Re: [PATCH] incorrect use of sizeof() in ioctl definitions

[Matthew Wilcox]

On Tue, Sep 30, 2003 at 02:08:05PM -0700, Andrew Morton wrote:
> Arun Sharma <arun.sharma@intel.com> wrote:
> >
> > Some drivers seem to use macros such as _IOR/_IOW in a way that ends up calling the sizeof() operator twice. For eg:
> > 
> > -#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, sizeof(__u32*))
> > +#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, __u32*)

The real problem is that ia64 doesn't have the typechecking ioctl
definitions yet.  Here's what I committed for parisc recently (it applies
to ia64 too):

+++ include/asm-parisc/ioctl.h  21 Sep 2003 01:01:38 -0000      1.2
@@ -44,11 +44,21 @@
         ((nr)   << _IOC_NRSHIFT) | \
         ((size) << _IOC_SIZESHIFT))
 
+/* provoke compile error for invalid uses of size argument */
+extern int __invalid_size_argument_for_IOC;
+#define _IOC_TYPECHECK(t) \
+       ((sizeof(t) == sizeof(t[1]) && \
+         sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
+         sizeof(t) : __invalid_size_argument_for_IOC)
+
 /* used to create numbers */
 #define _IO(type,nr)           _IOC(_IOC_NONE,(type),(nr),0)
-#define _IOR(type,nr,size)     _IOC(_IOC_READ,(type),(nr),sizeof(size))
-#define _IOW(type,nr,size)     _IOC(_IOC_WRITE,(type),(nr),sizeof(size))
-#define _IOWR(type,nr,size)    _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),sizeof(siz
e))
+#define _IOR(type,nr,size)     _IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)
))
+#define _IOW(type,nr,size)     _IOC(_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size
)))
+#define _IOWR(type,nr,size)    _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPE
CHECK(size)))
+#define _IOR_BAD(type,nr,size) _IOC(_IOC_READ,(type),(nr),sizeof(size))
+#define _IOW_BAD(type,nr,size) _IOC(_IOC_WRITE,(type),(nr),sizeof(size))
+#define _IOWR_BAD(type,nr,size)        _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),si
zeof(size))
 
 /* used to decode ioctl numbers.. */
 #define _IOC_DIR(nr)           (((nr) >> _IOC_DIRSHIFT) & _IOC_DIRMASK)

> Matthew Wilcox fixed all except one of these a while back.  What is left
> over is this chunk:
> 
> 
> diff -puN drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fix drivers/video/aty/aty128fb.c
> --- 25/drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fix	Tue Sep 30 14:04:12 2003
> +++ 25-akpm/drivers/video/aty/aty128fb.c	Tue Sep 30 14:04:12 2003
> @@ -2041,9 +2041,9 @@ aty128fb_setcolreg(u_int regno, u_int re
>  #define ATY_MIRROR_CRT_ON	0x00000002
>  
>  /* out param: u32*	backlight value: 0 to 15 */
> -#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, sizeof(__u32*))
> +#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, __u32*)
>  /* in param: u32*	backlight value: 0 to 15 */
> -#define FBIO_ATY128_SET_MIRROR	_IOW('@', 2, sizeof(__u32*))
> +#define FBIO_ATY128_SET_MIRROR	_IOW('@', 2, __u32*)
>  
>  static int aty128fb_ioctl(struct inode *inode, struct file *file, u_int cmd,
>  			  u_long arg, struct fb_info *info)
> 
> 
> Matthew's conversion mainly converted things to size_t, but from the looks
> of it, __u32* is the right thing to use in this case, I think?

I think so, given:

radeonfb.h:#define FBIO_RADEON_GET_MIRROR       _IOR('@', 3, size_t)
radeonfb.h:#define FBIO_RADEON_SET_MIRROR       _IOW('@', 4, size_t)

Though we can also change it to _IOR_BAD and _IOW_BAD.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [PATCH] incorrect use of sizeof() in ioctl definitions

[Andries Brouwer]

On Tue, Sep 30, 2003 at 11:25:56PM +0100, Matthew Wilcox wrote:
> On Tue, Sep 30, 2003 at 02:08:05PM -0700, Andrew Morton wrote:
> > Arun Sharma <arun.sharma@intel.com> wrote:
> > >
> > > Some drivers seem to use macros such as _IOR/_IOW in a way that ends up
> > > calling the sizeof() operator twice. For eg:
> > > 
> > > -#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, sizeof(__u32*))
> > > +#define FBIO_ATY128_GET_MIRROR	_IOR('@', 1, __u32*)

But this changes the define. You want

#define FBIO_ATY128_GET_MIRROR	_IOR_BAD('@', 1, __u32*)

> +#define _IOR(type,nr,size)     _IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)
> ))
> +#define _IOR_BAD(type,nr,size) _IOC(_IOC_READ,(type),(nr),sizeof(size))

Something else we should do is to change all occurrences of 'size'
here into 'argtype'. All this nonsense came because of the bad choice
of identifier.