From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750823AbeAOUvQ (ORCPT + 1 other); Mon, 15 Jan 2018 15:51:16 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43772 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750714AbeAOUvO (ORCPT ); Mon, 15 Jan 2018 15:51:14 -0500 Date: Mon, 15 Jan 2018 21:51:06 +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: <20180115205106.GA30922@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> <20180112164234.GA21532@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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]); Mon, 15 Jan 2018 20:51:14 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On 01/15, Kirill Tkhai wrote: > > On 12.01.2018 19:42, Oleg Nesterov wrote: > > > 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. Yes. If we don't send SIGKILL we do not care about the new child process/thread we can miss, it can't have this tty opened at fork() time. If the child opens this tty after that, __do_SAK can "miss" it anyway in that it can see it before it does open(tty). > 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? Yes, please feel free, > kill: > - force_sig(SIGKILL, p); > + send_sig(SIGKILL, p, 1); Agreed, I didn't actually want to use force_sig(SIGKILL), copy-and-paste error. But. on the second thought this probably needs another change... I don't understand these force_sig/send_sig in __do_SAK(). If signal->tty == tty it does send_sig(SIGKILL), this won't kill the global or sub-namespace init. However, if iterate_fd() finds this tty it does force_sig(SIGKILL) which clears SIGNAL_UNKILLABLE, so it can kill even the global init. This looks strange, and probably unintentional. So it seems yoou should start with "revert 20ac94378 [PATCH] do_SAK: Don't recursively take the tasklist_lock" ? The original reason for that commit has gone a long ago. At the same time, I do not know if we actually want to kill sub-namespace inits or not. If yes, we can use SEND_SIG_FORCED (better than ugly force_sig()) but skip the global init. But this will need yet another change. Oleg.