[PATCH] new ioctl type checking causes gcc warning

[PATCH] new ioctl type checking causes gcc warning

[Kevin P. Fleming]

Submitted by: Kevin P. Fleming (kpfleming at cox dot net)
Date: 2003-09-12
Initial Package Version: 2.6.0-test5
Origin: Kevin P. Fleming (kpfleming at cox dot net)
Description: The definition of __invalid_size_argument_for_IOC is signed,
              which causes an signed/unsigned comparison error to be
              emitted by GCC (at least for 3.3.1).

--- linux-2.6.0-test5/include/asm-i386/ioctl.h~	Mon Sep  8 12:49:52 2003
+++ linux-2.6.0-test5/include/asm-i386/ioctl.h	Fri Sep 12 11:58:41 2003
@@ -53,7 +53,7 @@
  	 ((size) << _IOC_SIZESHIFT))

  /* provoke compile error for invalid uses of size argument */
-extern int __invalid_size_argument_for_IOC;
+extern unsigned int __invalid_size_argument_for_IOC;
  #define _IOC_TYPECHECK(t) \
  	((sizeof(t) == sizeof(t[1]) && \
  	  sizeof(t) < (1 << _IOC_SIZEBITS)) ? \

Re: [PATCH] new ioctl type checking causes gcc warning

[Andreas Schwab]

"Kevin P. Fleming" <kpfleming@cox.net> writes:

> --- linux-2.6.0-test5/include/asm-i386/ioctl.h~	Mon Sep  8 12:49:52 2003
> +++ linux-2.6.0-test5/include/asm-i386/ioctl.h	Fri Sep 12 11:58:41 2003
> @@ -53,7 +53,7 @@
>   	 ((size) << _IOC_SIZESHIFT))
>
>   /* provoke compile error for invalid uses of size argument */
> -extern int __invalid_size_argument_for_IOC;
> +extern unsigned int __invalid_size_argument_for_IOC;

Why not make it size_t which is what sizeof actually returns?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] new ioctl type checking causes gcc warning

[Arnd Bergmann]

On Friday 12 September 2003 22:02, Andreas Schwab wrote:
> "Kevin P. Fleming" <kpfleming@cox.net> writes:
> >   /* provoke compile error for invalid uses of size argument */
> > -extern int __invalid_size_argument_for_IOC;
> > +extern unsigned int __invalid_size_argument_for_IOC;
>
> Why not make it size_t which is what sizeof actually returns?

I had tried that first, but found that there are places that
use asm/ioctl.h without including asm/posix_types.h first, so 
size_t might not be declared. unsigned int (or unsigned long)
is the better alternative here. Does this look ok to everyone?

	Arnd <><

===== include/asm-i386/ioctl.h 1.2 vs edited =====
--- 1.2/include/asm-i386/ioctl.h	Mon Sep  8 15:21:28 2003
+++ edited/include/asm-i386/ioctl.h	Fri Sep 12 14:42:58 2003
@@ -52,12 +52,16 @@
 	 ((nr)   << _IOC_NRSHIFT) | \
 	 ((size) << _IOC_SIZESHIFT))
 
+#ifdef __KERNEL__
 /* provoke compile error for invalid uses of size argument */
-extern int __invalid_size_argument_for_IOC;
+extern unsigned 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)
+#else
+#define _IOC_TYPECHECK(t) sizeof(t)
+#endif
 
 /* used to create numbers */
 #define _IO(type,nr)		_IOC(_IOC_NONE,(type),(nr),0)
===== include/asm-ppc/ioctl.h 1.4 vs edited =====
--- 1.4/include/asm-ppc/ioctl.h	Mon Sep  8 20:14:00 2003
+++ edited/include/asm-ppc/ioctl.h	Fri Sep 12 14:42:42 2003
@@ -37,12 +37,16 @@
 	 ((nr)   << _IOC_NRSHIFT) | \
 	 ((size) << _IOC_SIZESHIFT))
 
+#ifdef __KERNEL__
 /* provoke compile error for invalid uses of size argument */
-extern int __invalid_size_argument_for_IOC;
+extern unsigned 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)
+#else
+#define _IOC_TYPECHECK(t) sizeof(t)
+#endif
 
 /* used to create numbers */
 #define _IO(type,nr)		_IOC(_IOC_NONE,(type),(nr),0)
===== include/asm-ppc64/ioctl.h 1.2 vs edited =====
--- 1.2/include/asm-ppc64/ioctl.h	Tue Sep  9 22:23:09 2003
+++ edited/include/asm-ppc64/ioctl.h	Fri Sep 12 14:43:50 2003
@@ -42,12 +42,16 @@
 	 ((nr)   << _IOC_NRSHIFT) | \
 	 ((size) << _IOC_SIZESHIFT))
 
+#ifdef __KERNEL__
 /* provoke compile error for invalid uses of size argument */
-extern int __invalid_size_argument_for_IOC;
+extern unsigned long __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)
+#else
+#define _IOC_TYPECHECK(t) sizeof(t)
+#endif
 
 /* used to create numbers */
 #define _IO(type,nr)		_IOC(_IOC_NONE,(type),(nr),0)

Re: [PATCH] new ioctl type checking causes gcc warning

[Kevin P. Fleming]

Arnd Bergmann wrote:

> I had tried that first, but found that there are places that
> use asm/ioctl.h without including asm/posix_types.h first, so 
> size_t might not be declared. unsigned int (or unsigned long)
> is the better alternative here. Does this look ok to everyone?

