lkml.org 
[lkml]   [2020]   [May]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user()
    On Thu, May 28, 2020 at 8:10 PM Al Viro <viro@zeniv.linux.org.uk> 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

    \
     
     \ /
      Last update: 2020-05-29 05:45    [W:4.494 / U:0.208 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site