Re: [PATCH RESEND V12 2/8] fuse: 32-bit user space ioctl compat for fuse device

From: Alessio Balsini <balsini@android.com>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: Alessio Balsini <balsini@android.com>,
	Akilesh Kailash <akailash@google.com>,
	Amir Goldstein <amir73il@gmail.com>,
	Antonio SJ Musumeci <trapexit@spawn.link>,
	David Anderson <dvander@google.com>,
	Giuseppe Scrivano <gscrivan@redhat.com>,
	Jann Horn <jannh@google.com>, Jens Axboe <axboe@kernel.dk>,
	Martijn Coenen <maco@android.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Lawrence <paullawrence@google.com>,
	Peng Tao <bergwolf@gmail.com>,
	Stefano Duo <duostefano93@gmail.com>,
	Zimuzo Ezeozue <zezeozue@google.com>, wuyan <wu-yan@tcl.com>,
	fuse-devel <fuse-devel@lists.sourceforge.net>,
	kernel-team <kernel-team@android.com>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH RESEND V12 2/8] fuse: 32-bit user space ioctl compat for fuse device
Date: Mon, 1 Mar 2021 12:26:26 +0000	[thread overview]
Message-ID: <YDzdcnS1JJWboFYN@google.com> (raw)
In-Reply-To: <CAJfpegviqcgtE4qRHZFy6xdL6Re7gs30TV1epkn7cvUu3A4hqw@mail.gmail.com>

On Wed, Feb 17, 2021 at 11:21:04AM +0100, Miklos Szeredi wrote:
> On Mon, Jan 25, 2021 at 4:31 PM Alessio Balsini <balsini@android.com> wrote:
> >
> > With a 64-bit kernel build the FUSE device cannot handle ioctl requests
> > coming from 32-bit user space.
> > This is due to the ioctl command translation that generates different
> > command identifiers that thus cannot be used for direct comparisons
> > without proper manipulation.
> >
> > Explicitly extract type and number from the ioctl command to enable
> > 32-bit user space compatibility on 64-bit kernel builds.
> >
> > Signed-off-by: Alessio Balsini <balsini@android.com>
> > ---
> >  fs/fuse/dev.c             | 29 ++++++++++++++++++-----------
> >  include/uapi/linux/fuse.h |  3 ++-
> >  2 files changed, 20 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > index 588f8d1240aa..ff9f3b83f879 100644
> > --- a/fs/fuse/dev.c
> > +++ b/fs/fuse/dev.c
> > @@ -2233,37 +2233,44 @@ static int fuse_device_clone(struct fuse_conn *fc, struct file *new)
> >  static long fuse_dev_ioctl(struct file *file, unsigned int cmd,
> >                            unsigned long arg)
> >  {
> > -       int err = -ENOTTY;
> > +       int res;
> > +       int oldfd;
> > +       struct fuse_dev *fud = NULL;
> >
> > -       if (cmd == FUSE_DEV_IOC_CLONE) {
> > -               int oldfd;
> > +       if (_IOC_TYPE(cmd) != FUSE_DEV_IOC_MAGIC)
> > +               return -EINVAL;
> 
> Why change ENOTTY to EINVAL?
> 
> Thanks,
> MIklos

For the magic number mismatch I was thinking that EINVAL would have been
more appropriate, meaning: "this ioctl is definitely something this file
doesn't support".  ENOTTY, could be more specific to the subset of
ioctls supported by the file, so I kept this in the default case of the
switch.
After counting the use of ENOTTY vs EINVAL for these _IOC_TYPE checks,
EINVALs were less than half ENOTTYs, so I'm totally fine with switching
back to ENOTTY in V13.

Thanks!
Alessio