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)) ? \
"Kevin P. Fleming" <[email protected]> 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, [email protected]
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."
On Friday 12 September 2003 22:02, Andreas Schwab wrote:
> "Kevin P. Fleming" <[email protected]> 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)
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.
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 <><
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.
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.
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 :-)
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