Messages in this thread | | | From | Linus Torvalds <> | Date | Thu, 28 May 2020 20:42:25 -0700 | Subject | Re: [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
| |