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

[Arnd Bergmann <arnd@arndb.de>]

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