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 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 C1DA8C3F2D1 for ; Tue, 3 Mar 2020 15:21:14 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 829232073D for ; Tue, 3 Mar 2020 15:21:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 829232073D 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 00D256B0005; Tue, 3 Mar 2020 10:21:14 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id F009D6B0006; Tue, 3 Mar 2020 10:21:13 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DC74A6B0007; Tue, 3 Mar 2020 10:21:13 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0034.hostedemail.com [216.40.44.34]) by kanga.kvack.org (Postfix) with ESMTP id C2A006B0005 for ; Tue, 3 Mar 2020 10:21:13 -0500 (EST) Received: from smtpin25.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 6E7591C973 for ; Tue, 3 Mar 2020 15:21:13 +0000 (UTC) X-FDA: 76554414426.25.tub80_7eede8db73f26 X-HE-Tag: tub80_7eede8db73f26 X-Filterd-Recvd-Size: 9032 Received: from out02.mta.xmission.com (out02.mta.xmission.com [166.70.13.232]) by imf48.hostedemail.com (Postfix) with ESMTP for ; Tue, 3 Mar 2020 15:21:12 +0000 (UTC) Received: from in01.mta.xmission.com ([166.70.13.51]) by out02.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1j99Lt-0008Dj-DE; Tue, 03 Mar 2020 08:20:57 -0700 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 1j99Ls-00074l-Ha; Tue, 03 Mar 2020 08:20:57 -0700 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" , References: <87a74zmfc9.fsf@x220.int.ebiederm.org> <87k142lpfz.fsf@x220.int.ebiederm.org> <875zfmloir.fsf@x220.int.ebiederm.org> <87v9nmjulm.fsf@x220.int.ebiederm.org> <202003021531.C77EF10@keescook> <20200303085802.eqn6jbhwxtmz4j2x@wittgenstein> Date: Tue, 03 Mar 2020 09:18:44 -0600 In-Reply-To: (Bernd Edlinger's message of "Tue, 3 Mar 2020 13:02:51 +0000") Message-ID: <87v9nlii0b.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=1j99Ls-00074l-Ha;;;mid=<87v9nlii0b.fsf@x220.int.ebiederm.org>;;;hst=in01.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX19rGY6nP+Gy6pmv+g93hLTd2evDLDDhunA= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCHv5] exec: Fix a deadlock in ptrace 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 fixes a deadlock in the tracer when tracing a multi-threaded > application that calls execve while more than one thread are running. > > I observed that when running strace on the gcc test suite, it always > blocks after a while, when expect calls execve, because other threads > have to be terminated. They send ptrace events, but the strace is no > longer able to respond, since it is blocked in vm_access. > > The deadlock is always happening when strace needs to access the > tracees process mmap, while another thread in the tracee starts to > execve a child process, but that cannot continue until the > PTRACE_EVENT_EXIT is handled and the WIFEXITED event is received: A couple of things. Why do we think it is safe to change the behavior exposed to userspace? Not the deadlock but all of the times the current code would not deadlock? Especially given that this is a small window it might be hard for people to track down and report so we need a strong argument that this won't break existing userspace before we just change things. Usually surveying all of the users of a system call that we can find and checking to see if they might be affected by the change in behavior is difficult enough that we usually opt for not being lazy and preserving the behavior. This patch is up to two changes in behavior now, that could potentially affect a whole array of programs. Adding linux-api so that this change in behavior can be documented if/when this change goes through. If you can split the documentation and test fixes out into separate patches that would help reviewing this code, or please make it explicit that the your are changing documentation about behavior that is changing with this patch. Eric > diff --git a/tools/testing/selftests/ptrace/vmaccess.c b/tools/testing/selftests/ptrace/vmaccess.c > new file mode 100644 > index 0000000..6d8a048 > --- /dev/null > +++ b/tools/testing/selftests/ptrace/vmaccess.c > @@ -0,0 +1,66 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2020 Bernd Edlinger > + * All rights reserved. > + * > + * Check whether /proc/$pid/mem can be accessed without causing deadlocks > + * when de_thread is blocked with ->cred_guard_mutex held. > + */ > + > +#include "../kselftest_harness.h" > +#include > +#include > +#include > +#include > +#include > +#include > + > +static void *thread(void *arg) > +{ > + ptrace(PTRACE_TRACEME, 0, 0L, 0L); > + return NULL; > +} > + > +TEST(vmaccess) > +{ > + int f, pid = fork(); > + char mm[64]; > + > + if (!pid) { > + pthread_t pt; > + > + pthread_create(&pt, NULL, thread, NULL); > + pthread_join(pt, NULL); > + execlp("true", "true", NULL); > + } > + > + sleep(1); > + sprintf(mm, "/proc/%d/mem", pid); > + f = open(mm, O_RDONLY); > + ASSERT_LE(0, f); > + close(f); > + f = kill(pid, SIGCONT); > + ASSERT_EQ(0, f); > +} > + > +TEST(attach) > +{ > + int f, pid = fork(); > + > + if (!pid) { > + pthread_t pt; > + > + pthread_create(&pt, NULL, thread, NULL); > + pthread_join(pt, NULL); > + execlp("true", "true", NULL); > + } > + > + sleep(1); > + f = ptrace(PTRACE_ATTACH, pid, 0L, 0L); To be meaningful this code needs to learn to loop when ptrace returns -EAGAIN. Because that is pretty much what any self respecting user space process will do. At which point I am not certain we can say that the behavior has sufficiently improved not to be a deadlock. > + ASSERT_EQ(EAGAIN, errno); > + ASSERT_EQ(f, -1); > + f = kill(pid, SIGCONT); > + ASSERT_EQ(0, f); > +} > + > +TEST_HARNESS_MAIN Eric