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=-3.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 71177C1975A for ; Wed, 25 Mar 2020 14:30:10 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2D7312070A for ; Wed, 25 Mar 2020 14:30:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2D7312070A 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 D20AF6B000C; Wed, 25 Mar 2020 10:30:09 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id CAB806B000D; Wed, 25 Mar 2020 10:30:09 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B72EB6B000E; Wed, 25 Mar 2020 10:30:09 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0130.hostedemail.com [216.40.44.130]) by kanga.kvack.org (Postfix) with ESMTP id 9962F6B000C for ; Wed, 25 Mar 2020 10:30:09 -0400 (EDT) Received: from smtpin28.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 4EB681A4AA for ; Wed, 25 Mar 2020 14:30:09 +0000 (UTC) X-FDA: 76634119338.28.song35_58891d1fe6c43 X-HE-Tag: song35_58891d1fe6c43 X-Filterd-Recvd-Size: 7736 Received: from out03.mta.xmission.com (out03.mta.xmission.com [166.70.13.233]) by imf48.hostedemail.com (Postfix) with ESMTP for ; Wed, 25 Mar 2020 14:30:08 +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 1jH72b-0002ea-CW; Wed, 25 Mar 2020 08:29:57 -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 1jH72Z-0002AT-33; Wed, 25 Mar 2020 08:29:56 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: Bernd Edlinger Cc: "gregkh\@linuxfoundation.org" , Kirill Tkhai , Christian Brauner , Kees Cook , "jannh\@google.com" , Jonathan Corbet , Alexander Viro , Andrew Morton , "adobriyan\@gmail.com" , Thomas Gleixner , Oleg Nesterov , Frederic Weisbecker , "avagin\@gmail.com" , Ingo Molnar , "Peter Zijlstra \(Intel\)" , "duyuyang\@gmail.com" , David Hildenbrand , Sebastian Andrzej Siewior , Anshuman Khandual , David Howells , James Morris , Shakeel Butt , Jason Gunthorpe , "christian\@kellner.me" , 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: <077b63b7-6f5e-aa8e-bf96-a586b481cc46@hotmail.de> Date: Wed, 25 Mar 2020 09:27:18 -0500 In-Reply-To: (Bernd Edlinger's message of "Sat, 21 Mar 2020 02:46:39 +0000") Message-ID: <87a7448q7t.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=1jH72Z-0002AT-33;;;mid=<87a7448q7t.fsf@x220.int.ebiederm.org>;;;hst=in01.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1//grV9RcExZss0OZxt5aCJ/pLvokQjxgE= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH v6 15/16] exec: Fix dead-lock in de_thread with ptrace_attach 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: > This removes the last users of cred_guard_mutex > and replaces it with a new mutex exec_guard_mutex, > and a boolean unsafe_execve_in_progress. > > This addresses the case when at least one of the > sibling threads is traced, and therefore the trace > process may dead-lock in ptrace_attach, but de_thread > will need to wait for the tracer to continue execution. > > The solution is to detect this situation and make > ptrace_attach and similar functions return -EAGAIN, > but only in a situation where a dead-lock is imminent. > > This means this is an API change, but only when the > process is traced while execve happens in a > multi-threaded application. > > See tools/testing/selftests/ptrace/vmaccess.c > for a test case that gets fixed by this change. Hmm. The logic with unsafe_execve_in_progress is interesting. I think I see what you are aiming for. So far as you have hit what you are aiming for I think this is a safe change as the only cases that will change are the cases that would deadlock today. At a minimum the code is subtle and I don't see big fat warning comments that subtle code needs to keep people from using it wrong. Further while the change below to proc_pid_attr_write looks like it is being treated the same as ptrace_attach. When in fact proc_pid_attr_write needs the no_new_privs and ptrace_attach protection the same as exec. As the updated cred won't be used in an ongoing exec, exec does not need protection from proc_pid_attr_write, other than deadlock protection. Having the relevant lock be per task_struct lock would probably be a better way to avoid deadlock with a concurrent proc_pid_attr_write. So I am going to pass on these last two patches for now, and apply the rest and get them into linux-next. Eric > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 6b13fc4..a428536 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2680,14 +2680,17 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, > } > > /* Guard against adverse ptrace interaction */ > - rv = mutex_lock_interruptible(¤t->signal->cred_guard_mutex); > + rv = mutex_lock_interruptible(¤t->signal->exec_guard_mutex); > if (rv < 0) > goto out_free; > > - rv = security_setprocattr(PROC_I(inode)->op.lsm, > - file->f_path.dentry->d_name.name, page, > - count); > - mutex_unlock(¤t->signal->cred_guard_mutex); > + if (unlikely(current->signal->unsafe_execve_in_progress)) > + rv = -EAGAIN; > + else > + rv = security_setprocattr(PROC_I(inode)->op.lsm, > + file->f_path.dentry->d_name.name, > + page, count); > + mutex_unlock(¤t->signal->exec_guard_mutex); > out_free: > kfree(page); > out: