proc: deuglify task_state()
Subsystems (4)
Oleg Nesterov
maintainercover proc: deuglify task_state()
Hello.
I tried to optimize the usage of tasklist_lock in exit_notify() paths but found the bug which should be fixed first: the EXIT_DEAD setting in reparent_leader() can break the task_ppid_nr_ns()-like code and ptrace_parent().
I am still thinking about the "right" fix, but whatever we do we need to cleanup these users, probably before the fix.
One particular user is task_state(), but it also asks for other off- topic cleanups which I believe make sense anyway, lets do this first.
Oleg.
fs/proc/array.c | 47 ++++++++++++++++++++++------------------------- 1 files changed, 22 insertions(+), 25 deletions(-)
Oleg Nesterov
maintainerPATCH 1/4 proc: task_state: read cred->group_info outside of task_lock()
task_state() reads cred->group_info under task_lock() because a long ago it was task_struct->group_info and it was actually protected by task->alloc_lock. Today this task_unlock() after rcu_read_unlock() just adds the confusion, move task_unlock() up.
--- fs/proc/array.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
fs/proc/array.c +1 -2
@@ -201,11 +201,10 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,"FDSize:\t%d\n" "Groups:\t" , fdt ? fdt->max_fds : 0); +task_unlock (p);rcu_read_unlock (); group_info = cred->group_info; -task_unlock (p); -for (g =0 ; g < group_info->ngroups; g++)seq_printf (m,"%d " ,from_kgid_munged (user_ns,GROUP_AT (group_info, g)));
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 2/4 proc: task_state: deuglify the max_fds calculation
In reply to Oleg Nesterov1. The usage of fdt looks very ugly, it can't be NULL if ->files is not NULL. We can use "unsigned int max_fds" instead.
2. This also allows to move seq_printf(max_fds) outside of task_lock() and join it with the previous seq_printf(). See also the next patch.
--- fs/proc/array.c | 23 +++++++++++------------ 1 files changed, 11 insertions(+), 12 deletions(-)
fs/proc/array.c +11 -12
@@ -157,9 +157,9 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,struct user_namespace *user_ns =seq_user_ns (m);struct group_info *group_info;int g; -struct fdtable *fdt =NULL ;const struct cred *cred;pid_t ppid, tpid; +unsigned int max_fds =0 ;rcu_read_lock (); ppid = pid_alive(p) ?
@@ -171,6 +171,12 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns, tpid =task_pid_nr_ns (tracer, ns); } cred =get_task_cred (p); + +task_lock (p); +if (p->files) + max_fds =files_fdtable (p->files)->max_fds; +task_unlock (p); +seq_printf (m,"State:\t%s\n" "Tgid:\t%d\n"
@@ -179,7 +185,8 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,"PPid:\t%d\n" "TracerPid:\t%d\n" "Uid:\t%d\t%d\t%d\t%d\n" -"Gid:\t%d\t%d\t%d\t%d\n" , +"Gid:\t%d\t%d\t%d\t%d\n" +"FDSize:\t%d\nGroups:\t" ,get_task_state (p),task_tgid_nr_ns (p, ns), task_numa_group_id(p),
@@ -192,16 +199,8 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,from_kgid_munged (user_ns, cred->gid),from_kgid_munged (user_ns, cred->egid),from_kgid_munged (user_ns, cred->sgid), - from_kgid_munged(user_ns, cred->fsgid)); - -task_lock (p); -if (p->files) - fdt =files_fdtable (p->files); -seq_printf (m, -"FDSize:\t%d\n" -"Groups:\t" , - fdt ? fdt->max_fds :0 ); -task_unlock (p); +from_kgid_munged (user_ns, cred->fsgid), + max_fds);rcu_read_unlock (); group_info = cred->group_info;
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 3/4 proc: task_state: move the main seq_printf() outside of rcu_read_lock()
In reply to Oleg Nesterovtask_state() does seq_printf() under rcu_read_lock(), but this is only needed for task_tgid_nr_ns() and task_numa_group_id(). We can calculate tgid/ngid and drop rcu lock.
--- fs/proc/array.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)
fs/proc/array.c +6 -6
@@ -158,7 +158,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,struct group_info *group_info;int g;const struct cred *cred; -pid_t ppid, tpid; +pid_t ppid, tpid, tgid, ngid;unsigned int max_fds =0 ;rcu_read_lock ();
@@ -170,12 +170,16 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,if (tracer) tpid =task_pid_nr_ns (tracer, ns); } + + tgid =task_tgid_nr_ns (p, ns); + ngid =task_numa_group_id (p); cred =get_task_cred (p);task_lock (p);if (p->files) max_fds =files_fdtable (p->files)->max_fds;task_unlock (p); +rcu_read_unlock ();seq_printf (m, "State:\t%s\n"
@@ -188,10 +192,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,"Gid:\t%d\t%d\t%d\t%d\n" "FDSize:\t%d\nGroups:\t" ,get_task_state (p), -task_tgid_nr_ns (p, ns), -task_numa_group_id (p), -pid_nr_ns (pid, ns), - ppid, tpid, + tgid, ngid,pid_nr_ns (pid, ns), ppid, tpid,from_kuid_munged (user_ns, cred->uid),from_kuid_munged (user_ns, cred->euid), from_kuid_munged(user_ns, cred->suid),
@@ -201,7 +202,6 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,from_kgid_munged (user_ns, cred->sgid),from_kgid_munged (user_ns, cred->fsgid), max_fds); -rcu_read_unlock (); group_info = cred->group_info;for (g =0 ; g < group_info->ngroups; g++)
Signature · 1 line
1.5.5.1
Re: [PATCH 3/4] proc: task_state: move the main seq_printf() outside of rcu_read_lock()
Quoted 5 lines · Oleg Nesterov
task_state()doesseq_printf()underrcu_read_lock(), but this is only needed fortask_tgid_nr_ns()andtask_numa_group_id(). We can calculate tgid/ngid and drop rcu lock.Signed-off-by Oleg Nesterov <[email protected]>
Quoted 63 lines
--- fs/proc/array.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)fs/proc/array.c +6
-6@@ -158,7 +158,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,struct group_info *group_info;int g;const struct cred *cred; -pid_t ppid, tpid; +pid_t ppid, tpid, tgid, ngid;unsigned int max_fds =0 ;rcu_read_lock ();@@ -170,12 +170,16 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,if (tracer) tpid =task_pid_nr_ns (tracer, ns); } + + tgid =task_tgid_nr_ns (p, ns); + ngid =task_numa_group_id (p); cred =get_task_cred (p);task_lock (p);if (p->files) max_fds =files_fdtable (p->files)->max_fds;task_unlock (p); +rcu_read_unlock ();seq_printf (m, "State:\t%s\n"@@ -188,10 +192,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,"Gid:\t%d\t%d\t%d\t%d\n" "FDSize:\t%d\nGroups:\t" ,get_task_state (p), -task_tgid_nr_ns (p, ns), -task_numa_group_id (p), -pid_nr_ns (pid, ns), - ppid, tpid, + tgid, ngid,pid_nr_ns (pid, ns), ppid, tpid,from_kuid_munged (user_ns, cred->uid),from_kuid_munged (user_ns, cred->euid), from_kuid_munged(user_ns, cred->suid),@@ -201,7 +202,6 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,from_kgid_munged (user_ns, cred->sgid),from_kgid_munged (user_ns, cred->fsgid), max_fds); -rcu_read_unlock (); group_info = cred->group_info;for (g =0 ; g < group_info->ngroups; g++)Signature · 7 lines
1.5.5.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Oleg Nesterov
maintainerPATCH 4/4 proc: task_state: ptrace_parent() doesn't need pid_alive() check
In reply to Oleg Nesterovp->ptrace != 0 means that release_task(p) was not called, so pid_alive() buys nothing and we can remove this check. Other callers already use it directly without additional checks.
Note: with or without this patch ptrace_parent() can return the pointer to the freed task, this will be explained/fixed later.
--- fs/proc/array.c | 13 ++++++------- 1 files changed, 6 insertions(+), 7 deletions(-)
fs/proc/array.c +6 -7
@@ -157,19 +157,18 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,struct user_namespace *user_ns =seq_user_ns (m);struct group_info *group_info;int g; +struct task_struct *tracer;const struct cred *cred; -pid_t ppid, tpid, tgid, ngid; +pid_t ppid, tpid =0 , tgid, ngid;unsigned int max_fds =0 ;rcu_read_lock (); ppid =pid_alive (p) ?task_tgid_nr_ns (rcu_dereference (p->real_parent), ns) :0 ; - tpid =0 ; -if (pid_alive (p)) { -struct task_struct *tracer =ptrace_parent (p); -if (tracer) - tpid =task_pid_nr_ns (tracer, ns); - } + + tracer =ptrace_parent (p); +if (tracer) + tpid =task_pid_nr_ns (tracer, ns); tgid =task_tgid_nr_ns (p, ns); ngid =task_numa_group_id (p);
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainercover exit: reparent zombie fix + cleanups/optimizations
In reply to Oleg NesterovHello.
Quoted 8 lines · Oleg Nesterov
I tried to optimize the usage of
tasklist_lockinexit_notify()paths but found the bug which should be fixed first: theEXIT_DEADsetting inreparent_leader()can break the task_ppid_nr_ns()-like code andptrace_parent().I am still thinking about the "right" fix, but whatever we do we need to cleanup these users, probably before the fix.
Yes, we need to cleanup these users but lets fix the bug first. See the changelog in 2/5.
Plus cleanups + initial (micro)optimizations, more to come.
Oleg.
include/linux/ptrace.h | 2 +- kernel/exit.c | 51 +++++++++++++++++++---------------------------- kernel/ptrace.c | 23 ++------------------ kernel/sched/core.c | 4 ++- 4 files changed, 28 insertions(+), 52 deletions(-)
Oleg Nesterov
maintainerPATCH 1/5 sched_show_task: fix unsafe usage of ->real_parent
rcu_read_lock() can not protect p->real_parent if release_task(p) was already called, change sched_show_task() to check pis_alive() like other users do.
Note: we need some helpers to cleanup the code like this. And it seems that that the usage of cpu_curr(cpu) in dump_cpu_task() is not safe too.
--- kernel/sched/core.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
kernel/sched/core.c +3 -1
@@ -4572,8 +4572,10 @@ void sched_show_task(struct task_struct *p)#ifdef CONFIG_DEBUG_STACK_USAGE free =stack_not_used (p);#endif + ppid =0 ;rcu_read_lock (); - ppid =task_pid_nr (rcu_dereference (p->real_parent)); +if (pid_alive (p)) + ppid =task_pid_nr (rcu_dereference (p->real_parent));rcu_read_unlock (); printk(KERN_CONT "%5lu %5d %6d 0x%08lx\n", free, task_pid_nr(p), ppid,
Signature · 1 line
1.5.5.1
Re: [PATCH 1/5] sched_show_task: fix unsafe usage of ->real_parent
Quoted 8 lines · Oleg Nesterov
rcu_read_lock()can not protectp->real_parentifrelease_task(p)was already called, changesched_show_task()to checkpis_alive()like other users do.Note: we need some helpers to cleanup the code like this. And it seems that that the usage of
cpu_curr(cpu)indump_cpu_task()is not safe too.Signed-off-by Oleg Nesterov <[email protected]>
Quoted 23 lines
--- kernel/sched/core.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)kernel/sched/core.c +3
-1@@ -4572,8 +4572,10 @@ void sched_show_task(struct task_struct *p)#ifdef CONFIG_DEBUG_STACK_USAGE free =stack_not_used (p);#endif + ppid =0 ;rcu_read_lock (); - ppid =task_pid_nr (rcu_dereference (p->real_parent)); +if (pid_alive (p)) + ppid =task_pid_nr (rcu_dereference (p->real_parent));rcu_read_unlock (); printk(KERN_CONT "%5lu %5d %6d 0x%08lx\n", free, task_pid_nr(p), ppid,Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 2/5 exit: reparent: use ->ptrace_entry rather than ->sibling for EXIT_DEAD tasks
In reply to Oleg Nesterovreparent_leader() reuses ->sibling as a list node to add an EXIT_DEAD task into dead_children list we are going to release. This obviously removes the dead task from its real_parent->children list and this is even good; the parent can do nothing with the EXIT_DEAD reparented zombie, it only makes do_wait() slower.
But, this also means that it can not be reparented once again, so if its new parent dies too nobody will update ->parent/real_parent, they can point to the freed memory even before release_task() we are going to call, this breaks the code which relies on pid_alive() to access ->real_parent/parent.
Fortunately this is mostly theoretical, this can only happen if init or PR_SET_CHILD_SUBREAPER process ignores SIGCHLD and the new parent sub-thread exits right after we drop tasklist_lock.
Change this code to use ->ptrace_entry instead, we know that the child is not traced so nobody can ever use this member. This also allows to unify this logic with exit_ptrace(), see the next changes.
Note: we really need to change release_task() to nullify real_parent/ parent/group_leader pointers, but we need to change the current users first somehow. And it would be better to reap this zombie immediately but release_task_locked() we need is complicated by proc_flush_task().
--- kernel/exit.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
kernel/exit.c +3 -3
@@ -548,7 +548,7 @@ static void reparent_leader(struct task_struct *father, struct task_struct *p, p->exit_state == EXIT_ZOMBIE && thread_group_empty(p)) {if (do_notify_parent (p, p->exit_signal)) { p->exit_state =EXIT_DEAD ; -list_move_tail (&p->sibling, dead); +list_add (&p->ptrace_entry, dead); } }
@@ -587,8 +587,8 @@ static void forget_original_parent(struct task_struct *father)BUG_ON (!list_empty (&father->children)); -list_for_each_entry_safe (p, n, &dead_children, sibling) { -list_del_init (&p->sibling); +list_for_each_entry_safe (p, n, &dead_children, ptrace_entry) { +list_del_init (&p->ptrace_entry);release_task (p); } }
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 3/5 exit: reparent: cleanup the changing of ->parent
In reply to Oleg Nesterov1. Cosmetic, but "if (t->parent == father)" looks a bit confusing. We need to change t->parent if and only if t is not traced.
2. If we actually want this BUG_ON() to ensure that parent/ptrace match each other, then we should also take ptrace_reparented() case into account too.
3. Change this code to use for_each_thread() instead of deprecated while_each_thread().
--- kernel/exit.c | 13 +++++-------- 1 files changed, 5 insertions(+), 8 deletions(-)
kernel/exit.c +5 -8
@@ -557,7 +557,7 @@ static void reparent_leader(struct task_struct *father, struct task_struct *p,static void forget_original_parent (struct task_struct *father) { -struct task_struct *p, *n, *reaper; +struct task_struct *p, *t, *n, *reaper;LIST_HEAD (dead_children);write_lock_irq (&tasklist_lock);
@@ -569,18 +569,15 @@ static void forget_original_parent(struct task_struct *father) reaper =find_new_reaper (father);list_for_each_entry_safe (p, n, &father->children, sibling) { -struct task_struct *t = p; - - do { +for_each_thread (p, t) { t->real_parent = reaper; - if (t->parent == father) { - BUG_ON(t->ptrace); +BUG_ON (!t->ptrace != (t->parent == father)); +if (likely (!t->ptrace)) t->parent = t->real_parent; - }if (t->pdeath_signal)group_send_sig_info (t->pdeath_signal,SEND_SIG_NOINFO , t); - } while_each_thread(p, t); + }reparent_leader (father, p, &dead_children); }write_unlock_irq (&tasklist_lock);
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 4/5 exit: reparent: cleanup the usage of reparent_leader()
In reply to Oleg Nesterov1. Now that reparent_leader() doesn't abuse ->sibling we can shift list_move_tail() from reparent_leader() to forget_original_parent() and turn it into a single list_splice_tail_init(). This also makes BUG_ON(!list_empty()) and list_for_each_entry_safe() unnecessary.
2. This also allows to shift the same_thread_group() check, it looks a bit more clear in the caller.
--- kernel/exit.c | 22 +++++++++------------- 1 files changed, 9 insertions(+), 13 deletions(-)
kernel/exit.c +9 -13
@@ -529,15 +529,7 @@ static struct task_struct *find_new_reaper(struct task_struct *father)static void reparent_leader (struct task_struct *father,struct task_struct *p,struct list_head *dead) { -list_move_tail (&p->sibling, &p->real_parent->children); - -if (p->exit_state ==EXIT_DEAD ) -return ; -/* -* If this is a threaded reparent there is no need to -* notify anyone anything has happened. -*/ -if (same_thread_group (p->real_parent, father)) +if (unlikely (p->exit_state ==EXIT_DEAD ))return ;/* We don't want people slaying init. */
@@ -568,7 +560,7 @@ static void forget_original_parent(struct task_struct *father)exit_ptrace (father); reaper =find_new_reaper (father); -list_for_each_entry_safe (p, n, &father->children, sibling) { +list_for_each_entry (p, &father->children, sibling) {for_each_thread (p, t) { t->real_parent = reaper;BUG_ON (!t->ptrace != (t->parent == father));
@@ -578,12 +570,16 @@ static void forget_original_parent(struct task_struct *father)group_send_sig_info (t->pdeath_signal,SEND_SIG_NOINFO , t); } -reparent_leader (father, p, &dead_children); +/* +* If this is a threaded reparent there is no need to +* notify anyone anything has happened. +*/ +if (!same_thread_group (reaper, father)) +reparent_leader (father, p, &dead_children); } +list_splice_tail_init (&father->children, &reaper->children);write_unlock_irq (&tasklist_lock); -BUG_ON (!list_empty (&father->children)); -list_for_each_entry_safe (p, n, &dead_children, ptrace_entry) {list_del_init (&p->ptrace_entry);release_task (p);
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 5/5 exit: ptrace: shift "reap dead" code from exit_ptrace() to forget_original_parent()
In reply to Oleg NesterovNow that forget_original_parent() uses ->ptrace_entry for EXIT_DEAD tasks, we can simply pass "dead_children" list to exit_ptrace() and remove another release_task() loop. Plus this way we do not need to drop and reacquire tasklist_lock.
Also shift the list_empty(ptraced) check, if we want this optimization it makes sense to eliminate the function call altogether.
--- include/linux/ptrace.h | 2 +- kernel/exit.c | 10 ++++------ kernel/ptrace.c | 23 +++-------------------- 3 files changed, 8 insertions(+), 27 deletions(-)
include/linux/ptrace.h +1 -1
@@ -52,7 +52,7 @@ extern void ptrace_notify(int exit_code);extern void __ptrace_link (struct task_struct *child,struct task_struct *new_parent);extern void __ptrace_unlink (struct task_struct *child); -extern void exit_ptrace (struct task_struct *tracer); +extern void exit_ptrace (struct task_struct *tracer,struct list_head *dead);#define PTRACE_MODE_READ 0x01#define PTRACE_MODE_ATTACH 0x02#define PTRACE_MODE_NOAUDIT 0x04
kernel/exit.c +4 -6
@@ -553,13 +553,11 @@ static void forget_original_parent(struct task_struct *father)LIST_HEAD (dead_children);write_lock_irq (&tasklist_lock); -/* -* Note that exit_ptrace() and find_new_reaper() might -* drop tasklist_lock and reacquire it. -*/ -exit_ptrace (father); - reaper =find_new_reaper (father); +if (unlikely (!list_empty (&father->ptraced))) +exit_ptrace (father, &dead_children); +/* Can drop and reacquire tasklist_lock */ + reaper =find_new_reaper (father);list_for_each_entry (p, &father->children, sibling) {for_each_thread (p, t) { t->real_parent = reaper;
kernel/ptrace.c +3 -20
@@ -485,36 +485,19 @@ static int ptrace_detach(struct task_struct *child, unsigned int data)/* * Detach all tasks we were using ptrace on. Called with tasklist held -* for writing, and returns with it held too. But note it can release -* and reacquire the lock. +* for writing. */ -void exit_ptrace (struct task_struct *tracer) - __releases(&tasklist_lock) - __acquires(&tasklist_lock) +void exit_ptrace (struct task_struct *tracer,struct list_head *dead) {struct task_struct *p, *n; -LIST_HEAD (ptrace_dead); - -if (likely (list_empty (&tracer->ptraced))) -return ;list_for_each_entry_safe (p, n, &tracer->ptraced, ptrace_entry) {if (unlikely (p->ptrace &PT_EXITKILL ))send_sig_info (SIGKILL ,SEND_SIG_FORCED , p);if (__ptrace_detach (tracer, p)) -list_add (&p->ptrace_entry, &ptrace_dead); - } - -write_unlock_irq (&tasklist_lock); -BUG_ON (!list_empty (&tracer->ptraced)); - -list_for_each_entry_safe (p, n, &ptrace_dead, ptrace_entry) { -list_del_init (&p->ptrace_entry); -release_task (p); +list_add (&p->ptrace_entry, dead); } - -write_lock_irq (&tasklist_lock); }int ptrace_readdata (struct task_struct *tsk,unsigned long src,char __user *dst,int len)
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainercover exit: more cleanups/optimizations
In reply to Oleg NesterovQuoted 2 lines · Oleg Nesterov
Plus cleanups + initial (micro)optimizations, more to come.
Yes, exit_notify() needs more changes but I was distracted, will do later.
Until then some more "exit:" changes which also makes sense imo.
Oleg.
fs/proc/base.c | 3 ++ kernel/exit.c | 58 ++++++++++++++++++++++++------------------------------- 2 files changed, 28 insertions(+), 33 deletions(-)
Oleg Nesterov
maintainerPATCH 1/5 exit: wait: cleanup the ptrace_reparented() checks
Now that EXIT_DEAD is the terminal state we can kill "int traced" variable and check "state == EXIT_DEAD" instead to cleanup the code. In particular, this way it is clear that the check obviously doesn't need tasklist_lock.
Also fix the type of "unsigned long state", "long" was always wrong although this doesn't matter because cmpxchg/xchg uses typeof(*ptr).
--- kernel/exit.c | 14 ++++++-------- 1 files changed, 6 insertions(+), 8 deletions(-)
kernel/exit.c +6 -8
@@ -973,8 +973,7 @@ static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,*/ static int wait_task_zombie (struct wait_opts *wo,struct task_struct *p) { -unsigned long state; -int retval, status, traced; +int state, retval, status;pid_t pid =task_pid_vnr (p);uid_t uid =from_kuid_munged (current_user_ns (),task_uid (p));struct siginfo __user *infop;
@@ -997,19 +996,18 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p) }return wait_noreap_copyout (wo, p, pid, uid, why, status); } - - traced =ptrace_reparented (p);/* * Move the task's state to DEAD/TRACE, only one thread can do this. */ - state = traced &&thread_group_leader (p) ?EXIT_TRACE :EXIT_DEAD ; + state =ptrace_reparented (p) &&thread_group_leader (p) ? +EXIT_TRACE :EXIT_DEAD ;if (cmpxchg (&p->exit_state,EXIT_ZOMBIE , state) !=EXIT_ZOMBIE )return 0 ; +/* -* It can be ptraced but not reparented, check -* thread_group_leader() to filter out sub-threads. +* Check thread_group_leader() to exclude the traced sub-threads. */ -if (likely (!traced) &&thread_group_leader (p)) { +if (state ==EXIT_DEAD &&thread_group_leader (p)) {struct signal_struct *psig;struct signal_struct *sig;unsigned long maxrss;
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 2/5 exit: wait: don't use zombie->real_parent
In reply to Oleg Nesterov1. wait_task_zombie() uses p->real_parent to get psig/siglock. This is correct but needs tasklist_lock, ->real_parent can exit.
We can use "current" instead. This is our natural child, its parent must be our sub-thread.
2. Read psig/sig outside of ->siglock, ->signal is no longer protected by this lock.
3. Fix the outdated comments about tasklist_lock. We can not race with __exit_signal(), the whole thread group is dead, nobody but us can call it.
Also clarify the usage of ->stats_lock and ->siglock.
Note: thread_group_cputime_adjusted() is sub-optimal in this case, we probably want to export cputime_adjust() to avoid thread_group_cputime(). The comment says "all threads" but there are no other threads.
--- kernel/exit.c | 23 +++++++++++------------ 1 files changed, 11 insertions(+), 12 deletions(-)
kernel/exit.c +11 -12
@@ -1008,8 +1008,8 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)* Check thread_group_leader() to exclude the traced sub-threads. */ if (state ==EXIT_DEAD &&thread_group_leader (p)) { -struct signal_struct *psig; -struct signal_struct *sig; +struct signal_struct *sig = p->signal; +struct signal_struct *psig = current->signal;unsigned long maxrss;cputime_t tgutime, tgstime;
@@ -1021,21 +1021,20 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)* accumulate in the parent's signal_struct c* fields. * * We don't bother to take a lock here to protect these -* p->signal fields, because they are only touched by -* __exit_signal, which runs with tasklist_lock -* write-locked anyway, and so is excluded here. We do -* need to protect the access to parent->signal fields, -* as other threads in the parent group can be right -* here reaping other children at the same time. +* p->signal fields because the whole thread group is dead +* and nobody can change them. +* +* psig->stats_lock also protects us from our sub-theads +* which can reap other children at the same time. Until +* we change k_getrusage()-like users to rely on this lock +* we have to take ->siglock as well. * * We use thread_group_cputime_adjusted() to get times for * the thread group, which consolidates times for all threads * in the group including the group leader. */ thread_group_cputime_adjusted (p, &tgutime, &tgstime); -spin_lock_irq (&p->real_parent->sighand->siglock); - psig = p->real_parent->signal; - sig = p->signal; +spin_lock_irq (¤t->sighand->siglock);write_seqlock (&psig->stats_lock); psig->cutime += tgutime + sig->cutime; psig->cstime += tgstime + sig->cstime;
@@ -1060,7 +1059,7 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)task_io_accounting_add (&psig->ioac, &p->ioac);task_io_accounting_add (&psig->ioac, &sig->ioac);write_sequnlock (&psig->stats_lock); -spin_unlock_irq (&p->real_parent->sighand->siglock); +spin_unlock_irq (¤t->sighand->siglock); }/*
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 3/5 exit: wait: drop tasklist_lock before psig->c* accounting
In reply to Oleg Nesterovwait_task_zombie() no longer needs tasklist_lock to accumulate the psig->c* counters, we can drop it right after cmpxchg(exit_state).
--- kernel/exit.c | 10 ++++------ 1 files changed, 4 insertions(+), 6 deletions(-)
kernel/exit.c +4 -6
@@ -1003,6 +1003,10 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p) EXIT_TRACE :EXIT_DEAD ;if (cmpxchg (&p->exit_state,EXIT_ZOMBIE , state) !=EXIT_ZOMBIE )return 0 ; +/* +* We own this thread, nobody else can reap it. +*/ +read_unlock (&tasklist_lock);/* * Check thread_group_leader() to exclude the traced sub-threads.
@@ -1062,12 +1066,6 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)spin_unlock_irq (¤t->sighand->siglock); } -/* -* Now we are sure this task is interesting, and no other -* thread can reap it because we its state == DEAD/TRACE. -*/ -read_unlock (&tasklist_lock); - retval = wo->wo_rusage ?getrusage (p,RUSAGE_BOTH , wo->wo_rusage) :0 ; status = (p->signal->flags &SIGNAL_GROUP_EXIT )
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 4/5 exit: release_task: fix the comment about group leader accounting
In reply to Oleg NesterovContrary to what the comment in __exit_signal() says we do account the group leader. Fix this and explain why.
--- kernel/exit.c | 11 ++++------- 1 files changed, 4 insertions(+), 7 deletions(-)
kernel/exit.c +4 -7
@@ -118,13 +118,10 @@ static void __exit_signal(struct task_struct *tsk) }/* -* Accumulate here the counters for all threads but the group leader -* as they die, so they can be added into the process-wide totals -* when those are taken. The group leader stays around as a zombie as -* long as there are other threads. When it gets reaped, the exit.c -* code will add its counts into these totals. We won't ever get here -* for the group leader, since it will have been the last reference on -* the signal_struct. +* Accumulate here the counters for all threads as they die. We could +* skip the group leader because it is the last user of signal_struct, +* but we want to avoid the race with thread_group_cputime() which can +* see the empty ->thread_head list. */ task_cputime (tsk, &utime, &stime);write_seqlock (&sig->stats_lock);
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 5/5 exit: proc: don't try to flush /proc/tgid/task/tgid
In reply to Oleg Nesterovproc_flush_task_mnt() always tries to flush task/pid, but this is pointless if we reap the leader. d_invalidate() is recursive, and if nothing else the next d_hash_and_lookup(tgid) should fail anyway.
--- fs/proc/base.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
fs/proc/base.c +3 -0
@@ -2618,6 +2618,9 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)dput (dentry); } +if (pid == tgid) +return ; + name.name = buf; name.len =snprintf (buf,sizeof (buf),"%d" , tgid); leader =d_hash_and_lookup (mnt->mnt_root, &name);
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainercover exit: find_new_reaper() fixes/cleanups
In reply to Oleg NesterovQuoted 3 lines · Oleg Nesterov
Yes,
exit_notify()needs more changes but I was distracted, will do later.
but I found other bugs in this area. Fix them first. This is also preparation for the next changes in exit_notify() paths.
Oleg.
kernel/exit.c | 98 +++++++++++++++++++++++++++++++++----------------------- 1 files changed, 58 insertions(+), 40 deletions(-)
Oleg Nesterov
maintainerPATCH 1/6 exit: reparent: fix the dead-parent PR_SET_CHILD_SUBREAPER reparenting
The ->has_child_subreaper code in find_new_reaper() finds alive "thread" but returns another "reaper" thread which can be dead.
--- kernel/exit.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/exit.c +1 -1
@@ -512,7 +512,7 @@ static struct task_struct *find_new_reaper(struct task_struct *father) thread = reaper; do { if (!(thread->flags & PF_EXITING)) - return reaper; + return thread; } while_each_thread(reaper, thread); } }
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 2/6 exit: reparent: fix the cross-namespace PR_SET_CHILD_SUBREAPER reparenting
In reply to Oleg Nesterovfind_new_reaper() assumes that "has_child_subreaper" logic is safe as long as we are not the exiting ->child_reaper and this is doubly wrong:
1. In fact it is safe if "pid_ns->child_reaper == father"; there must be no children after zap_pid_ns_processes() returns, so it doesn't matter what we return in this case and even pid_ns->child_reaper is wrong otherwise: we can't reparent to ->child_reaper == current.
This is not a bug, but this is confusing.
2. It is not safe if we are not pid_ns->child_reaper but from the same thread group. We drop tasklist_lock before zap_pid_ns_processes(), so another thread can lock it and choose the new reaper from the upper namespace if has_child_subreaper == T, and this is obviously wrong.
This is not that bad, zap_pid_ns_processes() won't return until the the new reaper reaps all zombies, but this should be fixed anyway.
We could change for_each_thread() loop to use ->exit_state instead of PF_EXITING which we had to use until 8aac62706ada, or we could change copy_signal() to check CLONE_NEWPID before setting has_child_subreaper, but lets change this code so that it is clear we can't look outside of our namespace, otherwise same_thread_group(reaper, child_reaper) check will look wrong and confusing anyway.
We can simply start from "father" and fix the problem. We can't wrongly return a thread from the same thread group if ->is_child_subreaper == T, we know that all threads have PF_EXITING set.
--- kernel/exit.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
kernel/exit.c +4 -2
@@ -492,7 +492,9 @@ static struct task_struct *find_new_reaper(struct task_struct *father)zap_pid_ns_processes (pid_ns);write_lock_irq (&tasklist_lock); - }else if (father->signal->has_child_subreaper) { + } + +if (father->signal->has_child_subreaper) {struct task_struct *reaper;/*
@@ -502,7 +504,7 @@ static struct task_struct *find_new_reaper(struct task_struct *father)* PID namespace. However we still need the check above, see * http://marc.info/?l=linux-kernel&m=131385460420380 */ -for (reaper = father->real_parent; +for (reaper = father; reaper != &init_task; reaper = reaper->real_parent) {if (same_thread_group (reaper, pid_ns->child_reaper))
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 3/6 exit: reparent: s/while_each_thread/for_each_thread/ in find_new_reaper()
In reply to Oleg NesterovChange find_new_reaper() to use for_each_thread() instead of deprecated while_each_thread(). We do not bother to check "thread != father" in the 1st loop, we can rely on PF_EXITING check.
Note: this means the minor behavioural change: for_each_thread() starts from the group leader. But this should be fine, nobody should make any assumption about do_wait(__WNOTHREAD) when it comes to reparented tasks. And this can avoid the pointless reparenting to a short-living thread While zombie leaders are not that common.
--- kernel/exit.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-)
kernel/exit.c +3 -5
@@ -473,8 +473,7 @@ static struct task_struct *find_new_reaper(struct task_struct *father)struct pid_namespace *pid_ns =task_active_pid_ns (father);struct task_struct *thread; - thread = father; - while_each_thread(father, thread) { +for_each_thread (father, thread) {if (thread->flags &PF_EXITING )continue ;if (unlikely (pid_ns->child_reaper == father))
@@ -511,11 +510,10 @@ static struct task_struct *find_new_reaper(struct task_struct *father)break ;if (!reaper->signal->is_child_subreaper)continue ; - thread = reaper; - do { +for_each_thread (reaper, thread) {if (!(thread->flags &PF_EXITING ))return thread; - } while_each_thread(reaper, thread); + } } }
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 4/6 exit: reparent: document the ->has_child_subreaper checks
In reply to Oleg NesterovSwap the "init_task" and same_thread_group() checks. This way it is more simple to document these checks and we can remove the link to the previous discussion on lkml.
--- kernel/exit.c | 14 ++++++-------- 1 files changed, 6 insertions(+), 8 deletions(-)
kernel/exit.c +6 -8
@@ -495,18 +495,16 @@ static struct task_struct *find_new_reaper(struct task_struct *father)if (father->signal->has_child_subreaper) {struct task_struct *reaper; -/* -* 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 +* 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. */ for (reaper = father; - reaper != &init_task; + !same_thread_group (reaper, pid_ns->child_reaper); reaper = reaper->real_parent) { -if (same_thread_group (reaper, pid_ns->child_reaper)) +/* call_usermodehelper() descendants need this check */ +if (reaper == &init_task)break ;if (!reaper->signal->is_child_subreaper)continue ;
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 5/6 exit: reparent: introduce find_child_reaper()
In reply to Oleg Nesterovfind_new_reaper() does 2 completely different things. Not only it finds a reaper, it also updates pid_ns->child_reaper or kills the whole namespace if the caller is ->child_reaper.
Now that has_child_subreaper logic doesn't depend on child_reaper check we can move that pid_ns code into a separate helper. IMHO this makes the code more clean, and this allows the next changes.
--- kernel/exit.c | 56 +++++++++++++++++++++++++++++++++++--------------------- 1 files changed, 35 insertions(+), 21 deletions(-)
kernel/exit.c +35 -21
@@ -459,6 +459,34 @@ static void exit_mm(struct task_struct *tsk)clear_thread_flag (TIF_MEMDIE ); } +static struct task_struct *find_child_reaper (struct task_struct *father) + __releases(&tasklist_lock) + __acquires(&tasklist_lock) +{ +struct pid_namespace *pid_ns =task_active_pid_ns (father); +struct task_struct *reaper = pid_ns->child_reaper; + +if (likely (reaper != father)) +return reaper; + +for_each_thread (father, reaper) { +if (reaper->flags &PF_EXITING ) +continue ; + pid_ns->child_reaper = reaper; +return reaper; + } + +write_unlock_irq (&tasklist_lock); +if (unlikely (pid_ns == &init_pid_ns)) { +panic ("Attempted to kill init! exitcode=0x%08x\n" , + father->signal->group_exit_code ?: father->exit_code); + } +zap_pid_ns_processes (pid_ns); +write_lock_irq (&tasklist_lock); + +return father; +} +/* * When we die, we re-parent all our children, and try to: * 1. give them to another thread in our thread group, if such a member exists
@@ -466,33 +494,17 @@ static void exit_mm(struct task_struct *tsk)* child_subreaper for its children (like a service manager) * 3. give it to the init process (PID 1) in our pid namespace */ -static struct task_struct *find_new_reaper (struct task_struct *father) - __releases(&tasklist_lock) - __acquires(&tasklist_lock) +static struct task_struct *find_new_reaper (struct task_struct *father, +struct task_struct *child_reaper) { -struct pid_namespace *pid_ns =task_active_pid_ns (father);struct task_struct *thread;for_each_thread (father, thread) {if (thread->flags &PF_EXITING )continue ; -if (unlikely (pid_ns->child_reaper == father)) - pid_ns->child_reaper = thread;return thread; } -if (unlikely (pid_ns->child_reaper == father)) { -write_unlock_irq (&tasklist_lock); -if (unlikely (pid_ns == &init_pid_ns)) { -panic ("Attempted to kill init! exitcode=0x%08x\n" , - father->signal->group_exit_code ?: - father->exit_code); - } - -zap_pid_ns_processes (pid_ns); -write_lock_irq (&tasklist_lock); - } -if (father->signal->has_child_subreaper) {struct task_struct *reaper;/*
@@ -501,7 +513,7 @@ static struct task_struct *find_new_reaper(struct task_struct *father)* namespace, this is safe because all its threads are dead. */ for (reaper = father; - !same_thread_group (reaper, pid_ns->child_reaper); + !same_thread_group (reaper, child_reaper); reaper = reaper->real_parent) {/* call_usermodehelper() descendants need this check */ if (reaper == &init_task)
@@ -515,7 +527,7 @@ static struct task_struct *find_new_reaper(struct task_struct *father) } } -return pid_ns->child_reaper; +return child_reaper; }/*
@@ -552,7 +564,9 @@ static void forget_original_parent(struct task_struct *father)exit_ptrace (father, &dead_children);/* Can drop and reacquire tasklist_lock */ - reaper =find_new_reaper (father); + reaper =find_child_reaper (father); + + reaper =find_new_reaper (father, reaper);list_for_each_entry (p, &father->children, sibling) {for_each_thread (p, t) { t->real_parent = reaper;
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 6/6 exit: reparent: introduce find_alive_thread()
In reply to Oleg NesterovAdd the new simple helper to factor out the for_each_thread() code in find_child_reaper() and find_new_reaper(). It can also simplify the potential PF_EXITING -> exit_state change, plus perhaps we can change this code to take SIGNAL_GROUP_EXIT into account.
--- kernel/exit.c | 32 +++++++++++++++++++------------- 1 files changed, 19 insertions(+), 13 deletions(-)
kernel/exit.c +19 -13
@@ -459,6 +459,17 @@ static void exit_mm(struct task_struct *tsk)clear_thread_flag (TIF_MEMDIE ); } +static struct task_struct *find_alive_thread (struct task_struct *p) +{ +struct task_struct *t; + +for_each_thread (p, t) { +if (!(t->flags &PF_EXITING )) +return t; + } +return NULL ; +} +static struct task_struct *find_child_reaper (struct task_struct *father) __releases(&tasklist_lock) __acquires(&tasklist_lock)
@@ -469,9 +480,8 @@ static struct task_struct *find_child_reaper(struct task_struct *father)if (likely (reaper != father))return reaper; -for_each_thread (father, reaper) { -if (reaper->flags &PF_EXITING ) -continue ; + reaper =find_alive_thread (father); +if (reaper) { pid_ns->child_reaper = reaper;return reaper; }
@@ -497,16 +507,13 @@ static struct task_struct *find_child_reaper(struct task_struct *father)static struct task_struct *find_new_reaper (struct task_struct *father,struct task_struct *child_reaper) { -struct task_struct *thread; +struct task_struct *thread, *reaper; -for_each_thread (father, thread) { -if (thread->flags &PF_EXITING ) -continue ; + thread =find_alive_thread (father); +if (thread)return thread; - }if (father->signal->has_child_subreaper) { -struct task_struct *reaper;/* * Find the first ->is_child_subreaper ancestor in our pid_ns. * We start from father to ensure we can not look into another
@@ -520,10 +527,9 @@ static struct task_struct *find_new_reaper(struct task_struct *father,break ;if (!reaper->signal->is_child_subreaper)continue ; -for_each_thread (reaper, thread) { -if (!(thread->flags &PF_EXITING )) -return thread; - } + thread =find_alive_thread (reaper); +if (thread) +return thread; } }
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainercover exit: avoid O(n ** 2) thread-list scan on group-exit if possible
In reply to Oleg NesterovQuoted 3 lines · Oleg Nesterov
but I found other bugs in this area. Fix them first. This is also preparation for the next changes in
exit_notify()paths.
Finally the optimization I had in mind from the very beginning.
Probably this is not the last series... in particular it seems that we have some problems with sys_setns() in this area, but I need to recheck. Plus more cleanups/microoptimizations.
Oleg.
kernel/exit.c | 50 +++++++++++++++++++++++++------------------------- 1 files changed, 25 insertions(+), 25 deletions(-)
Oleg Nesterov
maintainerPATCH 1/3 exit: reparent: avoid find_new_reaper() if no children
Now that pid_ns logic was isolated we can change forget_original_parent() to return right after find_child_reaper() when father->children is empty, there is nothing to reparent in this case.
In particular this avoids find_alive_thread() and this can help if the whole process exits and it has a lot of PF_EXITING threads at the start of the thread list, this can easily lead to O(nr_threads ** 2) iterations.
Trivial test case (tested under KVM, 2 CPUs):
static void *tfunc(void *arg)
{
pause();
return NULL;
}
static int child(unsigned int nt)
{
pthread_t pt;
while (nt--)
assert(pthread_create(&pt, NULL, tfunc, NULL) == 0);
pthread_kill(pt, SIGTRAP);
pause();
return 0;
}
int main(int argc, const char *argv[])
{
int stat;
unsigned int nf = atoi(argv[1]);
unsigned int nt = atoi(argv[2]);
while (nf--) {
if (!fork())
return child(nt);
wait(&stat);
assert(stat == SIGTRAP);
}
return 0;
}$ time ./test 16 16536 shows:
real user sys - 5m37.628s 0m4.437s 8m5.560s + 0m50.032s 0m7.130s 1m4.927s
--- kernel/exit.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
kernel/exit.c +3 -0
@@ -571,6 +571,8 @@ static void forget_original_parent(struct task_struct *father)/* Can drop and reacquire tasklist_lock */ reaper =find_child_reaper (father); +if (list_empty (&father->children)) + goto unlock; reaper =find_new_reaper (father, reaper);list_for_each_entry (p, &father->children, sibling) {
@@ -591,6 +593,7 @@ static void forget_original_parent(struct task_struct *father)reparent_leader (father, p, &dead_children); }list_splice_tail_init (&father->children, &reaper->children); + unlock:write_unlock_irq (&tasklist_lock);list_for_each_entry_safe (p, n, &dead_children, ptrace_entry) {
Signature · 1 line
1.5.5.1
Re: [PATCH -mm 1/3] exit: reparent: avoid find_new_reaper() if no children
Quoted 5 lines · On Thu, 20 Nov 2014 19:34:23 +0100 Oleg Nesterov <[email protected]> wrote:
$ time ./test 16 16536 shows:real user sys - 5m37.628s 0m4.437s 8m5.560s + 0m50.032s 0m7.130s 1m4.927s
Is that the best you can do?
(I assume the increase in user time was a glitch?)
Oleg Nesterov
maintainerRe: [PATCH -mm 1/3] exit: reparent: avoid find_new_reaper() if no children
Quoted 10 lines · Andrew Morton
Quoted 5 lines · On Thu, 20 Nov 2014 19:34:23 +0100 Oleg Nesterov <[email protected]> wrote:
$ time ./test 16 16536 shows:real user sys - 5m37.628s 0m4.437s 8m5.560s + 0m50.032s 0m7.130s 1m4.927sIs that the best you can do?
Unfortunately these changes do not even try to solve the main problem, tasklist_lock doesn't scale simply because it is global. These changes make sense (I hope) anyway, even if/when we redesign the locking. But so far I do not have a good plan.
Quoted 1 line
(I assume the increase in user time was a glitch?)
To be honest, I didn't even notice this change. I repeated the testing before/after this patch and (to my surprize) the "user" numbers are more or less stable, and /usr/bin/time reports the increase.
1. First of all: this is impossible ;)
Note that this test-case uses SIGTRAP to trigger the coredumping. This means that exit_notify() can only be called when all threads are already in kernel mode, the coredumping thread sleeps until they all are parked in exit_mm(). Until then this patch has no effect.
2. With this patch applied, I added mdelay(2) into forget_original_parent(), right after find_child_reaper(). And yes, this changes the numbers too:
real user sys
10m1.225s 0m5.443s 17m25.797s
note that "user time" goes down.
3. So I think that this just reminds that utime/stime accounting isn't precise. sum_exec_runtime is accurate and thus we can more or less trust utime + stime, but utime/stime is random. Plus scale_stime() doesn't look very accurate too.
4. In this particular case the accounting is even more impresize, this test-case spends a lot of time in kernel mode with irqs disabled and this "freezes" task->stime.
5. That said, I still can't really understand why "user" grows. If I understand the calculations in cputime_adjust() correctly (probably I don't), it should not.
In short, I am a bit confused but I still don't think that this increase is real.
Oleg.
Oleg Nesterov
maintainerPATCH 2/3 exit: reparent: call forget_original_parent() under tasklist_lock
In reply to Oleg NesterovShift "release dead children" loop from forget_original_parent() to its caller, exit_notify(). It is safe to reap them even if our parent reaps us right after we drop tasklist_lock, those children no longer have any connection to the exiting task.
And this allows us to avoid write_lock_irq(tasklist_lock) right after it was released by forget_original_parent(), we can simply call it with tasklist_lock held.
While at it, move the comment about forget_original_parent() up to this function.
--- kernel/exit.c | 47 +++++++++++++++++++++++------------------------ 1 files changed, 23 insertions(+), 24 deletions(-)
kernel/exit.c +23 -24
@@ -560,19 +560,26 @@ static void reparent_leader(struct task_struct *father, struct task_struct *p,kill_orphaned_pgrp (p, father); } -static void forget_original_parent (struct task_struct *father) +/* +* This does two things: +* +* A. Make init inherit all the child processes +* B. Check to see if any process groups have become orphaned +* as a result of our exiting, and if they have any stopped +* jobs, send them a SIGHUP and then a SIGCONT. (POSIX 3.2.2.2) +*/ +static void forget_original_parent (struct task_struct *father, +struct list_head *dead) { -struct task_struct *p, *t, *n, *reaper; -LIST_HEAD (dead_children); +struct task_struct *p, *t, *reaper; -write_lock_irq (&tasklist_lock);if (unlikely (!list_empty (&father->ptraced))) -exit_ptrace (father, &dead_children); +exit_ptrace (father, dead);/* Can drop and reacquire tasklist_lock */ reaper =find_child_reaper (father);if (list_empty (&father->children)) - goto unlock; +return ; reaper =find_new_reaper (father, reaper);list_for_each_entry (p, &father->children, sibling) {
@@ -590,16 +597,9 @@ static void forget_original_parent(struct task_struct *father)* notify anyone anything has happened. */ if (!same_thread_group (reaper, father)) -reparent_leader (father, p, &dead_children); +reparent_leader (father, p, dead); }list_splice_tail_init (&father->children, &reaper->children); - unlock: -write_unlock_irq (&tasklist_lock); - -list_for_each_entry_safe (p, n, &dead_children, ptrace_entry) { -list_del_init (&p->ptrace_entry); -release_task (p); - } }/*
@@ -609,18 +609,12 @@ static void forget_original_parent(struct task_struct *father)static void exit_notify (struct task_struct *tsk,int group_dead) {bool autoreap; - -/* -* This does two things: -* -* A. Make init inherit all the child processes -* B. Check to see if any process groups have become orphaned -* as a result of our exiting, and if they have any stopped -* jobs, send them a SIGHUP and then a SIGCONT. (POSIX 3.2.2.2) -*/ -forget_original_parent (tsk); +struct task_struct *p, *n; +LIST_HEAD (dead);write_lock_irq (&tasklist_lock); +forget_original_parent (tsk, &dead); +if (group_dead)kill_orphaned_pgrp (tsk->group_leader,NULL );
@@ -644,6 +638,11 @@ static void exit_notify(struct task_struct *tsk, int group_dead)wake_up_process (tsk->signal->group_exit_task);write_unlock_irq (&tasklist_lock); +list_for_each_entry_safe (p, n, &dead, ptrace_entry) { +list_del_init (&p->ptrace_entry); +release_task (p); + } +/* If the process is dead, release it - nobody will wait for it */ if (autoreap)release_task (tsk);
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerPATCH 3/3 exit: exit_notify: re-use "dead" list to autoreap current
In reply to Oleg NesterovAfter the previous change we can add just the exiting EXIT_DEAD task to the "dead" list and remove another release_task(tsk).
--- kernel/exit.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)
kernel/exit.c +2 -4
@@ -632,6 +632,8 @@ static void exit_notify(struct task_struct *tsk, int group_dead) } tsk->exit_state = autoreap ?EXIT_DEAD :EXIT_ZOMBIE ; +if (tsk->exit_state ==EXIT_DEAD ) +list_add (&tsk->ptrace_entry, &dead);/* mt-exec, de_thread() is waiting for group leader */ if (unlikely (tsk->signal->notify_count <0 ))
@@ -642,10 +644,6 @@ static void exit_notify(struct task_struct *tsk, int group_dead)list_del_init (&p->ptrace_entry);release_task (p); } - -/* If the process is dead, release it - nobody will wait for it */ -if (autoreap) -release_task (tsk); }#ifdef CONFIG_DEBUG_STACK_USAGE
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainercover exit/pid_ns: comments + simple fix
In reply to Oleg NesterovEric, Pavel, could you review 1/2 ? (documentation only). It is based on the code inspection, I didn't bother to verify that my understanding matches the reality ;)
Quoted 4 lines · Oleg Nesterov
Probably this is not the last series... in particular it seems that we have some problems with
sys_setns()in this area, but I need to recheck.
So far only the documentation fix. I'll write another email (hopefully with the patch), afaics at least setns() doesn't play well with PR_SET_CHILD_SUBREAPER.
Contrary to what I thought zap_pid_ns_processes() looks fine, but it seems only by accident. Unless I am totally confused, wait for "nr_hashed == init_pids" could be removed after 0a01f2cc390e10633a "pidns: Make the pidns proc mount/ umount logic obvious". However, now that setns() + fork() can inject a task into a child namespace, we need this code again for another reason.
I _think_ we can actually remove it and simplify free_pid() as well, but lets discuss this later and fix the wrong/confusing documentation first.
2/2 looks "obviously correct", but I'll appreciate your review anyway.
Oleg.
kernel/pid.c | 7 +++---- kernel/pid_namespace.c | 23 +++++++++++++++++++---- 2 files changed, 22 insertions(+), 8 deletions(-)
Oleg Nesterov
maintainerPATCH 1/2 exit: reparent: fix/update the comments in zap_pid_ns_processes()
The comments in zap_pid_ns_processes() are simply wrong, we need to explain how this code actually works.
1. "Ignore SIGCHLD" looks like optimization but it is not, we also need this for correctness.
2. The comment above sys_wait4() could be more clear.
3. The comment about TASK_DEAD children is outdated. Contrary to what it says we do not need to make sure they all go away.
At the same time, we do need to wait for nr_hashed == init_pids, but the reason is quite different and not obvious.
--- kernel/pid_namespace.c | 23 +++++++++++++++++++---- 1 files changed, 19 insertions(+), 4 deletions(-)
kernel/pid_namespace.c +19 -4
@@ -190,7 +190,11 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)/* Don't allow any more processes into the pid namespace */ disable_pid_allocation (pid_ns); -/* Ignore SIGCHLD causing any terminated children to autoreap */ +/* +* Ignore SIGCHLD causing any terminated children to autoreap. +* This speeds up the namespace shutdown, plus see the comment +* below. +*/ spin_lock_irq (&me->sighand->siglock); me->sighand->action[SIGCHLD -1 ].sa.sa_handler =SIG_IGN ;spin_unlock_irq (&me->sighand->siglock);
@@ -223,15 +227,26 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns) }read_unlock (&tasklist_lock); -/* Firstly reap the EXIT_ZOMBIE children we may have. */ +/* Reap the EXIT_ZOMBIE children we had before we ignored SIGCHLD */ do {clear_thread_flag (TIF_SIGPENDING ); rc =sys_wait4 (-1 ,NULL , __WALL,NULL ); }while (rc != -ECHILD );/* -* sys_wait4() above can't reap the TASK_DEAD children. -* Make sure they all go away, see free_pid(). +* sys_wait4() above can't reap the TASK_DEAD children but we do not +* really care, we could reparent them to the global init. We could +* exit and reap ->child_reaper even if it is not the last thread in +* this pid_ns, free_pid(nr_hashed == 0) calls proc_cleanup_work(), +* pid_ns can not go away until proc_kill_sb() drops the reference. +* +* But this namespace can also have other tasks injected by setns(). +* Again, we do not really need to wait until they are all reaped, +* but they can be reparented to us and thus we need to ensure that +* pid->child_reaper is valid until they all go away. +* +* We rely on ignored SIGCHLD, an injected zombie must be autoreaped +* if reparented. */ for (;;) {set_current_state (TASK_UNINTERRUPTIBLE );
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerRe: [PATCH 1/2] exit: reparent: fix/update the comments in zap_pid_ns_processes()
kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 243
Quoted 8 lines · Oleg Nesterov
kernel/pid_namespace.c +7
-0+* sys_wait4() above can't reap the TASK_DEAD children but we do not +* really care, we could reparent them to the global init. We could +* exit and reap ->child_reaper even if it is not the last thread in +* this pid_ns, free_pid(nr_hashed == 0) calls proc_cleanup_work(), +* pid_ns can not go away until proc_kill_sb() drops the reference. +* +* But this namespace can also have other tasks injected by setns().
^^^^^
I meant setns() + fork(), but I hope this is clear.
Oleg.
Re: [PATCH 1/2] exit: reparent: fix/update the comments in zap_pid_ns_processes()
In reply to Oleg NesterovQuoted 10 lines · Oleg Nesterov
The comments in
zap_pid_ns_processes()are simply wrong, we need to explain how this code actually works.1. "Ignore SIGCHLD" looks like optimization but it is not, we also need this for correctness.
2. The comment above
sys_wait4()could be more clear.3. The comment about
TASK_DEADchildren is outdated. Contrary to what it says we do not need to make sure they all go away.
We absolutely do need to wait until they all go away.
- rusage will be wrong if we don't wait.
- We won't wait for an injected process in a pid namespace, or a processes debugged with gdb to be reaped before the pid init process exits if we don't wait.
Quoted 2 lines
At the same time, we do need to wait for
nr_hashed==init_pids, but the reason is quite different and not obvious.
The user visible semantics go from weird to completely insane if we relax the rule that the init process is the last process in a pid namespace.
I don't see anything approaching a good reason for changing the user space semantics.
kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 191
Quoted 12 lines
Signed-off-by Oleg Nesterov <[email protected]>--- kernel/pid_namespace.c | 23 +++++++++++++++++++---- 1 files changed, 19 insertions(+), 4 deletions(-)kernel/pid_namespace.c +0
-0@@ -190,7 +190,11 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)/* Don't allow any more processes into the pid namespace */ disable_pid_allocation (pid_ns);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The above line disables injections of new processes in this pid namespace.
kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 197
Quoted 7 lines
kernel/pid_namespace.c +5
-1-/* Ignore SIGCHLD causing any terminated children to autoreap */ +/* +* Ignore SIGCHLD causing any terminated children to autoreap. +* This speeds up the namespace shutdown, plus see the comment +* below. +*/
This speeds up the namespace shutdown and ensures that after we have waited for all existing zombies there will be no more zombies to wait for.
Quoted 9 lines
kernel/pid_namespace.c +1
-1spin_lock_irq (&me->sighand->siglock); me->sighand->action[SIGCHLD -1 ].sa.sa_handler =SIG_IGN ;spin_unlock_irq (&me->sighand->siglock);@@ -223,15 +227,26 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns) }read_unlock (&tasklist_lock); -/* Firstly reap the EXIT_ZOMBIE children we may have. */ +/* Reap the EXIT_ZOMBIE children we had before we ignored SIGCHLD */
kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 238
Quoted 10 lines
kernel/pid_namespace.c +2
-2do {clear_thread_flag (TIF_SIGPENDING ); rc =sys_wait4 (-1 ,NULL , __WALL,NULL ); }while (rc != -ECHILD );/* -* sys_wait4() above can't reap the TASK_DEAD children. -* Make sure they all go away, see free_pid(). +* sys_wait4() above can't reap the TASK_DEAD children but we do not +* really care, we could reparent them to the global init.
We do care.
kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 244
Quoted 8 lines
We could
kernel/pid_namespace.c +5
-0+* exit and reap ->child_reaper even if it is not the last thread in +* this pid_ns, free_pid(nr_hashed == 0) calls proc_cleanup_work(), +* pid_ns can not go away until proc_kill_sb() drops the reference. +* But this namespace can also have other tasks injected by setns(). +* Again, we do not really need to wait until they are all reaped,
We do, and setns does not matter here. Injection was stopped way up above.
kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 249
Quoted 6 lines
kernel/pid_namespace.c +4
-0+* but they can be reparented to us and thus we need to ensure that +* pid->child_reaper is valid until they all go away. +* We rely on ignored SIGCHLD, an injected zombie must be autoreaped +* if reparented.
Your new comment is about 90% wrong.
Quoted 3 lines
kernel/pid_namespace.c +0
-0*/ for (;;) {set_current_state (TASK_UNINTERRUPTIBLE );
Oleg Nesterov
maintainerRe: [PATCH 1/2] exit: reparent: fix/update the comments in zap_pid_ns_processes()
Quoted 9 lines · Eric W. Biederman
Quoted 6 lines · Oleg Nesterov
The comments in
zap_pid_ns_processes()are simply wrong, we need to explain how this code actually works.1. "Ignore SIGCHLD" looks like optimization but it is not, we also need this for correctness.
Quoted 6 lines
Quoted 4 lines
2. The comment above
sys_wait4()could be more clear.3. The comment about
TASK_DEADchildren is outdated. Contrary to what it says we do not need to make sure they all go away.We absolutely do need to wait until they all go away.
I can easily miss something, and that is why I asked you to review this change. But if I missed something, perhaps we should update the comments?
This "wait until TASK_DEAD children" loop was added to ensure that child_reaper can't be reaped before other tasks in this pid namespace. 6347e90091041e "pidns: guarantee that the pidns init will be the last pidns process reaped".
But this was needed because proc_flush_task(pid == 1) called kern_unmount(proc_mnt). After 0a01f2cc390e10633 "pidns: Make the pidns proc mount/umount logic obvious" we can rely on schedule_work() in free_pid(nr_hashed == 0).
So in any case I think that the current comment is outdated.
Quoted 1 line
- rusage will be wrong if we don't wait.
I already answered in 0/2, let me quote myself:
Do you mean cstime/cutime/c* accounting?
Firstly it is not clear what makes child_reaper special in _this_ sense, but this doesn't matter at all.
The auotoreaping/EXIT_DEAD children are not accounted, only wait_task_zombie() accumulates these counters. (just in case, accounting in __exit_signal() is another thing).
Quoted 3 lines
- We won't wait for an injected process in a pid namespace, or a processes debugged with gdb to be reaped before the pid init process exits if we don't wait.
Yes, and I do not see why this is bad, but this is off-topic.
Again, lets discuss this in another thread. This patch doesn't try to document the desired semantics, it only tries to explain why zap_pid_ns _has_ to wait until EXIT_DEAD (and in fact EXIT_ZOMBIE) tasks go away.
Quoted 6 lines
The user visible semantics go from weird to completely insane if we relax the rule that the init process is the last process in a pid namespace.
I don't see anything approaching a good reason for changing the user space semantics.
See above.
Quoted 8 lines
kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 191
Quoted 3 lines
kernel/pid_namespace.c +0
-0@@ -190,7 +190,11 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)/* Don't allow any more processes into the pid namespace */ disable_pid_allocation (pid_ns);^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^The above line disables injections of new processes in this pid namespace.
Yes, sure. See below.
Quoted 10 lines
kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 197
Quoted 6 lines
kernel/pid_namespace.c +5
-1-/* Ignore SIGCHLD causing any terminated children to autoreap */ +/* +* Ignore SIGCHLD causing any terminated children to autoreap. +* This speeds up the namespace shutdown, plus see the comment +* below. +*/ This speeds up the namespace shutdown and ensures that after we have waited for all existing zombies there will be no more zombies to wait for.
Afaics, no, see below.
Quoted 6 lines
kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns)
Quoted 4 lines
kernel/pid_namespace.c +2
-2-* sys_wait4() above can't reap the TASK_DEAD children. -* Make sure they all go away, see free_pid(). +* sys_wait4() above can't reap the TASK_DEAD children but we do not +* really care, we could reparent them to the global init. We do care.
See above. I only meant that nothing bad can happen from the kernel perspective.
Quoted 4 lines
kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 244
Quoted 2 lines
kernel/pid_namespace.c +2
-0+* But this namespace can also have other tasks injected by setns(). +* Again, we do not really need to wait until they are all reaped, We do, and setns does not matter here. Injection was stopped way up above.
I think that setns() does matter.
Yes injection was stopped. But a task T can be already injected before disable_pid_allocation() was called.
Now it is killed (or it could even exit before). To simplify, suppose it is already EXIT_ZOMBIE, although this doesn't really matter.
The sys_wait4() loop can't see it, it is not our child.
Now suppose that its parent doesn't do wait() but exits. This means that the exiting parent will try to reparent T to pid_ns->child_reaper.
child_reaper already sleeps in "wait for nr_hashed == init_pids" loop, it can do nothing with T.
So we rely on autoreaping (forced by ignored SIGCHLD), and this is the (technical) reason why child_reaper can't exit: pid_ns->child_reaper should be valid.
No?
Quoted 4 lines
kernel/pid_namespace.c void zap_pid_ns_processes(struct pid_namespace *pid_ns) · line 249
Quoted 2 lines
kernel/pid_namespace.c +2
-0+* We rely on ignored SIGCHLD, an injected zombie must be autoreaped +* if reparented. Your new comment is about 90% wrong.
See above.
Eric. I'd really ask you to take another look. But in any case: thanks for looking at this.
Oleg.
Oleg Nesterov
maintainerRe: [PATCH 1/2] exit: reparent: fix/update the comments in zap_pid_ns_processes()
forgot to mention...
Quoted 7 lines · Oleg Nesterov
Quoted 4 lines · Eric W. Biederman
- We won't wait for an injected process in a pid namespace, or a processes debugged with gdb to be reaped before the pid init process exits if we don't wait.
Quoted 5 lines
Yes, and I do not see why this is bad, but this is off-topic.
Again, lets discuss this in another thread. This patch doesn't try to document the desired semantics, it only tries to explain why
zap_pid_ns_has_ to wait untilEXIT_DEAD(and in factEXIT_ZOMBIE) tasks go away.
Just in case, as for "processes debugged with gdb" I obviously agree, but this has nothing to do with EXIT_DEAD threads. zap_pid_ns_processes() will sleep in sys_wait4() until debugger detaches the tracee and reaps it due to ignored SIGCHLD.
But again, this is off-topic now.
Quoted 2 lines
Eric. I'd really ask you to take another look. But in any case: thanks for looking at this.
Yes, please.
Oleg.
Oleg Nesterov
maintainerPATCH 2/2 exit: pidns: alloc_pid() leaks pid_namespace if child_reaper is exiting
In reply to Oleg Nesterovalloc_pid() does get_pid_ns() beforehand but forgets to put_pid_ns() if it fails because disable_pid_allocation() was called by the exiting child_reaper. We can move get_pid_ns() down to successful return.
While at it, simplify/cleanup the "goto out" mess, no need to confuse the success/error return paths.
--- kernel/pid.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-)
kernel/pid.c +3 -4
@@ -320,7 +320,6 @@ struct pid *alloc_pid(struct pid_namespace *ns) goto out_free; } -get_pid_ns (ns);atomic_set (&pid->count,1 );for (type =0 ; type <PIDTYPE_MAX ; ++type)INIT_HLIST_HEAD (&pid->tasks[type]);
@@ -336,7 +335,7 @@ struct pid *alloc_pid(struct pid_namespace *ns) }spin_unlock_irq (&pidmap_lock); -out: +get_pid_ns (ns);return pid; out_unlock:
@@ -346,8 +345,8 @@ out_free:free_pidmap (pid->numbers + i);kmem_cache_free (ns->pid_cachep, pid); - pid =NULL ; - goto out; +out: +return NULL ; }void disable_pid_allocation (struct pid_namespace *ns)
Signature · 1 line
1.5.5.1
Re: [PATCH 2/2] exit: pidns: alloc_pid() leaks pid_namespace if child_reaper is exiting
Quoted 25 lines · Oleg Nesterov
alloc_pid()doesget_pid_ns()beforehand but forgets toput_pid_ns()if it fails becausedisable_pid_allocation()was called by the exitingchild_reaper. We can moveget_pid_ns()down to successful return.While at it, simplify/cleanup the "goto out" mess, no need to confuse the success/error return paths.
Signed-off-by Oleg Nesterov <[email protected]>--- kernel/pid.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-)kernel/pid.c +0
-1@@ -320,7 +320,6 @@ struct pid *alloc_pid(struct pid_namespace *ns) goto out_free; } -get_pid_ns (ns);atomic_set (&pid->count,1 );for (type =0 ; type <PIDTYPE_MAX ; ++type)INIT_HLIST_HEAD (&pid->tasks[type]);
kernel/pid.c struct pid *alloc_pid(struct pid_namespace *ns) · line 338
Quoted 5 lines
kernel/pid.c +1
-1@@ -336,7 +335,7 @@ struct pid *alloc_pid(struct pid_namespace *ns) }spin_unlock_irq (&pidmap_lock); -out: +get_pid_ns (ns);
Moving the label and changing the goto out logic is gratuitous confusing and I think it probably even generates worse code.
Furthermore multiple exits make adding debugging code more difficult.
Moving get_pid_ns down does close a leak in the error handling path.
However at the moment my I can't figure out if it is safe to move get_pid_ns elow hlist_add_head_rcu. Because once we are on the rcu list the pid is findable, and being publicly visible with a bad refcount could cause problems.
The increment of ns->nr_hashed after the adding to the rcu list is only safe because we always access ns->nr_hashed with the pidmap_lock held.
Eric
Quoted 14 lines
kernel/pid.c +2
-2return pid; out_unlock:@@ -346,8 +345,8 @@ out_free:free_pidmap (pid->numbers + i);kmem_cache_free (ns->pid_cachep, pid); - pid =NULL ; - goto out; +out: +return NULL ; }void disable_pid_allocation (struct pid_namespace *ns)
Oleg Nesterov
maintainerRe: [PATCH 2/2] exit: pidns: alloc_pid() leaks pid_namespace if child_reaper is exiting
Quoted 17 lines · Eric W. Biederman
Quoted 14 lines · Oleg Nesterov
kernel/pid.c +0
-1@@ -320,7 +320,6 @@ struct pid *alloc_pid(struct pid_namespace *ns) goto out_free; } -get_pid_ns (ns);atomic_set (&pid->count,1 );for (type =0 ; type <PIDTYPE_MAX ; ++type)INIT_HLIST_HEAD (&pid->tasks[type]);@@ -336,7 +335,7 @@ struct pid *alloc_pid(struct pid_namespace *ns) }spin_unlock_irq (&pidmap_lock);
Quoted 7 lines
kernel/pid.c struct pid *alloc_pid(struct pid_namespace *ns) · line 338
Quoted 2 lines
kernel/pid.c +1
-1-out: +get_pid_ns (ns);Moving the label and changing the goto out logic is gratuitous confusing and I think it probably even generates worse code.
Furthermore multiple exits make adding debugging code more difficult.
Oh, I strongly disagree but I am not going to argue ;) cleanups are always subjective, and I do believe in "maintainer is always right" mantra. I can make v2 without this change.
Quoted 1 line
Moving
get_pid_nsdown does close a leak in the error handling path.
OK, good.
Quoted 4 lines
However at the moment my I can't figure out if it is safe to move
get_pid_nselowhlist_add_head_rcu. Because once we are on the rcu list the pid is findable, and being publicly visible with a bad refcount could cause problems.
The caller has a reference, this ns can't go away. Obviously, otherwise get_pid_ns(ns) is not safe.
We need this get_pid_ns() to balance put_pid()->put_pid_ns() which obviously won't be called until we return this pid, otherwise everything is wrong.
So I think this should be safe?
Oleg.
Re: [PATCH 2/2] exit: pidns: alloc_pid() leaks pid_namespace if child_reaper is exiting
Quoted 24 lines · Oleg Nesterov
Quoted 23 lines · Eric W. Biederman
Quoted 11 lines · Oleg Nesterov
kernel/pid.c +0
-1@@ -320,7 +320,6 @@ struct pid *alloc_pid(struct pid_namespace *ns) goto out_free; } -get_pid_ns (ns);atomic_set (&pid->count,1 );for (type =0 ; type <PIDTYPE_MAX ; ++type)INIT_HLIST_HEAD (&pid->tasks[type]);kernel/pid.c struct pid *alloc_pid(struct pid_namespace *ns) · line 338
Quoted 5 lines
kernel/pid.c +1
-1@@ -336,7 +335,7 @@ struct pid *alloc_pid(struct pid_namespace *ns) }spin_unlock_irq (&pidmap_lock); -out: +get_pid_ns (ns);Moving the label and changing the goto out logic is gratuitous confusing and I think it probably even generates worse code.
Quoted 5 lines
Quoted 1 line
Furthermore multiple exits make adding debugging code more difficult.
Oh, I strongly disagree but I am not going to argue ;) cleanups are always subjective, and I do believe in "maintainer is always right" mantra. I can make v2 without this change.
Fair enough. My primary complaint was that you were changing the logic and fixing a bug at the same time. That added noise and made analysis of what was really going on much more difficult.
Quoted 9 lines
Quoted 1 line
Moving
get_pid_nsdown does close a leak in the error handling path.OK, good.
Quoted 4 lines
However at the moment my I can't figure out if it is safe to move
get_pid_nselowhlist_add_head_rcu. Because once we are on the rcu list the pid is findable, and being publicly visible with a bad refcount could cause problems.
Quoted 7 lines
The caller has a reference, this ns can't go away. Obviously, otherwise
get_pid_ns(ns)is not safe.We need this
get_pid_ns()to balance put_pid()->put_pid_ns() which obviously won't be called until we return this pid, otherwise everything is wrong.So I think this should be safe?
My concern is exposing a half initialized struct pid to the world via an rcu data structure. In particular could one of the rcu users get into trouble because we haven't called get_pid_ns yet? That is unclear to me.
That is one of those weird nasty races I would rather not have to consider and moving the get_pid_ns after hlist_add requires that we think about it.
To fix the error handling and avoid thinking about the races we have two choices:
- In the error path that is currently called out_unlock we can drop the extra references.
- Immediately after we perform the test that on error jumps to out_unlock we call get_pid_ns.
My preference would be the first, as it is a trivially correct one line change.
Aka I think this is the obviously correct trivial fix.
out_unlock:
spin_unlock_irq(&pidmap_lock);
+ put_pid_ns(ns);
out_free:
while (++i <= ns->level)
free_pidmap(pid->numbers + i);
Eric
Oleg Nesterov
maintainerRe: [PATCH 2/2] exit: pidns: alloc_pid() leaks pid_namespace if child_reaper is exiting
Quoted 13 lines · Eric W. Biederman
Quoted 10 lines · Oleg Nesterov
Quoted 5 lines · Eric W. Biederman
However at the moment my I can't figure out if it is safe to move
get_pid_nselowhlist_add_head_rcu. Because once we are on the rcu list the pid is findable, and being publicly visible with a bad refcount could cause problems.The caller has a reference, this ns can't go away. Obviously, otherwise
get_pid_ns(ns)is not safe.
Quoted 8 lines
Quoted 4 lines
We need this
get_pid_ns()to balance put_pid()->put_pid_ns() which obviously won't be called until we return this pid, otherwise everything is wrong.So I think this should be safe?
My concern is exposing a half initialized struct pid to the world via an rcu data structure. In particular could one of the rcu users get into trouble because we haven't called
get_pid_nsyet? That is unclear to me.
They can't. This pid was fully initialized, in particular pid->numbers[pid->level].ns == ns has a reference.
Just it is not ready for put_pid() which will be called by the "owner" of this pid, the caller or the new child. So in this sense it doesn't matter when we call get_pid_ns(), just we need to do this before return.
Quoted 17 lines
That is one of those weird nasty races I would rather not have to consider and moving the
get_pid_nsafterhlist_addrequires that we think about it.To fix the error handling and avoid thinking about the races we have two choices:
- In the error path that is currently calledout_unlockwe can drop the extra references.
- Immediately after we perform the test that on error jumps toout_unlockwe callget_pid_ns.My preference would be the first, as it is a trivially correct one line change.
Aka I think this is the obviously correct trivial fix.
out_unlock:
Review
Quoted 2 lines
(quoted diff) +1
-0spin_unlock_irq(&pidmap_lock); + put_pid_ns(ns);
Sure, initially I was going to do this. But this is sub-optimal imo, I mainly mean less clear (imho).
But again, I won't argue. I'll send V2 once we finish the discussion about 2/2.
Oleg.
Re: [PATCH 2/2] exit: pidns: alloc_pid() leaks pid_namespace if child_reaper is exiting
Quoted 23 lines · Oleg Nesterov
Quoted 13 lines · Eric W. Biederman
Quoted 10 lines · Oleg Nesterov
Quoted 5 lines · Eric W. Biederman
However at the moment my I can't figure out if it is safe to move
get_pid_nselowhlist_add_head_rcu. Because once we are on the rcu list the pid is findable, and being publicly visible with a bad refcount could cause problems.The caller has a reference, this ns can't go away. Obviously, otherwise
get_pid_ns(ns)is not safe.Quoted 8 lines
Quoted 4 lines
We need this
get_pid_ns()to balance put_pid()->put_pid_ns() which obviously won't be called until we return this pid, otherwise everything is wrong.So I think this should be safe?
My concern is exposing a half initialized struct pid to the world via an rcu data structure. In particular could one of the rcu users get into trouble because we haven't called
get_pid_nsyet? That is unclear to me.
Quoted 6 lines
They can't. This pid was fully initialized, in particular pid->numbers[pid->level].ns == ns has a reference.
Just it is not ready for
put_pid()which will be called by the "owner" of this pid, the caller or the new child. So in this sense it doesn't matter when we callget_pid_ns(), just we need to do this before return.
Or by someone calling find_get_pid() ... put_pid().
Now the reference count should not hit zero in that case but I hate to think of that case separately.
Quoted 23 lines
Quoted 17 lines
That is one of those weird nasty races I would rather not have to consider and moving the
get_pid_nsafterhlist_addrequires that we think about it.To fix the error handling and avoid thinking about the races we have two choices:
- In the error path that is currently calledout_unlockwe can drop the extra references.
- Immediately after we perform the test that on error jumps toout_unlockwe callget_pid_ns.My preference would be the first, as it is a trivially correct one line change.
Aka I think this is the obviously correct trivial fix.
out_unlock:Review
Quoted 2 lines
(quoted diff) +1
-0spin_unlock_irq(&pidmap_lock); + put_pid_ns(ns);Sure, initially I was going to do this. But this is sub-optimal imo, I mainly mean less clear (imho).
Quoted 2 lines
But again, I won't argue. I'll send V2 once we finish the discussion about 2/2.
At this point, and especially since we need to Cc stable and get this fix backported to who knows how many kernel releases having something that is trivial to validate is correct is important.
If you prefer to call get_pid_ns() immedately after:
if (!(ns->nr_hashed & PIDNS_HASH_ADDING)) goto out_unlock;
That would be fine with me as well.
Anything else to too clever for my brain to verify is correct today.
Eric
Oleg Nesterov
maintainerRe: [PATCH 2/2] exit: pidns: alloc_pid() leaks pid_namespace if child_reaper is exiting
Quoted 12 lines · Eric W. Biederman
Quoted 9 lines · Oleg Nesterov
Quoted 4 lines · Eric W. Biederman
My concern is exposing a half initialized struct pid to the world via an rcu data structure. In particular could one of the rcu users get into trouble because we haven't called
get_pid_nsyet? That is unclear to me.They can't. This pid was fully initialized, in particular pid->numbers[pid->level].ns == ns has a reference.
Quoted 7 lines
Quoted 3 lines
Just it is not ready for
put_pid()which will be called by the "owner" of this pid, the caller or the new child. So in this sense it doesn't matter when we callget_pid_ns(), just we need to do this before return.Or by someone calling
find_get_pid()...put_pid().Now the reference count should not hit zero in that case
Yes. this get_pid_ns() connects to atomic_set(&pid->count, 1) set by us.
Quoted 2 lines
but I hate to
think of that case separately.
OK.
Quoted 3 lines
At this point, and especially since we need to Cc stable and get this fix backported to who knows how many kernel releases having something that is trivial to validate is correct is important.
OK. I'll send V2 with -stable cc'ed.
Oleg.
Re: [PATCH 0/2] exit/pid_ns: comments + simple fix
In reply to Oleg NesterovQuoted 3 lines · Oleg Nesterov
Eric, Pavel, could you review 1/2 ? (documentation only). It is based on the code inspection, I didn't bother to verify that my understanding matches the reality ;)
Oleg you are being subtle, introducing some gratuitous changes and slightly off base which makes this all very hard to review.
Eric
Quoted 25 lines · Oleg Nesterov
Quoted 4 lines
Probably this is not the last series... in particular it seems that we have some problems with
sys_setns()in this area, but I need to recheck.So far only the documentation fix. I'll write another email (hopefully with the patch), afaics at least
setns()doesn't play well withPR_SET_CHILD_SUBREAPER.Contrary to what I thought
zap_pid_ns_processes()looks fine, but it seems only by accident. Unless I am totally confused, wait for "nr_hashed==init_pids" could be removed after0a01f2cc390e10633a"pidns: Make the pidns proc mount/ umount logic obvious". However, now thatsetns()+fork()can inject a task into a child namespace, we need this code again for another reason.I _think_ we can actually remove it and simplify
free_pid()as well, but lets discuss this later and fix the wrong/confusing documentation first.2/2 looks "obviously correct", but I'll appreciate your review anyway.
Oleg.
kernel/pid.c | 7 +++---- kernel/pid_namespace.c | 23 +++++++++++++++++++---- 2 files changed, 22 insertions(+), 8 deletions(-)
Oleg Nesterov
maintainerRe: [PATCH 0/2] exit/pid_ns: comments + simple fix
Quoted 9 lines · Eric W. Biederman
Quoted 3 lines · Oleg Nesterov
Eric, Pavel, could you review 1/2 ? (documentation only). It is based on the code inspection, I didn't bother to verify that my understanding matches the reality ;)
Oleg you are being subtle, introducing some gratuitous changes and slightly off base which makes this all very hard to review.
Eric, this doesn't depend on other changes I sent ;)
Quoted 28 lines
Eric
Quoted 25 lines · Oleg Nesterov
Quoted 4 lines
Probably this is not the last series... in particular it seems that we have some problems with
sys_setns()in this area, but I need to recheck.So far only the documentation fix. I'll write another email (hopefully with the patch), afaics at least
setns()doesn't play well withPR_SET_CHILD_SUBREAPER.Contrary to what I thought
zap_pid_ns_processes()looks fine, but it seems only by accident. Unless I am totally confused, wait for "nr_hashed==init_pids" could be removed after0a01f2cc390e10633a"pidns: Make the pidns proc mount/ umount logic obvious". However, now thatsetns()+fork()can inject a task into a child namespace, we need this code again for another reason.I _think_ we can actually remove it and simplify
free_pid()as well, but lets discuss this later and fix the wrong/confusing documentation first.2/2 looks "obviously correct", but I'll appreciate your review anyway.
Oleg.
kernel/pid.c | 7 +++---- kernel/pid_namespace.c | 23 +++++++++++++++++++---- 2 files changed, 22 insertions(+), 8 deletions(-)
Re: [PATCH 0/2] exit/pid_ns: comments + simple fix
In reply to Oleg NesterovQuoted 13 lines · Oleg Nesterov
Eric, Pavel, could you review 1/2 ? (documentation only). It is based on the code inspection, I didn't bother to verify that my understanding matches the reality ;)
Quoted 4 lines · Oleg Nesterov
Probably this is not the last series... in particular it seems that we have some problems with
sys_setns()in this area, but I need to recheck.So far only the documentation fix. I'll write another email (hopefully with the patch), afaics at least
setns()doesn't play well withPR_SET_CHILD_SUBREAPER.
Quoted 8 lines
Contrary to what I thought
zap_pid_ns_processes()looks fine, but it seems only by accident. Unless I am totally confused, wait for "nr_hashed==init_pids" could be removed after0a01f2cc390e10633a"pidns: Make the pidns proc mount/ umount logic obvious". However, now thatsetns()+fork()can inject a task into a child namespace, we need this code again for another reason.I _think_ we can actually remove it and simplify
free_pid()as well, but lets discuss this later and fix the wrong/confusing documentation first.
At the very least there is the issue of rusage being wrong if we allow the init process to be reaped before all of it's children are reaped.
There is also a huge level of weird non-intuitive behavior that would require some substantial benefits to justify an optimization of letting a child exist longer than init.
Eric
Quoted 7 lines
2/2 looks "obviously correct", but I'll appreciate your review anyway.
Oleg.
kernel/pid.c | 7 +++---- kernel/pid_namespace.c | 23 +++++++++++++++++++---- 2 files changed, 22 insertions(+), 8 deletions(-)
Oleg Nesterov
maintainerRe: [PATCH 0/2] exit/pid_ns: comments + simple fix
Quoted 22 lines · Eric W. Biederman
Quoted 19 lines · Oleg Nesterov
Eric, Pavel, could you review 1/2 ? (documentation only). It is based on the code inspection, I didn't bother to verify that my understanding matches the reality ;)
Quoted 4 lines · Oleg Nesterov
Probably this is not the last series... in particular it seems that we have some problems with
sys_setns()in this area, but I need to recheck.So far only the documentation fix. I'll write another email (hopefully with the patch), afaics at least
setns()doesn't play well withPR_SET_CHILD_SUBREAPER.Contrary to what I thought
zap_pid_ns_processes()looks fine, but it seems only by accident. Unless I am totally confused, wait for "nr_hashed==init_pids" could be removed after0a01f2cc390e10633a"pidns: Make the pidns proc mount/ umount logic obvious". However, now thatsetns()+fork()can inject a task into a child namespace, we need this code again for another reason.
Quoted 5 lines
Quoted 2 lines
I _think_ we can actually remove it and simplify
free_pid()as well, but lets discuss this later and fix the wrong/confusing documentation first.At the very least there is the issue of rusage being wrong if we allow the init process to be reaped before all of it's children are reaped.
Do you mean cstime/cutime/c* accounting?
Firstly it is not clear what makes child_reaper special in _this_ sense, but this doesn't matter at all.
The auotoreaping/EXIT_DEAD children are not accounted, only wait_task_zombie() accumulates these counters. (just in case, accounting in __exit_signal() is another thing).
Quoted 3 lines
There is also a huge level of weird non-intuitive behavior that would require some substantial benefits to justify an optimization of letting a child exist longer than init.
Sure. That is why I said "lets discuss this later". This patch doesn't try to change the rules. It only tries to document the current code.
Oleg.
Oleg Nesterov
maintainerv2 cover exit/pid_ns: comments + simple fix
In reply to Oleg NesterovTo remind, this doesn't depend on other changes in -mm.
It is not clear to me if I addressed Eric's objections or not, but let me send V2 (I reordered these patches).
I updated the changelog/comments in 2/2, hopefully now it is more clear that this patch only tries to explain how the current code works, and why do we need to wait for nr_hashed==init_pids from the kernel perspective. Otherwise it is basically the same patch, except I added another note about sys_wait4().
And, Eric, let me repeat. In any case we do not need for EXIT_DEAD tasks. They are stealth. In fact this state should die, it only complicates do_wait/reparenting/etc but we can't avoid it right now (mostly because of the locking in wait paths).
IOW. If we could somehow detect that this pid_ns has only EXIT_DEAD tasks we could return and user-space won't notice this change. No, it is not that I think we should do this, but this should be documented.
Please review.
Oleg.
Oleg Nesterov
maintainerPATCH v2 1/2 exit: pidns: alloc_pid() leaks pid_namespace if child_reaper is exiting
alloc_pid() does get_pid_ns() beforehand but forgets to put_pid_ns() if it fails because disable_pid_allocation() was called by the exiting child_reaper.
We could simply move get_pid_ns() down to successful return, but this fix tries to be as trivial as possible.
--- kernel/pid.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
kernel/pid.c +2 -0
@@ -341,6 +341,8 @@ out: out_unlock:spin_unlock_irq (&pidmap_lock); +put_pid_ns (ns); + out_free:while (++i <= ns->level)free_pidmap (pid->numbers + i);
Signature · 1 line
1.5.5.1
Re: [PATCH v2 1/2] exit: pidns: alloc_pid() leaks pid_namespace if child_reaper is exiting
Quoted 9 lines · Oleg Nesterov
alloc_pid()doesget_pid_ns()beforehand but forgets toput_pid_ns()if it fails becausedisable_pid_allocation()was called by the exitingchild_reaper.We could simply move
get_pid_ns()down to successful return, but this fix tries to be as trivial as possible.Signed-off-by Oleg Nesterov <[email protected]>
Quoted 17 lines
--- kernel/pid.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)kernel/pid.c +2
-0@@ -341,6 +341,8 @@ out: out_unlock:spin_unlock_irq (&pidmap_lock); +put_pid_ns (ns); + out_free:while (++i <= ns->level)free_pidmap (pid->numbers + i);
Oleg Nesterov
maintainerPATCH v2 2/2 exit: pidns: fix/update the comments in zap_pid_ns_processes()
In reply to Oleg NesterovThe comments in zap_pid_ns_processes() are not clear, we need to explain how this code actually works.
1. "Ignore SIGCHLD" looks like optimization but it is not, we also need this for correctness.
2. The comment above sys_wait4() could tell more.
EXIT_ZOMBIE child is only possible if it has exited before we ignored SIGCHLD. Or if it is traced from the parent namespace, but in this case it will be reaped by debugger after detach, sys_wait4() acts as a synchronization point.
3. The comment about TASK_DEAD (EXIT_DEAD in fact) children is outdated. Contrary to what it says we do not need to make sure they all go away after 0a01f2cc390e "pidns: Make the pidns proc mount/umount logic obvious".
At the same time, we do need to wait for nr_hashed==init_pids, but the reasons are quite different and not obvious: setns().
--- kernel/pid_namespace.c | 28 ++++++++++++++++++++++++---- 1 files changed, 24 insertions(+), 4 deletions(-)
kernel/pid_namespace.c +24 -4
@@ -190,7 +190,11 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)/* Don't allow any more processes into the pid namespace */ disable_pid_allocation (pid_ns); -/* Ignore SIGCHLD causing any terminated children to autoreap */ +/* +* Ignore SIGCHLD causing any terminated children to autoreap. +* This speeds up the namespace shutdown, plus see the comment +* below. +*/ spin_lock_irq (&me->sighand->siglock); me->sighand->action[SIGCHLD -1 ].sa.sa_handler =SIG_IGN ;spin_unlock_irq (&me->sighand->siglock);
@@ -223,15 +227,31 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns) }read_unlock (&tasklist_lock); -/* Firstly reap the EXIT_ZOMBIE children we may have. */ +/* +* Reap the EXIT_ZOMBIE children we had before we ignored SIGCHLD. +* sys_wait4() will also block until our children traced from the +* parent namespace are detached and become EXIT_DEAD. +*/ do {clear_thread_flag (TIF_SIGPENDING ); rc =sys_wait4 (-1 ,NULL , __WALL,NULL ); }while (rc != -ECHILD );/* -* sys_wait4() above can't reap the TASK_DEAD children. -* Make sure they all go away, see free_pid(). +* sys_wait4() above can't reap the EXIT_DEAD children but we do not +* really care, we could reparent them to the global init. We could +* exit and reap ->child_reaper even if it is not the last thread in +* this pid_ns, free_pid(nr_hashed == 0) calls proc_cleanup_work(), +* pid_ns can not go away until proc_kill_sb() drops the reference. +* +* But this ns can also have other tasks injected by setns()+fork(). +* Again, ignoring the user visible semantics we do not really need +* to wait until they are all reaped, but they can be reparented to +* us and thus we need to ensure that pid->child_reaper stays valid +* until they all go away. See free_pid()->wake_up_process(). +* +* We rely on ignored SIGCHLD, an injected zombie must be autoreaped +* if reparented. */ for (;;) {set_current_state (TASK_UNINTERRUPTIBLE );
Signature · 1 line
1.5.5.1
Re: [PATCH v2 2/2] exit: pidns: fix/update the comments in zap_pid_ns_processes()
Quoted 2 lines · On Thu, 27 Nov 2014 00:54:37 +0100 Oleg Nesterov <[email protected]> wrote:
The comments in
zap_pid_ns_processes()are not clear, we need to explain how this code actually works.
Could we please get some documentation for PIDNS_HASH_ADDING? What it does, what is the protocol for handling it, etc? I was trying to review your [1/2] but this little mystery has me somewhat stumped.
Oleg Nesterov
maintainerRe: [PATCH v2 2/2] exit: pidns: fix/update the comments in zap_pid_ns_processes()
Quoted 9 lines · Andrew Morton
Quoted 2 lines · On Thu, 27 Nov 2014 00:54:37 +0100 Oleg Nesterov <[email protected]> wrote:
The comments in
zap_pid_ns_processes()are not clear, we need to explain how this code actually works.Could we please get some documentation for
PIDNS_HASH_ADDING? What it does, what is the protocol for handling it, etc? I was trying to review your [1/2] but this little mystery has me somewhat stumped.
OK, I'll send another simple doc patch.
But in short it is simple. "nr_hashed & PIDNS_HASH_ADDING" just means that this pid_ns is alive, and alloc_pid() can succeed. We need this to ensure that a new child can't be injected after zap_pid_ns_processes() has already killed all tasks in its pid_ns.
Oleg.