After working on this some more this afternoon, I realize now that 
it's much better to have the typechecking in place than not, even for 
userspace. Maybe the best solution is to still leave the typechecking 
(don't wrap it in #ifdef __KERNEL__), and just

#ifdef size_t
extern size_t __invalid_size_argument_for_IOC;
#else
extern unsigned int __invalid_size_argument_for_IOC;
#endif

Would the type specification of this non-existent variable ever 
actually effect the generated code? If not, then even putting this 
#ifdef in is overkill.

Re: [PATCH] new ioctl type checking causes gcc warning

[Arnd Bergmann]

On Saturday 13 September 2003 01:43, Kevin P. Fleming wrote:

> After working on this some more this afternoon, I realize now that
> it's much better to have the typechecking in place than not, even for
> userspace. Maybe the best solution is to still leave the typechecking
> (don't wrap it in #ifdef __KERNEL__), and just
>
> #ifdef size_t

This doesn't work, because size_t is a typedef, not a macro.

> extern size_t __invalid_size_argument_for_IOC;
> #else
> extern unsigned int __invalid_size_argument_for_IOC;
> #endif
>
> Would the type specification of this non-existent variable ever
> actually effect the generated code? If not, then even putting this
> #ifdef in is overkill.

No, but as Andreas pointed out earlier, doing non-optimized builds
with the _IOC_TYPECHECK macro in place always results in link
errors, even for correct code. Since we know that the kernel
is always built with -O2, '#ifdef __KERNEL__' is sufficient
here.

The type checking this in user space is not necessary, because 
the point of the check is only to keep people from adding *new*
invalid ioctl numbers and doing the check for the kernel does that.
However, the old numbers need to be kept for a long time and there
is no point in breaking user applications that use established
interfaces.

	Arnd <><

Re: [PATCH] new ioctl type checking causes gcc warning

[Kevin P. Fleming]

Arnd Bergmann wrote:

> This doesn't work, because size_t is a typedef, not a macro.

Yeah, I should have thought of that. Sorry.

> The type checking this in user space is not necessary, because 
> the point of the check is only to keep people from adding *new*
> invalid ioctl numbers and doing the check for the kernel does that.
> However, the old numbers need to be kept for a long time and there
> is no point in breaking user applications that use established
> interfaces.

Hmm, obviously I misunderstood how this worked. Does that mean that 
these two lines:

#define BLKGETSIZE64	_IOR(0x12,114,sizeof(__uint64_t))
#define BLKGETSIZE64	_IOR(0x12,114,__uint64_t)

actually produce different ioctl numbers? If so, then I don't 
understand how the kernel can continue to offer the old/invalid 
interface when the new _IOR macro won't accept the first version any 
longer.

Re: [PATCH] new ioctl type checking causes gcc warning

[Arnd Bergmann]

On Saturday 13 September 2003 02:31, Kevin P. Fleming wrote:
> Does that mean that
> these two lines:
>
> #define BLKGETSIZE64    _IOR(0x12,114,sizeof(__uint64_t))
> #define BLKGETSIZE64    _IOR(0x12,114,__uint64_t)
>
> actually produce different ioctl numbers?

Exactly. On 32 bit systems, the former is 0x80041272, the
latter is 0x80081272. On 64 bit systems, they are both
0x80081272.

>                                            If so, then I don't
> understand how the kernel can continue to offer the old/invalid
> interface when the new _IOR macro won't accept the first version any
> longer.

Inside the kernel, the first definition has to be changed to
something like:

#define BLKGETSIZE64    _IOR(0x12,114,size_t) /* broken: actually __u64 */
or
#define BLKGETSIZE64    _IOR_BAD(0x12,114,sizeof(__uint64_t)) /* broken */

in order to get a definition that will pass the check and
generate the well-known number.

Re: [PATCH] new ioctl type checking causes gcc warning

[Kevin P. Fleming]

Arnd Bergmann wrote:
> Inside the kernel, the first definition has to be changed to
> something like:
> 
> #define BLKGETSIZE64    _IOR(0x12,114,size_t) /* broken: actually __u64 */
> or
> #define BLKGETSIZE64    _IOR_BAD(0x12,114,sizeof(__uint64_t)) /* broken */
> 
> in order to get a definition that will pass the check and
> generate the well-known number.

That's strange. I did some testing with a small application (blockdev) 
that uses this ioctl yesterday, and strace did not show any difference 
between the correct and incorrect definitions. I could change the 
definition back and forth and the application continued to work 
correctly. I'll have to go back and figure out what I was doing wrong :-)

Re: [PATCH] new ioctl type checking causes gcc warning

[Jamie Lokier]

Kevin P. Fleming wrote:
> >I had tried that first, but found that there are places that
> >use asm/ioctl.h without including asm/posix_types.h first, so 
> >size_t might not be declared. unsigned int (or unsigned long)
> >is the better alternative here. Does this look ok to everyone?
> 
> After working on this some more this afternoon, I realize now that 
> it's much better to have the typechecking in place than not, even for 
> userspace. Maybe the best solution is to still leave the typechecking 
> (don't wrap it in #ifdef __KERNEL__), and just
> 
> #ifdef size_t
> extern size_t __invalid_size_argument_for_IOC;
> #else
> extern unsigned int __invalid_size_argument_for_IOC;
> #endif

What's wrong with __typeof__(sizeof(0))?

-- Jamie