what the hell is compat_sys_x86_waitpid() for?

what the hell is compat_sys_x86_waitpid() for?

[Al Viro]

You have
COMPAT_SYSCALL_DEFINE3(x86_waitpid, compat_pid_t, pid, unsigned int __user *,
                       stat_addr, int, options)
{
        return compat_sys_wait4(pid, stat_addr, options, NULL);
}
with
COMPAT_SYSCALL_DEFINE4(wait4,
        compat_pid_t, pid,
        compat_uint_t __user *, stat_addr,
        int, options,
        struct compat_rusage __user *, ru)
{
        struct rusage r;
        long err = kernel_wait4(pid, stat_addr, options, ru ? &r : NULL);
        if (err > 0) {
                if (ru && put_compat_rusage(&r, ru))
                        return -EFAULT;
        }
        return err;
}

so that turns into
	return kernel_wait4(pid, stat_addr, options, NULL);

Now, look at
SYSCALL_DEFINE3(waitpid, pid_t, pid, int __user *, stat_addr, int, options)
{
        return sys_wait4(pid, stat_addr, options, NULL);
}
and
SYSCALL_DEFINE4(wait4, pid_t, upid, int __user *, stat_addr,
                int, options, struct rusage __user *, ru)
{
        struct rusage r;
        long err = kernel_wait4(upid, stat_addr, options, ru ? &r : NULL);

        if (err > 0) {
                if (ru && copy_to_user(ru, &r, sizeof(struct rusage)))
                        return -EFAULT;
        }
        return err;
}

and tell me what is the difference between those.  In other words, the problem
with sys32_waitpid() was not that it didn't use proper wrappers - it's that
it was (and always had been) 100% pointless.

For fsck sake, look at the arguments.  waitpid(2) takes pid_t, pointer to int
and an int.  How the hell could it possibly have required a compat wrapper?

Let's get rid of the junk rather than covering it with more layers of crap...

Re: what the hell is compat_sys_x86_waitpid() for?

[Linus Torvalds]

On Sat, Mar 17, 2018 at 11:01 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> and tell me what is the difference between those.  In other words, the problem
> with sys32_waitpid() was not that it didn't use proper wrappers - it's that
> it was (and always had been) 100% pointless.

That long long predates Dominik's patches, though.

It clearly is worth fixing, but don't blame Dominik for just
mindlessly converting mindless code.

> For fsck sake, look at the arguments.  waitpid(2) takes pid_t, pointer to int
> and an int.  How the hell could it possibly have required a compat wrapper?

This seems to go all the way back to the original x86_64 merge in
2002, with the original version of arch/x86_64/ia32/sys_ia32.c
containing

   asmlinkage long
   sys32_waitpid(__kernel_pid_t32 pid, unsigned int *stat_addr, int options)
   {
        return sys32_wait4(pid, stat_addr, options, NULL);
   }

and it has only been modified syntactically since.

I suspect the reason is simply that the *regular* waitpid() was writtn
in terms of sys_wait4(), and sys_wait4() needed a compat function, so
then waitpid was basically just carried along.

               Linus

Re: what the hell is compat_sys_x86_waitpid() for?

[Al Viro]

On Sat, Mar 17, 2018 at 11:19:55AM -0700, Linus Torvalds wrote:
> On Sat, Mar 17, 2018 at 11:01 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > and tell me what is the difference between those.  In other words, the problem
> > with sys32_waitpid() was not that it didn't use proper wrappers - it's that
> > it was (and always had been) 100% pointless.
> 
> That long long predates Dominik's patches, though.

Sure.

> It clearly is worth fixing, but don't blame Dominik for just
> mindlessly converting mindless code.

The thing is, this area probably won't be looked at for many years in the
future, so we'd better take the crap out now.  Same problem as with mindless
checkpatch.pl "fixes" - something unusually-looking obviously has gotten
less attention, so removing the visual indication of that has a good chance
of hiding something dumb.  Not a problem if the code is actually OK, but
that's worth checking.

> I suspect the reason is simply that the *regular* waitpid() was writtn
> in terms of sys_wait4(), and sys_wait4() needed a compat function, so
> then waitpid was basically just carried along.

Probably nobody remembers now...

Another thing touched by the same commit: pread64().  Comparing it with
other biarch architectures shows at least one dubious thing, also there
since way back.  Namely, s390 has
        if ((compat_ssize_t) count < 0)
                return -EINVAL;
IOW, size >= 2G => -EINVAL.  It *does* match the behaviour on 32bit -
on all everything including i386.  rw_verify_area() starts with
        int retval = -EINVAL;

        inode = file_inode(file);
        if (unlikely((ssize_t) count < 0))
                return retval;
64bit read/write variants quietly truncate to 2Gb, so on amd64 a 32bit
binary calling pread() with negative size will do 2Gb read while on
i386 the same binary would've gotten -EINVAL.  I'm not sure if it's
worth bothering with, though - it's not just pread().  read(2)
has exact same issue and yes, s390 does have a separate wrapper for
it, with the same check.  The same goes for write(2) there.

Do we care about that, and if not - does s390 really need to bother?
The rest of biarch wrappers for sys_pread64() is very close to being
unifiable - the only real issue is whether this architecture has
a dummy padding argument between count and (halves of) pos.  arm64,
mips and ppc do; parisc, s390, sparc and x86 do not...