prctl: propagate has_child_subreaper flag to every descendant
Subsystems (3)
PATCH 1/1 prctl: propagate has_child_subreaper flag to every descendant
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.
Using csr_descendant list to collect descendants and do tree walk without recursion.
Optimize:
a) When descendant already has has_child_subreaper flag all his subtree has it too already.
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.
--- include/linux/sched.h | 2 ++ kernel/sys.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 1 deletion(-)
include/linux/sched.h +2 -0
@@ -1715,6 +1715,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;
kernel/sys.c +50 -1
63 lines not on this page · raw message
Signature · 1 line
2.9.3
Quoted 6 lines · Pavel Tikhomirov
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?
Anyway, afaics the patch is sub-optimal and not correct...
include/linux/sched.h struct task_struct { · line 1719
Quoted 8 lines
include/linux/sched.h +2
-0@@ -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.
Quoted 11 lines
include/linux/sched.h +11
-0+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); +
include/linux/sched.h struct task_struct {
Quoted 10 lines
include/linux/sched.h +10
-0+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.
include/linux/sched.h
Quoted 5 lines
include/linux/sched.h +5
-0+* 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
bool task_is_child_reaper(struct task_struct *p)
{
return same_thread_group(p, task_active_pid_ns(p)->child_reaper);
}Oleg.
Quoted 10 lines · Oleg Nesterov
Quoted 6 lines · Pavel Tikhomirov
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.
Quoted 9 lines
Anyway, afaics the patch is sub-optimal and not correct...
Quoted 6 lines
include/linux/sched.h +0
-0@@ -1715,6 +1715,8 @@ struct task_struct {struct signal_struct *signal;struct sighand_struct *sighand;
Quoted 5 lines
include/linux/sched.h struct task_struct { · line 1719
Quoted 2 lines
include/linux/sched.h +2
-0+struct list_head csr_descendant; +You don't need this new member and
descendants_lock.task_structhas 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.)
Quoted 17 lines
Quoted 17 lines
include/linux/sched.h +16
-0+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;
Quoted 8 lines
include/linux/sched.h
Quoted 5 lines
include/linux/sched.h +5
-0+ + 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.
Quoted 10 lines
include/linux/sched.h
Quoted 6 lines
include/linux/sched.h +5
-0+* 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.
Quoted 2 lines
Probably
is_child_reaper()should be renamed somehow and a new helper makes sense... something like
Will do.
Quoted 8 lines
bool task_is_child_reaper(struct task_struct *p) { return same_thread_group(p, task_active_pid_ns(p)->child_reaper); }Oleg.
Signature · 2 lines
Best regards, Tikhomirov Pavel Software Developer, Virtuozzo.
Sorry I had some problem with mail-agent, resend to be on the safe side.
Quoted 10 lines · Oleg Nesterov
Quoted 6 lines · Pavel Tikhomirov
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.
Quoted 9 lines
Anyway, afaics the patch is sub-optimal and not correct...
Quoted 6 lines
include/linux/sched.h +0
-0@@ -1715,6 +1715,8 @@ struct task_struct {struct signal_struct *signal;struct sighand_struct *sighand;
Quoted 5 lines
include/linux/sched.h struct task_struct { · line 1719
Quoted 2 lines
include/linux/sched.h +2
-0+struct list_head csr_descendant; +You don't need this new member and
descendants_lock.task_structhas 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.)
Quoted 17 lines
Quoted 17 lines
include/linux/sched.h +16
-0+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;
Quoted 8 lines
include/linux/sched.h
Quoted 5 lines
include/linux/sched.h +5
-0+ + 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.
Quoted 10 lines
include/linux/sched.h
Quoted 6 lines
include/linux/sched.h +5
-0+* 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.
Quoted 2 lines
Probably
is_child_reaper()should be renamed somehow and a new helper makes sense... something like
Will do.
Quoted 8 lines
bool task_is_child_reaper(struct task_struct *p) { return same_thread_group(p, task_active_pid_ns(p)->child_reaper); }Oleg.
Signature · 2 lines
Best regards, Tikhomirov Pavel Software Developer, Virtuozzo.
Quoted 14 lines · Pavel Tikhomirov
Quoted 4 lines
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
Quoted 3 lines
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 havehas_child_subreaper== 1) Everybody will reparent to 2.
This is clear, but this can only happen if 2 forks 3 and after that sets is_child_subreaper, right?
And if someone actually does this then your patch can break this application, no?
IOW. Currently CRIU can't restore the process tree with the same has_child_subreaper bits if some process forks before prctl(PR_SET_CHILD_SUBREAPER). It restores the tree as if prctl() was called before the 1st fork.
So you change the semantics of PR_SET_CHILD_SUBREAPER and now CRIU is fine simply because you remove this feature: the sub-reaper can no longer pre-fork the children which should reparent to the previous reaper.
I won't really argure, but I am not sure this is good idea... At least I think this should be clearly documented.
Quoted 8 lines
Quoted 2 lines
You don't need this new member and
descendants_lock.task_structhas 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()insecurity/yama/yama_lsm.cbut we will need to check it for every process, not only descendants, the latter can be a lot faster.)
I'll send a patch, probably a generic helper makes sense.
Btw task_is_descendant() looks wrong at first glance.
Oleg.
task_is_descendant() cleanup
Quoted 2 lines · Oleg Nesterov
Btw
task_is_descendant()looks wrong at first glance.
No, I missed the 2nd ->group_leader dereference. Still this function looks overcomplicated and the usage of thread_group_leader/group_leader just add the unnecessary confusion. It can be simplified a little bit:
static int task_is_descendant(struct task_struct *parent,
struct task_struct *child)
{
int rc = 0;
struct task_struct *walker;
if (!parent || !child)
return 0;
rcu_read_lock();
for (walker = child; walker->pid; walker = rcu_dereference(walker->real_parent))
if (same_thread_group(parent, walker)) {
rc = 1;
break;
}
rcu_read_unlock();
return rc;
}Kees, I can send a patch if you think this very minor cleanup makes any sense.
Oleg.
Re: task_is_descendant() cleanup
Quoted 28 lines · Oleg Nesterov
Quoted 2 lines · Oleg Nesterov
Btw
task_is_descendant()looks wrong at first glance.No, I missed the 2nd ->group_leader dereference. Still this function looks overcomplicated and the usage of thread_group_leader/group_leader just add the unnecessary confusion. It can be simplified a little bit:
static int task_is_descendant(struct task_struct *parent, struct task_struct *child) { int rc = 0; struct task_struct *walker; if (!parent || !child) return 0; rcu_read_lock(); for (walker = child; walker->pid; walker = rcu_dereference(walker->real_parent)) if (same_thread_group(parent, walker)) { rc = 1; break; } rcu_read_unlock(); return rc; }
Quoted 1 line
Kees, I can send a patch if you think this very minor cleanup makes any sense.
Err, isn't checking same_thread_group() at every level more expensive than what I currently have?
-Kees
Signature · 2 lines
Kees Cook Nexus Security
Re: task_is_descendant() cleanup
Quoted 27 lines · Kees Cook
Quoted 25 lines · Oleg Nesterov
Quoted 2 lines · Oleg Nesterov
Btw
task_is_descendant()looks wrong at first glance.No, I missed the 2nd ->group_leader dereference. Still this function looks overcomplicated and the usage of thread_group_leader/group_leader just add the unnecessary confusion. It can be simplified a little bit:
static int task_is_descendant(struct task_struct *parent, struct task_struct *child) { int rc = 0; struct task_struct *walker; if (!parent || !child) return 0; rcu_read_lock(); for (walker = child; walker->pid; walker = rcu_dereference(walker->real_parent)) if (same_thread_group(parent, walker)) { rc = 1; break; } rcu_read_unlock();
Quoted 7 lines
Quoted 4 lines
return rc; }Kees, I can send a patch if you think this very minor cleanup makes any sense.
Err, isn't checking
same_thread_group()at every level more expensive than what I currently have?
Well, same_thread_group(p1,p2) is just
p1->signal == p2->signal
yes this is a bit more expensive than
walker == parent
we currently have, yes. But this eliminates
if (!thread_group_leader(walker)) walker = rcu_dereference(walker->group_leader);
we currently do at every level. And note that "parent" can exec and change its ->group_leader at any time, we probably do not care but this looks confusing.
But please forget, this is really minor.
Oleg.
Add to cc Lennart Poettering <[email protected]>
Quoted 30 lines · Oleg Nesterov
Quoted 14 lines · Pavel Tikhomirov
Quoted 4 lines
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 == 0Quoted 3 lines
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 havehas_child_subreaper== 1) Everybody will reparent to 2.This is clear, but this can only happen if 2 forks 3 and after that sets
is_child_subreaper, right?And if someone actually does this then your patch can break this application, no?
IOW. Currently CRIU can't restore the process tree with the same
has_child_subreaperbits if some process forks beforeprctl(PR_SET_CHILD_SUBREAPER). It restores the tree as ifprctl()was called before the 1st fork.
Quoted 6 lines
So you change the semantics of
PR_SET_CHILD_SUBREAPERand now CRIU is fine simply because you remove this feature: the sub-reaper can no longer pre-fork the children which should reparent to the previous reaper.I won't really argure, but I am not sure this is good idea...
If one task uses these feature now it must be very carefull: if some our ancestor have enabled is_child_subreaper somewhere up the tree, forked our tree and after that disabled is_child_subreaper, so we already have has-flag and all children will inherit has-flag irrelevant to what is our order of fork/prctl-ing to become subreaper.
And only one way to check if the task has no has_child_subreaper flag is to create some childs kill them and see to where they will reparent, but I doubt that someone is doing these now.
Quoted 1 line
At least I think this should be clearly documented.
Yes, I surely need to add some documentation here. Thanks for mentioning that! - Will do.
Quoted 16 lines
Quoted 9 lines
Quoted 3 lines
You don't need this new member and
descendants_lock.task_structhas 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()insecurity/yama/yama_lsm.cbut we will need to check it for every process, not only descendants, the latter can be a lot faster.)I'll send a patch, probably a generic helper makes sense.
Btw
task_is_descendant()looks wrong at first glance.Oleg.
Signature · 2 lines
Best regards, Tikhomirov Pavel Software Developer, Virtuozzo.
Quoted 11 lines · Pavel Tikhomirov
Quoted 11 lines
IOW. Currently CRIU can't restore the process tree with the same
has_child_subreaperbits if some process forks beforeprctl(PR_SET_CHILD_SUBREAPER). It restores the tree as ifprctl()was called before the 1st fork.So you change the semantics of
PR_SET_CHILD_SUBREAPERand now CRIU is fine simply because you remove this feature: the sub-reaper can no longer pre-fork the children which should reparent to the previous reaper.
Quoted 7 lines
Quoted 1 line
I won't really argure, but I am not sure this is good idea...
If one task uses these feature now it must be very carefull: if some our ancestor have enabled
is_child_subreapersomewhere up the tree, forked our tree and after that disabledis_child_subreaper, so we already have has-flag and all children will inherit has-flag irrelevant to what is our order of fork/prctl-ing to become subreaper.
Agreed.
So let me reword my initial question, why did you make this patch? Did you actually hit a case when a child of is_child_subreaper process doesn't have has_child_subreaper bit set?
If yes, then perhaps that application has a reason to do this and your patch can break it? If no, then you can probably forget this until you have a CRIU bug report ;)
But let me repeat, I won't really argue. And I even agree that this change makes the semantics of PR_SET_CHILD_SUBREAPER more clear, just I am always nervous when we add the subtle user-visible changes like this, and I greatly misundestood the changelog as if CRIU needs to do prctl(SET_CHILD_SUBREAPER) after it has already restored the process tree and this can't work even in the common case.
Oleg.
PATCH 1/1 introduce the walk_process_tree() helper
In reply to Pavel TikhomirovAdd 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.
--- include/linux/sched.h | 3 +++ kernel/fork.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+)
include/linux/sched.h +3 -0
@@ -3015,6 +3015,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;
kernel/fork.c +32 -0
@@ -2042,6 +2042,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
Signature · 1 line
2.5.0
Re: [PATCH] introduce the walk_process_tree() helper
Quoted 5 lines · Oleg Nesterov
Add the new helper to walk the process tree, the next patch adds a user. Note that it visits the group leaders only,
proc_visitorcan dofor_each_threaditself or we can trivially extendwalk_process_tree()to do this.
Please consider this patch as a preparation for your change, or feel free to fold this code into your patch.
Now you can simply do
int xxx(struct task_struct *p, void *data)
{
if (p->signal->has_child_subreaper ||
is_child_reaper(task_pid(p))
return 0;
p->signal->has_child_subreaper = 1;
return 1;
}
walk_process_tree(xxx, NULL);walk_process_tree() handles the sub-threads correctly, but "p" is always the leader so is_child_reaper() is fine, but probably we need a new helper anyway.
Oleg.
Re: [PATCH] introduce the walk_process_tree() helper
In reply to Oleg NesterovWill include it in patch-set with documentation fix. Thanks Oleg!
Quoted 69 lines · Oleg Nesterov
Add the new helper to walk the process tree, the next patch adds a user. Note that it visits the group leaders only,
proc_visitorcan dofor_each_threaditself or we can trivially extendwalk_process_tree()to do this.Signed-off-by Oleg Nesterov <[email protected]>--- include/linux/sched.h | 3 +++ kernel/fork.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+)include/linux/sched.h +3
-0@@ -3015,6 +3015,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;kernel/fork.c +32
-0@@ -2042,6 +2042,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
Signature · 2 lines
Best regards, Tikhomirov Pavel Software Developer, Virtuozzo.
setns() && PR_SET_CHILD_SUBREAPER
In reply to Pavel TikhomirovAnd this discussion reminds me again that I do not understand how setns() and PR_SET_CHILD_SUBREAPER should play together... Add cc's.
Suppose we have a process P in the root namespace and another namespace X.
P does setns() and enters the X namespace.
P forks a child C.
C forks a grandchild G.
C exits.
The question is, where should we reparent the grandchild G? In the normal case it will be reparented to X->child_reaper and this looks correct.
But lets suppose that P runs with the ->has_child_subreaper bit set. In this case it will be reparented to P's sub-reaper or a global init, and given that P can't control its ->has_child_subreaper flag this does not look right to me.
I can make a simple patch but perhaps I missed something or we actually want this (imo strange) behaviour?
Oleg.
Re: setns() && PR_SET_CHILD_SUBREAPER
Quoted 2 lines · Oleg Nesterov
And this discussion reminds me again that I do not understand how
setns()andPR_SET_CHILD_SUBREAPERshould play together... Add cc's.
I agree that they are currently playing together incorrectly.
Quoted 11 lines
Suppose we have a process P in the root namespace and another namespace X.
P does
setns()and enters the X namespace.
P forks a child C.C forks a grandchild G.
C exits.The question is, where should we reparent the grandchild G? In the normal case it will be reparented to
X->child_reaperand this looks correct.
Quoted 7 lines
But lets suppose that P runs with the ->has_child_subreaper bit set. In this case it will be reparented to P's sub-reaper or a global init, and given that P can't control its ->has_child_subreaper flag this does not look right to me.
I can make a simple patch but perhaps I missed something or we actually want this (imo strange) behaviour?
We definitely do not want a child to be repareted out of a pid namespace when the pid namespace has a perfectly fine child_reaper.
The special case for the init_task in find_new_reaper appears to be the instance of this problem that was considered in the code.
Given the semantics described and asked for of PR_SET_CHILD_SUBREAPER I believe has_child_subreaper needs to be strictly considered an implementation detail and any way that userspace can observe it a bug in the code.
Semantically what we want to do is walk up the parents in the process tree. If a parent has is_child_subreaper we stop at it. If the transition from one parent to the next we are switching pid namespaces we want the reaper from the pid namespace.
As I recall has_child_subreaper was just supposed to be an optimization so the common case would not have to walk up the process tree when finding it's parent.
If we retain any optimizations such as has_child_subreaper please consider the case where a process with is_child_subreaper set exits, and what happens to it's children.
Eric
Re: setns() && PR_SET_CHILD_SUBREAPER
Quoted 19 lines · Eric W. Biederman
Quoted 16 lines · Oleg Nesterov
Suppose we have a process P in the root namespace and another namespace X.
P does
setns()and enters the X namespace.
P forks a child C.C forks a grandchild G.
C exits.The question is, where should we reparent the grandchild G? In the normal case it will be reparented to
X->child_reaperand this looks correct.But lets suppose that P runs with the ->has_child_subreaper bit set. In this case it will be reparented to P's sub-reaper or a global init, and given that P can't control its ->has_child_subreaper flag this does not look right to me.
Quoted 8 lines
Quoted 2 lines
I can make a simple patch but perhaps I missed something or we actually want this (imo strange) behaviour?
We definitely do not want a child to be repareted out of a pid namespace when the pid namespace has a perfectly fine
child_reaper.The special case for the
init_taskinfind_new_reaperappears to be the instance of this problem that was considered in the code.
Actually we should blame the same_thread_group(reaper, child_reaper) check, it should had ensured we could not cross the namespaces, but it is not enough. Because this logic predates setns().
Quoted 4 lines
Semantically what we want to do is walk up the parents in the process tree. If a parent has
is_child_subreaperwe stop at it. If the transition from one parent to the next we are switching pid namespaces we want the reaper from the pid namespace.
Yes, this is what I have in mind, see the patch below. I need to re-check it and update the comment to explain why we can't simply check child_reaper as we currently do.
This way we can start the search from father->real_parent, but the comment above the "reaper == &init_task" is no longer correct, we always need this check although perhaps is_idle_task(reaper) would be better.
Quoted 3 lines
As I recall
has_child_subreaperwas just supposed to be an optimization so the common case would not have to walk up the process tree when finding it's parent.
Yep.
Quoted 3 lines
If we retain any optimizations such as
has_child_subreaperplease consider the case where a process withis_child_subreaperset exits, and what happens to it's children.
Yes, in this case it should not have any effect. Well, there is another corner case, perhaps we should turn
if (!reaper->signal->is_child_subreaper) continue;
into
if (!reaper->signal->is_child_subreaper) {
if (!reaper->signal->has_child_subreaper)
break;
continue;
}this looks a bit more correct if the exited "is_child_subreaper" process was forked, and after that its parent called prctl(SET_CHILD_SUBREAPER). But I think we do not care and Pavel is going to eliminate the case when a child of is_child_subreaper task can run without has_child_subreaper flag set.
So what do you think about the patch below?
Oleg.
kernel/exit.c +5 -5
@@ -569,15 +569,15 @@ static struct task_struct *find_new_reaper(struct task_struct *father,return thread;if (father->signal->has_child_subreaper) { +unsigned int level =task_pid (father)->level;/* * Find the first ->is_child_subreaper ancestor in our pid_ns. -* We start from father to ensure we can not look into another -* namespace, this is safe because all its threads are dead. +* We check pid->level, this is slightly more efficient than +* task_active_pid_ns(reaper) != task_active_pid_ns(father). */ -for (reaper = father; - !same_thread_group (reaper, child_reaper); +for (reaper = father->real_parent; +task_pid (reaper)->level == level; reaper = reaper->real_parent) { -/* call_usermodehelper() descendants need this check */ if (reaper == &init_task)break ;if (!reaper->signal->is_child_subreaper)
Re: setns() && PR_SET_CHILD_SUBREAPER
Quoted 69 lines · Oleg Nesterov
Quoted 19 lines · Eric W. Biederman
Quoted 16 lines · Oleg Nesterov
Suppose we have a process P in the root namespace and another namespace X.
P does
setns()and enters the X namespace.
P forks a child C.C forks a grandchild G.
C exits.The question is, where should we reparent the grandchild G? In the normal case it will be reparented to
X->child_reaperand this looks correct.But lets suppose that P runs with the ->has_child_subreaper bit set. In this case it will be reparented to P's sub-reaper or a global init, and given that P can't control its ->has_child_subreaper flag this does not look right to me.
Quoted 8 lines
Quoted 2 lines
I can make a simple patch but perhaps I missed something or we actually want this (imo strange) behaviour?
We definitely do not want a child to be repareted out of a pid namespace when the pid namespace has a perfectly fine
child_reaper.The special case for the
init_taskinfind_new_reaperappears to be the instance of this problem that was considered in the code.Actually we should blame the
same_thread_group(reaper, child_reaper)check, it should had ensured we could not cross the namespaces, but it is not enough. Because this logic predatessetns().Quoted 4 lines
Semantically what we want to do is walk up the parents in the process tree. If a parent has
is_child_subreaperwe stop at it. If the transition from one parent to the next we are switching pid namespaces we want the reaper from the pid namespace.Yes, this is what I have in mind, see the patch below. I need to re-check it and update the comment to explain why we can't simply check
child_reaperas we currently do.This way we can start the search from
father->real_parent, but the comment above the "reaper ==&init_task" is no longer correct, we always need this check although perhapsis_idle_task(reaper)would be better.Quoted 3 lines
As I recall
has_child_subreaperwas just supposed to be an optimization so the common case would not have to walk up the process tree when finding it's parent.Yep.
Quoted 3 lines
If we retain any optimizations such as
has_child_subreaperplease consider the case where a process withis_child_subreaperset exits, and what happens to it's children.Yes, in this case it should not have any effect. Well, there is another corner case, perhaps we should turn
if (!reaper->signal->is_child_subreaper) continue;into
if (!reaper->signal->is_child_subreaper) { if (!reaper->signal->has_child_subreaper) break; continue; }
Quoted 5 lines
this looks a bit more correct if the exited "
is_child_subreaper" process was forked, and after that its parent calledprctl(SET_CHILD_SUBREAPER). But I think we do not care and Pavel is going to eliminate the case when a child ofis_child_subreapertask can run withouthas_child_subreaperflag set.
As long as we update the flag when reparenting so that it is accurate and we clear it when creating a child in a new pid namespace.
Quoted 1 line
So what do you think about the patch below?
That does look like the correct logic.
Whose tree do we want to run merge these fixes through? I can pick them up if that would be convinient.
Eric
Quoted 25 lines
Oleg.
kernel/exit.c +5
-5@@ -569,15 +569,15 @@ static struct task_struct *find_new_reaper(struct task_struct *father,return thread;if (father->signal->has_child_subreaper) { +unsigned int level =task_pid (father)->level;/* * Find the first ->is_child_subreaper ancestor in our pid_ns. -* We start from father to ensure we can not look into another -* namespace, this is safe because all its threads are dead. +* We check pid->level, this is slightly more efficient than +* task_active_pid_ns(reaper) != task_active_pid_ns(father). */ -for (reaper = father; - !same_thread_group (reaper, child_reaper); +for (reaper = father->real_parent; +task_pid (reaper)->level == level; reaper = reaper->real_parent) { -/* call_usermodehelper() descendants need this check */ if (reaper == &init_task)break ;if (!reaper->signal->is_child_subreaper)
Re: setns() && PR_SET_CHILD_SUBREAPER
Hi Eric,
sorry for delay, I was sick.
Quoted 6 lines · Eric W. Biederman
Quoted 1 line · Oleg Nesterov
So what do you think about the patch below?
That does look like the correct logic.
Great,
Quoted 2 lines
Whose tree do we want to run merge these fixes through? I can pick them up if that would be convinient.
Thanks! this would be nice, I am sending the patch.
Oleg.
PATCH 1/1 exit: fix the setns() && PR_SET_CHILD_SUBREAPER interaction
In reply to Eric W. Biedermanfind_new_reaper() checks same_thread_group(reaper, child_reaper) to prevent the cross-namespace reparenting but this is not enough if the exiting parent was injected by setns() + fork().
Suppose we have a process P in the root namespace and some namespace X. P does setns() to enter the X namespace, and forks the child C. C forks a grandchild G and exits.
The grandchild G should be re-parented to X->child_reaper, but in this case the ->real_parent chain does not lead to ->child_reaper, so it will be wrongly reparanted to P's sub-reaper or a global init.
--- kernel/exit.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
kernel/exit.c +8 -5
@@ -569,15 +569,18 @@ static struct task_struct *find_new_reaper(struct task_struct *father,return thread;if (father->signal->has_child_subreaper) { +unsigned int ns_level =task_pid (father)->level;/* * Find the first ->is_child_subreaper ancestor in our pid_ns. -* We start from father to ensure we can not look into another -* namespace, this is safe because all its threads are dead. +* We can't check reaper != child_reaper to ensure we do not +* cross the namespaces, the exiting parent could be injected +* by setns() + fork(). +* We check pid->level, this is slightly more efficient than +* task_active_pid_ns(reaper) != task_active_pid_ns(father). */ -for (reaper = father; - !same_thread_group (reaper, child_reaper); +for (reaper = father->real_parent; +task_pid (reaper)->level == ns_level; reaper = reaper->real_parent) { -/* call_usermodehelper() descendants need this check */ if (reaper == &init_task)break ;if (!reaper->signal->is_child_subreaper)
Signature · 1 line
2.5.0