Re: [PATCH] prctl: propagate has_child_subreaper flag to every descendant

[Pavel Tikhomirov <ptikhomirov@virtuozzo.com>]

Sorry I had some problem with mail-agent, resend to be on the safe side.

On 01/20/2017 09:14 PM, Oleg Nesterov wrote:
> On 01/19, Pavel Tikhomirov wrote:
>>
>> Having these two
>> differently behaving groups can lead to confusion. Also it is
>> a problem for CRIU, as when we restore process tree we need to
>> somehow determine which descendants belong to which group and
>> much harder - to put them exactly to these group.
>
> Hmm. could you explain how this change helps CRIU? I mean, why
> restorer can't do prctl(CHILD_SUBREAPER) before the first fork?

Imagine we have these tree in pidns:

1: has_child_subreaper == 0 && is_child_subreaper == 0
|-2: has_child_subreaper == 0 && is_child_subreaper == 1
| |-3: has_child_subreaper == 0 && is_child_subreaper == 0
| | |-5: has_child_subreaper == 0 && is_child_subreaper == 0
| |-4: has_child_subreaper == 1 && is_child_subreaper == 0
| | |-6: has_child_subreaper == 1 && is_child_subreaper == 0

before c/r: If 4 dies 6 will reparent to 2, if 3 dies 5 will reparent to 1.
after c/r: (where restorer had is_child_subreaper == 1, everybody in the 
tree will have has_child_subreaper == 1) Everybody will reparent to 2.

>
> Anyway, afaics the patch is sub-optimal and not correct...
>
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1715,6 +1715,8 @@ struct task_struct {
>>  	struct signal_struct *signal;
>>  	struct sighand_struct *sighand;
>>
>> +	struct list_head csr_descendant;
>> +
>
> You don't need this new member and descendants_lock. task_struct has
> the ->real_parent pointer so you can work the tree without recursion.

Sorry I don't get how I can walk down the tree of all descendants with 
help of ->real_parent pointer, can you please point on some example or 
explain a bit more? (I see task_is_descendant() in 
security/yama/yama_lsm.c but we will need to check it for every process, 
not only descendants, the latter can be a lot faster.)

>
>> +static void prctl_set_child_subreaper(struct task_struct *reaper, bool arg2)
>> +{
>> +	LIST_HEAD(descendants);
>> +
>> +	reaper->signal->is_child_subreaper = arg2;
>> +	if (!arg2)
>> +		return;
>> +
>> +	spin_lock(&descendants_lock);
>> +	read_lock(&tasklist_lock);
>> +
>> +	list_add(&reaper->csr_descendant, &descendants);
>> +
>> +	while (!list_empty(&descendants)) {
>> +		struct task_struct *tsk;
>> +		struct task_struct *p;
>> +
>> +		tsk = list_first_entry(&descendants, struct task_struct,
>> +				csr_descendant);
>> +
>> +		list_for_each_entry(p, &tsk->children, sibling) {
>
> This is not enough. Every thread has its own ->children list, you need
> to walk the sub-threads as well.

Will do.

>
>> +			 * If we've found child_reaper - skip descendants in
>> +			 * it's subtree as they will never get out pidns
>> +			 */
>> +			if (is_child_reaper(task_pid(p)))
>> +				continue;
>
> Again, a child reaper can be multi-threaded, this check can be false
> negative.
>
> Probably is_child_reaper() should be renamed somehow and a new helper
> makes sense... something like

Will do.

>
> 	bool task_is_child_reaper(struct task_struct *p)
> 	{
> 		return same_thread_group(p, task_active_pid_ns(p)->child_reaper);
> 	}
>
> Oleg.
>

-- 
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.