From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932250AbdELOsB (ORCPT ); Fri, 12 May 2017 10:48:01 -0400 Received: from mail-eopbgr10118.outbound.protection.outlook.com ([40.107.1.118]:12001 "EHLO EUR02-HE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756759AbdELOr7 (ORCPT ); Fri, 12 May 2017 10:47:59 -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> CC: , , , , , , , , , , From: Kirill Tkhai Message-ID: Date: Fri, 12 May 2017 17:47:51 +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: <87shka40dw.fsf@xmission.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [195.214.232.6] X-ClientProxiedBy: DB6P191CA0015.EURP191.PROD.OUTLOOK.COM (10.175.236.153) To AM5PR0801MB1332.eurprd08.prod.outlook.com (10.167.217.10) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 31d0ba2a-34af-414f-ef7c-08d49945dfe5 X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(201703131423075)(201703031133081);SRVR:AM5PR0801MB1332; X-Microsoft-Exchange-Diagnostics: 1;AM5PR0801MB1332;3:PhX0fE3ksi5Nh3Fkuju4a6t0lVGe9MAGDzPAjU1WZYeUSrcRpUriYsdh/QuTXIf869IYL+m4VWLnWZXsbPwC4Jyxfljlmaim/Rabw/cLLvkFafpQaVvGIXMY4YR9r/YiFf6o7GvFg6DoywuYDUnTOyuZbHUYmzz5uvmYQZ0KF1kuZiHQSQQj6WO0DlHiqZ943MvSo7ePqwFhkJrCpacJmcWpmanfRD9sVm2m2WwnzxBLLiKwgrpWTaxtlbDrO33qu6LBrinWO3m5G05DzSxOwAzzeHra/kS3c/BtLw0dvoIW6KnJJLfkETUpRAA1+ZhDMKSwQlNuyiYxgGD7JEF/WA==;25:0ewoLAVlLDcBg7+KT3Y9L5c22rseQ5FlKG9iJFfd8H7NsRlpCq3XvUKmvnEHxQYtz8mZfab529FliFWFhO1PBYhqsanziHsLhyq+2U8gO4bTWyY4LkFiEPBu61RpmYd4T+qjIFRi3HCkyMXpzSTfsPd6FW8IEtLFPcv+WqnW49d2xQ57NS6GJl3gyhf8+fkU5yBeovV0Fwxbvyg5CssjAgCgVBvIO8h/h2+0tjie5Xz96j1Y7qDLcIK4Z8EtvGSePfkG0iJJhinkumpvO3qnvw0yvRcmenaUcA6obZeNJ1oQxxMWK78lLRlPaTPcBYEUWIkxWAkrvSHtOqOqHMfTO5ME96RokCLjSDnFPVOb7HtWJK5dm9rwOuhipAcCJVzTaDPK8DUSwZkafCn5bQuVT9ssdurqnEzLD+IBY4zvG3/mf4BrJOEvowbvQI8jipImY1AiEcBGyBPMLz0QOIxQOE4OcxdVr2PBvCV0eEHwv5w= X-Microsoft-Exchange-Diagnostics: 1;AM5PR0801MB1332;31:2H0BU4vTjMmV5rx0s4Grgn5bg6jN8jb59r0cXvg2XG9DZNTVXac85hsOotjZYo7Zv+awV9Fz48QO5HVgTG7MBpvfHSvQXRN+2S4ox91P8opnZ2Y5vYnM6x8kYcAAShMtjlbsjrjHyGm+oYbdm6U/IK4srCQ8hf0Lp4DXeIbdq1cw5M/58sceG4k3FhO77jU4uw6UAreGVqMIHYZsv6+09nHdulWXA9dvmBeJZ5vxk2M=;20:+bI3R+tmrNOlb3wQuyjWfzIaTIZNDKsJHn2sAM77iSO9OeQevhJt4cX/7VeulvZSuwdnJYH/vdiENUX4Q4TEQtyrOwKw3UYdbVAlONDSJPfl/EmDmECAJJ1XJj8QZxAfVcS6fM/OWe4cW44EJnrV8qSO/muhFTHDDxRJywFnQnse+5UEzNXieetKg+l3zeFAY2R4w56WaU65wokgXwXX6awCFRbWFDPFlqyxouLyudOh++j36Pa1zmSl7Xv4im0b58vYR9mX797dL/32pkmzWU2Fodd1SY5XepaXqPHmUT7kVGQL2IcnEZmedhKopYAdA7WjAFfhQh4wx1NqNujNi1UtDw1YBT9czANaWMxq5YzGrpIE9VFRn4p5kRqaCU2Sfoks47G/29Q/Hm6Ave9d9tcLfJ0vGxtRnShQZI6cw6M= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(20558992708506)(104084551191319); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040450)(601004)(2401047)(5005006)(8121501046)(93006095)(93001095)(3002001)(10201501046)(6041248)(20161123564025)(20161123558100)(20161123555025)(20161123562025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123560025)(6072148);SRVR:AM5PR0801MB1332;BCL:0;PCL:0;RULEID:;SRVR:AM5PR0801MB1332; X-Microsoft-Exchange-Diagnostics: 1;AM5PR0801MB1332;4:38xiVzdUKAAhOU4qxkQ9kifgZzv4ha5Q7PG2/ltvIJVRMjHXDh+hTDtaNPXdvGEDIyuPK8xX0uDv+SzQ8Okcp1x/nNA9BnavEQlSIdtPGZNKYWvILF0TYRhSsSeibx5Nvx2shBz7dCtPy3AJwjMP/meWJHAJVEGAhcxSoNFmtVtC3CilKWi2T2ygF09zKJbOkNR27/ERKu5ZPukiVXihCKVM80bvK/ZYhm/4WZBg610qv0duA+R64PTixsE48sFj8lxcEHZH3QMCD3N/uWKb8De3Z3gLefxHejZQ/6974Il/93me9dFYScZYKygGHecvW8tpksZapGVws+Wh/bHEuQi0PshcHLSZBLQJ8nKhRHfp/1BloPv9TrM7XRqQwSzJgZhh3QqMTL2IvzUxtpP9EeHjwMpfMAA54Q8oeenZKP9xvcwOy/1u41uzAm8H9JU+TpVbdKPALHEpj2b6nGaEraR9SuPhZdKwGnCpUjONsiuFgCabeiJvyH45qmPLIYFSMRC/+lb+I0tHPkzEhyH/22ON3byTl8Eib0VWvBRVuCqNPGRK//gip8eEOzTDZ/FDdXOPM8HMiNy930bqdC/CWjW2ITRhkDhpV5CYQsz79bYYAeFBBbEuS9LfVVDwGqXcz50r1OJC9lFzoz0NbYIfPdNTe7jXYb1vnCuXyvJ2kWqr4RB8uzcbZKvI00JhHknCZQscWToFP5gljrY/+Ya9YsE8OSSB7S/Eq3XN4ouObiCXVg1RrxBo08y7xzC8abUJUbGqTuJWM4efdckdBsTdj49SWxhFa0BFoi5i79FxjDit9RmfCwUupDLpMt74DOT8IccM6rUafxJL1HjMB0q+Qf1VDH4btKMxw8qDvxVjxHg= X-Forefront-PRVS: 0305463112 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6049001)(6009001)(39840400002)(39450400003)(39400400002)(39410400002)(24454002)(229853002)(6486002)(5660300001)(77096006)(6246003)(189998001)(575784001)(5890100001)(3846002)(6116002)(6916009)(2950100002)(31686004)(31696002)(6666003)(53936002)(2906002)(42186005)(7416002)(38730400002)(305945005)(110136004)(65826007)(7736002)(90366009)(50466002)(33646002)(47776003)(36756003)(66066001)(54356999)(76176999)(50986999)(65806001)(65956001)(25786009)(83506001)(478600001)(230700001)(8676002)(81166006)(23676002)(53546009)(4326008);DIR:OUT;SFP:1102;SCL:1;SRVR:AM5PR0801MB1332;H:[172.16.24.209];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtBTTVQUjA4MDFNQjEzMzI7MjM6Sjd6eFZOTzdOeW9OOHhXRkVPanhKSUw2?= =?utf-8?B?U2U2NGNhQ0lOY1h1Q3M2M2ZZYjQ2WktJQ2NlTUU2ZFY3bDcyaUMwVFBQRVV5?= =?utf-8?B?Ny8xU0NCdjk2ZnBZUkh1Z05aaHExc280citLRy9DcmdGTFgyRnl2Zml1Zi9H?= =?utf-8?B?c3preXBtaGtPcUtnbGM1KzduKzBGK0YraU53N05BbllwOXRpTTRJcWJPcTly?= =?utf-8?B?TXNqWm9SaHdJZzgrWGhPRko3aDlTU25vUFBsZzFUeDNyMlc3ZXJQQUl0VzhG?= =?utf-8?B?eEN0c2lDQlI4enJZeFkwTHR6eG4vZENlcGxrS25kblY5U1B1NTd1cXhzeVNu?= =?utf-8?B?aFFJNzhjaDNWdlhLSkhVNGtoY1pmTm5HK1dpckVzTk5sRkNhNHpkT1hrc2xi?= =?utf-8?B?NVY3VFozZU5GeEYySFpvUE9KVEVzd1FPNyt5SWR6c3dmNEpyV2J4d2ZHNEtZ?= =?utf-8?B?dUpHdHcvODM5aU55NHlvandTanFvMEZvK09sbVY0NkZlamZFQU9DUDYxOFdq?= =?utf-8?B?V3pGTTh2ejZBeW9xd0M3b2cxaVZLa1FkcE1VZkozSFI1bDJmbnB6NnI2bGdG?= =?utf-8?B?ZTBoa2ZZZVJ3aXRRWFJBYzZFczVXejBSS3pOZ1VZTFRyTkU5L2RVeEg0NFRK?= =?utf-8?B?QWc2cmlCQkNRaUJ3dmNWM3BLa3NXb2JaTWlPNW0reHRHWlVnQTlMRUk2Z3Nt?= =?utf-8?B?eWpRMGtuTVdGUGVib09HVmxseG5wNDMvZVROeTI4RUVmMlFtQlpNWnN4OHdv?= =?utf-8?B?OEN4bmFZeVVnOW5qbWZESGNVcThoLzdSbS93NUg1K2R0K2FpZEsxVHhuWFJk?= =?utf-8?B?NnFhRHJXMkt0RkJDT3RnZGJQc25WV0IxWVFlWEN5MlVtVWd1bDZESXovT2NQ?= =?utf-8?B?OHV5MVZDSitCMk14NS94bjBHVmh4ZVRpa2ZvN1J6eC9HbTBTV2VIOTZqM2t1?= =?utf-8?B?Nnk2bW81cHgraENPM1YzRnZ6bmZrZEVod0t4bmRTaytTQ0t4MHVBMUFPUDYz?= =?utf-8?B?SWJUcW0wUm5Zb2pMS09KUUwvdVM4RHVidTJzY2NLTEtpNmxvbFE4WE84Sk5p?= =?utf-8?B?c1QyRnBPL2tNOVZxM2FMVHJEZXJtTUpXQzB1T0IyZVBWdkJKR3lMSStMSits?= =?utf-8?B?QVU2Ujh2L3ExSDhFZGZVUFFrUkJRM2dCaE81SzEyV0w4aFNmeW9GUU1RZ3BC?= =?utf-8?B?Zm5GZi9jencyeXdZR3habzA4Y1c1ci9NZHBpbEpJSUw1WlI3QitpVmZYUmVt?= =?utf-8?B?cnhvUTBHYlpNT2JnOEhUUXNMc3M1UWg4WW1PcnRRVmhVaXpDbFIvdWgySEpI?= =?utf-8?B?T050ekU3L0tYTWRodWJwRzMzdi91Z3pjNklHQ3prcnI2c2VwYU9QRGNMdjBz?= =?utf-8?B?WjdxNUZoUUJFK0JBQkxQbFcwZnRKU01BRVFpS3ZLdXdoS2pSTXpXSGExUHFK?= =?utf-8?B?ZjNxUyt2Njd1L0FLeW5FSjFsYnVDOUFvdGllMExyRTU5LzVkUW9Pa2dxaHVN?= =?utf-8?B?WmNEWitCN08xNVRSRDFTeGN4VWRoV0F0dCtEWDhUR0MyM1dnb21kRWVVQkxY?= =?utf-8?B?U2xPYVdHWG95UVVEdHpTYXB2ZDhwNUl2MWduMEhNVjFlb0tuVXVUS3BReTlw?= =?utf-8?B?SzFQM3BZbTVDOEh6MjJPVzNVZ3k4MzVOdGFsRFArMDRYTG1KbDljVUNWUWJp?= =?utf-8?Q?fLXcdfA7QlJpbyF2bnR1Gb2sLrnRINHkIIaiMKC0t?= X-Microsoft-Exchange-Diagnostics: 1;AM5PR0801MB1332;6:JLHPMNWNwVIToLK0k7+r9sItjUnj1nbAkx007AsykPAuxTABnnKJAgbHddSIbVbnOitjCpx1hSH+Ew0bWUplVp9oWOiT8mli5/3sFIsFyCz9u97c+iKmPh47tVq/j7gJAC2kjFrAT5eAFv2Iw8I78Ud2ANNq0asjOhgzuINpOg7nJkTZskYHQ7dbc1MD2jiDFp2h1Jx1zL3qkuZkv4ocXmrBc511hDQifeBS38RoU3X83GbUvQbrr+otynM/TEVFrQgYi6Ln5NN4MHz8+gO88Fdm48pIxbTBBXG6KDNTxBAkhtJ+7vp59D7NiJ7wv2v24kbPbl+1ZaYuiFOvb3U3t8PUgvmwsgYuB8qZudCnmqzITzMOkvr1Gt4NgDu6UOrcDeLGPXPQXV+dYh8j39wwhYkX82tpH7KJb6cHhEXhX/y+UWRwgUk6wcZmBfQ/EXmMw0LQD4rr7/Ok5YJRG9xJf6cUyDmdgxt8J0Kz7M9LU2Y0OkUaXHuanbzaeESX02CPiqS8/hzYv6IyWT+Qf+aUgA==;5:YmYPXFUmEn4iZ8bBgTQVuHoTHOmF9RCIKb5R/fLY2u5/w5QhBg6lnu6d53mc8YzBBLAoUlFKgeujpTUH5Nmt0JDF6SUnbFfotydiwHrSZIfd0gXhaz4W/qR5JyPzlF4EHfZ8IB1ETFkHSOcfa1zvrQ==;24:fEPzRy9JRMBD90QVTAgdy7FLQZjdI+UlZaM2p0nGt7WgPUz+1S2vHBEntghEsA5vWxl95Ne2R7DhYVQy948ae2Th1G2MJp0Zl1LNv/Sm3fw= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;AM5PR0801MB1332;7:5Nwu3l+HD+ZVXe4lSJwwixxR82cJk3MRUrQP1BV/ngD9smOiEZfPSYoIYlVp/hYo8SN8vUOo1vTHP8gp6Xh8zTBpyTkK5fowQ9FjvTCYzQYO+M0zptzd/6VX9y3bNJPwK3ungsBMAKjPUvrRtHNwKHtUz/WYJYOm+JCmXwK2PECmHhDcB1DqYPOxD2TzaGepVmEIU7j4yzOdlhiN4atl3kBxxeXViHHmfKdBFbvbCgcVL+qh1gHdvGlMp3icJY1crvPYmK2Dp1DBUldmwtni0i57zxdYlDLNbEfjVTIHdcyFRdr8lz365XdiaZR1Oa0W72d5aQ7ZLre04y/oxQjr+Q==;20:e7gujMsFQJ3YIxFJZxfpXg0/378javB3sWEIIU9TxMN4WuY2Fu9X6+1f2DB9Y+Yt5POjXwNweo/GlwKDrJjfOvc3w91Y6P+ZrJXPcFdcGO3P+s14QWrflV2ZXXmjWbRAw/aUiuG3I2zYBKuZLmTIE4C5T0hLhaq98q/0n54fuqs= X-OriginatorOrg: virtuozzo.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 May 2017 14:47:54.2700 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0801MB1332 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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? Kirill > All of that said I think this is a fix worth fixing. > > Eric > >> 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 >> --- >> kernel/exit.c | 2 ++ >> kernel/fork.c | 15 ++++++++++----- >> kernel/pid_namespace.c | 3 --- >> 3 files changed, 12 insertions(+), 8 deletions(-) >> >> diff --git a/kernel/exit.c b/kernel/exit.c >> index 516acdb0e0ec..9310e69fbc5f 100644 >> --- a/kernel/exit.c >> +++ b/kernel/exit.c >> @@ -586,6 +586,8 @@ static struct task_struct *find_child_reaper(struct task_struct *father) >> return reaper; >> } >> >> + /* Don't allow any more processes into the pid namespace */ >> + disable_pid_allocation(pid_ns); >> write_unlock_irq(&tasklist_lock); >> if (unlikely(pid_ns == &init_pid_ns)) { >> panic("Attempted to kill init! exitcode=0x%08x\n", >> diff --git a/kernel/fork.c b/kernel/fork.c >> index bfd91b180778..dbafabf6c7b1 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; >> int retval; >> struct task_struct *p; >> >> @@ -1735,8 +1736,9 @@ static __latent_entropy struct task_struct *copy_process( >> if (retval) >> goto bad_fork_cleanup_io; >> >> + pid_ns = p->nsproxy->pid_ns_for_children; >> if (pid != &init_struct_pid) { >> - pid = alloc_pid(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->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); >> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c >> index d1f3e9f558b8..aedf86a8017e 100644 >> --- a/kernel/pid_namespace.c >> +++ b/kernel/pid_namespace.c >> @@ -210,9 +210,6 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns) >> struct task_struct *task, *me = current; >> int init_pids = thread_group_leader(me) ? 1 : 2; >> >> - /* Don't allow any more processes into the pid namespace */ >> - disable_pid_allocation(pid_ns); >> - >> /* >> * Ignore SIGCHLD causing any terminated children to autoreap. >> * This speeds up the namespace shutdown, plus see the comment