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
