Re: ioctl get_info: Bad address
Brought to you by:
aeb,
bencollins
From: Stefan R. <st...@s5...> - 2011-08-09 20:17:39
|
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. Gcc C isn't ANSI C of course. Their site says: "A cast from pointer to integer discards most-significant bits if the pointer representation is larger than the integer type, sign-extends¹ if the pointer representation is smaller than the integer type, otherwise the bits are unchanged." "¹) Future versions of GCC may zero-extend, or use a target-defined ptr_extend pattern. Do not rely on sign extension." http://gcc.gnu.org/onlinedocs/gcc/Arrays-and-pointers-implementation.html I wonder whether C90/C99 turned the ANSI way around or made it an implementation specific thing. 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. By sheer luck or maybe because I was warned by gcc on x86-32, the simple C examples in http://user.in-berlin.de/~s5r6/linux1394/utils/ cast to unsigned long first, too. > 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. Let's pretend for a minute that we do care: --- drivers/firewire/core-cdev.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -214,19 +214,37 @@ struct outbound_phy_packet_event { struct inbound_phy_packet_event { struct event event; struct fw_cdev_event_phy_packet phy_packet; }; +#ifdef CONFIG_COMPAT +static inline void __user *u64_to_uptr(__u64 value) +{ + if (is_compat_task()) + return compat_ptr(value); + else + return (void __user *)(unsigned long)value; +} + +static inline __u64 uptr_to_u64(void __user *ptr) +{ + if (is_compat_task()) + return ptr_to_compat(ptr); + else + return (__u64)(unsigned long)ptr; +} +#else static inline void __user *u64_to_uptr(__u64 value) { return (void __user *)(unsigned long)value; } static inline __u64 uptr_to_u64(void __user *ptr) { return (__u64)(unsigned long)ptr; } +#endif /* CONFIG_COMPAT */ static int fw_device_op_open(struct inode *inode, struct file *file) { struct fw_device *device; struct client *client; gcc 4.4.5 x86-64 makes this of it: size before size after delta --------------------------------------------------------------- CONFIG_CC_OPTIMIZE_FOR_SIZE=y core-cdev.s 342946 352407 +9461 core-cdev.o 126544 130600 +4056 firewire-core.ko 454874 458930 +4056 --------------------------------------------------------------- CONFIG_CC_OPTIMIZE_FOR_SIZE=n core-cdev.s 369209 379158 +9949 core-cdev.o 133488 137616 +4128 firewire-core.ko 481898 486002 +4104 Is it worthwhile? -- Stefan Richter -=====-==-== =--- -=--= http://arcgraph.de/sr/ |