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

[Pavel Tikhomirov]

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.