From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1553FC10DCE for ; Tue, 10 Mar 2020 21:22:38 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id DFE00205F4 for ; Tue, 10 Mar 2020 21:22:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DFE00205F4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xmission.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 7EA946B0007; Tue, 10 Mar 2020 17:22:37 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 79A926B0008; Tue, 10 Mar 2020 17:22:37 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 661EC6B000A; Tue, 10 Mar 2020 17:22:37 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0236.hostedemail.com [216.40.44.236]) by kanga.kvack.org (Postfix) with ESMTP id 4C4B66B0007 for ; Tue, 10 Mar 2020 17:22:37 -0400 (EDT) Received: from smtpin28.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 139E2181AEF07 for ; Tue, 10 Mar 2020 21:22:37 +0000 (UTC) X-FDA: 76580726754.28.rod32_33e9d4a31aa48 X-HE-Tag: rod32_33e9d4a31aa48 X-Filterd-Recvd-Size: 9560 Received: from out03.mta.xmission.com (out03.mta.xmission.com [166.70.13.233]) by imf36.hostedemail.com (Postfix) with ESMTP for ; Tue, 10 Mar 2020 21:22:36 +0000 (UTC) Received: from in02.mta.xmission.com ([166.70.13.52]) by out03.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jBmKe-0001lA-LQ; Tue, 10 Mar 2020 15:22:32 -0600 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95] helo=x220.xmission.com) by in02.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.87) (envelope-from ) id 1jBmKd-0001Rk-94; Tue, 10 Mar 2020 15:22:32 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: Kees Cook Cc: Bernd Edlinger , Christian Brauner , Jann Horn , Jonathan Corbet , Alexander Viro , Andrew Morton , Alexey Dobriyan , Thomas Gleixner , Oleg Nesterov , Frederic Weisbecker , Andrei Vagin , Ingo Molnar , "Peter Zijlstra \(Intel\)" , Yuyang Du , David Hildenbrand , Sebastian Andrzej Siewior , Anshuman Khandual , David Howells , James Morris , Greg Kroah-Hartman , Shakeel Butt , Jason Gunthorpe , Christian Kellner , Andrea Arcangeli , Aleksa Sarai , "Dmitry V. Levin" , "linux-doc\@vger.kernel.org" , "linux-kernel\@vger.kernel.org" , "linux-fsdevel\@vger.kernel.org" , "linux-mm\@kvack.org" , "stable\@vger.kernel.org" , "linux-api\@vger.kernel.org" References: <87v9nlii0b.fsf@x220.int.ebiederm.org> <87a74xi4kz.fsf@x220.int.ebiederm.org> <87r1y8dqqz.fsf@x220.int.ebiederm.org> <87tv32cxmf.fsf_-_@x220.int.ebiederm.org> <87v9ne5y4y.fsf_-_@x220.int.ebiederm.org> <875zfe5xzb.fsf_-_@x220.int.ebiederm.org> <202003101337.AC1A30576@keescook> Date: Tue, 10 Mar 2020 16:20:12 -0500 In-Reply-To: <202003101337.AC1A30576@keescook> (Kees Cook's message of "Tue, 10 Mar 2020 13:44:09 -0700") Message-ID: <87sgifq54j.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1jBmKd-0001Rk-94;;;mid=<87sgifq54j.fsf@x220.int.ebiederm.org>;;;hst=in02.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1/uSgxqQ7yJ0LmMGrR7SNxCBN/dTq54Iqs= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH v2 4/5] exec: Move exec_mmap right after de_thread in flush_old_exec X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Kees Cook writes: > On Sun, Mar 08, 2020 at 04:38:00PM -0500, Eric W. Biederman wrote: >> >> I have read through the code in exec_mmap and I do not see anything >> that depends on sighand or the sighand lock, or on signals in anyway >> so this should be safe. >> >> This rearrangement of code has two siginficant benefits. It makes >> the determination of passing the point of no return by testing bprm->mm >> accurate. All failures prior to that point in flush_old_exec are >> either truly recoverable or they are fatal. > > Agreed. Though I see a use of "current", which maybe you want to > parameterize to a "me" argument in acct_arg_size(). (Though looking at > the callers, perhaps there is no benefit?) My testing suggests there is a small benefit on x86. The code is just "#define current get_current()" and get_current() revoles into a read of "%gs:current_task". But looking at the code I find gcc can sometimes when the reads are close in the source code can optimize the read away. But gcc does not manage to optimize the extra read of "%gs:current_task" away. So I think things are much much better than they used to be, code generation wise. But it still helps to cache current in a local variable. >> Futher this consolidates all of the possible indefinite waits for >> userspace together at the top of flush_old_exec. The possible wait >> for a ptracer on PTRACE_EVENT_EXIT, the possible wait for a page fault >> to be resolved in clear_child_tid, and the possible wait for a page >> fault in exit_robust_list. >> >> This consolidation allows the creation of a mutex to replace >> cred_guard_mutex that is not held of possible indefinite userspace >> waits. Which will allow removing deadlock scenarios from the kernel. >> >> Signed-off-by: "Eric W. Biederman" >> --- >> fs/exec.c | 24 ++++++++++++------------ >> 1 file changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/fs/exec.c b/fs/exec.c >> index 215d86f77b63..d820a7272a76 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -1272,18 +1272,6 @@ int flush_old_exec(struct linux_binprm * bprm) >> if (retval) >> goto out; >> >> -#ifdef CONFIG_POSIX_TIMERS >> - exit_itimers(me->signal); >> - flush_itimer_signals(); >> -#endif > > I think this comment: > > /* > * This is called by do_exit or de_thread, only when there are no more > * references to the shared signal_struct. > */ > void exit_itimers(struct signal_struct *sig) > > Refers to there being other threads, yes? Not that the signal table is > private yet? The signal table is in sighand_struct. So yes that refers to the other threads being gone. >> - >> - /* >> - * Make the signal table private. >> - */ >> - retval = unshare_sighand(me); >> - if (retval) >> - goto out; >> - >> /* >> * Must be called _before_ exec_mmap() as bprm->mm is >> * not visibile until then. This also enables the update >> @@ -1307,6 +1295,18 @@ int flush_old_exec(struct linux_binprm * bprm) >> */ >> bprm->mm = NULL; >> >> +#ifdef CONFIG_POSIX_TIMERS >> + exit_itimers(me->signal); >> + flush_itimer_signals(); >> +#endif > > I've mostly convinced myself that there are no "side-effects" from having > these timers expire as the mm is going away. I think some kind of comment > of that intent should be explicitly stated here above the timer work. The timers can at most generate signals. And we are not handling signals in the middle of exec. So the only possible interaction would be to set a timeout and then try exec, and have the timer kill the caller. Maybe we get a killable signal from a scenario like that and maybe this changes the time before the timer expires into the dangerous zone. But that is all I can think of. We have to return to the edge of userspace before any signals are delivered. > Beyond that: > > Reviewed-by: Kees Cook > > -Kees > >> + >> + /* >> + * Make the signal table private. >> + */ >> + retval = unshare_sighand(me); >> + if (retval) >> + goto out; >> + >> set_fs(USER_DS); >> me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | >> PF_NOFREEZE | PF_NO_SETAFFINITY); >> -- >> 2.25.0 >> Eric