[PATCH v2 05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg
jgg at ziepe.ca
Mon Sep 24 20:35:05 UTC 2018
On Mon, Sep 24, 2018 at 10:18:52PM +0200, Arnd Bergmann wrote:
> On Tue, Sep 18, 2018 at 7:59 PM Jason Gunthorpe <jgg at ziepe.ca> wrote:
> > On Tue, Sep 18, 2018 at 10:51:08AM -0700, Darren Hart wrote:
> > > On Fri, Sep 14, 2018 at 09:57:48PM +0100, Al Viro wrote:
> > > > On Fri, Sep 14, 2018 at 01:35:06PM -0700, Darren Hart wrote:
> > > >
> > > > > Acked-by: Darren Hart (VMware) <dvhart at 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
That could work well, but the first idea could be done globally and
mechanically, while this would require very careful per-driver
Particularly if the core code has worse performance.. ie due to
kmalloc calls or something.
I think it would make more sense to start by having the core do the
case to __user and then add another entry point to have the core do
the copy_from_user, and so on.
More information about the dri-devel