| Message ID | 20190814204259.120942-2-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | compat_ioctl.c removal, part 2/3 | expand |
On Wed, Aug 14, 2019 at 10:42:28PM +0200, Arnd Bergmann wrote: > For 31-bit s390 user space, we have to pass pointer arguments through > compat_ptr() in the compat_ioctl handler. Seems fair enough, but... > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > fs/xfs/xfs_ioctl32.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c > index 7fcf7569743f..ad91e81a2fcf 100644 > --- a/fs/xfs/xfs_ioctl32.c > +++ b/fs/xfs/xfs_ioctl32.c > @@ -547,7 +547,7 @@ xfs_file_compat_ioctl( > struct inode *inode = file_inode(filp); > struct xfs_inode *ip = XFS_I(inode); > struct xfs_mount *mp = ip->i_mount; > - void __user *arg = (void __user *)p; > + void __user *arg = compat_ptr(p); > int error; > > trace_xfs_file_compat_ioctl(ip); > @@ -576,7 +576,7 @@ xfs_file_compat_ioctl( > case XFS_IOC_SCRUB_METADATA: > case XFS_IOC_BULKSTAT: > case XFS_IOC_INUMBERS: > - return xfs_file_ioctl(filp, cmd, p); > + return xfs_file_ioctl(filp, cmd, (unsigned long)arg); I don't really like having to sprinkle special casts through the code because of this. Perhaps do something like: static inline unsigned long compat_ptr_mask(unsigned long p) { return (unsigned long)compat_ptr(p); } and then up front you can do: void __user *arg; p = compat_ptr_mask(p); arg = (void __user *)p; and then the rest of the code remains unchanged by now uses p correctly instead of having to change all the code to cast arg back to an unsigned long... Cheers, Dave.
On Wed, Aug 14, 2019 at 11:39 PM Dave Chinner <david@fromorbit.com> wrote: > > case XFS_IOC_BULKSTAT: > > case XFS_IOC_INUMBERS: > > - return xfs_file_ioctl(filp, cmd, p); > > + return xfs_file_ioctl(filp, cmd, (unsigned long)arg); > > I don't really like having to sprinkle special casts through the > code because of this. > > Perhaps do something like: > > static inline unsigned long compat_ptr_mask(unsigned long p) > { > return (unsigned long)compat_ptr(p); > } > > and then up front you can do: > > void __user *arg; > > p = compat_ptr_mask(p); > arg = (void __user *)p; > > > and then the rest of the code remains unchanged by now uses p > correctly instead of having to change all the code to cast arg back > to an unsigned long... > In part 1 of the series, I define this function as a global: long compat_ptr_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { if (!file->f_op->unlocked_ioctl) return -ENOIOCTLCMD; return file->f_op->unlocked_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); } How about using that to replace the individual casts: - return xfs_file_ioctl(filp, cmd, (unsigned long)arg); + return compat_ptr_ioctl(filp, cmd, arg); It adds another indirection, but it avoids all the casts and uses existing mechanism. Arnd
On Thu, Aug 15, 2019 at 07:37:53AM +1000, Dave Chinner wrote: > > @@ -576,7 +576,7 @@ xfs_file_compat_ioctl( > > case XFS_IOC_SCRUB_METADATA: > > case XFS_IOC_BULKSTAT: > > case XFS_IOC_INUMBERS: > > - return xfs_file_ioctl(filp, cmd, p); > > + return xfs_file_ioctl(filp, cmd, (unsigned long)arg); > > I don't really like having to sprinkle special casts through the > code because of this. True. But the proper fix is to not do the indirection through xfs_file_ioctl but instead to call xfs_ioc_scrub_metadata, xfs_ioc_bulkstat, etc directly which all take a void __user arguments already.
On Thu, Aug 15, 2019 at 9:13 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Thu, Aug 15, 2019 at 07:37:53AM +1000, Dave Chinner wrote: > > > @@ -576,7 +576,7 @@ xfs_file_compat_ioctl( > > > case XFS_IOC_SCRUB_METADATA: > > > case XFS_IOC_BULKSTAT: > > > case XFS_IOC_INUMBERS: > > > - return xfs_file_ioctl(filp, cmd, p); > > > + return xfs_file_ioctl(filp, cmd, (unsigned long)arg); > > > > I don't really like having to sprinkle special casts through the > > code because of this. > > True. But the proper fix is to not do the indirection through > xfs_file_ioctl but instead to call xfs_ioc_scrub_metadata, > xfs_ioc_bulkstat, etc directly which all take a void __user > arguments already. I'm not sure that's better: This would end up duplicating all of xfs_file_ioctl(), which is already a fairly long function, compared to the current way of having a large set of commands all handled with a single line. From looking at other subsystems, what I find to work best is to move the compat handler into the same file as the native code and then structure the files so that shared handlers get put into one place, something like /* these are the ones that have the same ABI for 32-bit and 64-bit tasks */ static int xfs_compatible_file_ioctl(struct file *filp, unsigned cmd, void __user *p) { int ret = -ENOIOCTLCMD; switch (cmd) { case XFS_IOC_DIOINFO: ... case ... } return ret; } long xfs_file_compat_ioctl( struct file *filp, unsigned cmd, unsigned long p) { ret = xfs_compatible_file_ioctl(filp, cmd, compat_ptr(p)); if (ret != -ENOIOCTLCMD) return ret; /* all incompatible ones below */ switch (cmd) { ... } } Having them in one place makes it more obvious to readers how the native and compat handlers fit together, and makes it easier to keep the two in sync. That would of course be a much larger change to how it's done today, and it's way out of scope of what I want to achieve in my (already too long) series. Arnd
In many ways I'd actually much rather have a table driven approach. Let me try something..
On Thu, Aug 15, 2019 at 01:02:11AM -0700, Christoph Hellwig wrote: > In many ways I'd actually much rather have a table driven approach. > Let me try something.. Ok, it seems like we don't even need a table containing native and compat as we can just fall back. The tables still seem nicer to read, though. Let me know what you think of this: http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-ioctl-table I also wonder if we should life the ioctl handler tables to the VFS..
On Thu, Aug 15, 2019 at 12:26 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Thu, Aug 15, 2019 at 01:02:11AM -0700, Christoph Hellwig wrote: > > In many ways I'd actually much rather have a table driven approach. > > Let me try something.. > > Ok, it seems like we don't even need a table containing native and > compat as we can just fall back. The tables still seem nicer to read, > though. > > Let me know what you think of this: > > http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-ioctl-table These all look like useful cleanups, but I'm a little worried about introducing merge conflicts with my own patches. I would want to have my series get merged as a complete branch since each patch that removes a bit of fs/compat_ioctl.c would clash with a patch removing the adjacent bits otherwise. I still haven't heard from Al regarding what he thinks of my v5 series. If he wants me to send a pull request for it, I can of course add in your patches after they are fully reviewed. > I also wonder if we should life the ioctl handler tables to the VFS. The idea of these tables has come up a few times in the past, and there are a couple of subsystems that have something like it, e.g. drivers/media. Usually you'd want to combine the table with a more generic way to do the copy_from_user()/copy_to_user() on the argument, but that in turn requires all commands to be defined correctly (a lot of drivers have some commands that specify the wrong direction or the wrong size, or one that predates the _IO() macro). What I could imaging having in the long run is to have the ioctl table attached to the file_operations structure, and then define it in a way that handles at least the more common variations: - copy_from_user to stack, pass a kernel pointer to handler - a single entry for commands that are 32/64-bit compatible - entries that are only used for native vs compat mode if they have incompatible arguments (this could also be handled by calling in_compat_syscall() in the handler itself). - a flag to specify handlers that require the __user pointer instead of the implied copy. Doing this right will certainly require several revisions of patch series and lots of discussions, and is unrelated to the removal of fs/compat_ioctl.c, so I'd much prefer to get this series merged before we start working on that. Arnd
On Thu, Aug 15, 2019 at 03:26:49AM -0700, Christoph Hellwig wrote: > On Thu, Aug 15, 2019 at 01:02:11AM -0700, Christoph Hellwig wrote: > > In many ways I'd actually much rather have a table driven approach. > > Let me try something.. > > Ok, it seems like we don't even need a table containing native and > compat as we can just fall back. The tables still seem nicer to read, > though. > > Let me know what you think of this: > > http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-ioctl-table Lots to like in that handful of patches. :) It can easily go before or after Arnd's patch, and the merge conflict either way would be minor, so I'm not really fussed either way this gets sorted out... Cheers, Dave.
On Thu, Aug 15, 2019 at 10:15:12PM +1000, Dave Chinner wrote: > > http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-ioctl-table > > Lots to like in that handful of patches. :) > > It can easily go before or after Arnd's patch, and the merge > conflict either way would be minor, so I'm not really fussed either > way this gets sorted out... The other thing we could do is to just pick the two important ones: http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-ioctl-table-5.3 and throw that into Arnds series, or even 5.3, and then defer the table thing until later.
On Thu, Aug 15, 2019 at 4:04 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Thu, Aug 15, 2019 at 10:15:12PM +1000, Dave Chinner wrote: > > > http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-ioctl-table > > > > Lots to like in that handful of patches. :) > > > > It can easily go before or after Arnd's patch, and the merge > > conflict either way would be minor, so I'm not really fussed either > > way this gets sorted out... > > The other thing we could do is to just pick the two important ones: > > http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-ioctl-table-5.3 > > and throw that into Arnds series, or even 5.3, and then defer the > table thing until later. If we can have your "xfs: fall back to native ioctls for unhandled compat ones" in 5.3, that would be ideal from my side, then I can just drop the corresponding patch from my series and have the rest merged for 5.4. The compat_ptr addition is independent of my series, I just added it because I noticed it was missing, so we can merged that through the xfs tree along with your other changes, either for 5.3 or 5.4. Arnd
On Thu, Aug 15, 2019 at 09:20:32PM +0200, Arnd Bergmann wrote: > On Thu, Aug 15, 2019 at 4:04 PM Christoph Hellwig <hch@infradead.org> wrote: > > > > On Thu, Aug 15, 2019 at 10:15:12PM +1000, Dave Chinner wrote: > > > > http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-ioctl-table > > > > > > Lots to like in that handful of patches. :) > > > > > > It can easily go before or after Arnd's patch, and the merge > > > conflict either way would be minor, so I'm not really fussed either > > > way this gets sorted out... > > > > The other thing we could do is to just pick the two important ones: > > > > http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-ioctl-table-5.3 > > > > and throw that into Arnds series, or even 5.3, and then defer the > > table thing until later. > > If we can have your "xfs: fall back to native ioctls for unhandled compat > ones" in 5.3, that would be ideal from my side, then I can just drop the > corresponding patch from my series and have the rest merged for 5.4. > > The compat_ptr addition is independent of my series, I just added it > because I noticed it was missing, so we can merged that through > the xfs tree along with your other changes, either for 5.3 or 5.4. Er... do the two patches in the -5.3 branch actually fix something that's broken? I sense s390 is missing a pointer sanitization check or something...? --D > Arnd
On Thu, Aug 15, 2019 at 9:28 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Thu, Aug 15, 2019 at 09:20:32PM +0200, Arnd Bergmann wrote: > > On Thu, Aug 15, 2019 at 4:04 PM Christoph Hellwig <hch@infradead.org> wrote: > > > > > > On Thu, Aug 15, 2019 at 10:15:12PM +1000, Dave Chinner wrote: > > > > > http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-ioctl-table > > > > > > > > Lots to like in that handful of patches. :) > > > > > > > > It can easily go before or after Arnd's patch, and the merge > > > > conflict either way would be minor, so I'm not really fussed either > > > > way this gets sorted out... > > > > > > The other thing we could do is to just pick the two important ones: > > > > > > http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-ioctl-table-5.3 > > > > > > and throw that into Arnds series, or even 5.3, and then defer the > > > table thing until later. > > > > If we can have your "xfs: fall back to native ioctls for unhandled compat > > ones" in 5.3, that would be ideal from my side, then I can just drop the > > corresponding patch from my series and have the rest merged for 5.4. > > > > The compat_ptr addition is independent of my series, I just added it > > because I noticed it was missing, so we can merged that through > > the xfs tree along with your other changes, either for 5.3 or 5.4. > > Er... do the two patches in the -5.3 branch actually fix something > that's broken? I sense s390 is missing a pointer sanitization check or > something...? s390 is indeed missing the pointer conversion, the other patch adds compat ioctl support for FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL, which were missing, and it ensures that FITRIM keeps working after I remove it from the list in fs/compat_ioctl.c Arnd
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c index 7fcf7569743f..ad91e81a2fcf 100644 --- a/fs/xfs/xfs_ioctl32.c +++ b/fs/xfs/xfs_ioctl32.c @@ -547,7 +547,7 @@ xfs_file_compat_ioctl( struct inode *inode = file_inode(filp); struct xfs_inode *ip = XFS_I(inode); struct xfs_mount *mp = ip->i_mount; - void __user *arg = (void __user *)p; + void __user *arg = compat_ptr(p); int error; trace_xfs_file_compat_ioctl(ip); @@ -576,7 +576,7 @@ xfs_file_compat_ioctl( case XFS_IOC_SCRUB_METADATA: case XFS_IOC_BULKSTAT: case XFS_IOC_INUMBERS: - return xfs_file_ioctl(filp, cmd, p); + return xfs_file_ioctl(filp, cmd, (unsigned long)arg); #if !defined(BROKEN_X86_ALIGNMENT) || defined(CONFIG_X86_X32) /* * These are handled fine if no alignment issues. To support x32 @@ -602,7 +602,7 @@ xfs_file_compat_ioctl( */ case XFS_IOC_SWAPEXT: #endif - return xfs_file_ioctl(filp, cmd, p); + return xfs_file_ioctl(filp, cmd, (unsigned long)arg); #endif #if defined(BROKEN_X86_ALIGNMENT) case XFS_IOC_ALLOCSP_32: @@ -653,7 +653,7 @@ xfs_file_compat_ioctl( case XFS_IOC_SETXFLAGS_32: case XFS_IOC_GETVERSION_32: cmd = _NATIVE_IOC(cmd, long); - return xfs_file_ioctl(filp, cmd, p); + return xfs_file_ioctl(filp, cmd, (unsigned long)arg); case XFS_IOC_SWAPEXT_32: { struct xfs_swapext sxp; struct compat_xfs_swapext __user *sxu = arg;
For 31-bit s390 user space, we have to pass pointer arguments through compat_ptr() in the compat_ioctl handler. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- fs/xfs/xfs_ioctl32.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)