From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755101AbeAOJcd (ORCPT + 1 other); Mon, 15 Jan 2018 04:32:33 -0500 Received: from mail-eopbgr10098.outbound.protection.outlook.com ([40.107.1.98]:8160 "EHLO EUR02-HE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754291AbeAOJca (ORCPT ); Mon, 15 Jan 2018 04:32:30 -0500 Subject: Re: [PATCH 3/4] tty: Iterate only thread group leaders in __do_SAK() To: Oleg Nesterov Cc: linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org, jslaby@suse.com, viro@zeniv.linux.org.uk, keescook@chromium.org, serge@hallyn.com, james.l.morris@oracle.com, luto@kernel.org, john.johansen@canonical.com, mingo@kernel.org, akpm@linux-foundation.org, mhocko@suse.com, peterz@infradead.org References: <151568564127.6090.3546718160925256054.stgit@localhost.localdomain> <151568582337.6090.931248807289363396.stgit@localhost.localdomain> <20180111183412.GA18725@redhat.com> <9a854275-7a72-fc54-99fa-66161732fbf9@virtuozzo.com> <50c23f46-f4ad-b6c8-b7bc-0a8d8449c62f@virtuozzo.com> <20180112164234.GA21532@redhat.com> From: Kirill Tkhai Message-ID: Date: Mon, 15 Jan 2018 12:32:22 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180112164234.GA21532@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [195.214.232.6] X-ClientProxiedBy: DB6PR0301CA0052.eurprd03.prod.outlook.com (2603:10a6:4:54::20) To HE1PR0801MB1340.eurprd08.prod.outlook.com (2603:10a6:3:3a::8) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 7aa0596c-937a-4222-465d-08d55bfae497 X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(7020095)(4652020)(5600026)(4604075)(2017052603307)(7153060)(7193020);SRVR:HE1PR0801MB1340; X-Microsoft-Exchange-Diagnostics: 1;HE1PR0801MB1340;3:CyYcpDTwX8lo4FgSkF82maJ9xk6C7+C5OFy+2gVWkAUPNn/HAaecYN2G9U0HNZoCMVbU/bL0zQLu8dijdL/7IFQqMPBqkDUh6qcpsJNiv5FffJGBMzLUChrDzUgJH8OYKwlNibi3/BfjETtFKgq2sL6Bx1jDMSp5m62EjezBlM+1yNUnInWdyASnlGafjaZJ9JxEBxkjWR5+y8++XVl/9RsB9qQSmgcN+ASNZbb7YvopzLyfxhMtjXPqWlauY0m0;25:jbgGsvPu4F6uPxGAJvA5SV7BSPnjDx2VJ7pwhw1X1rB/rQYQJUz4wbo894tViILiuCINqYRSuE5RH+o4aup+nS0DuJmuqCVj/fGZeAckO4szb80qlyAREsbcVO6jUGzbdmdQNTYrKT6AnxxIV1sBH2iRwocilvQGh22HkQVCh+o45AMhUdONBKowSvW4B0Bxtz82FyPPCQLiMYa9gYmYo1aM9SIuar1hc54qIY4KT3wle3AA3/MYHOUiYrG47m50qQNyUhDZr63+NzvviIGZMZk8M0pNW45HxfkzpnKhZlq715iQ8H4vk/o8rEPP/rNfrSAA9YhESRDgCC1/79AGcQ==;31:/NGCOasEYy4XHj/53vesmrL89cLi03EEdXU1fjOE5R6hg4dZ7Sj8+lxWh+M5evfGLZmz0CZiDd6BFrXrrXp6UPGOzeQ25S3i//V16hF0zA6NmJ+H+hNUfUdliXkHkuzxJ4MqD+wUjgSjjKitIX2J+ICLuGAaeXwW2W5YUgxVDwLkkM78MTFQJ+GQqiE7/H04dR8VvrEkiyVs23DpVrmj6D/EGH449rLkBjkpNbwxxBQ= X-MS-TrafficTypeDiagnostic: HE1PR0801MB1340: Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=ktkhai@virtuozzo.com; X-Microsoft-Exchange-Diagnostics: 1;HE1PR0801MB1340;20:jTax53B0iXVBsOEhODiIv4sVPDsj/bV66EqwIJ56mhxsTeTL0F9R7/CrqZbY+Tqcs0i1ejnjnWMIoHAtykZnpxSdDI6l3rhMTaP3pAQYU6avw4vt3I4KYOztcevfl9Lm2+iKVOLutf6wlgQ6HIw07NWWj1/fQN7hS9xui6kEvXw6dO+avrTFxMBBg5hhYIqYM8nj8RB5cfc8mcM4EstlrygUkd2w0r56RN1ey+aToeV8gc6NWvQZKo9cMeza6rGKJgzyWSu+rYSWEknp/79wbFdDq2CIafZfLZbr434mnE7AgzdS/lDmKksXzGkqa7FIouldAiwgSiSWn2zIFvdJXdCo6DjPcSOmrGOvli6JAdMFEH4cm+THP8FOz0JfFdlDPijOxcYCmHlUyeTLtyDgd3Hrk/2AwWWCzvc9ypt9umw=;4:kAOv/GgPKZQJ7ITWjl1+6o+ujwlQp74OKO3dyzdLnanwqsVr+QnH6iVmh0OhYu6AYpM2dlrQMA6l/VogxKTK+6797P61bP6megHpdi/FT29lKMhZDpnjwJXb9orcDRt7wYAesFARgUSs2yJNEhSNCQJ+Dzmqo2/1rYA5GYgMuXo7zRrWaFiPZYgm10Kv9NfayblyRBS/N4mH3cctKdhebfxzRZxyq1ysp3Mw4r0+XLzFvYvC7SYPbw1GA3WIElieG/7S0zkjidF+FijhoOlGpg== X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040470)(2401047)(5005006)(8121501046)(3231023)(944501161)(10201501046)(3002001)(93006095)(93001095)(6041268)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123562045)(20161123564045)(20161123560045)(20161123558120)(6072148)(201708071742011);SRVR:HE1PR0801MB1340;BCL:0;PCL:0;RULEID:(100000803101)(100110400095);SRVR:HE1PR0801MB1340; X-Forefront-PRVS: 0553CBB77A X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(6049001)(39840400004)(39380400002)(366004)(396003)(376002)(346002)(52314003)(24454002)(189003)(199004)(52146003)(23676004)(2486003)(68736007)(6916009)(106356001)(52116002)(76176011)(386003)(59450400001)(53546011)(6486002)(77096006)(36756003)(50466002)(7416002)(83506002)(53936002)(8936002)(2950100002)(8676002)(229853002)(81156014)(7736002)(305945005)(47776003)(64126003)(65806001)(65956001)(105586002)(66066001)(6666003)(6116002)(31686004)(5660300001)(65826007)(2906002)(86362001)(97736004)(6246003)(58126008)(31696002)(230700001)(93886005)(25786009)(16576012)(16526018)(316002)(4326008)(3846002)(81166006)(478600001);DIR:OUT;SFP:1102;SCL:1;SRVR:HE1PR0801MB1340;H:[172.16.25.196];FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtIRTFQUjA4MDFNQjEzNDA7MjM6eDduTlcyV05IWnhJbGIzQmowRCtPeEJx?= =?utf-8?B?SUE0ZjV6ZDlNMFdkdjUxK0d0TzVLL2lmS1h6Vmdzb1U2ZnpGZkp1QXVUTmZh?= =?utf-8?B?dnUwWTBTSTRiRkZyQ3poaU9sRTVUenhyKzNla0IwSkduck13YTNwUlFjL002?= =?utf-8?B?SlAzcDZjQWVEdTlxZlg0N21lRC9vcXl3UUZKVHZjRWRDOUJIa2E3ZUVka3ZI?= =?utf-8?B?ak5TU3FhelFoWDFKR3c3VlBxalZJSjlZRGJNWGxuTVdBdDBZNWhZOWhzVFVX?= =?utf-8?B?MFhtM09YcGRxYzZCME1sODVPeHF6OGM5MEFMOEJ2TmZ5cmdNNmUwOUtDWUdY?= =?utf-8?B?YjBEOEYwSENLaTJYRElLZUF1S085emlCK09YZ0M2STVCeW5XZ2t5dUFGNnZJ?= =?utf-8?B?NWtzcmhoaFZXRHlIYi9mR0dyL0tsODd0M0VVaDcvNmVjeFVDdDhhaEp3YUIw?= =?utf-8?B?UHczZlVPdFF5K1ZRcWFnVDVMbHhRTTYwLzZVbExjR2U3TXpPS2J6ODluY2hM?= =?utf-8?B?eTUyOGpETTFKZ1pqYll2VGxDejNwNFRzZDZNNy9NK2dCZzdTWXp6b2dudFZJ?= =?utf-8?B?MG9ObWNjS28xdWNXRk53MXlzaDRKSEhucENpREJPZGRQZnRybG85RGFqY2Ir?= =?utf-8?B?Z25ocWdBekZsbDBlc3Bldm1QdXp6R2t6TS9jKzZWK3N4TjBKdW5sZll0V0Vt?= =?utf-8?B?OXJYbkVJd1ZaVENVZUFWanY5TVhmU0JUY2hYdElXcklwcXExdUJFU2NtbCti?= =?utf-8?B?VmJOTlR2UGt3alA4SDZ4MUVHQUp0NWVuQUMyMmhjdXNRTjk3SWZvR1VCc3Qz?= =?utf-8?B?eWZ6NlpRQzc1WVV0a1dLZ2dJUHdPcFJBZ3Nja2MvcGgxYldwR1VNYW51SVdM?= =?utf-8?B?RUhYbFpiVzlmZHhtYm84OGNsWWcrQmVzeURGeG9ncithcjhZeVBPTTVIOE9K?= =?utf-8?B?T0ZlVWlqeVlRNSs2VzN6bWpmZ2hod0REZDllVmdFRWEzakI0WWw1ZmEyWFF0?= =?utf-8?B?MmdGQWJiQ2Q1OWVGaHM3bWVqUzJPQmdpd1YxUzJyazJsM055YXh1Nm8zaC8v?= =?utf-8?B?d0hXMHByblZ5TE1OeWttWFIxd3J6UWQyZENCMWpPSmZwUkJoL3psOU8xZWpi?= =?utf-8?B?T1cyRnR0aWQ2ZlgwQU9lUlBwaDZ1TEZzM1VQNVNGWkdVRzBVQytnWk5EYWtL?= =?utf-8?B?emhwR0dST1RrS1pYOGFQYlA2bElnT3I2QXY2UStWWGM0dmNNeXFoV1h1YzI5?= =?utf-8?B?MW1PVHRwaWVpT3J5NjdjdEdEWnk1bTIrMWpzMU5YVnAyK0FLdlVFaDRENFQ1?= =?utf-8?B?SVdac1JSSVQ2dFgwRmUrTU1qTnVSczVXdTE1eVF3Ny9FV2xQZWVzSW9QWWdD?= =?utf-8?B?NGpQTzNZVUg1MTZOQitucjNZdnluQXRBSXEvdnduU0ZQTmhhYWROVUJ5ZjBQ?= =?utf-8?B?S2RYcXdTRjFiOSt6SFRPV3ZYU0xZbVZBK0d5V0xRLzI4WnRIb0dkRGtNWng5?= =?utf-8?B?ZXhpVVVoNXlBMVFXR2JBc1I2V2RKUUc3T3hiQzdvcVhWdVFMVHhzblhtYTh1?= =?utf-8?B?TythU2pEMGJqTi91NVpIS3J3ZzJuMW83M29YMzdOQ1FpTjMrSzRXM2xNVUha?= =?utf-8?B?VkMzQmVwY0s0a0sxcDdka290cm04QTBFSVB1S3dqMHJiRkxXRVUzcGdWRWRL?= =?utf-8?B?bm52bURRWXV6YWlSV01XUDVMc2VPMjhpK0xnbysxNWRNaUZZRk1sUUVrMDFL?= =?utf-8?B?UDRaQzRxZFVkbnlLcFpaVmR2VmtKL2dFUGZROXd0TmVjaHQ2VHpqdGNhTnFm?= =?utf-8?B?Q21Dd3Y4Y0Y2Mm5la1J2UXBxVmp5Rm91cnZIM1ZzYWpGVE1XNk5QR211dWxm?= =?utf-8?B?TUo3aTUva1liSmFvTGxXMWlzR3BVRlpXcVNrTDcwRjRTdmN0MXlINFhWR1l4?= =?utf-8?Q?TBNmJ/SDz2fMPNtz2Jur1ua5JEtjMVv0=3D?= X-Microsoft-Exchange-Diagnostics: 1;HE1PR0801MB1340;6:CDD7UDpfIvPgYKA4ghseSS3upCNAnQhIt561tKnVucdPjdqfxXoSiab2FBxnZ1Pcc/9ouGqBee3WqOHr57HhAAaOeRsKGZghdqu27GLjEotHEqudYZOLJKK0oKN2l2oQy6DPjY6ApE49RPp9M3p+xMOE73v3vZvOM+9SXd2vRh57t/XiT76mqRsVt34EUOIQmVPHbWQs6uMAM40rUJ50DwwPLXdtW2JM48B7XnykKWaCzNCxHp6OOtagzwViT5s6+C+KYvRAoQZ7CMF8wb42Mp1Poiflt4r+eih2IsokXEUhTnnYiDfyas2UiLvzAqYXufBnNKU4MU20NHgZf7GS712/NlsoNcRqfVMjlBFFRJc=;5:S0pKAFE5iTHkBU9V5sBR6SvfRvRE4Qm2PUmQvpeTdUb2EZvUvcvVMRoVVgJQZKTf+RxDBWJWkKF3HdCYHXkS3m5CpJ8GeGzz+CcXVJOAaHH+vX8P/ysjygeSiFSR2EJmOmxEuQyT7agFTNg56TD4kQUOicQ5hpGo5bhgkVAkr5A=;24:ksibSamZ1XO3RWDznv6JJV+kuZg4r2/vIZj9mE0RHdlNs1czrgKhCUb7Jp0hU0LqnIisl5mIfbstJcBcshOHS7gHZ+22okvo4FA5Rp4WWtM=;7:/iOnad16vLByQ4OaJSyTz5ad0SeI+v+AbAZtjxHNf9tMDnrn5wBCnMYxymbtTDdHSzSN0w7Gj1W1CWI0xvyR2fvxpyoMU0H+d8Z+CKW631yCD7hRoxTRNMwT3H2jN8KXRDjn6kFT1pyh1OihiV+65o0XqrWEjF9rOJKbZieP+RRLYL+t1e6+gKLRufKKGbdx7hxTgvet9Niv7ovpd641IjBwY2w5AWlOgQk8ho/TRjySVC/HG6/+iNLmZ72JLDoF SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;HE1PR0801MB1340;20:QFYAeCFe0kIi8PIIMNpdZsrs/wpj1iV0skdkXq7+VJ+Pm/6z0w7v4ZSTFfvS1yCCb6wG7t83Iva6fMY3i3DMKf/4CIU4w+lwp+ESJfKbI3qELYu+cI3TvfPPbO2UrBKTMKfFxFCqO4cPXw1FOSk/gVltllrSnPvC5BabZ5D22fU= X-OriginatorOrg: virtuozzo.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Jan 2018 09:32:26.0050 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 7aa0596c-937a-4222-465d-08d55bfae497 X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 0bc7f26d-0264-416e-a6fc-8352af79c58f X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0801MB1340 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On 12.01.2018 19:42, Oleg Nesterov wrote: > On 01/12, Kirill Tkhai wrote: >> >> How about this patch instead of the whole set? I left thread iterations >> and added sighand locking for visability. > > Kirill, I didn't really read this series so I don't quite understand what > are you actually trying to do... > > __do_SAK() is racy anyway, a process can open tty right after it was checked, > and I do not understand why should we care about races with execve. Please, just ignore two first patches. As I wrote I thought we iterate threads to close race with exec (and missed that threads may have unshared fd table). So, if we didn't use to care about such situations, I don't care them now. My main target is just to speed up __do_SAK(). > IOW, I do not understand why we can't simply use rcu_read_lock() after > do_each_pid_task/while_each_pid_task. Yes we can miss the new process/thread, > but if the creator process had this tty opened it should be killed by us so > fork/clone can't succeed: both do_fork() and send_sig() take the same lock > and do_fork() checks signal_pending() under ->siglock. > > No? Yes, but we send signal not every time. So, this was the only reason I added lock/unlock the locks. But anyway, __do_SAK() is racy and the effect of that is minimal, so it seems we may skip this. > And whatever we do, I think you are right and for_each_process() makes more > sense, and in the likely case all sub-threads should share the same file_struct. > So perhaps we should start with the simple cleanup? Say, > > for_each_process(p) { > if (p->signal->tty == tty) { > tty_notice(tty, "SAK: killed process %d (%s): by controlling tty\n", > task_pid_nr(p), p->comm); > goto kill; > } > > files = NULL; > for_each_thread(p, t) { > if (t->files == files) /* racy but we do not care */ > continue; > > task_lock(t); > files = t->files; > i = iterate_fd(files, 0, this_tty, tty); > task_unlock(t); > > if (i != 0) { > tty_notice(tty, "SAK: killed process %d (%s): by fd#%d\n", > task_pid_nr(p), p->comm, i - 1); > goto kill; > } > } > > continue; > kill: > force_sig(SIGKILL, p); > } > > (see the uncompiled/untested patch below), then make another change to avoid > tasklist_lock? > > > --- a/drivers/tty/tty_io.c > +++ b/drivers/tty/tty_io.c > @@ -2704,7 +2704,8 @@ void __do_SAK(struct tty_struct *tty) > #ifdef TTY_SOFT_SAK > tty_hangup(tty); > #else > - struct task_struct *g, *p; > + struct task_struct *p, *t; > + struct files_struct files; > struct pid *session; > int i; > > @@ -2725,22 +2726,34 @@ void __do_SAK(struct tty_struct *tty) > } while_each_pid_task(session, PIDTYPE_SID, p); > > /* Now kill any processes that happen to have the tty open */ > - do_each_thread(g, p) { > + for_each_process(p) { > if (p->signal->tty == tty) { > tty_notice(tty, "SAK: killed process %d (%s): by controlling tty\n", > task_pid_nr(p), p->comm); > - send_sig(SIGKILL, p, 1); > - continue; > + goto kill; > } > - task_lock(p); > - i = iterate_fd(p->files, 0, this_tty, tty); > - if (i != 0) { > - tty_notice(tty, "SAK: killed process %d (%s): by fd#%d\n", > - task_pid_nr(p), p->comm, i - 1); > - force_sig(SIGKILL, p); > + > + files = NULL; > + for_each_thread(p, t) { > + if (t->files == files) /* racy but we do not care */ > + continue; > + > + task_lock(t); > + files = t->files; > + i = iterate_fd(files, 0, this_tty, tty); > + task_unlock(t); > + > + if (i != 0) { > + tty_notice(tty, "SAK: killed process %d (%s): by fd#%d\n", > + task_pid_nr(p), p->comm, i - 1); > + goto kill; > + } > } > - task_unlock(p); > - } while_each_thread(g, p); > + > + continue; > +kill: > + force_sig(SIGKILL, p); > + } > read_unlock(&tasklist_lock); > #endif > } I tested your patch with small modification in "struct files_struct *files;" ('*' is added). Could I send it with your "Signed-off-by" as the second version? Also, the below patch will go on top of yours: diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index 979eb5d80fe9..20b74b4c9f84 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -2724,7 +2724,9 @@ void __do_SAK(struct tty_struct *tty) task_pid_nr(p), p->comm); send_sig(SIGKILL, p, 1); } while_each_pid_task(session, PIDTYPE_SID, p); + read_unlock(&tasklist_lock); + rcu_read_lock(); /* Now kill any processes that happen to have the tty open */ for_each_process(p) { if (p->signal->tty == tty) { @@ -2752,9 +2754,9 @@ void __do_SAK(struct tty_struct *tty) continue; kill: - force_sig(SIGKILL, p); + send_sig(SIGKILL, p, 1); } - read_unlock(&tasklist_lock); + rcu_read_unlock(); #endif } I replaced force_sig() as it does not check for task's sighand. Kirill