[PATCH v2 0/2] prctl: make PR_SET_CHILD_SUBREAPER deterministic

[PATCH v2 0/2] prctl: make PR_SET_CHILD_SUBREAPER deterministic

[Pavel Tikhomirov]

will send documentation change proposal in reply to these letter

Oleg Nesterov (1):
  introduce the walk_process_tree() helper

Pavel Tikhomirov (1):
  prctl: propagate has_child_subreaper flag to every descendant

 include/linux/sched.h |  5 +++++
 kernel/fork.c         | 42 +++++++++++++++++++++++++++++++++++++++---
 kernel/sys.c          | 22 ++++++++++++++++++++++
 3 files changed, 66 insertions(+), 3 deletions(-)

-- 
2.9.3

[PATCH 1/2] introduce the walk_process_tree() helper

[Pavel Tikhomirov]

From: Oleg Nesterov <oleg@redhat.com>

Add the new helper to walk the process tree, the next patch adds a user.
Note that it visits the group leaders only, proc_visitor can do
for_each_thread itself or we can trivially extend walk_process_tree() to
do this.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 include/linux/sched.h |  3 +++
 kernel/fork.c         | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index ad3ec9e..7f8ab91 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -3067,6 +3067,9 @@ extern bool current_is_single_threaded(void);
 #define for_each_process_thread(p, t)	\
 	for_each_process(p) for_each_thread(p, t)
 
+typedef int (*proc_visitor)(struct task_struct *p, void *data);
+void walk_process_tree(struct task_struct *top, proc_visitor, void *);
+
 static inline int get_nr_threads(struct task_struct *tsk)
 {
 	return tsk->signal->nr_threads;
diff --git a/kernel/fork.c b/kernel/fork.c
index 11c5c8a..135b7a4 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2053,6 +2053,38 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, unsigned long, newsp,
 }
 #endif
 
+void walk_process_tree(struct task_struct *top, proc_visitor visitor, void *data)
+{
+	struct task_struct *leader, *parent, *child;
+	int res;
+
+	read_lock(&tasklist_lock);
+	leader = top = top->group_leader;
+down:
+	for_each_thread(leader, parent) {
+		list_for_each_entry(child, &parent->children, sibling) {
+			res = visitor(child, data);
+			if (res) {
+				if (res < 0)
+					goto out;
+				leader = child;
+				goto down;
+			}
+up:
+			;
+		}
+	}
+
+	if (leader != top) {
+		child = leader;
+		parent = child->real_parent;
+		leader = parent->group_leader;
+		goto up;
+	}
+out:
+	read_unlock(&tasklist_lock);
+}
+
 #ifndef ARCH_MIN_MMSTRUCT_ALIGN
 #define ARCH_MIN_MMSTRUCT_ALIGN 0
 #endif
-- 
2.9.3

[PATCH v2 2/2] prctl: propagate has_child_subreaper flag to every descendant

[Pavel Tikhomirov]

If process forks some children when it has is_child_subreaper
flag enabled they will inherit has_child_subreaper flag - first
group, when is_child_subreaper is disabled forked children will
not inherit it - second group. So child-subreaper does not reparent
all his descendants when their parents die. 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.

To simplify these we can add a propagation of has_child_subreaper
flag on PR_SET_CHILD_SUBREAPER, walking all descendants of child-
subreaper to setup has_child_subreaper flag.

In common cases when process like systemd first sets itself to
be a child-subreaper and only after that forks its services, we will
have zero-length list of descendants to walk. Testing with binary
subtree of 2^15 processes prctl took < 0.007 sec and has shown close
to linear dependency(~0.2 * n * usec) on lower numbers of processes.

Moreover, I doubt someone intentionaly pre-forks the children whitch
should reparent to init before becoming subreaper, because some our
ancestor migh have had is_child_subreaper flag while forking our
sub-tree and our childs will all inherit has_child_subreaper flag,
and we have no way to influence it. And only way to check if we have
no has_child_subreaper flag is to create some childs, kill them
and see where they will reparent to.

