| Message ID | 20190814204259.120942-4-arnd@arndb.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,01/18] xfs: compat_ioctl: use compat_ptr() | expand |
----- Original Message ----- > Out of the four ioctl commands supported on gfs2, only FITRIM > works in compat mode. > > Add a proper handler based on the ext4 implementation. > > Fixes: 6ddc5c3ddf25 ("gfs2: getlabel support") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > fs/gfs2/file.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) Hi, Reviewed-by: Bob Peterson <rpeterso@redhat.com> Regards, Bob Peterson
Arnd, On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann <arnd@arndb.de> wrote: > > Out of the four ioctl commands supported on gfs2, only FITRIM > works in compat mode. > > Add a proper handler based on the ext4 implementation. > > Fixes: 6ddc5c3ddf25 ("gfs2: getlabel support") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > fs/gfs2/file.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c > index 52fa1ef8400b..49287f0b96d0 100644 > --- a/fs/gfs2/file.c > +++ b/fs/gfs2/file.c > @@ -6,6 +6,7 @@ > > #include <linux/slab.h> > #include <linux/spinlock.h> > +#include <linux/compat.h> > #include <linux/completion.h> > #include <linux/buffer_head.h> > #include <linux/pagemap.h> > @@ -354,6 +355,25 @@ static long gfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > return -ENOTTY; > } > > +#ifdef CONFIG_COMPAT > +static long gfs2_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > +{ > + /* These are just misnamed, they actually get/put from/to user an int */ > + switch(cmd) { > + case FS_IOC32_GETFLAGS: > + cmd = FS_IOC_GETFLAGS; > + break; > + case FS_IOC32_SETFLAGS: > + cmd = FS_IOC_SETFLAGS; > + break; I'd like the code to be more explicit here: case FITRIM: case FS_IOC_GETFSLABEL: break; default: return -ENOIOCTLCMD; > + } > + > + return gfs2_ioctl(filp, cmd, (unsigned long)compat_ptr(arg)); > +} > +#else > +#define gfs2_compat_ioctl NULL > +#endif > + > /** > * gfs2_size_hint - Give a hint to the size of a write request > * @filep: The struct file > @@ -1294,6 +1314,7 @@ const struct file_operations gfs2_file_fops = { > .write_iter = gfs2_file_write_iter, > .iopoll = iomap_dio_iopoll, > .unlocked_ioctl = gfs2_ioctl, > + .compat_ioctl = gfs2_compat_ioctl, > .mmap = gfs2_mmap, > .open = gfs2_open, > .release = gfs2_release, > @@ -1309,6 +1330,7 @@ const struct file_operations gfs2_file_fops = { > const struct file_operations gfs2_dir_fops = { > .iterate_shared = gfs2_readdir, > .unlocked_ioctl = gfs2_ioctl, > + .compat_ioctl = gfs2_compat_ioctl, > .open = gfs2_open, > .release = gfs2_release, > .fsync = gfs2_fsync, > @@ -1325,6 +1347,7 @@ const struct file_operations gfs2_file_fops_nolock = { > .write_iter = gfs2_file_write_iter, > .iopoll = iomap_dio_iopoll, > .unlocked_ioctl = gfs2_ioctl, > + .compat_ioctl = gfs2_compat_ioctl, > .mmap = gfs2_mmap, > .open = gfs2_open, > .release = gfs2_release, > @@ -1338,6 +1361,7 @@ const struct file_operations gfs2_file_fops_nolock = { > const struct file_operations gfs2_dir_fops_nolock = { > .iterate_shared = gfs2_readdir, > .unlocked_ioctl = gfs2_ioctl, > + .compat_ioctl = gfs2_compat_ioctl, > .open = gfs2_open, > .release = gfs2_release, > .fsync = gfs2_fsync, > -- > 2.20.0 > Should we feed this through the gfs2 tree? Thanks, Andreas
On Fri, Aug 16, 2019 at 7:32 PM Andreas Gruenbacher <agruenba@redhat.com> wrote: > > On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann <arnd@arndb.de> wrote: > > + /* These are just misnamed, they actually get/put from/to user an int */ > > + switch(cmd) { > > + case FS_IOC32_GETFLAGS: > > + cmd = FS_IOC_GETFLAGS; > > + break; > > + case FS_IOC32_SETFLAGS: > > + cmd = FS_IOC_SETFLAGS; > > + break; > > I'd like the code to be more explicit here: > > case FITRIM: > case FS_IOC_GETFSLABEL: > break; > default: > return -ENOIOCTLCMD; I've looked at it again: if we do this, the function actually becomes longer than the native gfs2_ioctl(). Should we just make a full copy then? static long gfs2_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) { switch(cmd) { case FS_IOC32_GETFLAGS: return gfs2_get_flags(filp, (u32 __user *)arg); case FS_IOC32_SETFLAGS: return gfs2_set_flags(filp, (u32 __user *)arg); case FITRIM: return gfs2_fitrim(filp, (void __user *)arg); case FS_IOC_GETFSLABEL: return gfs2_getlabel(filp, (char __user *)arg); } return -ENOTTY; } > Should we feed this through the gfs2 tree? A later patch that removes the FITRIM handling from fs/compat_ioctl.c depends on it, so I'd like to keep everything together. Arnd
Am So., 18. Aug. 2019 um 21:32 Uhr schrieb Arnd Bergmann <arnd@arndb.de>: > On Fri, Aug 16, 2019 at 7:32 PM Andreas Gruenbacher <agruenba@redhat.com> wrote: > > On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > + /* These are just misnamed, they actually get/put from/to user an int */ > > > + switch(cmd) { > > > + case FS_IOC32_GETFLAGS: > > > + cmd = FS_IOC_GETFLAGS; > > > + break; > > > + case FS_IOC32_SETFLAGS: > > > + cmd = FS_IOC_SETFLAGS; > > > + break; > > > > I'd like the code to be more explicit here: > > > > case FITRIM: > > case FS_IOC_GETFSLABEL: > > break; > > default: > > return -ENOIOCTLCMD; > > I've looked at it again: if we do this, the function actually becomes > longer than the native gfs2_ioctl(). Should we just make a full copy then? I don't think the length of gfs2_compat_ioctl is really an issue as long as the function is that simple. > static long gfs2_compat_ioctl(struct file *filp, unsigned int cmd, > unsigned long arg) > { > switch(cmd) { > case FS_IOC32_GETFLAGS: > return gfs2_get_flags(filp, (u32 __user *)arg); > case FS_IOC32_SETFLAGS: > return gfs2_set_flags(filp, (u32 __user *)arg); > case FITRIM: > return gfs2_fitrim(filp, (void __user *)arg); > case FS_IOC_GETFSLABEL: > return gfs2_getlabel(filp, (char __user *)arg); > } > > return -ENOTTY; > } Don't we still need the compat_ptr conversion? That seems to be the main point of having a compat_ioctl operation. > > Should we feed this through the gfs2 tree? > > A later patch that removes the FITRIM handling from fs/compat_ioctl.c > depends on it, so I'd like to keep everything together. Ok, fine for me. Thanks, Andreas
On Sun, Aug 18, 2019 at 10:17 PM Andreas Grünbacher <andreas.gruenbacher@gmail.com> wrote: > Am So., 18. Aug. 2019 um 21:32 Uhr schrieb Arnd Bergmann <arnd@arndb.de>: > > On Fri, Aug 16, 2019 at 7:32 PM Andreas Gruenbacher <agruenba@redhat.com> wrote: > > > On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > + /* These are just misnamed, they actually get/put from/to user an int */ > > > > + switch(cmd) { > > > > + case FS_IOC32_GETFLAGS: > > > > + cmd = FS_IOC_GETFLAGS; > > > > + break; > > > > + case FS_IOC32_SETFLAGS: > > > > + cmd = FS_IOC_SETFLAGS; > > > > + break; > > > > > > I'd like the code to be more explicit here: > > > > > > case FITRIM: > > > case FS_IOC_GETFSLABEL: > > > break; > > > default: > > > return -ENOIOCTLCMD; > > > > I've looked at it again: if we do this, the function actually becomes > > longer than the native gfs2_ioctl(). Should we just make a full copy then? > > I don't think the length of gfs2_compat_ioctl is really an issue as > long as the function is that simple. True. The most important goal should just be to make it easy to add the correct handler the next time another command is added to the ioctl function. Just let me know which version you want for that: 1. my original patch 2. the version from your reply 3. my version below with compat_ptr() added 4. ... > > static long gfs2_compat_ioctl(struct file *filp, unsigned int cmd, > > unsigned long arg) > > { > > switch(cmd) { > > case FS_IOC32_GETFLAGS: > > return gfs2_get_flags(filp, (u32 __user *)arg); > > case FS_IOC32_SETFLAGS: > > return gfs2_set_flags(filp, (u32 __user *)arg); > > case FITRIM: > > return gfs2_fitrim(filp, (void __user *)arg); > > case FS_IOC_GETFSLABEL: > > return gfs2_getlabel(filp, (char __user *)arg); > > } > > > > return -ENOTTY; > > } > > Don't we still need the compat_ptr conversion? That seems to be the > main point of having a compat_ioctl operation. Right, of course. Fixed now in my tree. Arnd
On Mon, Aug 19, 2019 at 11:09 AM Arnd Bergmann <arnd@arndb.de> wrote: > On Sun, Aug 18, 2019 at 10:17 PM Andreas Grünbacher > <andreas.gruenbacher@gmail.com> wrote: > > Am So., 18. Aug. 2019 um 21:32 Uhr schrieb Arnd Bergmann <arnd@arndb.de>: > > > On Fri, Aug 16, 2019 at 7:32 PM Andreas Gruenbacher <agruenba@redhat.com> wrote: > > > > On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > + /* These are just misnamed, they actually get/put from/to user an int */ > > > > > + switch(cmd) { > > > > > + case FS_IOC32_GETFLAGS: > > > > > + cmd = FS_IOC_GETFLAGS; > > > > > + break; > > > > > + case FS_IOC32_SETFLAGS: > > > > > + cmd = FS_IOC_SETFLAGS; > > > > > + break; > > > > > > > > I'd like the code to be more explicit here: > > > > > > > > case FITRIM: > > > > case FS_IOC_GETFSLABEL: > > > > break; > > > > default: > > > > return -ENOIOCTLCMD; > > > > > > I've looked at it again: if we do this, the function actually becomes > > > longer than the native gfs2_ioctl(). Should we just make a full copy then? > > > > I don't think the length of gfs2_compat_ioctl is really an issue as > > long as the function is that simple. > > True. The most important goal should just be to make it easy to > add the correct handler the next time another command is added > to the ioctl function. > > Just let me know which version you want for that: > > 1. my original patch > 2. the version from your reply That one, please. > 3. my version below with compat_ptr() added > 4. ... > > > > static long gfs2_compat_ioctl(struct file *filp, unsigned int cmd, > > > unsigned long arg) > > > { > > > switch(cmd) { > > > case FS_IOC32_GETFLAGS: > > > return gfs2_get_flags(filp, (u32 __user *)arg); > > > case FS_IOC32_SETFLAGS: > > > return gfs2_set_flags(filp, (u32 __user *)arg); > > > case FITRIM: > > > return gfs2_fitrim(filp, (void __user *)arg); > > > case FS_IOC_GETFSLABEL: > > > return gfs2_getlabel(filp, (char __user *)arg); > > > } > > > > > > return -ENOTTY; > > > } > > > > Don't we still need the compat_ptr conversion? That seems to be the > > main point of having a compat_ioctl operation. > > Right, of course. Fixed now in my tree. > > Arnd Thanks, Andreas
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c index 52fa1ef8400b..49287f0b96d0 100644 --- a/fs/gfs2/file.c +++ b/fs/gfs2/file.c @@ -6,6 +6,7 @@ #include <linux/slab.h> #include <linux/spinlock.h> +#include <linux/compat.h> #include <linux/completion.h> #include <linux/buffer_head.h> #include <linux/pagemap.h> @@ -354,6 +355,25 @@ static long gfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) return -ENOTTY; } +#ifdef CONFIG_COMPAT +static long gfs2_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) +{ + /* These are just misnamed, they actually get/put from/to user an int */ + switch(cmd) { + case FS_IOC32_GETFLAGS: + cmd = FS_IOC_GETFLAGS; + break; + case FS_IOC32_SETFLAGS: + cmd = FS_IOC_SETFLAGS; + break; + } + + return gfs2_ioctl(filp, cmd, (unsigned long)compat_ptr(arg)); +} +#else +#define gfs2_compat_ioctl NULL +#endif + /** * gfs2_size_hint - Give a hint to the size of a write request * @filep: The struct file @@ -1294,6 +1314,7 @@ const struct file_operations gfs2_file_fops = { .write_iter = gfs2_file_write_iter, .iopoll = iomap_dio_iopoll, .unlocked_ioctl = gfs2_ioctl, + .compat_ioctl = gfs2_compat_ioctl, .mmap = gfs2_mmap, .open = gfs2_open, .release = gfs2_release, @@ -1309,6 +1330,7 @@ const struct file_operations gfs2_file_fops = { const struct file_operations gfs2_dir_fops = { .iterate_shared = gfs2_readdir, .unlocked_ioctl = gfs2_ioctl, + .compat_ioctl = gfs2_compat_ioctl, .open = gfs2_open, .release = gfs2_release, .fsync = gfs2_fsync, @@ -1325,6 +1347,7 @@ const struct file_operations gfs2_file_fops_nolock = { .write_iter = gfs2_file_write_iter, .iopoll = iomap_dio_iopoll, .unlocked_ioctl = gfs2_ioctl, + .compat_ioctl = gfs2_compat_ioctl, .mmap = gfs2_mmap, .open = gfs2_open, .release = gfs2_release, @@ -1338,6 +1361,7 @@ const struct file_operations gfs2_file_fops_nolock = { const struct file_operations gfs2_dir_fops_nolock = { .iterate_shared = gfs2_readdir, .unlocked_ioctl = gfs2_ioctl, + .compat_ioctl = gfs2_compat_ioctl, .open = gfs2_open, .release = gfs2_release, .fsync = gfs2_fsync,
Out of the four ioctl commands supported on gfs2, only FITRIM works in compat mode. Add a proper handler based on the ext4 implementation. Fixes: 6ddc5c3ddf25 ("gfs2: getlabel support") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- fs/gfs2/file.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) -- 2.20.0