[PATCH 0/5] wait_task_* cleanups V2

[PATCH 0/5] wait_task_* cleanups V2

[Vitaly Mayatskikh]

These patches eliminate duplicate code for filling rusage and siginfo
structures in wait_task_*() routines. The work is based on top of recent
Oleg's changes in do_wait().

I'm running kernel with these changes on my laptop for one day, nothing
exploded (yet).

More changes to come.

Vitaly Mayatskikh (5):
  Split wait_noreap_copyout()
  Use copy_wait_opts_to_user() in wait_task_stopped()
  Use copy_wait_opts_to_user() in do_wait()
  Use copy_wait_opts_to_user() in wait_task_zombie()
  Use copy_wait_opts_to_user() in wait_task_continued()

 kernel/exit.c |  136 +++++++++++++++++++-------------------------------------
 1 files changed, 46 insertions(+), 90 deletions(-)

[PATCH 1/5] Split wait_noreap_copyout()

[Vitaly Mayatskikh]

Move getrusage() and put_user() code from wait_noreap_copyout()
to copy_wait_opts_to_user(). The same code is spreaded across all
wait_task_*() routines, it's better to reuse one copy.

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
 kernel/exit.c |   39 +++++++++++++++++++++++----------------
 1 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 25782da..9546362 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1123,27 +1123,34 @@ static int eligible_child(struct wait_opts *wo, struct task_struct *p)
 	return 1;
 }
 
