diff mbox series

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

Message ID 20210125153057.3623715-3-balsini@android.com (mailing list archive)
State New
Headers show
Series fuse: Add support for passthrough read/write | expand

Commit Message

Alessio Balsini Jan. 25, 2021, 3:30 p.m. UTC
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(-)

Comments

Alessio Balsini Jan. 27, 2021, 1:40 p.m. UTC | #1
On Wed, Jan 27, 2021 at 08:15:19PM +0800, qxy wrote:
> Hi Allesio,
> 
> Thank you for your supply for 32-bit user space.
> Actually we have already met this issue on our product and we resolved it
> like this:
> 
> Project *platform/external/libfuse*
> diff --git a/include/fuse_kernel.h b/include/fuse_kernel.h
> index e9d4f1a..fe0cb6d 100644
> --- a/include/fuse_kernel.h
> +++ b/include/fuse_kernel.h
> @@ -562,7 +562,11 @@
>          uint32_t fd;
>          /* For future implementation */
>          uint32_t len;
> -        void *   vec;
> +        union {
> +                void *   vec;
> +                /* compatible for 32-bit libfuse and 64-bit kernel */
> +                uint64_t padding;
> +        };
>  };
> 
> However, if we need to use *vec in the future,  we still need to do more in
> 32-bit libfuse to work with 64-bit kernel :(
> 

Good point.
What I had in mind as a possible use was the possibility of enabling
passthrough for only a few regions of the file, similar to fuse2.
But it doesn't really make sense to define the data structure fields for
uncertain future extensions, so what we could do is:

struct fuse_passthrough_out {
+	uint32_t	size;	// Size of this data structure
	uint32_t	fd;
-	/* For future implementation */
-	uint32_t	len;
-	void		*vec;
};

Similar to what "struct sched_attr" does.
This is probably the most flexible solution, that would allow to extend
this data structure in the future with no headaches both in kernel and
user space.
What do you think?

Thanks!
Alessio
Alessio Balsini Jan. 28, 2021, 2:15 p.m. UTC | #2
Hi all,

I'm more than happy to change the interface into something that is
objectively better and accepted by everyone.
I would really love to reach the point at which we have a "stable-ish"
UAPI as soon as possible.

I've been thinking about a few possible approaches to fix the issue, yet
to preserve its flexibility. These are mentioned below.


  Solution 1: Size

As mentioned in my previous email, one solution could be to introduce
the "size" field to allow the structure to grow in the future.

struct fuse_passthrough_out {
    uint32_t        size;   // Size of this data structure
    uint32_t        fd;
};

The problem here is that we are making the promise that all the upcoming
fields are going to be maintained forever and at the offsets they were
originally defined.


  Solution 2: Version

Another solution could be to s/size/version, where for every version of
FUSE passthrough we reserve the right to modifying the fields over time,
casting them to the right data structure according to the version.


  Solution 3: Type

Using an enumerator to define the data structure content and purpose is
the most flexible solution I can think of.  This would for example allow
us to substitute FUSE_DEV_IOC_PASSTHROUGH_OPEN with the generic
FUSE_DEV_IOC_PASSTHROUGH and having a single ioctl for any eventually
upcoming passthrough requests.

enum fuse_passthrough_type {
    FUSE_PASSTHROUGH_OPEN
};

struct fuse_passthrough_out {
    uint32_t type; /* as defined by enum fuse_passthrough_type */
    union {
        uint32_t fd;
    };
};

This last is my favorite, as regardless the minimal logic required to
detect the size and content of the struct (not required now as we only
have a single option), it would also allow to do some kind of interface
versioning (e.g., in case we want to implement
FUSE_PASSTHROUGH_OPEN_V2).

What do you think?

Thanks,
Alessio

P.S.
Sorry if you received a duplicate email. I first sent this in reply to an email
without realizing it was a private message.

On Thu, Jan 28, 2021 at 11:01:59AM +0800, qxy wrote:
> Hi Alessio,
> 
> I have received a failure from the Mail Delivery System for times and feel
> really sorry if you have already received the duplicate message...
> 
> Thank you for your reply.
> I think it's wonderful to remove *vec from the data structure fields since
> we consider that it is not a good idea to use pointer when there is a need
> for cross-platform.
> Do you have a plan to modify the kernel fuse_passthrough_out data structure
> the same way as you mentioned?
> 
> Thanks!
> qixiaoyu
Peng Tao Feb. 5, 2021, 9:54 a.m. UTC | #3
On Thu, Jan 28, 2021 at 10:15 PM Alessio Balsini <balsini@android.com> wrote:
>
> Hi all,
>
> I'm more than happy to change the interface into something that is
> objectively better and accepted by everyone.
> I would really love to reach the point at which we have a "stable-ish"
> UAPI as soon as possible.
>
> I've been thinking about a few possible approaches to fix the issue, yet
> to preserve its flexibility. These are mentioned below.
>
>
>   Solution 1: Size
>
> As mentioned in my previous email, one solution could be to introduce
> the "size" field to allow the structure to grow in the future.
>
> struct fuse_passthrough_out {
>     uint32_t        size;   // Size of this data structure
>     uint32_t        fd;
> };
>
> The problem here is that we are making the promise that all the upcoming
> fields are going to be maintained forever and at the offsets they were
> originally defined.
>
>
>   Solution 2: Version
>
> Another solution could be to s/size/version, where for every version of
> FUSE passthrough we reserve the right to modifying the fields over time,
> casting them to the right data structure according to the version.
>
>
>   Solution 3: Type
>
> Using an enumerator to define the data structure content and purpose is
> the most flexible solution I can think of.  This would for example allow
> us to substitute FUSE_DEV_IOC_PASSTHROUGH_OPEN with the generic
> FUSE_DEV_IOC_PASSTHROUGH and having a single ioctl for any eventually
> upcoming passthrough requests.
>
> enum fuse_passthrough_type {
>     FUSE_PASSTHROUGH_OPEN
> };
>
> struct fuse_passthrough_out {
>     uint32_t type; /* as defined by enum fuse_passthrough_type */
>     union {
>         uint32_t fd;
>     };
> };
>
> This last is my favorite, as regardless the minimal logic required to
> detect the size and content of the struct (not required now as we only
> have a single option), it would also allow to do some kind of interface
> versioning (e.g., in case we want to implement
> FUSE_PASSTHROUGH_OPEN_V2).
>
Usually a new type of ioctl will be added in such cases. If we want to
stick to the same ioctl number, it might be easier to add a `flags`
field to differentiate compatible passthrough ioctls. So in future, if
a new interface is compatible with the existing one, we can use flags
to tell it. If it is not compatible, we still need to add a new ioctl.
wdyt?

struct fuse_passthrough_out {
    uint32_t flags;
    union {
        uint32_t fd;
    };
};

This somehow follows the "Flags as a system call API design pattern"
(https://lwn.net/Articles/585415/).

Just my two cents.

Cheers,
Tao
Miklos Szeredi Feb. 17, 2021, 10:21 a.m. UTC | #4
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
diff mbox series

Patch

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;
 
-		err = -EFAULT;
-		if (!get_user(oldfd, (__u32 __user *) arg)) {
+	switch (_IOC_NR(cmd)) {
+	case _IOC_NR(FUSE_DEV_IOC_CLONE):
+		res = -EFAULT;
+		if (!get_user(oldfd, (__u32 __user *)arg)) {
 			struct file *old = fget(oldfd);
 
-			err = -EINVAL;
+			res = -EINVAL;
 			if (old) {
-				struct fuse_dev *fud = NULL;
-
 				/*
 				 * Check against file->f_op because CUSE
 				 * uses the same ioctl handler.
 				 */
 				if (old->f_op == file->f_op &&
-				    old->f_cred->user_ns == file->f_cred->user_ns)
+				    old->f_cred->user_ns ==
+					    file->f_cred->user_ns)
 					fud = fuse_get_dev(old);
 
 				if (fud) {
 					mutex_lock(&fuse_mutex);
-					err = fuse_device_clone(fud->fc, file);
+					res = fuse_device_clone(fud->fc, file);
 					mutex_unlock(&fuse_mutex);
 				}
 				fput(old);
 			}
 		}
+		break;
+	default:
+		res = -ENOTTY;
+		break;
 	}
-	return err;
+	return res;
 }
 
 const struct file_operations fuse_dev_operations = {
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 98ca64d1beb6..54442612c48b 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -903,7 +903,8 @@  struct fuse_notify_retrieve_in {
 };
 
 /* Device ioctls: */
-#define FUSE_DEV_IOC_CLONE	_IOR(229, 0, uint32_t)
+#define FUSE_DEV_IOC_MAGIC		229
+#define FUSE_DEV_IOC_CLONE		_IOR(FUSE_DEV_IOC_MAGIC, 0, uint32_t)
 
 struct fuse_lseek_in {
 	uint64_t	fh;