[PATCH -mm 2/2] do_wait: cleanup delay_group_leader() usage

[PATCH -mm 2/2] do_wait: cleanup delay_group_leader() usage

[Oleg Nesterov]

eligible_child() == 2 means delay_group_leader(). With the previous patch this
only matters for EXIT_ZOMBIE task, we can move that special check to the only
place it is really needed.

Also, with this patch we don't skip security_task_wait() for the group leaders
in a non-empty thread group. I don't really understand the exact semantics of
security_task_wait(), but imho this change is a bugfix.

Also rearrange the code a bit to kill an ugly "check_continued" backdoor.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- PT/kernel/exit.c~6_delay_leader	2007-11-23 20:31:21.000000000 +0300
+++ PT/kernel/exit.c	2007-11-23 21:29:44.000000000 +0300
@@ -1137,12 +1137,6 @@ static int eligible_child(pid_t pid, int
 	if (((p->exit_signal != SIGCHLD) ^ ((options & __WCLONE) != 0))
 	    && !(options & __WALL))
 		return 0;
-	/*
-	 * Do not consider thread group leaders that are
-	 * in a non-empty thread group:
-	 */
-	if (delay_group_leader(p))
-		return 2;
 
 	err = security_task_wait(p);
 	if (err)
@@ -1494,10 +1488,9 @@ repeat:
 	tsk = current;
 	do {
 		struct task_struct *p;
-		int ret;
 
 		list_for_each_entry(p, &tsk->children, sibling) {
-			ret = eligible_child(pid, options, p);
+			int ret = eligible_child(pid, options, p);
 			if (!ret)
 				continue;
 
@@ -1521,19 +1514,17 @@ repeat:
 				retval = wait_task_stopped(p,
 						(options & WNOWAIT), infop,
 						stat_addr, ru);
-			} else if (p->exit_state == EXIT_ZOMBIE) {
+			} else if (p->exit_state == EXIT_ZOMBIE &&
+					!delay_group_leader(p)) {
 				/*
-				 * Eligible but we cannot release it yet:
+				 * We don't reap group leaders with subthreads.
 				 */
-				if (ret == 2)
-					goto check_continued;
 				if (!likely(options & WEXITED))
 					continue;
 				retval = wait_task_zombie(p,
 						(options & WNOWAIT), infop,
 						stat_addr, ru);
 			} else if (p->exit_state != EXIT_DEAD) {
-check_continued:
 				/*
 				 * It's running now, so it might later
 				 * exit, stop, or stop and then continue.

Re: [PATCH -mm 2/2] do_wait: cleanup delay_group_leader() usage

[James Morris]

[added LSM list]

On Fri, 23 Nov 2007, Oleg Nesterov wrote:

> eligible_child() == 2 means delay_group_leader(). With the previous patch this
> only matters for EXIT_ZOMBIE task, we can move that special check to the only
> place it is really needed.
> 
> Also, with this patch we don't skip security_task_wait() for the group leaders
> in a non-empty thread group. I don't really understand the exact semantics of
> security_task_wait(), but imho this change is a bugfix.

The semantics in terms of SELinux are that calls to wait() type calls need 
to be mediated by policy, as information can be transfered via the exit 
status.

It looks like a correct bugfix to me, but I'd be interested to see what 
others have to say.



> Also rearrange the code a bit to kill an ugly "check_continued" backdoor.
> 
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> 
> --- PT/kernel/exit.c~6_delay_leader	2007-11-23 20:31:21.000000000 +0300
> +++ PT/kernel/exit.c	2007-11-23 21:29:44.000000000 +0300
> @@ -1137,12 +1137,6 @@ static int eligible_child(pid_t pid, int
>  	if (((p->exit_signal != SIGCHLD) ^ ((options & __WCLONE) != 0))
>  	    && !(options & __WALL))
>  		return 0;
> -	/*
> -	 * Do not consider thread group leaders that are
> -	 * in a non-empty thread group:
> -	 */
> -	if (delay_group_leader(p))
> -		return 2;
>  
>  	err = security_task_wait(p);
>  	if (err)
> @@ -1494,10 +1488,9 @@ repeat:
>  	tsk = current;
>  	do {
>  		struct task_struct *p;
> -		int ret;
>  
>  		list_for_each_entry(p, &tsk->children, sibling) {
> -			ret = eligible_child(pid, options, p);
> +			int ret = eligible_child(pid, options, p);
>  			if (!ret)
>  				continue;
>  
> @@ -1521,19 +1514,17 @@ repeat:
>  				retval = wait_task_stopped(p,
>  						(options & WNOWAIT), infop,
>  						stat_addr, ru);
> -			} else if (p->exit_state == EXIT_ZOMBIE) {
> +			} else if (p->exit_state == EXIT_ZOMBIE &&
> +					!delay_group_leader(p)) {
>  				/*
> -				 * Eligible but we cannot release it yet:
> +				 * We don't reap group leaders with subthreads.
>  				 */
> -				if (ret == 2)
> -					goto check_continued;
>  				if (!likely(options & WEXITED))
>  					continue;
>  				retval = wait_task_zombie(p,
>  						(options & WNOWAIT), infop,
>  						stat_addr, ru);
>  			} else if (p->exit_state != EXIT_DEAD) {
> -check_continued:
>  				/*
>  				 * It's running now, so it might later
>  				 * exit, stop, or stop and then continue.
> 

-- 
James Morris
<jmorris@namei.org>