Re: ioctl get_info: Bad address
Brought to you by:
aeb,
bencollins
From: Clemens L. <cl...@la...> - 2011-08-10 07:19:21
|
Stefan Richter wrote: > On Aug 09 Clemens Ladisch wrote: > > I wrote: > > > get_info.bus_reset = (u64)&bus_reset; > > > > Uh-oh. 32-bit gcc 4.4.4 says "cast from pointer to integer of different > > size" and sign-extends the value. > > Hm, an old C book here about ANSI C says that your code is correct, > since the compiler would first cast the pointer into an unsigned integer > of the same size as the pointer's, then go on with integer-to-integer > conversion to whatever target type you have. > [...] > I wonder whether C90/C99 turned the ANSI way around or made it an > implementation specific thing. Both ISO C90 and C99 are identical with the respective ANSI standards. latest C99 draft: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf 6.3.2.3 6: | Any pointer type may be converted to an integer type. | Except [for a null pointer], the result is implementation-defined. C90 draft (third public review): http://flash-gordon.me.uk/ansi.c.txt 3.3.4: | A pointer may be converted to an integral type. The size of integer | required and the result are implementation-defined. [...] | An arbitrary integer may be converted to a pointer. The result is | implementation-defined./37/ | | 37. The mapping functions for converting a pointer to an integer or | an integer to a pointer are intended to be consistent with the | addressing structure of the execution environment. C90 Rationale for 3.3.4: | Nothing portable can be said about casting integers to pointers, or | vice versa, since the two are now incommensurate. | The definition of these conversions adopted in the Standard resembles | that in the Base Document, but with several significant differences. | The Base Document required that a pointer successfully converted to | an integer must be guaranteed to be convertible back to the same | pointer. This integer-to-pointer conversion is now specified as | implementation-defined. While a high-quality implementation would | preserve the same address value whenever possible, it was considered | impractical to require that the identical representation be preserved. | The Committee noted that, on some current machine implementations, | identical representations are required for efficient code generation | for pointer comparisons and arithmetic operations. (The "Base Document" is Ritchie's "The C Reference Manual", which was also published as appendix A of the old K&R.) > Anyhow. Perhaps not entirely by accident, Kristian for example put the > following into libraw1394's fw-cdev backend: > > #define ptr_to_u64(p) ((__u64)(unsigned long)(p)) > #define u64_to_ptr(p) ((void *)(unsigned long)(p)) > > Which looks good to me. Because in Linux, pointers are guaranteed to fit into unsigned long. C99 has uintptr_t for this (but says it's optional because such an integer type might not exist). > > firewire-cdev.c's u64_to_uptr() differs from compat_ptr() in that the > > latter correctly masks the source pointer to 32 bits. > > > > This is definitely a bug in async-test.c, but shouldn't the kernel use > > compat_ptr() for all pointers when is_compat_task()? u64_to_uptr() > > will definitely be wrong on s390 and tile (not that we care about those). > > Indeed we don't care about s390; they don't have PCI and we don't have any > non-PCI controller driver. > > Tile does support PCI (Express), but the userbase of FireWire on Tile (&& > 32 bit userland on 64 bit kernel) has silently put up with our bugs so far. arch/tile/Kconfig: | # Please note: TILE-Gx support is not yet finalized; this is | # the preliminary support. TILE-Gx drivers are only provided | # with the alpha or beta test versions for Tilera customers. | config TILEGX | depends on EXPERIMENTAL | bool "Building with TILE-Gx (64-bit) compiler and toolchain" > Let's pretend for a minute that we do care: > ... > +#ifdef CONFIG_COMPAT > +static inline void __user *u64_to_uptr(__u64 value) Nitpick: __u64 is meant to be used (only) in userspace-visible kernel headers, because u64 is not a reserved identifier in C9x. Kernel code can use u64. > +{ > + if (is_compat_task()) > + return compat_ptr(value); > ... > > gcc 4.4.5 x86-64 makes this of it: > > size before size after delta > --------------------------------------------------------------- > core-cdev.s 342946 352407 +9461 > core-cdev.o 126544 130600 +4056 Is this with debug information? Because I cannot see how this many bytes could be actual code, even with inlining (is_compat_task()'s current_thread_info() call should end up as one pointer read). So I'd guess the actual cost (.code section) is not nearly that high. > Is it worthwhile? Arch-independent code should not depend on knowledge of compat_ptr()'s implementation details. Regards, Clemens |