[lkml]   [2017]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
Patch in this message
Subject[PATCH 3.16 014/233] pid_ns: Fix race between setns'ed fork() and zap_pid_ns_processes()
3.16.48-rc1 review patch.  If anyone has any objections, please let me know.


From: Kirill Tkhai <>

commit 3fd37226216620c1a468afa999739d5016fbc349 upstream.

Imagine we have a pid namespace and a task from its parent's pid_ns,
which made setns() to the pid namespace. The task is doing fork(),
while the pid namespace's child reaper is dying. We have the race
between them:

Task from parent pid_ns Child reaper
copy_process() ..
alloc_pid() ..
.. zap_pid_ns_processes()
.. disable_pid_allocation()
.. read_lock(&tasklist_lock)
.. iterate over pids in pid_ns
.. kill tasks linked to pids
.. read_unlock(&tasklist_lock)
write_lock_irq(&tasklist_lock); ..
attach_pid(p, PIDTYPE_PID); ..
.. ..

So, just created task p won't receive SIGKILL signal,
and the pid namespace will be in contradictory state.
Only manual kill will help there, but does the userspace
care about this? I suppose, the most users just inject
a task into a pid namespace and wait a SIGCHLD from it.

The patch fixes the problem. It simply checks for
(pid_ns->nr_hashed & PIDNS_HASH_ADDING) in copy_process().
We do it under the tasklist_lock, and can't skip
PIDNS_HASH_ADDING as noted by Oleg:

"zap_pid_ns_processes() does disable_pid_allocation()
and then takes tasklist_lock to kill the whole namespace.
Given that copy_process() checks PIDNS_HASH_ADDING
under write_lock(tasklist) they can't race;
if copy_process() takes this lock first, the new child will
be killed, otherwise copy_process() can't miss
the change in ->nr_hashed."

If allocation is disabled, we just return -ENOMEM
like it's made for such cases in alloc_pid().

v2: Do not move disable_pid_allocation(), do not
introduce a new variable in copy_process() and simplify
the patch as suggested by Oleg Nesterov.
Account the problem with double irq enabling
found by Eric W. Biederman.

Fixes: c876ad768215 ("pidns: Stop pid allocation when init dies")
Signed-off-by: Kirill Tkhai <>
CC: Andrew Morton <>
CC: Ingo Molnar <>
CC: Peter Zijlstra <>
CC: Oleg Nesterov <>
CC: Mike Rapoport <>
CC: Michal Hocko <>
CC: Andy Lutomirski <>
CC: "Eric W. Biederman" <>
CC: Andrei Vagin <>
CC: Cyrill Gorcunov <>
CC: Serge Hallyn <>
Acked-by: Oleg Nesterov <>
Signed-off-by: Eric W. Biederman <>
[bwh: Backported to 3.16: the proper cleanup label is bad_fork_free_pid, not
Signed-off-by: Ben Hutchings <>
kernel/fork.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1444,11 +1444,13 @@ static struct task_struct *copy_process(
if (signal_pending(current)) {
- spin_unlock(&current->sighand->siglock);
- write_unlock_irq(&tasklist_lock);
goto bad_fork_free_pid;
+ if (unlikely(!(ns_of_pid(pid)->nr_hashed & PIDNS_HASH_ADDING))) {
+ retval = -ENOMEM;
+ goto bad_fork_free_pid;
+ }

if (likely(p->pid)) {
ptrace_init_task(p, (clone_flags & CLONE_PTRACE) || trace);
@@ -1500,6 +1502,8 @@ static struct task_struct *copy_process(
return p;

+ spin_unlock(&current->sighand->siglock);
+ write_unlock_irq(&tasklist_lock);
if (pid != &init_struct_pid)
 \ /
  Last update: 2017-09-10 00:32    [W:0.571 / U:5.308 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site