Re: [PATCH v5 03/18] gfs2: add compat_ioctl support

2019-08-19 Thread Andreas Gruenbacher
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

2019-08-19 Thread Arnd Bergmann
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

2019-08-18 Thread Andreas Grünbacher
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

2019-08-18 Thread 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?

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: [PATCH v5 03/18] gfs2: add compat_ioctl support

2019-08-15 Thread Bob Peterson
- 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