From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756210AbdE0LIC (ORCPT ); Sat, 27 May 2017 07:08:02 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:40737 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750928AbdE0LH7 (ORCPT ); Sat, 27 May 2017 07:07:59 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Kirill Tkhai Cc: , , , , , , , , , , References: <149555694775.4786.18241837604458780213.stgit@localhost.localdomain> Date: Sat, 27 May 2017 06:01:11 -0500 In-Reply-To: <149555694775.4786.18241837604458780213.stgit@localhost.localdomain> (Kirill Tkhai's message of "Tue, 23 May 2017 19:29:44 +0300") Message-ID: <87y3tizhqg.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1dEZZU-0007mj-H6;;;mid=<87y3tizhqg.fsf@xmission.com>;;;hst=in02.mta.xmission.com;;;ip=97.121.81.159;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX19N/FC8b4JlLklKk7hBltIwNM4c//OVx4g= X-SA-Exim-Connect-IP: 97.121.81.159 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.7 XMSubLong Long Subject * 1.2 LotsOfNums_01 BODY: Lots of long strings of numbers * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa01 1397; Body=1 Fuz1=1 Fuz2=1] * 1.0 T_XMDrugObfuBody_12 obfuscated drug references X-Spam-DCC: XMission; sa01 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: **;Kirill Tkhai X-Spam-Relay-Country: X-Spam-Timing: total 5890 ms - load_scoreonly_sql: 0.06 (0.0%), signal_user_changed: 6 (0.1%), b_tie_ro: 4.6 (0.1%), parse: 1.39 (0.0%), extract_message_metadata: 59 (1.0%), get_uri_detail_list: 6 (0.1%), tests_pri_-1000: 28 (0.5%), tests_pri_-950: 2.0 (0.0%), tests_pri_-900: 1.64 (0.0%), tests_pri_-400: 46 (0.8%), check_bayes: 44 (0.7%), b_tokenize: 19 (0.3%), b_tok_get_all: 11 (0.2%), b_comp_prob: 6 (0.1%), b_tok_touch_all: 4.3 (0.1%), b_finish: 0.80 (0.0%), tests_pri_0: 885 (15.0%), check_dkim_signature: 0.92 (0.0%), check_dkim_adsp: 5 (0.1%), tests_pri_500: 4855 (82.4%), poll_dns_idle: 4846 (82.3%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH] pid_ns: Allow to get pid_for_children ns before child_reaper is created X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Kirill Tkhai writes: > This patch prohibits pid allocation till child_reaper > of pid namespace is set, and it makes possible and safe > to get just unshared pid_ns from "/proc/[pid]/ns/pid_for_children" > file. This may be useful to determine user_ns of such a created > pid_ns, which is not possible now. > > It was prohibited till now, because the architecture of pid namespaces > assumes child reaper is the firstly created process of the namespace, > and it initializes pid_namespace::proc_mnt. Child reaper creation > mustn't race with creation of another processes from this namespace, > otherwise a process with pid > 1 may die before pid_namespace::proc_mnt > is populated and it will get a null pointer dereference in proc_flush_task(). > Also, child reaper mustn't die before processes from the namespace. This patch introduces the possibility that two or more processes may have the same pid namespace (with no processes) as pid_ns_for_children. Which means you can now have a race for the first pid in alloc_pid. Making it indeterminant who allocates the init process. Which is not acceptable. It is not acceptable on two grounds. 1) It is a bogus user space semantic. Because userspace needs to know who allocates init. 2) It is horrible for maintenance becuase now the code has to be very clever to deal with a case that no one cares about. Which is a general formula for buggy code. Eric > The patch prevents such races. It allows to alloc_pid() only if > ns->child_reaper is already set, and it guarantees, that > pid_namespace::proc_mnt is already populated. Also, we do the assignment > under the tasklist_lock in the copy_process() stage, when it can't fail. > This guarantees the child_reaper will be hashed before the concurrent > process, so the concurrent process can't die before it. When child reaper > dies before the concurrent hashes to task list, fork() of the concurrent > will aborts as it's prohibited after commit 3fd372262166: > "pid_ns: Fix race between setns'ed fork() and zap_pid_ns_processes()". > So, we can't safely allow to get "/proc/[pid]/ns/pid_for_children" > since it's created. > > Signed-off-by: Kirill Tkhai > CC: Andrew Morton > CC: "Eric W. Biederman" > CC: Oleg Nesterov > CC: Andy Lutomirski > CC: Serge Hallyn > CC: Michal Hocko > CC: Andrei Vagin > CC: Cyrill Gorcunov > CC: Mike Rapoport > CC: Ingo Molnar > CC: Peter Zijlstra > --- > kernel/pid.c | 3 ++- > kernel/pid_namespace.c | 9 --------- > 2 files changed, 2 insertions(+), 10 deletions(-) > > diff --git a/kernel/pid.c b/kernel/pid.c > index fd1cde1e4576..eeeb01fdd87c 100644 > --- a/kernel/pid.c > +++ b/kernel/pid.c > @@ -334,7 +334,8 @@ struct pid *alloc_pid(struct pid_namespace *ns) > > upid = pid->numbers + ns->level; > spin_lock_irq(&pidmap_lock); > - if (!(ns->nr_hashed & PIDNS_HASH_ADDING)) > + if (!(ns->nr_hashed & PIDNS_HASH_ADDING) || > + (!ns->child_reaper && !is_child_reaper(pid))) > goto out_unlock; > for ( ; upid >= pid->numbers; --upid) { > hlist_add_head_rcu(&upid->pid_chain, > diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c > index 74a5a7255b4d..51dd1d490542 100644 > --- a/kernel/pid_namespace.c > +++ b/kernel/pid_namespace.c > @@ -385,15 +385,6 @@ static struct ns_common *pidns_for_children_get(struct task_struct *task) > } > task_unlock(task); > > - if (ns) { > - read_lock(&tasklist_lock); > - if (!ns->child_reaper) { > - put_pid_ns(ns); > - ns = NULL; > - } > - read_unlock(&tasklist_lock); > - } > - > return ns ? &ns->ns : NULL; > } >