Re: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user()

[Linus Torvalds]

On Thu, May 28, 2020 at 8:10 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> BTW, regarding uaccess - how badly does the following offend your taste?
> Normally I'd just go for copy_from_user(), but these syscalls just might
> be hot enough for overhead to matter...

Hmm. So the code itself per se doesn't really offend me, but:

> +static inline int unkludge_sigmask(void __user *sig,
> +                                  sigset_t __user **up,
> +                                  size_t *sigsetsize)

That's a rather odd function, and if there's a reason for it I have no
issue, but I dislike the combination of "odd semantics" together with
"nondescriptive naming".

"unkludge" really doesn't describe anything.

Why is that "sig" pointer "void __user *" instead of being an actually
descriptive structure pointer:

   struct sigset_argpack {
        sigset_t __user *sigset;
        size_t sigset_size;
  };

and then it would be "struct sigset_size_argpack __user *" instead?
And same with compat_uptr_t */compat_size_t for the compat case?

Yeah, yeah, maybe I got that struct definition wrong when writing it
in the email, but wouldn't that make it much more understandable?

Then the output arguments could be just a pointer to that struct too
(except now in kernel space), and change that "unkludge" to
"get_sigset_argpack()", and the end result would be

    static inline int get_sigset_argpack(
          struct sigset_argpack __user *uarg,
          struct sigset_argpack *out)

and I think the implementation would be simpler and more
understandable too when it didn't need those odd casts and "+sizeof"
things etc..

So then the call-site would go from

>         size_t sigsetsize = 0;
>         sigset_t __user *up = NULL;
>
>         if (unkludge_sigmask(sig, &up, &sigsetsize))
>                 return -EFAULT;

to

>         struct sigset_argpack argpack = { NULL, 0 };
>
>         if (get_sigset_argpack(sig, &argpack))
>                 return -EFAULT;

and now you can use "argpack.sigset" and "argpack.sigset_size".

No?

Same exact deal for the compat case, where you'd just need that compat
struct (using "compat_uptr_t" and "compat_size_t"), and then

>         struct compat_sigset_argpack argpack = { 0, 0 };
>
> +       if (get_compat_sigset_argpack(sig, &argpack))
> +               return -EFAULT;

and then you use the result with "compat_ptr(argpack.sigset)" and
"argpack.sigset_size".

Or did I mis-read anything and get confused by that code in your patch?

                 Linus