| Message ID | 20210125153057.3623715-3-balsini@android.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: Add support for passthrough read/write | expand |
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
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
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
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
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
On Mon, Jan 25, 2021 at 4:48 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> I saw this commit go into the mainline kernel, and I'm worried that this doesn't do what the description says. Since the argument is a 'uint32_t', it is the same on both 32-bit and 64-bit user space, and the patch won't make any difference for compat mode, as long as that is using the normal uapi headers. If there is any user space that has a different definition of FUSE_DEV_IOC_CLONE, that may now successfully call this ioctl command, but the kernel will now also accept any other command code that has the same type and number, but an arbitrary direction or size argument. I think this should be changed back to specifically allow the command code(s) that are actually used and nothing else. Arnd
On Thu, Jan 28, 2021 at 3:17 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. It's in the mainline kernel, so you already have a stable uapi and cannot change that in any incompatible way! > 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. Please read Documentation/driver-api/ioctl.rst for how to design ioctls. Neither 'size' nor 'version' fields are appropriate here. If you have a new behavior, you need a new command code. Arnd
On Tue, Mar 16, 2021 at 07:53:06PM +0100, Arnd Bergmann wrote: > On Mon, Jan 25, 2021 at 4:48 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> > > I saw this commit go into the mainline kernel, and I'm worried that this > doesn't do what the description says. Since the argument is a 'uint32_t', > it is the same on both 32-bit and 64-bit user space, and the patch won't > make any difference for compat mode, as long as that is using the normal > uapi headers. > > If there is any user space that has a different definition of > FUSE_DEV_IOC_CLONE, that may now successfully call > this ioctl command, but the kernel will now also accept any other > command code that has the same type and number, but an > arbitrary direction or size argument. > > I think this should be changed back to specifically allow the > command code(s) that are actually used and nothing else. > > Arnd Hi Arnd, Thanks for spotting this possible criticality. I noticed that 32-bit users pace was unable to use the FUSE_DEV_IOC_CLONE ioctl on 64-bit kernels, so this change avoid this issue by forcing the kernel to interpret 32 and 64 bit FUSE_DEV_IOC_CLONE command as if they were the same. This is the simplest solution I could find as the UAPI is not changed as, as you mentioned, the argument doesn't require any conversion. I understand that this might limit possible future extensions of the FUSE_DEV_IOC_XXX ioctls if their in/out argument changed depending on the architecture, but only at that point we can switch to using the compat layer, right? What I'm worried about is the direction, do you think this would be an issue? I can start working on a compat layer fix meanwhile. Thanks, Alessio
On Thu, Mar 18, 2021 at 5:13 PM Alessio Balsini <balsini@android.com> wrote: > On Tue, Mar 16, 2021 at 07:53:06PM +0100, Arnd Bergmann wrote: > > On Mon, Jan 25, 2021 at 4:48 PM Alessio Balsini <balsini@android.com> wrote: > > > > > Thanks for spotting this possible criticality. > > I noticed that 32-bit users pace was unable to use the > FUSE_DEV_IOC_CLONE ioctl on 64-bit kernels, so this change avoid this > issue by forcing the kernel to interpret 32 and 64 bit > FUSE_DEV_IOC_CLONE command as if they were the same. As far as I can tell from the kernel headers, the command code should be the same for both 32-bit and 64-bit tasks: 0x8004e500. Can you find out what exact value you see in the user space that was causing problems, and how it ended up with a different value than the 64-bit version? If there are two possible command codes, I'd suggest you just change the driver to handle both variants explicitly, but not any other one. > This is the simplest solution I could find as the UAPI is not changed > as, as you mentioned, the argument doesn't require any conversion. > > I understand that this might limit possible future extensions of the > FUSE_DEV_IOC_XXX ioctls if their in/out argument changed depending on > the architecture, but only at that point we can switch to using the > compat layer, right? > > What I'm worried about is the direction, do you think this would be an > issue? > > I can start working on a compat layer fix meanwhile. For a proper well-designed ioctl interface, compat support should not need anything beyond the '.compat_ioctl = compat_ptr_ioctl' assignment. Arnd
On Thu, Mar 18, 2021 at 10:15:43PM +0100, Arnd Bergmann wrote: > On Thu, Mar 18, 2021 at 5:13 PM Alessio Balsini <balsini@android.com> wrote: > > On Tue, Mar 16, 2021 at 07:53:06PM +0100, Arnd Bergmann wrote: > > > On Mon, Jan 25, 2021 at 4:48 PM Alessio Balsini <balsini@android.com> wrote: > > > > > > > > Thanks for spotting this possible criticality. > > > > I noticed that 32-bit users pace was unable to use the > > FUSE_DEV_IOC_CLONE ioctl on 64-bit kernels, so this change avoid this > > issue by forcing the kernel to interpret 32 and 64 bit > > FUSE_DEV_IOC_CLONE command as if they were the same. > > As far as I can tell from the kernel headers, the command code should > be the same for both 32-bit and 64-bit tasks: 0x8004e500. > Can you find out what exact value you see in the user space that was > causing problems, and how it ended up with a different value than > the 64-bit version? > > If there are two possible command codes, I'd suggest you just change > the driver to handle both variants explicitly, but not any other one. > > > This is the simplest solution I could find as the UAPI is not changed > > as, as you mentioned, the argument doesn't require any conversion. > > > > I understand that this might limit possible future extensions of the > > FUSE_DEV_IOC_XXX ioctls if their in/out argument changed depending on > > the architecture, but only at that point we can switch to using the > > compat layer, right? > > > > What I'm worried about is the direction, do you think this would be an > > issue? > > > > I can start working on a compat layer fix meanwhile. > > For a proper well-designed ioctl interface, compat support should not > need anything beyond the '.compat_ioctl = compat_ptr_ioctl' > assignment. > > Arnd You are right and now I can see what happened here. When I introduce the PASSTHROUGH ioctl, because of the 'void *', the command mismatches on _IOC_SIZE(nr). I solved this by only testing _IOC_NUMBER and _IOC_TYPE, implicitely (mistakenly) removing the _IOC_SIZE check. So, the fuse_dev_ioctl was correctly rejecting the ioctl request from 32-bit userspace because of the wrong size and I was just forcing it to digest the wrong data regardless. Ouch! The commit message for this patch would still be incorrect, but I posted a fix here to bring the ioctl checking back to the original behavior: https://lore.kernel.org/lkml/20210319150514.1315985-1-balsini@android.com/ Thanks for spotting this! Alessio
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;
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(-)