Use walk_process_tree helper to walk subtree, thanks to Oleg! Timing
seems to be the same.

Optimize:

a) When descendant already has has_child_subreaper flag all his subtree
has it too already.

* for a) to be true need to move has_child_subreaper inheritance under
the same tasklist_lock with adding task to its ->real_parent->children
as without it process can inherit zero has_child_subreaper, then we
set 1 to it's parent flag, check that parent has no more children, and
only after child with wrong flag is added to the tree.

b) When some descendant is child_reaper, it's subtree is in different
pidns from us(original child-subreaper) and processes from other pidns
will never reparent to us.

So we can skip their(a,b) subtree from walk.

v2: switch to walk_process_tree() general helper, move
has_child_subreaper inheritance

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 include/linux/sched.h |  2 ++
 kernel/fork.c         | 10 +++++++---
 kernel/sys.c          | 22 ++++++++++++++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 7f8ab91..a08f006 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1725,6 +1725,8 @@ struct task_struct {
 	struct signal_struct *signal;
 	struct sighand_struct *sighand;
 
+	struct list_head csr_descendant;
+
 	sigset_t blocked, real_blocked;
 	sigset_t saved_sigmask;	/* restored if set_restore_sigmask() was used */
 	struct sigpending pending;
diff --git a/kernel/fork.c b/kernel/fork.c
index 135b7a4..5874d01 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1367,9 +1367,6 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	sig->oom_score_adj = current->signal->oom_score_adj;
 	sig->oom_score_adj_min = current->signal->oom_score_adj_min;
 
-	sig->has_child_subreaper = current->signal->has_child_subreaper ||
-				   current->signal->is_child_subreaper;
-
 	mutex_init(&sig->cred_guard_mutex);
 
 	return 0;
@@ -1800,6 +1797,13 @@ static __latent_entropy struct task_struct *copy_process(
 
 			p->signal->leader_pid = pid;
 			p->signal->tty = tty_kref_get(current->signal->tty);
+			/*
+			 * Inherit has_child_subreaper flag under the same
+			 * tasklist_lock with adding child to the process tree
+			 * for propagate_has_child_subreaper optimization.
+			 */
+			p->signal->has_child_subreaper = current->signal->has_child_subreaper ||
+							 current->signal->is_child_subreaper;
 			list_add_tail(&p->sibling, &p->real_parent->children);
 			list_add_tail_rcu(&p->tasks, &init_task.tasks);
 			attach_pid(p, PIDTYPE_PGID);
diff --git a/kernel/sys.c b/kernel/sys.c
index 842914e..0e4d566 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2063,6 +2063,24 @@ static int prctl_get_tid_address(struct task_struct *me, int __user **tid_addr)
 }
 #endif
 
+static int propagate_has_child_subreaper(struct task_struct *p, void *data)
+{
+	/*
+	 * If task has has_child_subreaper - all its decendants
+	 * already have these flag too and new decendants will
+	 * inherit it on fork, skip them.
+	 *
+	 * If we've found child_reaper - skip descendants in
+	 * it's subtree as they will never get out pidns.
+	 */
+	if (p->signal->has_child_subreaper ||
+	    is_child_reaper(task_pid(p)))
+		return 0;
+
+	p->signal->has_child_subreaper = 1;
+	return 1;
+}
+
 SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		unsigned long, arg4, unsigned long, arg5)
 {
@@ -2214,6 +2232,10 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		break;
 	case PR_SET_CHILD_SUBREAPER:
 		me->signal->is_child_subreaper = !!arg2;
+		if (!arg2)
+			break;
+
+		walk_process_tree(me, propagate_has_child_subreaper, NULL);
 		break;
 	case PR_GET_CHILD_SUBREAPER:
 		error = put_user(me->signal->is_child_subreaper,
-- 
2.9.3

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

[Oleg Nesterov]

On 01/27, Pavel Tikhomirov wrote:
>
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1725,6 +1725,8 @@ struct task_struct {
>  	struct signal_struct *signal;
>  	struct sighand_struct *sighand;
>  
> +	struct list_head csr_descendant;
> +

You forgot to remove this part ;)

> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1367,9 +1367,6 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
>  	sig->oom_score_adj = current->signal->oom_score_adj;
>  	sig->oom_score_adj_min = current->signal->oom_score_adj_min;
>  
> -	sig->has_child_subreaper = current->signal->has_child_subreaper ||
> -				   current->signal->is_child_subreaper;
> -
>  	mutex_init(&sig->cred_guard_mutex);
>  
>  	return 0;
> @@ -1800,6 +1797,13 @@ static __latent_entropy struct task_struct *copy_process(
>  
>  			p->signal->leader_pid = pid;
>  			p->signal->tty = tty_kref_get(current->signal->tty);
> +			/*
> +			 * Inherit has_child_subreaper flag under the same
> +			 * tasklist_lock with adding child to the process tree
> +			 * for propagate_has_child_subreaper optimization.
> +			 */
> +			p->signal->has_child_subreaper = current->signal->has_child_subreaper ||
> +							 current->signal->is_child_subreaper;

Ah yes, we need this change too...

And perhaps it would be more correct to do

			p->signal->has_child_subreaper =
				p->real_parent->signal->has_child_subreaper ||
				p->real_parent->signal->is_child_subreaper;

the current code is not exactly right if CLONE_PARENT...

Oleg.

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

[Pavel Tikhomirov]

On 01/30/2017 03:51 PM, Oleg Nesterov wrote:
> On 01/27, Pavel Tikhomirov wrote:
>>
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1725,6 +1725,8 @@ struct task_struct {
>>  	struct signal_struct *signal;
>>  	struct sighand_struct *sighand;
>>
>> +	struct list_head csr_descendant;
>> +
>
> You forgot to remove this part ;)

Oh, sure.

>
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -1367,9 +1367,6 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
>>  	sig->oom_score_adj = current->signal->oom_score_adj;
>>  	sig->oom_score_adj_min = current->signal->oom_score_adj_min;
>>
>> -	sig->has_child_subreaper = current->signal->has_child_subreaper ||
>> -				   current->signal->is_child_subreaper;
>> -
>>  	mutex_init(&sig->cred_guard_mutex);
>>
>>  	return 0;
>> @@ -1800,6 +1797,13 @@ static __latent_entropy struct task_struct *copy_process(
>>
>>  			p->signal->leader_pid = pid;
>>  			p->signal->tty = tty_kref_get(current->signal->tty);
>> +			/*
>> +			 * Inherit has_child_subreaper flag under the same
>> +			 * tasklist_lock with adding child to the process tree
>> +			 * for propagate_has_child_subreaper optimization.
>> +			 */
>> +			p->signal->has_child_subreaper = current->signal->has_child_subreaper ||
>> +							 current->signal->is_child_subreaper;
>
> Ah yes, we need this change too...
>
> And perhaps it would be more correct to do
>
> 			p->signal->has_child_subreaper =
> 				p->real_parent->signal->has_child_subreaper ||
> 				p->real_parent->signal->is_child_subreaper;
>
> the current code is not exactly right if CLONE_PARENT...

I'm fine with inheriting 'has' flag from real_parent, because if 
real_parent does not have 'has' flag set but parent has 'has' set, we 
inherited the flag in vain.

But I don't actually think that inheritance from parent not real_parent 
breaks my optimization: if real_parent has the flag, so does the parent.

>
> Oleg.
>

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

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

[Oleg Nesterov]

On 01/30, Pavel Tikhomirov wrote:
>
> On 01/30/2017 03:51 PM, Oleg Nesterov wrote:
> >>+			/*
> >>+			 * Inherit has_child_subreaper flag under the same
> >>+			 * tasklist_lock with adding child to the process tree
> >>+			 * for propagate_has_child_subreaper optimization.
> >>+			 */
> >>+			p->signal->has_child_subreaper = current->signal->has_child_subreaper ||
> >>+							 current->signal->is_child_subreaper;
> >
> >Ah yes, we need this change too...
> >
> >And perhaps it would be more correct to do
> >
> >			p->signal->has_child_subreaper =
> >				p->real_parent->signal->has_child_subreaper ||
> >				p->real_parent->signal->is_child_subreaper;
> >
> >the current code is not exactly right if CLONE_PARENT...
>
> I'm fine with inheriting 'has' flag from real_parent, because if real_parent
> does not have 'has' flag set but parent has 'has' set, we inherited the flag
> in vain.
>
> But I don't actually think that inheritance from parent not real_parent
> breaks my optimization: if real_parent has the flag, so does the parent.

Not sure I understand... I meant, the usage of p->real_parent instead of "current"
_looks_ more correct and clear, imo.

Say, a is_child_subreaper task does clone(CLONE_PARENT), in this case (with or
without your change) we set p->signal->has_child_subreaper = 1 and this is not
really correct even if we do not really care.

Oleg.

[PATCH] prctl.2: Document new PR_SET_CHILD_SUBREAPER semantics

[Pavel Tikhomirov]

old semantics was non deterministic and worked differently
depending on the external factors, but nothing changes if
process first sets itself subreaper and only after forks

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 man2/prctl.2 | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/man2/prctl.2 b/man2/prctl.2
index 97cf21a..84fbd7e 100644
--- a/man2/prctl.2
+++ b/man2/prctl.2
@@ -162,20 +162,30 @@ if
 is zero, unset the attribute.
 
 When a process is marked as a child subreaper,
-all of the children that it creates, and their descendants,
+all of the children that it creates or have created already, and their descendants,
 will be marked as having a subreaper.
 In effect, a subreaper fulfills the role of
 .BR init (1)
 for its descendant processes.
-Upon termination of a process
-that is orphaned (i.e., its immediate parent has already terminated)
-and marked as having a subreaper,
-the nearest still living ancestor subreaper
-will receive a
+Upon termination of a process having a subreaper,
+all its children become orphaned
+and will be reparented to the nearest still living ancestor subreaper.
+So that on it's adopted child termination
+these subreaper will receive a
 .BR SIGCHLD
 signal and will be able to
 .BR wait (2)
-on the process to discover its termination status.
+on the child to discover its termination status.
+
+Note, that on older kernels these prctl works slightly different.
+Child subreaper process was not actualy the
+.BR init (1)
+for all its descendants.
+If process forks a child while not been a child subreaper,
+and after sets himself child subreaper,
+sub-tree of the child might or might not reparent to the subreaper,
+depending on the configuration of ancestors of the subreaper,
+at the time of forking our subtree.
 .TP
 .BR PR_GET_CHILD_SUBREAPER " (since Linux 3.4)"
 Return the "child subreaper" setting of the caller,
-- 
2.9.3

Re: [PATCH] prctl.2: Document new PR_SET_CHILD_SUBREAPER semantics

[Michael Kerrisk (man-pages)]

Hello Pavel,

On 27 January 2017 at 23:11, Pavel Tikhomirov <ptikhomirov@virtuozzo.com> wrote:
> old semantics was non deterministic and worked differently
> depending on the external factors, but nothing changes if
> process first sets itself subreaper and only after forks

When did the kernel behavior change?

Cheers,

Michael


> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ---
>  man2/prctl.2 | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/man2/prctl.2 b/man2/prctl.2
> index 97cf21a..84fbd7e 100644
> --- a/man2/prctl.2
> +++ b/man2/prctl.2
> @@ -162,20 +162,30 @@ if
>  is zero, unset the attribute.
>
>  When a process is marked as a child subreaper,
> -all of the children that it creates, and their descendants,
> +all of the children that it creates or have created already, and their descendants,
>  will be marked as having a subreaper.
>  In effect, a subreaper fulfills the role of
>  .BR init (1)
>  for its descendant processes.
> -Upon termination of a process
> -that is orphaned (i.e., its immediate parent has already terminated)
> -and marked as having a subreaper,
> -the nearest still living ancestor subreaper
> -will receive a
> +Upon termination of a process having a subreaper,
> +all its children become orphaned
> +and will be reparented to the nearest still living ancestor subreaper.
> +So that on it's adopted child termination
> +these subreaper will receive a
>  .BR SIGCHLD
>  signal and will be able to
>  .BR wait (2)
> -on the process to discover its termination status.
> +on the child to discover its termination status.
> +
> +Note, that on older kernels these prctl works slightly different.
> +Child subreaper process was not actualy the
> +.BR init (1)
> +for all its descendants.
> +If process forks a child while not been a child subreaper,
> +and after sets himself child subreaper,
> +sub-tree of the child might or might not reparent to the subreaper,
> +depending on the configuration of ancestors of the subreaper,
> +at the time of forking our subtree.
>  .TP
>  .BR PR_GET_CHILD_SUBREAPER " (since Linux 3.4)"
>  Return the "child subreaper" setting of the caller,
> --
> 2.9.3
>



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH] prctl.2: Document new PR_SET_CHILD_SUBREAPER semantics

[Pavel Tikhomirov]

On 01/28/2017 01:47 AM, Michael Kerrisk (man-pages) wrote:
> Hello Pavel,
>
> On 27 January 2017 at 23:11, Pavel Tikhomirov <ptikhomirov@virtuozzo.com> wrote:
>> old semantics was non deterministic and worked differently
>> depending on the external factors, but nothing changes if
>> process first sets itself subreaper and only after forks
>
> When did the kernel behavior change?

Sorry for inconvenience, should have added you to cc of a main patch also.

Kernel behavior haven't changed yet, doc change will be a continuation 
of "[PATCH v2 0/2] prctl: make PR_SET_CHILD_SUBREAPER deterministic", 
see https://lkml.org/lkml/2017/1/27/108

>
> Cheers,
>
> Michael
>
>
>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>> ---
>>  man2/prctl.2 | 24 +++++++++++++++++-------
>>  1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/man2/prctl.2 b/man2/prctl.2
>> index 97cf21a..84fbd7e 100644
>> --- a/man2/prctl.2
>> +++ b/man2/prctl.2
>> @@ -162,20 +162,30 @@ if
>>  is zero, unset the attribute.
>>
>>  When a process is marked as a child subreaper,
>> -all of the children that it creates, and their descendants,
>> +all of the children that it creates or have created already, and their descendants,
>>  will be marked as having a subreaper.
>>  In effect, a subreaper fulfills the role of
>>  .BR init (1)
>>  for its descendant processes.
>> -Upon termination of a process
>> -that is orphaned (i.e., its immediate parent has already terminated)
>> -and marked as having a subreaper,
>> -the nearest still living ancestor subreaper
>> -will receive a
>> +Upon termination of a process having a subreaper,
>> +all its children become orphaned
>> +and will be reparented to the nearest still living ancestor subreaper.
>> +So that on it's adopted child termination
>> +these subreaper will receive a
>>  .BR SIGCHLD
>>  signal and will be able to
>>  .BR wait (2)
>> -on the process to discover its termination status.
>> +on the child to discover its termination status.
>> +
>> +Note, that on older kernels these prctl works slightly different.
>> +Child subreaper process was not actualy the
>> +.BR init (1)
>> +for all its descendants.
>> +If process forks a child while not been a child subreaper,
>> +and after sets himself child subreaper,
>> +sub-tree of the child might or might not reparent to the subreaper,
>> +depending on the configuration of ancestors of the subreaper,
>> +at the time of forking our subtree.
>>  .TP
>>  .BR PR_GET_CHILD_SUBREAPER " (since Linux 3.4)"
>>  Return the "child subreaper" setting of the caller,
>> --
>> 2.9.3
>>
>
>
>

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