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 autolearn=unavailable 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 6F895C10F27 for ; Sun, 8 Mar 2020 18:14:52 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 1D24B20637 for ; Sun, 8 Mar 2020 18:14:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1D24B20637 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 68DAD6B0003; Sun, 8 Mar 2020 14:14:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 637A96B0006; Sun, 8 Mar 2020 14:14:51 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4FE6F6B0007; Sun, 8 Mar 2020 14:14:51 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0231.hostedemail.com [216.40.44.231]) by kanga.kvack.org (Postfix) with ESMTP id 320086B0003 for ; Sun, 8 Mar 2020 14:14:51 -0400 (EDT) Received: from smtpin18.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id AFCA0181AC9BF for ; Sun, 8 Mar 2020 18:14:50 +0000 (UTC) X-FDA: 76572995940.18.cows71_2a3be8e1dd70f X-HE-Tag: cows71_2a3be8e1dd70f X-Filterd-Recvd-Size: 9923 Received: from out03.mta.xmission.com (out03.mta.xmission.com [166.70.13.233]) by imf08.hostedemail.com (Postfix) with ESMTP for ; Sun, 8 Mar 2020 18:14:50 +0000 (UTC) Received: from in01.mta.xmission.com ([166.70.13.51]) by out03.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jB0Rg-0000s1-Lt; Sun, 08 Mar 2020 12:14:36 -0600 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95] helo=x220.xmission.com) by in01.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.87) (envelope-from ) id 1jB0Rf-00049Y-Pk; Sun, 08 Mar 2020 12:14:36 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: Bernd Edlinger Cc: Christian Brauner , Kees Cook , 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> <87imjicxjw.fsf_-_@x220.int.ebiederm.org> <87k13yawpp.fsf@x220.int.ebiederm.org> <87sgil87s3.fsf@x220.int.ebiederm.org> <87a74t86cs.fsf@x220.int.ebiederm.org> <87v9nh6koh.fsf@x220.int.ebiederm.org> Date: Sun, 08 Mar 2020 13:12:14 -0500 In-Reply-To: (Bernd Edlinger's message of "Sun, 8 Mar 2020 12:58:33 +0000") Message-ID: <87d09m7m2p.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=1jB0Rf-00049Y-Pk;;;mid=<87d09m7m2p.fsf@x220.int.ebiederm.org>;;;hst=in01.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX18zivdmh9crMBehDVbxDTXy5jTo3YZzPPQ= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH] exec: make de_thread alloc new signal struct earlier X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.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: Bernd Edlinger writes: > It was pointed out that de_thread may return -ENOMEM > when it already terminated threads, and returning > an error from execve, except when a fatal signal is > being delivered is not an option any more. > > Allocate the memory for the signal table earlier, > and make sure that -ENOMEM is returned before the > unrecoverable actions are started. > > Signed-off-by: Bernd Edlinger > --- > Eric, what do you think, might this be helpful > to move the "point of no return" lower, and simplify > your patch? Good thinking but no. In this case it is possible to move the entire allocation lower. As well as the posix timer cleanup. That code is actually much clearer outside of de_thread. I will post a patch in that direction in a moment. It is something of a bad idea to move the new sighand allocation sooner because in practice it does not happen. It only exists to support the CLONE_VM | CLONE_SIGHAND without CLONE_SIGNAL case which is not used by the modern posix thread libraries. There are just enough old executables floating out there that I don't think we can remove the CLONE_SIGHAND case in general but I keep dreaming about it. We get a lot of complexity in the code to support something that no one really does anymore. Eric > fs/exec.c | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 74d88da..a0328dc 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1057,16 +1057,26 @@ static int exec_mmap(struct mm_struct *mm) > * disturbing other processes. (Other processes might share the signal > * table via the CLONE_SIGHAND option to clone().) > */ > -static int de_thread(struct task_struct *tsk) > +static int de_thread(void) > { > + struct task_struct *tsk = current; > struct signal_struct *sig = tsk->signal; > struct sighand_struct *oldsighand = tsk->sighand; > spinlock_t *lock = &oldsighand->siglock; > + struct sighand_struct *newsighand = NULL; > > if (thread_group_empty(tsk)) > goto no_thread_group; > > /* > + * This is the last time for an out of memory error. > + * After this point only fatal signals are are okay. > + */ > + newsighand = kmem_cache_alloc(sighand_cachep, GFP_KERNEL); > + if (!newsighand) > + return -ENOMEM; > + > + /* > * Kill all other threads in the thread group. > */ > spin_lock_irq(lock); > @@ -1076,7 +1086,7 @@ static int de_thread(struct task_struct *tsk) > * return so that the signal is processed. > */ > spin_unlock_irq(lock); > - return -EAGAIN; > + goto err_free; > } > > sig->group_exit_task = tsk; > @@ -1191,14 +1201,16 @@ static int de_thread(struct task_struct *tsk) > #endif > > if (refcount_read(&oldsighand->count) != 1) { > - struct sighand_struct *newsighand; > /* > * This ->sighand is shared with the CLONE_SIGHAND > * but not CLONE_THREAD task, switch to the new one. > */ > - newsighand = kmem_cache_alloc(sighand_cachep, GFP_KERNEL); > - if (!newsighand) > - return -ENOMEM; > + if (!newsighand) { > + newsighand = kmem_cache_alloc(sighand_cachep, > + GFP_KERNEL); > + if (!newsighand) > + return -ENOMEM; > + } > > refcount_set(&newsighand->count, 1); > memcpy(newsighand->action, oldsighand->action, > @@ -1211,7 +1223,8 @@ static int de_thread(struct task_struct *tsk) > write_unlock_irq(&tasklist_lock); > > __cleanup_sighand(oldsighand); > - } > + } else if (newsighand) > + kmem_cache_free(sighand_cachep, newsighand); > > BUG_ON(!thread_group_leader(tsk)); > return 0; > @@ -1222,6 +1235,8 @@ static int de_thread(struct task_struct *tsk) > sig->group_exit_task = NULL; > sig->notify_count = 0; > read_unlock(&tasklist_lock); > +err_free: > + kmem_cache_free(sighand_cachep, newsighand); > return -EAGAIN; > } > > @@ -1262,7 +1277,7 @@ int flush_old_exec(struct linux_binprm * bprm) > * Make sure we have a private signal table and that > * we are unassociated from the previous thread group. > */ > - retval = de_thread(current); > + retval = de_thread(); > if (retval) > goto out;