On Tue, Sep 18, 2018 at 7:59 PM Jason Gunthorpe [off-list ref] wrote:

On Tue, Sep 18, 2018 at 10:51:08AM -0700, Darren Hart wrote:

quoted

On Fri, Sep 14, 2018 at 09:57:48PM +0100, Al Viro wrote:

quoted

On Fri, Sep 14, 2018 at 01:35:06PM -0700, Darren Hart wrote:

quoted

Acked-by: Darren Hart (VMware) <dvhart@infradead.org>

As for a longer term solution, would it be possible to init fops in such
a way that the compat_ioctl call defaults to generic_compat_ioctl_ptrarg
so we don't have to duplicate this boilerplate for every ioctl fops
structure?

    Bad idea, that...  Because several years down the road somebody will add
an ioctl that takes an unsigned int for argument.  Without so much as looking
at your magical mystery macro being used to initialize file_operations.

Fair, being explicit in the declaration as it is currently may be
preferable then.

It would be much cleaner and safer if you could arrange things to add
something like this to struct file_operations:

  long (*ptr_ioctl) (struct file *, unsigned int, void __user *);

Where the core code automatically converts the unsigned long to the
void __user * as appropriate.

Then it just works right always and the compiler will help address
Al's concern down the road.

I think if we wanted to do this with a new file operation, the best
way would be to do the copy_from_user()/copy_to_user() in the caller
as well.

We already do this inside of some subsystems, notably drivers/media/,
and it simplifies the implementation of the ioctl handler function
significantly. We obviously cannot do this in general, both because of
traditional drivers that have 16-bit command codes (drivers/tty and others)
and also because of drivers that by accident defined the commands
incorrectly and use the wrong type or the wrong direction in the
definition.

       Arnd