Re: [PATCH] prctl: add PR_{SET,GET}_CHILD_SUBREAPER to allow simpleprocess supervision

[Kay Sievers]

On Sat, Jan 14, 2012 at 01:35, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, 9 Jan 2012 16:07:01 +0100
> Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
>> > > + Â Â Â Â /* find the first ancestor marked as child_subreaper */
>> > > + Â Â Â Â for (reaper = father->real_parent;
>> > > + Â Â Â Â Â Â Âreaper != &init_task;
>> > > + Â Â Â Â Â Â Âreaper = reaper->real_parent) {
>> >
>> > I admit being insufficiently caffienated - does this DTRT in a PID namespace? That
>> > &init_task looks fishy to me...
>>
>> Probably this needs a comment. Initially I was confused too.
>>
>> Note that the code below checks same_thread_group(reaper, pid_ns->child_reaper),
>> this is what we need to DTRT in a PID namespace. However we still need the
>> check above, see http://marc.info/?l=linux-kernel&m=131385460420380
>
> In light of Kay's haughty silence, I did this:
>
> --- a/kernel/exit.c~prctl-add-pr_setget_child_subreaper-to-allow-simple-process-supervision-fix-fix
> +++ a/kernel/exit.c
> @@ -724,7 +724,13 @@ static struct task_struct *find_new_reap
> Â Â Â Â} else if (father->signal->has_child_subreaper) {
> Â Â Â Â Â Â Â Âstruct task_struct *reaper;
>
> - Â Â Â Â Â Â Â /* find the first ancestor marked as child_subreaper */
> + Â Â Â Â Â Â Â /*
> + Â Â Â Â Â Â Â Â* Find the first ancestor marked as child_subreaper.
> + Â Â Â Â Â Â Â Â* Note that the code below checks same_thread_group(reaper,
> + Â Â Â Â Â Â Â Â* pid_ns->child_reaper). ÂThis is what we need to DTRT in a
> + Â Â Â Â Â Â Â Â* PID namespace. However we still need the check above, see
> + Â Â Â Â Â Â Â Â* http://marc.info/?l=linux-kernel&m=131385460420380
> + Â Â Â Â Â Â Â Â*/
> Â Â Â Â Â Â Â Âfor (reaper = father->real_parent;
> Â Â Â Â Â Â Â Â Â Â reaper != &init_task;
> Â Â Â Â Â Â Â Â Â Â reaper = reaper->real_parent) {

Oleg and I got nowhere really, discussing if we should switch the
check in the for() parameters and the one inside the loop; so we just
decided to keep it the way it is, and did not reply any further. It
wasn't that we didn't care, we just couldn't decide. Sorry for that.

The comment indeed sounds good to have. Thanks for adding this.

Kay
--
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/