diff mbox series

[v5,01/18] xfs: compat_ioctl: use compat_ptr()

Message ID 20190814204259.120942-2-arnd@arndb.de (mailing list archive)
State New, archived
Headers show
Series compat_ioctl.c removal, part 2/3 | expand

Commit Message

Arnd Bergmann Aug. 14, 2019, 8:42 p.m. UTC
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(-)

Comments

Dave Chinner Aug. 14, 2019, 9:37 p.m. UTC | #1
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.
Arnd Bergmann Aug. 15, 2019, 6:43 a.m. UTC | #2
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
Christoph Hellwig Aug. 15, 2019, 7:13 a.m. UTC | #3
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.
Arnd Bergmann Aug. 15, 2019, 7:56 a.m. UTC | #4
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
Christoph Hellwig Aug. 15, 2019, 8:02 a.m. UTC | #5
In many ways I'd actually much rather have a table driven approach.
Let me try something..
Christoph Hellwig Aug. 15, 2019, 10:26 a.m. UTC | #6
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..
Arnd Bergmann Aug. 15, 2019, 11:02 a.m. UTC | #7
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
Dave Chinner Aug. 15, 2019, 12:15 p.m. UTC | #8
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.
Christoph Hellwig Aug. 15, 2019, 2:03 p.m. UTC | #9
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.
Arnd Bergmann Aug. 15, 2019, 7:20 p.m. UTC | #10
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
Darrick J. Wong Aug. 15, 2019, 7:28 p.m. UTC | #11
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
Arnd Bergmann Aug. 15, 2019, 7:46 p.m. UTC | #12
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 mbox series

Patch

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;