Re: [patch] orangefs: cleanup orangefs_debugfs_new_client_string()

Re: [patch] orangefs: cleanup orangefs_debugfs_new_client_string()

[Mike Marshall]

Hi Dan...

Your patch applies, and compiles, and works. Thanks!

1) Al Viro tried to get me to fix all the places where I returned wrong
   error codes before we went upstream, I guess I slipped some by him <g>...

2) Some system administrators have admonished me because
   of a place where I put annoying messages into the ring
   buffer when a particular error occurs during op processing.
   I liked seeing it during development, but on a busy production cluster
   filled with people hitting CTRL-C and whatever else people whimsically
   do, there were thousands of "No one's waiting for tag #such-and-such"
   messages in dmesg and syslog.

   This particular message you mention, though, should almost never
   come out, and never because of Joe Blow users, rather because
   some awful thing happened when the sysadmin tried to load the
   client-core (userspace connector). Wouldn't something important
   have to be broken for that copy_from_user to fail?

   Anyhow, let me know if you think it might be OK to leave this one
   in, else I'll take it out.

3) Those weren't just tabs, those two lines were indented with all
   spaces (oops), and thanks for taking out the cast if it is not needed.

   When there's too many arguments to type a whole function call
   out on one line, though, I like to "stack" the arguments, it makes
   it easier for me to see them... what do you think about that? Martin,
   the other developer who does a lot of work on Orangefs, doesn't like
   the way I put each argument on a line by itself, so maybe it is not
   helpful to most people, or important...

4) The preserved error code will find its way back to vfs through
   file_operations->unlocked_ioctl in the context of the pseudo device
   through which the kernel module and Orangefs' userspace communicate. It
   could end up being EINVAL or ENOMEM. Is that OK? When Al was getting
   after me for returning the wrong error codes, he said we shouldn't
   pick ones that seem reasonable to us, rather we should pick from the ones
   that POSIX said would be valid ones. I try to pick valid ones now by
   looking at the associated syscall's man page. There's no ENOMEM in
   the ioctl(2) man page.

5) OK

On Fri, Dec 16, 2016 at 5:45 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Several small things in this function:
> 1) If copy to user fails we should return -EFAULT not -EIO
> 2) Don't print an error message, just fail.  It's annoying to let the
>    users fill up dmesg and especially for something small like this.
> 3) Remove a stray tab.
> 4) Preserve the error code if orangefs_prepare_debugfs_help_string()
>    fails.
> 5) "return 0;" is more explicit and clear than "return ret;".
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/fs/orangefs/orangefs-debugfs.c b/fs/orangefs/orangefs-debugfs.c
> index 27e75cf28b3a..409fa6b0d339 100644
> --- a/fs/orangefs/orangefs-debugfs.c
> +++ b/fs/orangefs/orangefs-debugfs.c
> @@ -966,15 +966,9 @@ int orangefs_debugfs_new_client_string(void __user *arg)
>  {
>         int ret;
>
> -       ret = copy_from_user(&client_debug_array_string,
> -                                     (void __user *)arg,
> -                                     ORANGEFS_MAX_DEBUG_STRING_LEN);
> -
> -       if (ret != 0) {
> -               pr_info("%s: CLIENT_STRING: copy_from_user failed\n",
> -                       __func__);
> -               return -EIO;
> -       }
> +       if (copy_from_user(&client_debug_array_string, arg,
> +                          ORANGEFS_MAX_DEBUG_STRING_LEN))
> +               return -EFAULT;
>
>         /*
>          * The real client-core makes an effort to ensure
> @@ -988,17 +982,18 @@ int orangefs_debugfs_new_client_string(void __user *arg)
>          */
>         client_debug_array_string[ORANGEFS_MAX_DEBUG_STRING_LEN - 1] =
>                 '\0';
> -
> +
>         pr_info("%s: client debug array string has been received.\n",
>                 __func__);
>
>         if (!help_string_initialized) {
>
>                 /* Build a proper debug help string. */
> -               if (orangefs_prepare_debugfs_help_string(0)) {
> +               ret = orangefs_prepare_debugfs_help_string(0);
> +               if (ret) {
>                         gossip_err("%s: no debug help string \n",
>                                    __func__);
> -                       return -EIO;
> +                       return ret;
>                 }
>
>         }
> @@ -1011,7 +1006,7 @@ int orangefs_debugfs_new_client_string(void __user *arg)
>
>         help_string_initialized++;
>
> -       return ret;
> +       return 0;
>  }
>
>  int orangefs_debugfs_new_debug(void __user *arg)

Re: [patch] orangefs: cleanup orangefs_debugfs_new_client_string()

[Dan Carpenter]

So the story with this patch is that I was looking at the code for
unrelated reasons and I was just dorking in my editor and decided to
click send at the end.  I often muck about and then just decide to move
on without hitting send.  It's not something I feel strongly about.

On Fri, Dec 16, 2016 at 03:35:34PM -0500, Mike Marshall wrote:
> 2) Some system administrators have admonished me because
>    of a place where I put annoying messages into the ring
>    buffer when a particular error occurs during op processing.
>    I liked seeing it during development, but on a busy production cluster
>    filled with people hitting CTRL-C and whatever else people whimsically
>    do, there were thousands of "No one's waiting for tag #such-and-such"
>    messages in dmesg and syslog.
> 
>    This particular message you mention, though, should almost never
>    come out, and never because of Joe Blow users, rather because
>    some awful thing happened when the sysadmin tried to load the
>    client-core (userspace connector). Wouldn't something important
>    have to be broken for that copy_from_user to fail?
> 
>    Anyhow, let me know if you think it might be OK to leave this one
>    in, else I'll take it out.

If the user passes a bogus pointer to the ioctl, then copy_from_user()
will fail and the program will segfault.  It's simple enough to run
valgrind or strace on the failing program and figure out why the program
segfaulted surely?

I don't know this code well enough, can regular users call the
ORANGEFS_DEV_CLIENT_STRING?  If so then they can trigger a DoS attack so
it's a considered a security violation.  If it's root only it doesn't
matter.

> 
> 3) Those weren't just tabs, those two lines were indented with all
>    spaces (oops), and thanks for taking out the cast if it is not needed.
> 
>    When there's too many arguments to type a whole function call
>    out on one line, though, I like to "stack" the arguments, it makes
>    it easier for me to see them... what do you think about that? Martin,
>    the other developer who does a lot of work on Orangefs, doesn't like
>    the way I put each argument on a line by itself, so maybe it is not
>    helpful to most people, or important...
> 

The way I changed it is the normal way but few people one feel strongly
about it.  I just did that because I removed the unneeded casting (and
forgot to mention it in the changelog).

> 4) The preserved error code will find its way back to vfs through
>    file_operations->unlocked_ioctl in the context of the pseudo device
>    through which the kernel module and Orangefs' userspace communicate. It
>    could end up being EINVAL or ENOMEM. Is that OK? When Al was getting
>    after me for returning the wrong error codes, he said we shouldn't
>    pick ones that seem reasonable to us, rather we should pick from the ones
>    that POSIX said would be valid ones. I try to pick valid ones now by
>    looking at the associated syscall's man page. There's no ENOMEM in
>    the ioctl(2) man page.

Preserving the error code is fine most of the time with a very few
exceptions.  -EIO was the wrong error code because that's for when you
can't read/write to the hardware because a drive fails or something.

regards,
dan carpenter