From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Gunthorpe Subject: Re: [PATCH v2 05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg Date: Mon, 24 Sep 2018 14:35:05 -0600 Message-ID: <20180924203505.GC6008@ziepe.ca> References: <20180912150142.157913-1-arnd@arndb.de> <20180912151134.436719-1-arnd@arndb.de> <20180914203506.GE35251@wrath> <20180914205748.GC19965@ZenIV.linux.org.uk> <20180918175108.GF35251@wrath> <20180918175952.GJ11367@ziepe.ca> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Arnd Bergmann Cc: Darren Hart , Al Viro , Linux FS-devel Mailing List , gregkh , David Miller , driverdevel , Linux Kernel Mailing List , qat-linux@intel.com, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , Linux Media Mailing List , dri-devel , linaro-mm-sig@lists.linaro.org, amd-gfx@lists.freedesktop.org, "open list:HID CORE LAYER" , linux-iio@vger.kernel.org, linux-rdma , linux-nvdimm@lists.01.org, linux-nvme@lists.infradead.org, linux-pci List-Id: amd-gfx.lists.freedesktop.org On Mon, Sep 24, 2018 at 10:18:52PM +0200, Arnd Bergmann wrote: > On Tue, Sep 18, 2018 at 7:59 PM Jason Gunthorpe 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) > > > > > > > > > > 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. That could work well, but the first idea could be done globally and mechanically, while this would require very careful per-driver investigation. 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. Jason From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-f66.google.com ([209.85.166.66]:46764 "EHLO mail-io1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727756AbeIYCjG (ORCPT ); Mon, 24 Sep 2018 22:39:06 -0400 Received: by mail-io1-f66.google.com with SMTP id y12-v6so18685712ioj.13 for ; Mon, 24 Sep 2018 13:35:07 -0700 (PDT) Date: Mon, 24 Sep 2018 14:35:05 -0600 From: Jason Gunthorpe To: Arnd Bergmann Cc: Darren Hart , Al Viro , Linux FS-devel Mailing List , gregkh , David Miller , driverdevel , Linux Kernel Mailing List , qat-linux@intel.com, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , Linux Media Mailing List , dri-devel , linaro-mm-sig@lists.linaro.org, amd-gfx@lists.freedesktop.org, "open list:HID CORE LAYER" , linux-iio@vger.kernel.org, linux-rdma , linux-nvdimm@lists.01.org, linux-nvme@lists.infradead.org, linux-pci , Platform Driver , linux-remoteproc@vger.kernel.org, sparclinux , linux-scsi , USB list , linux-fbdev@vger.kernel.org, linuxppc-dev , linux-btrfs , ceph-devel , linux-wireless , Networking Subject: Re: [PATCH v2 05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg Message-ID: <20180924203505.GC6008@ziepe.ca> References: <20180912150142.157913-1-arnd@arndb.de> <20180912151134.436719-1-arnd@arndb.de> <20180914203506.GE35251@wrath> <20180914205748.GC19965@ZenIV.linux.org.uk> <20180918175108.GF35251@wrath> <20180918175952.GJ11367@ziepe.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: Sender: linux-btrfs-owner@vger.kernel.org List-ID: On Mon, Sep 24, 2018 at 10:18:52PM +0200, Arnd Bergmann wrote: > On Tue, Sep 18, 2018 at 7:59 PM Jason Gunthorpe 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) > > > > > > > > > > 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. That could work well, but the first idea could be done globally and mechanically, while this would require very careful per-driver investigation. 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. Jason From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Gunthorpe Date: Mon, 24 Sep 2018 20:35:05 +0000 Subject: Re: [PATCH v2 05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg Message-Id: <20180924203505.GC6008@ziepe.ca> List-Id: References: <20180912150142.157913-1-arnd@arndb.de> <20180912151134.436719-1-arnd@arndb.de> <20180914203506.GE35251@wrath> <20180914205748.GC19965@ZenIV.linux.org.uk> <20180918175108.GF35251@wrath> <20180918175952.GJ11367@ziepe.ca> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Arnd Bergmann Cc: Darren Hart , Al Viro , Linux FS-devel Mailing List , gregkh , David Miller , driverdevel , Linux Kernel Mailing List , qat-linux@intel.com, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , Linux Media Mailing List , dri-devel , linaro-mm-sig@lists.linaro.org, amd-gfx@lists.freedesktop.org, "open list:HID CORE LAYER" , linux-iio@vger.kernel.org, linux-rdma , linux-nvdimm@lists.01.org, linux-nvme@lists.infradead.org, linux-pci On Mon, Sep 24, 2018 at 10:18:52PM +0200, Arnd Bergmann wrote: > On Tue, Sep 18, 2018 at 7:59 PM Jason Gunthorpe 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) > > > > > > > > > > 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. That could work well, but the first idea could be done globally and mechanically, while this would require very careful per-driver investigation. 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. Jason From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v2,05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg From: Jason Gunthorpe Message-Id: <20180924203505.GC6008@ziepe.ca> Date: Mon, 24 Sep 2018 14:35:05 -0600 To: Arnd Bergmann Cc: Darren Hart , Al Viro , Linux FS-devel Mailing List , gregkh , David Miller , driverdevel , Linux Kernel Mailing List , qat-linux@intel.com, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , Linux Media Mailing List , dri-devel , linaro-mm-sig@lists.linaro.org, amd-gfx@lists.freedesktop.org, "open list:HID CORE LAYER" , linux-iio@vger.kernel.org, linux-rdma , linux-nvdimm@lists.01.org, linux-nvme@lists.infradead.org, linux-pci , Platform Driver , linux-remoteproc@vger.kernel.org, sparclinux , linux-scsi , USB list , linux-fbdev@vger.kernel.org, linuxppc-dev , linux-btrfs , ceph-devel , linux-wireless , Networking List-ID: T24gTW9uLCBTZXAgMjQsIDIwMTggYXQgMTA6MTg6NTJQTSArMDIwMCwgQXJuZCBCZXJnbWFubiB3 cm90ZToKPiBPbiBUdWUsIFNlcCAxOCwgMjAxOCBhdCA3OjU5IFBNIEphc29uIEd1bnRob3JwZSA8 amdnQHppZXBlLmNhPiB3cm90ZToKPiA+Cj4gPiBPbiBUdWUsIFNlcCAxOCwgMjAxOCBhdCAxMDo1 MTowOEFNIC0wNzAwLCBEYXJyZW4gSGFydCB3cm90ZToKPiA+ID4gT24gRnJpLCBTZXAgMTQsIDIw MTggYXQgMDk6NTc6NDhQTSArMDEwMCwgQWwgVmlybyB3cm90ZToKPiA+ID4gPiBPbiBGcmksIFNl cCAxNCwgMjAxOCBhdCAwMTozNTowNlBNIC0wNzAwLCBEYXJyZW4gSGFydCB3cm90ZToKPiA+ID4g Pgo+ID4gPiA+ID4gQWNrZWQtYnk6IERhcnJlbiBIYXJ0IChWTXdhcmUpIDxkdmhhcnRAaW5mcmFk ZWFkLm9yZz4KPiA+ID4gPiA+Cj4gPiA+ID4gPiBBcyBmb3IgYSBsb25nZXIgdGVybSBzb2x1dGlv biwgd291bGQgaXQgYmUgcG9zc2libGUgdG8gaW5pdCBmb3BzIGluIHN1Y2gKPiA+ID4gPiA+IGEg d2F5IHRoYXQgdGhlIGNvbXBhdF9pb2N0bCBjYWxsIGRlZmF1bHRzIHRvIGdlbmVyaWNfY29tcGF0 X2lvY3RsX3B0cmFyZwo+ID4gPiA+ID4gc28gd2UgZG9uJ3QgaGF2ZSB0byBkdXBsaWNhdGUgdGhp cyBib2lsZXJwbGF0ZSBmb3IgZXZlcnkgaW9jdGwgZm9wcwo+ID4gPiA+ID4gc3RydWN0dXJlPwo+ ID4gPiA+Cj4gPiA+ID4gICAgIEJhZCBpZGVhLCB0aGF0Li4uICBCZWNhdXNlIHNldmVyYWwgeWVh cnMgZG93biB0aGUgcm9hZCBzb21lYm9keSB3aWxsIGFkZAo+ID4gPiA+IGFuIGlvY3RsIHRoYXQg dGFrZXMgYW4gdW5zaWduZWQgaW50IGZvciBhcmd1bWVudC4gIFdpdGhvdXQgc28gbXVjaCBhcyBs b29raW5nCj4gPiA+ID4gYXQgeW91ciBtYWdpY2FsIG15c3RlcnkgbWFjcm8gYmVpbmcgdXNlZCB0 byBpbml0aWFsaXplIGZpbGVfb3BlcmF0aW9ucy4KPiA+ID4KPiA+ID4gRmFpciwgYmVpbmcgZXhw bGljaXQgaW4gdGhlIGRlY2xhcmF0aW9uIGFzIGl0IGlzIGN1cnJlbnRseSBtYXkgYmUKPiA+ID4g cHJlZmVyYWJsZSB0aGVuLgo+ID4KPiA+IEl0IHdvdWxkIGJlIG11Y2ggY2xlYW5lciBhbmQgc2Fm ZXIgaWYgeW91IGNvdWxkIGFycmFuZ2UgdGhpbmdzIHRvIGFkZAo+ID4gc29tZXRoaW5nIGxpa2Ug dGhpcyB0byBzdHJ1Y3QgZmlsZV9vcGVyYXRpb25zOgo+ID4KPiA+ICAgbG9uZyAoKnB0cl9pb2N0 bCkgKHN0cnVjdCBmaWxlICosIHVuc2lnbmVkIGludCwgdm9pZCBfX3VzZXIgKik7Cj4gPgo+ID4g V2hlcmUgdGhlIGNvcmUgY29kZSBhdXRvbWF0aWNhbGx5IGNvbnZlcnRzIHRoZSB1bnNpZ25lZCBs b25nIHRvIHRoZQo+ID4gdm9pZCBfX3VzZXIgKiBhcyBhcHByb3ByaWF0ZS4KPiA+Cj4gPiBUaGVu IGl0IGp1c3Qgd29ya3MgcmlnaHQgYWx3YXlzIGFuZCB0aGUgY29tcGlsZXIgd2lsbCBoZWxwIGFk ZHJlc3MKPiA+IEFsJ3MgY29uY2VybiBkb3duIHRoZSByb2FkLgo+IAo+IEkgdGhpbmsgaWYgd2Ug d2FudGVkIHRvIGRvIHRoaXMgd2l0aCBhIG5ldyBmaWxlIG9wZXJhdGlvbiwgdGhlIGJlc3QKPiB3 YXkgd291bGQgYmUgdG8gZG8gdGhlIGNvcHlfZnJvbV91c2VyKCkvY29weV90b191c2VyKCkgaW4g dGhlIGNhbGxlcgo+IGFzIHdlbGwuCj4KPiBXZSBhbHJlYWR5IGRvIHRoaXMgaW5zaWRlIG9mIHNv bWUgc3Vic3lzdGVtcywgbm90YWJseSBkcml2ZXJzL21lZGlhLywKPiBhbmQgaXQgc2ltcGxpZmll cyB0aGUgaW1wbGVtZW50YXRpb24gb2YgdGhlIGlvY3RsIGhhbmRsZXIgZnVuY3Rpb24KPiBzaWdu aWZpY2FudGx5LiBXZSBvYnZpb3VzbHkgY2Fubm90IGRvIHRoaXMgaW4gZ2VuZXJhbCwgYm90aCBi ZWNhdXNlIG9mCj4gdHJhZGl0aW9uYWwgZHJpdmVycyB0aGF0IGhhdmUgMTYtYml0IGNvbW1hbmQg Y29kZXMgKGRyaXZlcnMvdHR5IGFuZCBvdGhlcnMpCj4gYW5kIGFsc28gYmVjYXVzZSBvZiBkcml2 ZXJzIHRoYXQgYnkgYWNjaWRlbnQgZGVmaW5lZCB0aGUgY29tbWFuZHMKPiBpbmNvcnJlY3RseSBh bmQgdXNlIHRoZSB3cm9uZyB0eXBlIG9yIHRoZSB3cm9uZyBkaXJlY3Rpb24gaW4gdGhlCj4gZGVm aW5pdGlvbi4KClRoYXQgY291bGQgd29yayB3ZWxsLCBidXQgdGhlIGZpcnN0IGlkZWEgY291bGQg YmUgZG9uZSBnbG9iYWxseSBhbmQKbWVjaGFuaWNhbGx5LCB3aGlsZSB0aGlzIHdvdWxkIHJlcXVp cmUgdmVyeSBjYXJlZnVsIHBlci1kcml2ZXIKaW52ZXN0aWdhdGlvbi4gCgpQYXJ0aWN1bGFybHkg aWYgdGhlIGNvcmUgY29kZSBoYXMgd29yc2UgcGVyZm9ybWFuY2UuLiBpZSBkdWUgdG8Ka21hbGxv YyBjYWxscyBvciBzb21ldGhpbmcuCgpJIHRoaW5rIGl0IHdvdWxkIG1ha2UgbW9yZSBzZW5zZSB0 byBzdGFydCBieSBoYXZpbmcgdGhlIGNvcmUgZG8gdGhlCmNhc2UgdG8gX191c2VyIGFuZCB0aGVu IGFkZCBhbm90aGVyIGVudHJ5IHBvaW50IHRvIGhhdmUgdGhlIGNvcmUgZG8KdGhlIGNvcHlfZnJv bV91c2VyLCBhbmQgc28gb24uCgpKYXNvbgo=