-static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
-				pid_t pid, uid_t uid, int why, int status)
+static int copy_wait_opts_to_user(struct wait_opts *wo, struct task_struct *p,
+				  pid_t pid, uid_t uid, int why, int status, int signal)
 {
-	struct siginfo __user *infop;
+	struct siginfo __user *infop = wo->wo_info;
 	int retval = wo->wo_rusage
 		? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
 
+	if (!retval && infop) {
+		retval = put_user(signal, &infop->si_signo);
+		if (!retval)
+			retval = put_user(0, &infop->si_errno);
+		if (!retval)
+			retval = put_user((short)why, &infop->si_code);
+		if (!retval)
+			retval = put_user(pid, &infop->si_pid);
+		if (!retval)
+			retval = put_user(uid, &infop->si_uid);
+		if (!retval)
+			retval = put_user(status, &infop->si_status);
+	}
+	return retval;
+}
+
+static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
+				pid_t pid, uid_t uid, int why, int status)
+{
+	int retval = copy_wait_opts_to_user(wo, p, pid, uid, why, status, SIGCHLD);
 	put_task_struct(p);
-	infop = wo->wo_info;
-	if (!retval)
-		retval = put_user(SIGCHLD, &infop->si_signo);
-	if (!retval)
-		retval = put_user(0, &infop->si_errno);
-	if (!retval)
-		retval = put_user((short)why, &infop->si_code);
-	if (!retval)
-		retval = put_user(pid, &infop->si_pid);
-	if (!retval)
-		retval = put_user(uid, &infop->si_uid);
-	if (!retval)
-		retval = put_user(status, &infop->si_status);
 	if (!retval)
 		retval = pid;
 	return retval;
-- 
1.6.2.2

[PATCH 2/5] Use copy_wait_opts_to_user() in wait_task_stopped()

[Vitaly Mayatskikh]

All copy-paste getrusage() and put_user() code in wait_task_* functions
is replaced by call to copy_wait_opts_to_user()

Also, there's no reason to have two almost similar branches of copy out
in wait_task_stopped(). The later branch also puts stat_addr to user,
but it can't affect WNOWAIT flag, and it's ok to merge both branches.

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
 kernel/exit.c |   20 +-------------------
 1 files changed, 1 insertions(+), 19 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 9546362..0bf2d3c 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1345,7 +1345,6 @@ static int *task_stopped_code(struct task_struct *p, bool ptrace)
 static int wait_task_stopped(struct wait_opts *wo,
 				int ptrace, struct task_struct *p)
 {
-	struct siginfo __user *infop;
 	int retval, exit_code, *p_code, why;
 	uid_t uid = 0; /* unneeded, required by compiler */
 	pid_t pid;
@@ -1389,27 +1388,10 @@ unlock_sig:
 	why = ptrace ? CLD_TRAPPED : CLD_STOPPED;
 	read_unlock(&tasklist_lock);
 
-	if (unlikely(wo->wo_flags & WNOWAIT))
-		return wait_noreap_copyout(wo, p, pid, uid, why, exit_code);
+	retval = copy_wait_opts_to_user(wo, p, pid, uid, why, exit_code, SIGCHLD);
 
-	retval = wo->wo_rusage
-		? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
 	if (!retval && wo->wo_stat)
 		retval = put_user((exit_code << 8) | 0x7f, wo->wo_stat);
-
-	infop = wo->wo_info;
-	if (!retval && infop)
-		retval = put_user(SIGCHLD, &infop->si_signo);
-	if (!retval && infop)
-		retval = put_user(0, &infop->si_errno);
-	if (!retval && infop)
-		retval = put_user((short)why, &infop->si_code);
-	if (!retval && infop)
-		retval = put_user(exit_code, &infop->si_status);
-	if (!retval && infop)
-		retval = put_user(pid, &infop->si_pid);
-	if (!retval && infop)
-		retval = put_user(uid, &infop->si_uid);
 	if (!retval)
 		retval = pid;
 	put_task_struct(p);
-- 
1.6.2.2

[PATCH 3/5] Use copy_wait_opts_to_user() in do_wait()

[Vitaly Mayatskikh]

All copy-paste getrusage() and put_user() code in wait_task_* functions
is replaced by call to copy_wait_opts_to_user()

Use copy_wait_opts_to_user() in do_wait() to clean up user's siginfo structure.

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
 kernel/exit.c |   15 +--------------
 1 files changed, 1 insertions(+), 14 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 0bf2d3c..a742ae9 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1600,8 +1600,6 @@ end:
 	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(&current->signal->wait_chldexit,&wait);
 	if (wo->wo_info) {
-		struct siginfo __user *infop = wo->wo_info;
-
 		if (retval > 0)
 			retval = 0;
 		else {
@@ -1610,18 +1608,7 @@ end:
 			 * we would set so the user can easily tell the
 			 * difference.
 			 */
-			if (!retval)
-				retval = put_user(0, &infop->si_signo);
-			if (!retval)
-				retval = put_user(0, &infop->si_errno);
-			if (!retval)
-				retval = put_user(0, &infop->si_code);
-			if (!retval)
-				retval = put_user(0, &infop->si_pid);
-			if (!retval)
-				retval = put_user(0, &infop->si_uid);
-			if (!retval)
-				retval = put_user(0, &infop->si_status);
+			retval = copy_wait_opts_to_user(wo, 0, 0, 0, 0, 0, 0);
 		}
 	}
 	return retval;
-- 
1.6.2.2

[PATCH 4/5] Use copy_wait_opts_to_user() in wait_task_zombie()

[Vitaly Mayatskikh]

All copy-paste getrusage() and put_user() code in wait_task_* functions
is replaced by call to copy_wait_opts_to_user()

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
 kernel/exit.c |   39 +++++++++++----------------------------
 1 files changed, 11 insertions(+), 28 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index a742ae9..9806332 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1165,17 +1165,15 @@ static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
 static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
 {
 	unsigned long state;
-	int retval, status, traced;
+	int retval, why, status, traced;
 	pid_t pid = task_pid_vnr(p);
 	uid_t uid = __task_cred(p)->uid;
-	struct siginfo __user *infop;
 
 	if (!likely(wo->wo_flags & WEXITED))
 		return 0;
 
 	if (unlikely(wo->wo_flags & WNOWAIT)) {
 		int exit_code = p->exit_code;
-		int why, status;
 
 		get_task_struct(p);
 		read_unlock(&tasklist_lock);
@@ -1267,36 +1265,21 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
 	 */
 	read_unlock(&tasklist_lock);
 
-	retval = wo->wo_rusage
-		? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
 	status = (p->signal->flags & SIGNAL_GROUP_EXIT)
 		? p->signal->group_exit_code : p->exit_code;
-	if (!retval && wo->wo_stat)
+	if (wo->wo_stat)
 		retval = put_user(status, wo->wo_stat);
 
-	infop = wo->wo_info;
-	if (!retval && infop)
-		retval = put_user(SIGCHLD, &infop->si_signo);
-	if (!retval && infop)
-		retval = put_user(0, &infop->si_errno);
-	if (!retval && infop) {
-		int why;
-
-		if ((status & 0x7f) == 0) {
-			why = CLD_EXITED;
-			status >>= 8;
-		} else {
-			why = (status & 0x80) ? CLD_DUMPED : CLD_KILLED;
-			status &= 0x7f;
-		}
-		retval = put_user((short)why, &infop->si_code);
-		if (!retval)
-			retval = put_user(status, &infop->si_status);
+	if ((status & 0x7f) == 0) {
+		why = CLD_EXITED;
+		status >>= 8;
+	} else {
+		why = (status & 0x80) ? CLD_DUMPED : CLD_KILLED;
+		status &= 0x7f;
 	}
-	if (!retval && infop)
-		retval = put_user(pid, &infop->si_pid);
-	if (!retval && infop)
-		retval = put_user(uid, &infop->si_uid);
+
+	retval = copy_wait_opts_to_user(wo, p, pid, uid, why, status, SIGCHLD);
+
 	if (!retval)
 		retval = pid;
 
-- 
1.6.2.2

[PATCH 5/5] Use copy_wait_opts_to_user() in wait_task_continued()

[Vitaly Mayatskikh]

All copy-paste getrusage() and put_user() code in wait_task_* functions
is replaced by call to copy_wait_opts_to_user()

It makes no sense to do conditional siginfo's fill, copy_wait_opts_to_user() knows
how to handle it right.

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
 kernel/exit.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 9806332..f22e82c 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1416,19 +1416,16 @@ static int wait_task_continued(struct wait_opts *wo, struct task_struct *p)
 	get_task_struct(p);
 	read_unlock(&tasklist_lock);
 
-	if (!wo->wo_info) {
-		retval = wo->wo_rusage
-			? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
-		put_task_struct(p);
-		if (!retval && wo->wo_stat)
-			retval = put_user(0xffff, wo->wo_stat);
-		if (!retval)
-			retval = pid;
-	} else {
-		retval = wait_noreap_copyout(wo, p, pid, uid,
-					     CLD_CONTINUED, SIGCONT);
-		BUG_ON(retval == 0);
-	}
+	retval = copy_wait_opts_to_user(wo, p, pid, uid,
+					CLD_CONTINUED, SIGCONT, SIGCHLD);
+	put_task_struct(p);
+
+	if (!retval && wo->wo_stat)
+		retval = put_user(0xffff, wo->wo_stat);
+	if (!retval)
+		retval = pid;
+
+	BUG_ON(retval == 0);
 
 	return retval;
 }
-- 
1.6.2.2

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Andrew Morton]

On Mon, 11 May 2009 15:25:50 +0200
Vitaly Mayatskikh <v.mayatskih@gmail.com> wrote:

> +	if (!retval && infop) {
> +		retval = put_user(signal, &infop->si_signo);
> +		if (!retval)
> +			retval = put_user(0, &infop->si_errno);
> +		if (!retval)
> +			retval = put_user((short)why, &infop->si_code);
> +		if (!retval)
> +			retval = put_user(pid, &infop->si_pid);
> +		if (!retval)
> +			retval = put_user(uid, &infop->si_uid);
> +		if (!retval)
> +			retval = put_user(status, &infop->si_status);
> +	}

I'll assume that "More changes to come" includes making all this somewhat
less inefficient ;)

Re: [PATCH 0/5] wait_task_* cleanups V2

[Roland McGrath]

Oleg is out this week, and we should let him react.
It looks generally good to me.

Can't the wo_stat handling be rolled into copy_wait_opts_to_user too?
Also it looks like every(?) caller does:
	if (!retval)
		retval = pid;
so why not roll that into copy_wait_opts_to_user too?


Thanks,
Roland

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Vitaly Mayatskikh]

At Mon, 11 May 2009 15:25:50 +0200, Vitaly Mayatskikh wrote:
> 
> Move getrusage() and put_user() code from wait_noreap_copyout()
> to copy_wait_opts_to_user(). The same code is spreaded across all
> wait_task_*() routines, it's better to reuse one copy.
> 
> Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
> ---
>  kernel/exit.c |   39 +++++++++++++++++++++++----------------
>  1 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 25782da..9546362 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1123,27 +1123,34 @@ static int eligible_child(struct wait_opts *wo, struct task_struct *p)
>  	return 1;
>  }
>  
> -static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
> -				pid_t pid, uid_t uid, int why, int status)
> +static int copy_wait_opts_to_user(struct wait_opts *wo, struct task_struct *p,
> +				  pid_t pid, uid_t uid, int why, int status, int signal)
>  {
> -	struct siginfo __user *infop;
> +	struct siginfo __user *infop = wo->wo_info;
>  	int retval = wo->wo_rusage
>  		? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
>  
> +	if (!retval && infop) {
> +		retval = put_user(signal, &infop->si_signo);
...
> +static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
> +				pid_t pid, uid_t uid, int why, int status)
> +{
> +	int retval = copy_wait_opts_to_user(wo, p, pid, uid, why, status, SIGCHLD);
>  	put_task_struct(p);
> -	infop = wo->wo_info;
> -	if (!retval)
> -		retval = put_user(SIGCHLD, &infop->si_signo);
...

Oleg has pointed me to broken behaviour here. Previously
wait_noreap_copyout was doing unconditional put_user and was returning
EFAULT when infop is NULL. Now it uses copy_wait_opts_to_user, which
checks infop and return NULL in the same case. This change is visible
from userspace in waitid() function.

There're 2 opportunities how to deal with new behaviour:

1. Assume wait_task_zombie had a bug previously, and let this patch go.
2. Fix copy_wait_opts_to_user to old behaviour by something like:

	if (!retval && (infop || WNOWAIT)) {

What's your opinion?

-- 
wbr, Vitaly

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Oleg Nesterov]

On 05/20, Vitaly Mayatskikh wrote:
>
> At Mon, 11 May 2009 15:25:50 +0200, Vitaly Mayatskikh wrote:
> >
> Oleg has pointed me to broken behaviour here. Previously
> wait_noreap_copyout was doing unconditional put_user and was returning
> EFAULT when infop is NULL. Now it uses copy_wait_opts_to_user, which
> checks infop and return NULL in the same case. This change is visible
> from userspace in waitid() function.

To me, this behaviour change looks like the cleanup (if not fix) too.
But of course, this should be discussed and at least documented in the
changelog.

do_wait() && infop interaction is really strange before the patch.

When do_wait() is called without WNOWAIT, then infop == NULL is fine.

If WNOWAIT is set, we return -EFAULT. Except in WCONTINUED case
infop == NULL is fine again.


Can somebody explain what is the supposed behaviour?


Otherwise, in my opinion this series realy makes the code better,
and afaics it allows to simplify the code more.

Oleg.

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Ingo Molnar]

* Vitaly Mayatskikh <v.mayatskih@gmail.com> wrote:

> At Mon, 11 May 2009 15:25:50 +0200, Vitaly Mayatskikh wrote:
> > 
> > Move getrusage() and put_user() code from wait_noreap_copyout()
> > to copy_wait_opts_to_user(). The same code is spreaded across all
> > wait_task_*() routines, it's better to reuse one copy.
> > 
> > Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
> > ---
> >  kernel/exit.c |   39 +++++++++++++++++++++++----------------
> >  1 files changed, 23 insertions(+), 16 deletions(-)
> > 
> > diff --git a/kernel/exit.c b/kernel/exit.c
> > index 25782da..9546362 100644
> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -1123,27 +1123,34 @@ static int eligible_child(struct wait_opts *wo, struct task_struct *p)
> >  	return 1;
> >  }
> >  
> > -static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
> > -				pid_t pid, uid_t uid, int why, int status)
> > +static int copy_wait_opts_to_user(struct wait_opts *wo, struct task_struct *p,
> > +				  pid_t pid, uid_t uid, int why, int status, int signal)
> >  {
> > -	struct siginfo __user *infop;
> > +	struct siginfo __user *infop = wo->wo_info;
> >  	int retval = wo->wo_rusage
> >  		? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
> >  
> > +	if (!retval && infop) {
> > +		retval = put_user(signal, &infop->si_signo);
> ...
> > +static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
> > +				pid_t pid, uid_t uid, int why, int status)
> > +{
> > +	int retval = copy_wait_opts_to_user(wo, p, pid, uid, why, status, SIGCHLD);
> >  	put_task_struct(p);
> > -	infop = wo->wo_info;
> > -	if (!retval)
> > -		retval = put_user(SIGCHLD, &infop->si_signo);
> ...
> 
> Oleg has pointed me to broken behaviour here. Previously
> wait_noreap_copyout was doing unconditional put_user and was returning
> EFAULT when infop is NULL. Now it uses copy_wait_opts_to_user, which
> checks infop and return NULL in the same case. This change is visible
> from userspace in waitid() function.
> 
> There're 2 opportunities how to deal with new behaviour:
> 
> 1. Assume wait_task_zombie had a bug previously, and let this patch go.
> 2. Fix copy_wait_opts_to_user to old behaviour by something like:
> 
> 	if (!retval && (infop || WNOWAIT)) {
> 
> What's your opinion?

I'd suggest a variant of 2: keep this large-ish patch an equivalent 
transformation - i.e. an impact: cleanup type of change.

Then queue up a patch that removes this quirk. Should this change 
break any user-space, the quirk can be reinstated promptly, without 
affecting the cleanups. It will also be a very clear, easy target to 
bisect to - not obscured by clean-up details.

Does this sound good to you?

	Ingo

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Roland McGrath]

> do_wait() && infop interaction is really strange before the patch.

do_wait() underlies sys_wait4() and sys_waitid().  The original intent was
that all the infop==NULL cases are just for the sys_wait4() path.  In the
sys_waitid() path, infop comes from the user and NULL always ought to have
been invalid.

See http://lkml.org/lkml/2009/1/13/446 for the previous thread about this.
We wanted to clean it up, but Linus objected to changing the userland
behavior of passing NULL to waitid on the grounds of "never regress the
ABI, even if it was not supposed to be the ABI".

> When do_wait() is called without WNOWAIT, then infop == NULL is fine.
> 
> If WNOWAIT is set, we return -EFAULT. Except in WCONTINUED case
> infop == NULL is fine again.

WNOWAIT can only be set in the sys_waitid() path, not by sys_wait4().
Without WNOWAIT, it might be sys_wait4(), where infop==NULL is normal.
The WCONTINUED variance was unintended.

I would be fine with any way you want to clean this up.
But presumably Linus would object again if any combination of userland
arguments that is now permitted were to start returning an error.
I'm guessing he won't object to making the WNOWAIT case consistent
with other sys_waitid() calls that pass NULL (i.e. -EFAULT -> success
acceptable, but success -> -EFAULT not acceptable).


Thanks,
Roland

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Oleg Nesterov]

On 05/20, Ingo Molnar wrote:
>
> * Vitaly Mayatskikh <v.mayatskih@gmail.com> wrote:
>
> > 2. Fix copy_wait_opts_to_user to old behaviour by something like:
> >
> > 	if (!retval && (infop || WNOWAIT)) {
> >
> > What's your opinion?
>
> I'd suggest a variant of 2: keep this large-ish patch an equivalent
> transformation - i.e. an impact: cleanup type of change.
>
> Then queue up a patch that removes this quirk.

Yes, this would be the best option.

The problem is, it is not trivial to keep the current behaviour and
make the patch which looks like a cleanup, not uglification.

copy_wait_opts_to_user() needs the new "called_from_wait_task_continued"
argument, and it should do

	if (called_from_wait_task_continued && !wo->wo_info)
		return -EFAULT;

Or we should add

	if (!infop && WNOWAIT)
		return -EFAULT;

to all callers except wait_task_continued().


Roland thinks that "-EFAULT -> success" change is acceptable, and I think
the same. So, to me the best option is just  change the changelog of this
patch and that is all.

Or. We can make a trivial patch which adds the behavior change first:

	Changelog: always accept the NULL infop, because it is not
	possible to understand the current behaviour ;)

	User-visible change! needs Acks!

	--- a/kernel/exit.c
	+++ b/kernel/exit.c
	@@ -1126,24 +1126,26 @@ static int eligible_child(struct wait_op
	 static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
					pid_t pid, uid_t uid, int why, int status)
	 {
	-	struct siginfo __user *infop;
		int retval = wo->wo_rusage
			? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
	-
		put_task_struct(p);
	-	infop = wo->wo_info;
	-	if (!retval)
	-		retval = put_user(SIGCHLD, &infop->si_signo);
	-	if (!retval)
	-		retval = put_user(0, &infop->si_errno);
	-	if (!retval)
	-		retval = put_user((short)why, &infop->si_code);
	-	if (!retval)
	-		retval = put_user(pid, &infop->si_pid);
	-	if (!retval)
	-		retval = put_user(uid, &infop->si_uid);
	-	if (!retval)
	-		retval = put_user(status, &infop->si_status);
	+
	+	if (wo->wo_info) {
	+		struct siginfo __user *infop = wo->wo_info;
	+
	+		if (!retval)
	+			retval = put_user(SIGCHLD, &infop->si_signo);
	+		if (!retval)
	+			retval = put_user(0, &infop->si_errno);
	+		if (!retval)
	+			retval = put_user((short)why, &infop->si_code);
	+		if (!retval)
	+			retval = put_user(pid, &infop->si_pid);
	+		if (!retval)
	+			retval = put_user(uid, &infop->si_uid);
	+		if (!retval)
	+			retval = put_user(status, &infop->si_status);
	+	}
		if (!retval)
			retval = pid;
		return retval;

And then redo Vitaly's patches on top of this change.

What do you and Vitaly think?

Oleg.

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Vitaly Mayatskikh]

At Thu, 21 May 2009 16:12:50 +0200, Oleg Nesterov wrote:

> Roland thinks that "-EFAULT -> success" change is acceptable, and I think
> the same. So, to me the best option is just  change the changelog of this
> patch and that is all.
> 
> Or. We can make a trivial patch which adds the behavior change first:
> 
> 	Changelog: always accept the NULL infop, because it is not
> 	possible to understand the current behaviour ;)
> 
> 	User-visible change! needs Acks!
> 
> 	--- a/kernel/exit.c
> 	+++ b/kernel/exit.c
> 	@@ -1126,24 +1126,26 @@ static int eligible_child(struct wait_op
> 	 static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
> 					pid_t pid, uid_t uid, int why, int status)
> 	 {
> 	-	struct siginfo __user *infop;
> 		int retval = wo->wo_rusage
> 			? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
> 	-
> 		put_task_struct(p);
> 	-	infop = wo->wo_info;
> 	-	if (!retval)
> 	-		retval = put_user(SIGCHLD, &infop->si_signo);
> 	-	if (!retval)
> 	-		retval = put_user(0, &infop->si_errno);
> 	-	if (!retval)
> 	-		retval = put_user((short)why, &infop->si_code);
> 	-	if (!retval)
> 	-		retval = put_user(pid, &infop->si_pid);
> 	-	if (!retval)
> 	-		retval = put_user(uid, &infop->si_uid);
> 	-	if (!retval)
> 	-		retval = put_user(status, &infop->si_status);
> 	+
> 	+	if (wo->wo_info) {
> 	+		struct siginfo __user *infop = wo->wo_info;
> 	+
> 	+		if (!retval)
> 	+			retval = put_user(SIGCHLD, &infop->si_signo);
> 	+		if (!retval)
> 	+			retval = put_user(0, &infop->si_errno);
> 	+		if (!retval)
> 	+			retval = put_user((short)why, &infop->si_code);
> 	+		if (!retval)
> 	+			retval = put_user(pid, &infop->si_pid);
> 	+		if (!retval)
> 	+			retval = put_user(uid, &infop->si_uid);
> 	+		if (!retval)
> 	+			retval = put_user(status, &infop->si_status);
> 	+	}
> 		if (!retval)
> 			retval = pid;
> 		return retval;
> 
> And then redo Vitaly's patches on top of this change.
> 
> What do you and Vitaly think?

I like your idea.

-- 
wbr, Vitaly

Re: [PATCH 1/5] Split wait_noreap_copyout()

[Vitaly Mayatskikh]

Hi, Andrew!

Can you, please, update description of
wait_task_-cleanups-split-wait_noreap_copyout.patch?

New description:
----------------------------------------------------------------------
Move getrusage() and put_user() code from wait_noreap_copyout()
to copy_wait_opts_to_user(). The same code is spreaded across all
wait_task_*() routines, it's better to reuse one copy.

User visible change: previously, waitid() was doing unconditional
put_user() in wait_noreap_copyout(), and was returning EFAULT
if infop pointer, provided by user, was NULL *and* WCONTINUED was not
set in options. Now all variants of wait() function use the same generic
copy_wait_opts_to_user(), and, thus, have the same behaviour regarding
to filling of siginfo structure. Starting from this commit waitid()
returns NULL if infop argument is NULL.
----------------------------------------------------------------------

Thanks!

> 
> Move getrusage() and put_user() code from wait_noreap_copyout()
> to copy_wait_opts_to_user(). The same code is spreaded across all
> wait_task_*() routines, it's better to reuse one copy.
> 
> Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
> ---
>  kernel/exit.c |   39 +++++++++++++++++++++++----------------
>  1 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 25782da..9546362 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1123,27 +1123,34 @@ static int eligible_child(struct wait_opts *wo, struct task_struct *p)
>  	return 1;
>  }
>  
> -static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
> -				pid_t pid, uid_t uid, int why, int status)
> +static int copy_wait_opts_to_user(struct wait_opts *wo, struct task_struct *p,
> +				  pid_t pid, uid_t uid, int why, int status, int signal)
>  {
> -	struct siginfo __user *infop;
> +	struct siginfo __user *infop = wo->wo_info;
>  	int retval = wo->wo_rusage
>  		? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
>  
> +	if (!retval && infop) {
> +		retval = put_user(signal, &infop->si_signo);
> +		if (!retval)
> +			retval = put_user(0, &infop->si_errno);
> +		if (!retval)
> +			retval = put_user((short)why, &infop->si_code);
> +		if (!retval)
> +			retval = put_user(pid, &infop->si_pid);
> +		if (!retval)
> +			retval = put_user(uid, &infop->si_uid);
> +		if (!retval)
> +			retval = put_user(status, &infop->si_status);
> +	}
> +	return retval;
> +}
> +
> +static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
> +				pid_t pid, uid_t uid, int why, int status)
> +{
> +	int retval = copy_wait_opts_to_user(wo, p, pid, uid, why, status, SIGCHLD);
>  	put_task_struct(p);
> -	infop = wo->wo_info;
> -	if (!retval)
> -		retval = put_user(SIGCHLD, &infop->si_signo);
> -	if (!retval)
> -		retval = put_user(0, &infop->si_errno);
> -	if (!retval)
> -		retval = put_user((short)why, &infop->si_code);
> -	if (!retval)
> -		retval = put_user(pid, &infop->si_pid);
> -	if (!retval)
> -		retval = put_user(uid, &infop->si_uid);
> -	if (!retval)
> -		retval = put_user(status, &infop->si_status);
>  	if (!retval)
>  		retval = pid;
>  	return retval;
> -- 
> 1.6.2.2
> 
> 

-- 
wbr, Vitaly

Re: [PATCH 3/5] Use copy_wait_opts_to_user() in do_wait()

[Oleg Nesterov]

Damn. I am sorry for the huge delay. Finally I have read this series
carefully.

On 05/11, Vitaly Mayatskikh wrote:
>
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1600,8 +1600,6 @@ end:
>  	__set_current_state(TASK_RUNNING);
>  	remove_wait_queue(&current->signal->wait_chldexit,&wait);
>  	if (wo->wo_info) {
> -		struct siginfo __user *infop = wo->wo_info;
> -
>  		if (retval > 0)
>  			retval = 0;
>  		else {
> @@ -1610,18 +1608,7 @@ end:
>  			 * we would set so the user can easily tell the
>  			 * difference.
>  			 */
> -			if (!retval)
> -				retval = put_user(0, &infop->si_signo);
> -			if (!retval)
> -				retval = put_user(0, &infop->si_errno);
> -			if (!retval)
> -				retval = put_user(0, &infop->si_code);
> -			if (!retval)
> -				retval = put_user(0, &infop->si_pid);
> -			if (!retval)
> -				retval = put_user(0, &infop->si_uid);
> -			if (!retval)
> -				retval = put_user(0, &infop->si_status);
> +			retval = copy_wait_opts_to_user(wo, 0, 0, 0, 0, 0, 0);

This looks wrong.

copy_wait_opts_to_user()->getrusage() will OOPS if ->wo_rusage != NULL,
because we pass p == NULL.

Easy to fix, but I am not sure what is the most clean fix...

Oleg.

Re: [PATCH 4/5] Use copy_wait_opts_to_user() in wait_task_zombie()

[Oleg Nesterov]

On 05/11, Vitaly Mayatskikh wrote:
>
> @@ -1267,36 +1265,21 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
>  	 */
>  	read_unlock(&tasklist_lock);
>
> -	retval = wo->wo_rusage
> -		? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
>  	status = (p->signal->flags & SIGNAL_GROUP_EXIT)
>  		? p->signal->group_exit_code : p->exit_code;
> -	if (!retval && wo->wo_stat)
> +	if (wo->wo_stat)
>  		retval = put_user(status, wo->wo_stat);

We don't check retval, and then later

> [...snip...]
> +	retval = copy_wait_opts_to_user(wo, p, pid, uid, why, status, SIGCHLD);

we overwrite it.

This needs a fix.

Oleg.

Re: [PATCH 5/5] Use copy_wait_opts_to_user() in wait_task_continued()

[Oleg Nesterov]

On 05/11, Vitaly Mayatskikh wrote:
>
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1416,19 +1416,16 @@ static int wait_task_continued(struct wait_opts *wo, struct task_struct *p)
>  	get_task_struct(p);
>  	read_unlock(&tasklist_lock);
>
> -	if (!wo->wo_info) {
> -		retval = wo->wo_rusage
> -			? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
> -		put_task_struct(p);
> -		if (!retval && wo->wo_stat)
> -			retval = put_user(0xffff, wo->wo_stat);
> -		if (!retval)
> -			retval = pid;
> -	} else {
> -		retval = wait_noreap_copyout(wo, p, pid, uid,
> -					     CLD_CONTINUED, SIGCONT);
> -		BUG_ON(retval == 0);
> -	}
> +	retval = copy_wait_opts_to_user(wo, p, pid, uid,
> +					CLD_CONTINUED, SIGCONT, SIGCHLD);
> +	put_task_struct(p);
> +
> +	if (!retval && wo->wo_stat)
> +		retval = put_user(0xffff, wo->wo_stat);

This adds another user-visible change. Before the patch, we ignore
->wo_stat if ->wo_info != NULL.

This change looks like the fix to me, but again, this should be
documented/discussed.


I could send the fixes for 3 and 4, but unfortunately Vitaly has
the vacation now. I think it is better to drop

	wait_task_-cleanups-split-wait_noreap_copyout.patch
	wait_task_-cleanups-use-copy_wait_opts_to_user-in-wait_task_stopped.patch
	wait_task_-cleanups-use-copy_wait_opts_to_user-in-do_wait.patch
	wait_task_-cleanups-use-copy_wait_opts_to_user-in-wait_task_zombie.patch
	wait_task_-cleanups-use-copy_wait_opts_to_user-in-wait_task_continued.patch

patches, and wait for Vitaly to redo/resend.

Oleg.

Re: [PATCH 5/5] Use copy_wait_opts_to_user() in wait_task_continued()

[Oleg Nesterov]

On 06/15, Oleg Nesterov wrote:
>
> On 05/11, Vitaly Mayatskikh wrote:
> >
> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -1416,19 +1416,16 @@ static int wait_task_continued(struct wait_opts *wo, struct task_struct *p)
> >  	get_task_struct(p);
> >  	read_unlock(&tasklist_lock);
> >
> > -	if (!wo->wo_info) {
> > -		retval = wo->wo_rusage
> > -			? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
> > -		put_task_struct(p);
> > -		if (!retval && wo->wo_stat)
> > -			retval = put_user(0xffff, wo->wo_stat);
> > -		if (!retval)
> > -			retval = pid;
> > -	} else {
> > -		retval = wait_noreap_copyout(wo, p, pid, uid,
> > -					     CLD_CONTINUED, SIGCONT);
> > -		BUG_ON(retval == 0);
> > -	}
> > +	retval = copy_wait_opts_to_user(wo, p, pid, uid,
> > +					CLD_CONTINUED, SIGCONT, SIGCHLD);
> > +	put_task_struct(p);
> > +
> > +	if (!retval && wo->wo_stat)
> > +		retval = put_user(0xffff, wo->wo_stat);
>
> This adds another user-visible change. Before the patch, we ignore
> ->wo_stat if ->wo_info != NULL.

Ugh! sorry I was wrong.

->wo_info != NULL means waitid() which sets ->wo_stat = NULL, so this
change is correct.

> I could send the fixes for 3 and 4, but unfortunately Vitaly has
> the vacation now. I think it is better to drop
>
> 	wait_task_-cleanups-split-wait_noreap_copyout.patch
> 	wait_task_-cleanups-use-copy_wait_opts_to_user-in-wait_task_stopped.patch
> 	wait_task_-cleanups-use-copy_wait_opts_to_user-in-do_wait.patch
> 	wait_task_-cleanups-use-copy_wait_opts_to_user-in-wait_task_zombie.patch
> 	wait_task_-cleanups-use-copy_wait_opts_to_user-in-wait_task_continued.patch
>
> patches, and wait for Vitaly to redo/resend.

Still I think this would be right :/


Unless Vitaly can see my emails and reply.

Oleg.

Re: [PATCH 5/5] Use copy_wait_opts_to_user() in wait_task_continued()

[Andrew Morton]

On Mon, 15 Jun 2009 18:55:36 +0200 Oleg Nesterov <oleg@redhat.com> wrote:

> I think it is better to drop
> 
> 	wait_task_-cleanups-split-wait_noreap_copyout.patch
> 	wait_task_-cleanups-use-copy_wait_opts_to_user-in-wait_task_stopped.patch
> 	wait_task_-cleanups-use-copy_wait_opts_to_user-in-do_wait.patch
> 	wait_task_-cleanups-use-copy_wait_opts_to_user-in-wait_task_zombie.patch
> 	wait_task_-cleanups-use-copy_wait_opts_to_user-in-wait_task_continued.patch
> 
> patches, and wait for Vitaly to redo/resend.

OK, done.  Thanks, Oleg.