Re: [PATCH v5 03/18] gfs2: add compat_ioctl support
On Mon, Aug 19, 2019 at 11:09 AM Arnd Bergmann wrote:
> On Sun, Aug 18, 2019 at 10:17 PM Andreas Grünbacher
> wrote:
> > Am So., 18. Aug. 2019 um 21:32 Uhr schrieb Arnd Bergmann :
> > > On Fri, Aug 16, 2019 at 7:32 PM Andreas Gruenbacher
> > > wrote:
> > > > On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann 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
Re: [PATCH v5 03/18] gfs2: add compat_ioctl support
On Sun, Aug 18, 2019 at 10:17 PM Andreas Grünbacher
wrote:
> Am So., 18. Aug. 2019 um 21:32 Uhr schrieb Arnd Bergmann :
> > On Fri, Aug 16, 2019 at 7:32 PM Andreas Gruenbacher
> > wrote:
> > > On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann 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
Re: [PATCH v5 03/18] gfs2: add compat_ioctl support
Am So., 18. Aug. 2019 um 21:32 Uhr schrieb Arnd Bergmann :
> On Fri, Aug 16, 2019 at 7:32 PM Andreas Gruenbacher
> wrote:
> > On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann 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
Re: [PATCH v5 03/18] gfs2: add compat_ioctl support
On Fri, Aug 16, 2019 at 7:32 PM Andreas Gruenbacher wrote:
>
> On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann 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
Re: [Cluster-devel] [PATCH v5 03/18] gfs2: add compat_ioctl support
Arnd,
On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann 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
> ---
> 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
> #include
> +#include
> #include
> #include
> #include
> @@ -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
Re: [PATCH v5 03/18] gfs2: add compat_ioctl support
- 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
> ---
> fs/gfs2/file.c | 24
> 1 file changed, 24 insertions(+)
Hi,
Reviewed-by: Bob Peterson
Regards,
Bob Peterson
[PATCH v5 03/18] gfs2: add compat_ioctl support
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
---
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
#include
+#include
#include
#include
#include
@@ -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,
--
2.20.0
