2011/4/20 Arnd Bergmann <arnd@xxxxxxxx>:
> On Wednesday 20 April 2011 21:06:49 John Calixto wrote:
>> On Wed, 20 Apr 2011, MichaÅ MirosÅaw wrote:
>> > Hmm. This might be even better:
>> >
>> > static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
>> > unsigned int cmd, unsigned long arg)
>> > {
>> > Â struct mmc_ioc_cmd blk;
>> >
>> > Â if (cmd != MMC_IOC_CMD)
>> > Â Â return -EINVAL;
>> >
>> > Â copy_from_user((void __user *)arg, &blk) ...
>> >
>> > #ifdef CONFIG_COMPAT
>> > Â if (is_compat_task())
>> > Â Â blk.data_ptr = compat_ptr(*(u32 *)&blk.data_ptr);
>> > #endif
>> >
>> > Â return mmc_blk_ioctl_cmd(bdev, &blk);
>> > }
>
> No, please don't try to invent random new ways of doing this.
> Your example relies on the assumption that the task is calling
> the entry point for its native word size. Some architectures
> intentionally allow calling the 32 bit entry point from 64 bit
> tasks and vice versa, e.g. for user space emulators converting
> to a different ABI, and in that case is_compat_task() produces
> the wrong result. Don't ever rely on that.
This doesn't make sense to me. If you call 32-bit entry point from
64-bit process, you can't reliably pass pointers through the call
(unless you limit 64-bit process to 32-bit address space).
Do you know a working example of something using this kind of cross-call?
>> I'm okay with the anon union + ``compat_ptr(*(u32 *))`` part of your
>> solution. ÂIf everyone else thinks it is reasonable, I'll submit a v7
>> with it.
> No need for a union or a ptr_size member in the struct. Just use
> a single __u64 and let the user cast the pointer to that. This
> will work on all architectures.
Union is just hiding this cast (it will be done in kernel) and allows
cleaner code for userspace (there's a single kernel and possibly
multiple applications that will implement this call).
>> However, I still think it should be implemented in compat_ioctl()
>> because compat_blkdev_ioctl() expects it. ÂEither that, or I add to the
>> big switch in compat_blkdev_driver_ioctl(), and spreading this change
>> out to block/compat_ioctl.c does not seem like The Right Thing to me.
> Yes, you definitely need to fill the .compat_ioctl member. We don't want
> new entries in the switch statement, in particular none that are specific
> to a single driver.
Hmm, you're right. fs/compat_ioctl.c falls back to plain .ioctl if
.compat_ioctl == NULL. That's not the case for block ioctls, though.
The separate compat_ioctl is the way to go.
Best Regards,
MichaÅ MirosÅaw
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
