From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964845AbeALQm4 (ORCPT + 1 other); Fri, 12 Jan 2018 11:42:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44092 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934044AbeALQmy (ORCPT ); Fri, 12 Jan 2018 11:42:54 -0500 Date: Fri, 12 Jan 2018 17:42:34 +0100 From: Oleg Nesterov To: Kirill Tkhai 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 Subject: Re: [PATCH 3/4] tty: Iterate only thread group leaders in __do_SAK() Message-ID: <20180112164234.GA21532@redhat.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50c23f46-f4ad-b6c8-b7bc-0a8d8449c62f@virtuozzo.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Fri, 12 Jan 2018 16:42:49 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: 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. 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? 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? Oleg. --- 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 }