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

From: James Morris
Date: Mon Nov 26 2007 - 19:53:20 EST


[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@xxxxxxxxxx>
>
> --- 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@xxxxxxxxx>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/