Re: [PATCH 4/4] orangefs: simplify compat ioctl handling

[Mike Marshall <hubcap@omnibond.com>]

Thanks for the cleanup. This runs through xfstests with no regressions
on 4.17-rc7.

I studied what to do about the sparse warning, looked at the code, and
looked for hints from the original authors in the pvfs svn commit messages.

No luck with the old commit messages.

I got the sparse warning to quit with this change, which also runs through
xfstests with no regressions, does it seem OK?


$ git diff
diff --git a/fs/orangefs/protocol.h b/fs/orangefs/protocol.h
index 61ee8d64c842..d403cf29a99b 100644
--- a/fs/orangefs/protocol.h
+++ b/fs/orangefs/protocol.h
@@ -342,7 +342,7 @@ enum {
  * that may be 32 bit!
  */
 struct ORANGEFS_dev_map_desc {
-       void *ptr;
+       void __user *ptr;
        __s32 total_size;
        __s32 size;
        __s32 count;


Please add: Tested-by: Mike Marshall <hubcap@omnibond.com>

-Mike

On Mon, May 28, 2018 at 6:20 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
>
> no need to mess with copy_in_user(), etc...
>
> Cc: Mike Marshall <hubcap@omnibond.com>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/orangefs/devorangefs-req.c | 54 ++++++++++---------------------------------
>  1 file changed, 12 insertions(+), 42 deletions(-)
>
> diff --git a/fs/orangefs/devorangefs-req.c b/fs/orangefs/devorangefs-req.c
> index 66369ec90020..8581daf19634 100644
> --- a/fs/orangefs/devorangefs-req.c
> +++ b/fs/orangefs/devorangefs-req.c
> @@ -716,37 +716,6 @@ struct ORANGEFS_dev_map_desc32 {
>         __s32 count;
>  };
>
> -static unsigned long translate_dev_map26(unsigned long args, long *error)
> -{
> -       struct ORANGEFS_dev_map_desc32 __user *p32 = (void __user *)args;
> -       /*
> -        * Depending on the architecture, allocate some space on the
> -        * user-call-stack based on our expected layout.
> -        */
> -       struct ORANGEFS_dev_map_desc __user *p =
> -           compat_alloc_user_space(sizeof(*p));
> -       compat_uptr_t addr;
> -
> -       *error = 0;
> -       /* get the ptr from the 32 bit user-space */
> -       if (get_user(addr, &p32->ptr))
> -               goto err;
> -       /* try to put that into a 64-bit layout */
> -       if (put_user(compat_ptr(addr), &p->ptr))
> -               goto err;
> -       /* copy the remaining fields */
> -       if (copy_in_user(&p->total_size, &p32->total_size, sizeof(__s32)))
> -               goto err;
> -       if (copy_in_user(&p->size, &p32->size, sizeof(__s32)))
> -               goto err;
> -       if (copy_in_user(&p->count, &p32->count, sizeof(__s32)))
> -               goto err;
> -       return (unsigned long)p;
> -err:
> -       *error = -EFAULT;
> -       return 0;
> -}
> -
>  /*
>   * 32 bit user-space apps' ioctl handlers when kernel modules
>   * is compiled as a 64 bit one
> @@ -755,25 +724,26 @@ static long orangefs_devreq_compat_ioctl(struct file *filp, unsigned int cmd,
>                                       unsigned long args)
>  {
>         long ret;
> -       unsigned long arg = args;
>
>         /* Check for properly constructed commands */
>         ret = check_ioctl_command(cmd);
>         if (ret < 0)
>                 return ret;
>         if (cmd == ORANGEFS_DEV_MAP) {
> -               /*
> -                * convert the arguments to what we expect internally
> -                * in kernel space
> -                */
> -               arg = translate_dev_map26(args, &ret);
> -               if (ret < 0) {
> -                       gossip_err("Could not translate dev map\n");
> -                       return ret;
> -               }
> +               struct ORANGEFS_dev_map_desc desc;
> +               struct ORANGEFS_dev_map_desc32 d32;
> +
> +               if (copy_from_user(&d32, (void __user *)args, sizeof(d32)))
> +                       return -EFAULT;
> +
> +               desc.ptr = compat_ptr(d32.ptr);
> +               desc.total_size = d32.total_size;
> +               desc.size = d32.size;
> +               desc.count = d32.count;
> +               return orangefs_bufmap_initialize(&desc);
>         }
>         /* no other ioctl requires translation */
> -       return dispatch_ioctl_command(cmd, arg);
> +       return dispatch_ioctl_command(cmd, args);
>  }
>
>  #endif /* CONFIG_COMPAT is in .config */
> --
> 2.11.0
>