From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933127AbdELPSG (ORCPT ); Fri, 12 May 2017 11:18:06 -0400 Received: from mail-db5eur01on0110.outbound.protection.outlook.com ([104.47.2.110]:56601 "EHLO EUR01-DB5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S933056AbdELPSE (ORCPT ); Fri, 12 May 2017 11:18:04 -0400 Authentication-Results: hallyn.com; dkim=none (message not signed) header.d=none;hallyn.com; dmarc=none action=none header.from=virtuozzo.com; Subject: Re: [PATCH] pid_ns: Fix race between setns'ed fork() and zap_pid_ns_processes() To: "Eric W. Biederman" References: <149459442953.20826.15806664408144117255.stgit@localhost.localdomain> <87shka40dw.fsf@xmission.com> <87h90q2kqq.fsf@xmission.com> CC: , , , , , , , , , , From: Kirill Tkhai Message-ID: Date: Fri, 12 May 2017 18:17:56 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <87h90q2kqq.fsf@xmission.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [195.214.232.6] X-ClientProxiedBy: VI1PR07CA0117.eurprd07.prod.outlook.com (2a01:111:e400:7a52::43) To DB6PR0801MB1333.eurprd08.prod.outlook.com (2603:10a6:4:a::27) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: ca26748c-8cd0-486e-9c54-08d4994a1442 X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(201703131423075)(201703031133081);SRVR:DB6PR0801MB1333; X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1333;3:KWbG0/htAVXUM4TkvEModkEY/ZYB1IAdpU9LF1uSr6Junl/fsquhRYEdVupFNawrWi8lPrgOAeX7xp/XLnXuqnOUwPUWZrMm5FzdRbm3btzVr9o6OFTf6GkfZB65Ytvxd+NnrO7rd78SQZXWXA1o0CDauRImzDQS+iJXJo1oOuU8WgiLIBA3VwgPV+x9whtWSinOY8/OAUB/JFyEd2gYKax5kshkLsXP7NsX1sLWOKWl5x7fi3T65k+yJxUJwE7Wl7L4LCN7l+zENaWnzHTmd8hGXhCw+trCxbbydX8vkqDAvtrQJUUlYs8BYqjWgNlwapajD1XtOVzX2E88N+yn2g==;25:pJP9RmzzlQhNyaRILusOuZJMl9mZY9pZXeEAHhAeflzCM7TAYjL9GbRwGBsxRqcnvJCCS/kF2/JJrPAfSb0dpx48hsE8FNMGwvE9kqFcLAz7XKTBLLnl9QH/Wm3o2p3z2pVzIso7xqZ3WXnJkUM+ymB8yVNysup4JPwYJR+SWx36wT8uZOkUWGdfDlC3HsCdJiiZahUnjc3p1yOsBolTiOKKJs/nOEHf+2dtQWei9n7hSd7bN7sQTQnbSocI+6kxo5Bm8KMcyOx56x89nhKKYPCt+3EQNT2fm8bkEMNoy6yjdMZvBN1Vxh4v6bqOHz1LqCb02lTDqAjvpcF7zD9u6PAsPw3uQfUG9B8i9eOl/iYfmu+uM1ZOuRYnIXv2eV5iOZjTKPeef5CS2IdF3mIhNx40ubzQ3UUusFGncC+Lz1oFa3TqY2qQ7epK3qilXV5B9vSG+wb5Cxa+fKp6NIa68yeUTPryQvoDUyu2+C2SpUc= X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1333;31:CWlqbBhXnS0Az0PzA+KwVLhw0j4Wh/rVskV3kvACnLJ13rVWWuwIPQpvJJnOIJvitg51RUq1oeYFigM2prwD8nkHJrWCHsB4g25BPqXydwli0MLumEERp6StE/UUbwuG+t/ZyrrKL7Iz8P+GWJ0rE6TfO13gEgbCnLKfAwDUOLeXbaqCW+7RWj5/kvuj7udYtCmPFptATEarHaNkoGMSCRauS4kS1bp1wdjjvoKTf38=;20:RvhO7IKoQ/yjXPlv2RfKE8Kw4XusHitNluXSr6Al64qwz03uBRZn6CAUCyVKFQFyEwO1r41Kz1BO/pXO5SyVhRP8KIH3MpE/2nz/LIIAP0hphwFQuo2mcBg09EeTGc3g9QWkWTIkr/1bM6Mgt/GkxYX/PqMWqmj+FnhgmdPwyb2gEOlHlMfTfOsh8kAIhDChi/WhtWMj5dP9q/Cj15EwBaSIURja2yYA8339VmFSZkUA+aS3FWq9BfGbVglE07oQiV7jTzD7zmo8npDoTMRwv2+sPyY4b3NiB5Ur/p4iEbWjZB+Bx0kdBl0y4XOty6VEL0kluaclUScCL9cyn9fWuip8qhDq8oYdEqhkuRMkJAV8CzhgR1RTiovhFrJgu7DmNBoZ0znUywGTpxYPFpXvl/SODjAyuPxER0qmilXzWPQ= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(20558992708506); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040450)(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(93006095)(93001095)(6041248)(20161123560025)(20161123555025)(20161123562025)(20161123564025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123558100)(6072148);SRVR:DB6PR0801MB1333;BCL:0;PCL:0;RULEID:;SRVR:DB6PR0801MB1333; X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1333;4:nqkkMhImFtCgjJ6v8kSUsdiCMmf2kirLBue6ZFn15FKVhX7xclEAWKeBrWM50rexaI4AzB1ZxqYcrxVQ9yXWcRiMoOzHBrdW1UlLIW+OewGNGQzsLIcsrBXxhZ2rCnxh+G2GPNO3cG+g5iK7+7Jf3slxtR9NELAkLC79LLb0ssesYDQRbIkJrCljNnxuHTc7SUMwUG8P+iMIo/V7qtnfeQYpjXgWcxpF/RMMH6hAAASIK3Zv1ksHmCAjlDRHd9MLVfS5pv8wdWZM/2qHuuup0SQOsw09kuUh8p8hdzQuy09e3BJV1DWcqWI1k9psUGGEYRFvAL0Zejg13dOkcpphW20Cm5YiOogMKCe+94gO+ILzW80wMjqs6yLIDN1BPcx6wx+uyIzlEWMeV85gjXek8/GA8jplSkONa5k+CTEvoDF+8rMOnAsPQOahwjP8qYTlOY3Lef4F1tlHgVDMYY4LXXBvICiFuZIh8bJ4ttN5bdN7BmZIoJHykWoZisJwWe4oYtclmQHpZNeyM5GUreFpS9bbQnpa8+6sKbL0A7yzutFX+Kf3jEFd8yFEa+O2ayKuHvlG9eZMBj6gn/yGh0eCNDIQ2ZiZihLpJfWTBXi9PZbjzmeEXcHc9gcXzC5OdM1kWggzFoAr5FxCAqdXY7QgrV8ngwBV8csxqJ2GiIaf8gB7CVZnfGq56YowylDZq/hObW56Fs8uH5fbIuKVCDAD1/5ZzBEKOUntoK7t9tA9IlvsGNdsnZJZJc4dOoSaTd7m1wxMvK3lQIROcOqV0ver5dZ8BKnzRd1USYs9xwFli+jzug3BwNzb+1ocK9d0H85CIgBRgU5hyz11XXYcTgMkBA== X-Forefront-PRVS: 0305463112 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6009001)(6049001)(39840400002)(39400400002)(39410400002)(39450400003)(24454002)(23676002)(65826007)(7416002)(77096006)(65956001)(65806001)(66066001)(47776003)(36756003)(2950100002)(5890100001)(53546009)(478600001)(25786009)(189998001)(6916009)(83506001)(93886004)(7736002)(81166006)(5660300001)(305945005)(8676002)(50466002)(6666003)(2906002)(6486002)(4326008)(33646002)(229853002)(31696002)(53936002)(42186005)(31686004)(6116002)(6246003)(38730400002)(110136004)(230700001)(3846002)(50986999)(54356999)(76176999);DIR:OUT;SFP:1102;SCL:1;SRVR:DB6PR0801MB1333;H:[172.16.24.209];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtEQjZQUjA4MDFNQjEzMzM7MjM6N1didkV6ZjhEbmxwLzk4K2tsUFoyNCtO?= =?utf-8?B?MGt3Wkg1RDBTekFDSzRMbERNMUxDZW50S3VTL0lRck43Qno5bWcza2FPeDBY?= =?utf-8?B?OFkrMjVORGJXeHg0Y3dzNE9ESkZZNTgramk2QXNhUjNob1M3RU1ieS9Va0ta?= =?utf-8?B?U0RpYjRaNENXRjd0VFlwaVJxRUU5RkVUclFsMmMyM2dKZWtxTmNWejgwbUQ1?= =?utf-8?B?dGM3Y29aNzNXK1BKbDIyNDBxMHUzcTQ1S1lkY1ljcTJhUFpMVm9iS3ovbmJM?= =?utf-8?B?OEVFVXJFd0F5aE1TWS9XVVpjK1pQa09ZZUVRbzNzRzJvNFl3YmY4a200YkxP?= =?utf-8?B?MExIQmxwMCszTU1rSXhjczBIaThZeWFsOXV1bG9sb25GbVp3UnM4UklWZ2ph?= =?utf-8?B?Y2JMdFZRS3Q1K1ZlYUZXbTE3aTlTWXBXdXZ1MG1DNlNCcVloT1l4MUpJSDZa?= =?utf-8?B?RUdPK3VINis4QzFGYVYyQmFjTzEzOGsxdzZiQWwyQkFXRkUwdlY4U1Zhb2kr?= =?utf-8?B?ZzdPeTBudjlSbG9yZ3BHOXd3bXFGL1Z0dDhDajQxRFBoSWFXMGx1L1NURmhI?= =?utf-8?B?ZFBqdTNQaDVsWjJCTERGZjNOc0JBNVZJTXllTGZrZzRVclJwUnk2djgwbUZ2?= =?utf-8?B?ZkRBeGRIMHR0WnRpVXhzS1ZzZWxBVERVdk1YWHBFaVZndDNDZFdQU05yYU5T?= =?utf-8?B?Vll0NXpHUytKYW9XSnYrZllHS0IySitHN1E0WlhVQ0pJR21MUHhuWkljaE1j?= =?utf-8?B?b0tud05LM2dQdFRMeVgwaWx1S0tmMjg0TS9tcWtKaU41VysrblJ3KzJTTGZs?= =?utf-8?B?NVRPbEdaQ1gyRXlqS3VoUVkrSzNaYWJaanVDbmZTSW00cWRGUDhHK3N6ZmJt?= =?utf-8?B?N05MdU9mUkxYcXRTSTNTYmlaaGdOc2MzN3hiSmVxMmdsNnk4QzhrMnpXb3hQ?= =?utf-8?B?c2VsSDY5UStEUXE3SERIWVR4K2UvNkRBNzQ2VWhrUHBGT2w0UXUzbGZ1SVlX?= =?utf-8?B?WUtQd1ZZTG9qYlkwU1VGdThkKzlqZkJQQzJNR3p4dWt5RWw4REU1R2RKT1VF?= =?utf-8?B?RkNXM3ZDWXlUcHA1bU54RTlwbDlwVE5YRmx5d0d0Um1EVHBxbkJ1Zk9aSThL?= =?utf-8?B?NjBwYTcvclBLSUdERDBTaTJTOE9MaVFEdmNrQm9DOGVaenpac0ZVTTBBMDdQ?= =?utf-8?B?aG50QmdOa2s3ZllTOG1HM1V0S3pkQ3psMVZvN2YyZk1oUVBMQjVrL1lGb2Nl?= =?utf-8?B?cHRoNUhGSy84aWRjTm5FYUJnMUZzdHl4VWp1Wnh6YWpMY2VNbUZURDVMUXpN?= =?utf-8?B?ZStJRzVTdEQ1cW15cVVjaUFiV0hpa3JVMkdpWnJMV29LbzBpaXB1dUxFbEpp?= =?utf-8?B?WjVXUTlYYkk5U21BRXpCWTFpWUhXVlFsVHdpbHhpOWJmYU5jZzdMRU02OWtp?= =?utf-8?B?S0pZWlJaTXlHaFdWd3Y2Q28rVHE0OFlkSi9maVp0d3EyL3NkL1JXby8vdlg3?= =?utf-8?B?REV2QjRJdTVDTGEyc1BOUHY0ZXBCTk5kZHJPTFZKOUxJNXdteGhGMUs3QThZ?= =?utf-8?B?YW1Ga3J0V09KWHo2MWYvVHJpeHdVMnNDb1RqNjVaYWJ4dzFPZ04xcDdKRW1V?= =?utf-8?B?UjhKYWJzM3hHdGloRzN5L01lQ1ArVE5hZTJUd1haNmRjZUx4YmFGV3hyb25j?= =?utf-8?Q?VDzOCTFY36CHdkJUZWPs=3D?= X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1333;6:rLl6zAdOFGr4+HUcrg2KgBrkdMof9gjDdLSjiffoIrvYNAxaUecTXvSajyDu8UU1pPlVWIJjM0PFw1OR4Qq5FlCrsy7qXcD59iWACNWQ+aBUSOiK/UPMxzUZsy5hqIKmKzaIaO27Wao5+sZRrBGBA7QWO39CGFoqLdr6A68MUv2dy/DznkKhWVw9DWiB1+ggvMFNMP6jJ+6XpgPmoFySPS+rn+emUWrmNcaisZr+oKNr+7g1XJefO91hTwa+1kzbtl3SyqPaH9qtwyMvztiKujBElc2vFZd+Zp4wup+zsTl0EE5RNZneAz+ZKkr76G0PWPcZlUPdjS9ARMW5RfkLfWTpJeKNY9LmbBMpwve2GKk90IDg1VBBT0XLmlHda+xWS3BEVW7WtLuWKVFFVQn7BtFBHPaH9WI5rVIzDKwNDGWkshpRykd40pE4FmYZqz1w9P6y3yX73UcZt3NmPb0FXauJL0OZr9B0DmFjJnIQwULKbc1Vfdo86GjbbZ0HO15x/T6jXi1jgpNXnijCgS6M+A==;5:ikyeFqX6LoNX2OShGEMeHEqXkEAXjePkxERysPykdb5/iYlfkXMUmLGxghqsl6J4/2vKSbulBcg8ja+FaLId2neZjSA0fE6rfGy06SYB0TFI6IPLePZNbrSbk2CnJfFfWae0LiIthfLttXvm7IdMzQ==;24:eWDRbL2k8xx7V7kT4riNHeEcPLigYuvRP4eqGFAvrxqnHhfYihxooNIi5PrZ9753uhtivM0uS+9wRGCWCpZm7y3gk/+2Pqd4fFvM///6KL0= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1333;7:ozM+gB2YFQa48kU/Ti7VhQgWAkWIuIFcm3KS9uEQJT9eNbwI7LuXBNGEQ2bBwPKAHBLjW13YpHfpDmOp2iJozoK43N755ajznzwBMlnVgXjqkzVy8orMufo6tMdayOLZfxnsH2JXlTT/QLkTfheU2kVph7bxkLXeN4d+PC6rhrwIdMYMNev47FrRrX1BU7PE91Ee0GIz5+eJCTTsxJVZWj/wopgr4XcfWDmmYyRfPa9XN68kUcjbEpYi/kix99zWpqUkA4f/NBS9OKTzMmSGvcHNDvrwT31l1Hz6e6vqPBLcYGMG0pLL4AALOSGLEmxXj4sWAXFtYwJiSh2fkdLZeQ==;20:B7OJ/PKK3VGB2Uc45q31bVBl6Uvkq5BgvRzFQCzTipUiuFjz+y1AF+NBEnXuxEYzVlSrXHKq+bFfSizkjB3bXz1Buxu7mSM49PVIQh+780wwX2efyd7jAoVlmhIru8B6BZ2GaSdQ428JB7TLMxCT6Zjqrmbrb01xBv0c7Bij3nA= X-OriginatorOrg: virtuozzo.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 May 2017 15:17:59.8871 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0801MB1333 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12.05.2017 17:49, Eric W. Biederman wrote: > Kirill Tkhai writes: > >> On 12.05.2017 17:26, Eric W. Biederman wrote: >>> Kirill Tkhai writes: >>> >>>> 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 moves disable_pid_allocation() >>>> into find_child_reaper() where tasklist_lock is held, >>>> and this allows to simply check for (pid_ns->nr_hashed & PIDNS_HASH_ADDING) >>>> in copy_process(). If allocation is disabled, we just >>>> return -ENOMEM like it's made for such cases in alloc_pid(). >>> >>> This problem sounds very theoretical has it ever come up in practice? >>> I am asking to see if this is something we will care enough about to >>> backport. >> >> I haven't seen this on practice. I think we may apply the policy, which >> used to coverity reports, though it's not a one. >> >>> Please look at what happens when you call >>> spin_unlock_irq(&pidmap_lock) under writelock_irq(&tasklist_lock); >> >> Ah, missed that, thanks. >> >>> Please also look at what happens when pid == &init_pid but >>> p->nsproxy->pid_ns_for_children happens to be have PIDNS_HASH_ADDING >>> set. > > Apologies I meant PIDNS_HASH_ADDING clear. > >> init pid refers to init_pid_ns, which has PIDNS_HASH_ADDING set. So, >> there shouldn't be a problem. >> >> Could you explain, what do you mean? > > I mean locally in copy_process your code is not correct. > Instead of caching pid_ns you want to use ns_of_pid(pid) so that > if pid == &init_pid you don't care what strange things are going on > in the calling process. Hm. If pid is init_struct_pid, then we're forking INIT_TASK. p = dup_task_struct(current, node), so the p->nsproxy == INIT_TASK->nsproxy, i.e. init_nsproxy. Its pid_ns_for_children refers to init_pid_ns. There is no a problem, it's just a code simplification. But if it seems not clear for you, I may do something like below. How you are about that? --- diff --git a/kernel/fork.c b/kernel/fork.c index bfd91b180778..e9835693d299 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1523,6 +1523,7 @@ static __latent_entropy struct task_struct *copy_process( unsigned long tls, int node) { + struct pid_namespace *pid_ns = NULL; int retval; struct task_struct *p; @@ -1736,7 +1737,8 @@ static __latent_entropy struct task_struct *copy_process( goto bad_fork_cleanup_io; if (pid != &init_struct_pid) { - pid = alloc_pid(p->nsproxy->pid_ns_for_children); + pid_ns = p->nsproxy->pid_ns_for_children; + pid = alloc_pid(pid_ns); if (IS_ERR(pid)) { retval = PTR_ERR(pid); goto bad_fork_cleanup_thread; @@ -1845,10 +1847,11 @@ static __latent_entropy struct task_struct *copy_process( */ recalc_sigpending(); if (signal_pending(current)) { - spin_unlock(¤t->sighand->siglock); - write_unlock_irq(&tasklist_lock); retval = -ERESTARTNOINTR; - goto bad_fork_cancel_cgroup; + goto bad_fork_unlock_siglock; + } else if (unlikely(pid_ns && !(pid_ns->nr_hashed & PIDNS_HASH_ADDING))) { + retval = -ENOMEM; + goto bad_fork_unlock_siglock; } if (likely(p->pid)) { @@ -1906,7 +1909,9 @@ static __latent_entropy struct task_struct *copy_process( return p; -bad_fork_cancel_cgroup: +bad_fork_unlock_siglock: + spin_unlock(¤t->sighand->siglock); + write_unlock_irq(&tasklist_lock); cgroup_cancel_fork(p); bad_fork_free_pid: cgroup_threadgroup_change_end(current);