commit 67d7ddded32 (waitid(2): leave copyout of siginfo to syscall itself) breaks glibc posix/tst-waitid

commit 67d7ddded32 (waitid(2): leave copyout of siginfo to syscall itself) breaks glibc posix/tst-waitid

[Markus Trippelsdorf]

Since:
 commit 67d7ddded322db99f451a7959d56ed6c70a6c4aa
 Author: Al Viro <viro@zeniv.linux.org.uk>
 Date:   Sun May 14 20:53:13 2017 -0400

     waitid(2): leave copyout of siginfo to syscall itself

the glibc posix/tst-waitid.c testcase fails:

markus@x4 glibc-build % ./posix/tst-waitid
waitid WNOHANG on stopped status 0

-- 
Markus

Re: commit 67d7ddded32 (waitid(2): leave copyout of siginfo to syscall itself) breaks glibc posix/tst-waitid

[Al Viro]

On Sat, Jul 08, 2017 at 11:56:44AM +0200, Markus Trippelsdorf wrote:
> Since:
>  commit 67d7ddded322db99f451a7959d56ed6c70a6c4aa
>  Author: Al Viro <viro@zeniv.linux.org.uk>
>  Date:   Sun May 14 20:53:13 2017 -0400
> 
>      waitid(2): leave copyout of siginfo to syscall itself
> 
> the glibc posix/tst-waitid.c testcase fails:
> 
> markus@x4 glibc-build % ./posix/tst-waitid
> waitid WNOHANG on stopped status 0

I think I see what's going on...  We lose the distinction between "got
a PID" and "nothing, but that's not an error" a bit too early waitid()
case.  Could you see if there are any problems left with the following
applied?

diff --git a/kernel/exit.c b/kernel/exit.c
index 462fc25eec6e..aa5d5e4de804 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1604,9 +1604,6 @@ static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
 	wo.wo_rusage	= ru;
 	ret = do_wait(&wo);
 
-	if (ret > 0)
-		ret = 0;
-
 	put_pid(pid);
 	return ret;
 }
@@ -1617,6 +1614,11 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
 	struct rusage r;
 	struct waitid_info info = {.status = 0};
 	long err = kernel_waitid(which, upid, &info, options, ru ? &r : NULL);
+	int signo = 0;
+	if (err > 0) {
+		signo = SIGCHLD;
+		err = 0;
+	}
 
 	if (!err) {
 		if (ru && copy_to_user(ru, &r, sizeof(struct rusage)))
@@ -1626,7 +1628,7 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
 		return err;
 
 	user_access_begin();
-	unsafe_put_user(err ? 0 : SIGCHLD, &infop->si_signo, Efault);
+	unsafe_put_user(signo, &infop->si_signo, Efault);
 	unsafe_put_user(0, &infop->si_errno, Efault);
 	unsafe_put_user((short)info.cause, &infop->si_code, Efault);
 	unsafe_put_user(info.pid, &infop->si_pid, Efault);
@@ -1728,6 +1730,11 @@ COMPAT_SYSCALL_DEFINE5(waitid,
 	struct rusage ru;
 	struct waitid_info info = {.status = 0};
 	long err = kernel_waitid(which, pid, &info, options, uru ? &ru : NULL);
+	int signo = 0;
+	if (err > 0) {
+		signo = SIGCHLD;
+		err = 0;
+	}
 
 	if (!err && uru) {
 		/* kernel_waitid() overwrites everything in ru */
@@ -1743,7 +1750,7 @@ COMPAT_SYSCALL_DEFINE5(waitid,
 		return err;
 
 	user_access_begin();
-	unsafe_put_user(err ? 0 : SIGCHLD, &infop->si_signo, Efault);
+	unsafe_put_user(signo, &infop->si_signo, Efault);
 	unsafe_put_user(0, &infop->si_errno, Efault);
 	unsafe_put_user((short)info.cause, &infop->si_code, Efault);
 	unsafe_put_user(info.pid, &infop->si_pid, Efault);

Re: commit 67d7ddded32 (waitid(2): leave copyout of siginfo to syscall itself) breaks glibc posix/tst-waitid

[Markus Trippelsdorf]

On 2017.07.08 at 14:12 +0100, Al Viro wrote:
> On Sat, Jul 08, 2017 at 11:56:44AM +0200, Markus Trippelsdorf wrote:
> > Since:
> >  commit 67d7ddded322db99f451a7959d56ed6c70a6c4aa
> >  Author: Al Viro <viro@zeniv.linux.org.uk>
> >  Date:   Sun May 14 20:53:13 2017 -0400
> > 
> >      waitid(2): leave copyout of siginfo to syscall itself
> > 
> > the glibc posix/tst-waitid.c testcase fails:
> > 
> > markus@x4 glibc-build % ./posix/tst-waitid
> > waitid WNOHANG on stopped status 0
> 
> I think I see what's going on...  We lose the distinction between "got
> a PID" and "nothing, but that's not an error" a bit too early waitid()
> case.  Could you see if there are any problems left with the following
> applied?

No, everything looks nice now. Thanks.

-- 
Markus