diff mbox series

[v5,03/18] gfs2: add compat_ioctl support

Message ID 20190814204259.120942-4-arnd@arndb.de
State Superseded
Headers show
Series [v5,01/18] xfs: compat_ioctl: use compat_ptr() | expand

Commit Message

Arnd Bergmann Aug. 14, 2019, 8:42 p.m. UTC
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

Comments

Bob Peterson Aug. 15, 2019, 12:07 p.m. UTC | #1
----- 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
Andreas Gruenbacher Aug. 16, 2019, 5:31 p.m. UTC | #2
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
Arnd Bergmann Aug. 18, 2019, 7:31 p.m. UTC | #3
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
Andreas Grünbacher Aug. 18, 2019, 8:17 p.m. UTC | #4
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
Arnd Bergmann Aug. 19, 2019, 9:09 a.m. UTC | #5
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
Andreas Gruenbacher Aug. 19, 2019, 9:37 a.m. UTC | #6
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 mbox series

Patch

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,