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.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,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 B1407C433FE for ; Sat, 5 Dec 2020 17:56:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7696823100 for ; Sat, 5 Dec 2020 17:56:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728019AbgLERyE (ORCPT ); Sat, 5 Dec 2020 12:54:04 -0500 Received: from out02.mta.xmission.com ([166.70.13.232]:39170 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727787AbgLERov (ORCPT ); Sat, 5 Dec 2020 12:44:51 -0500 Received: from in02.mta.xmission.com ([166.70.13.52]) by out02.mta.xmission.com with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1klbb1-00BDEo-Vt; Sat, 05 Dec 2020 10:43:48 -0700 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.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1klbb0-00GEQ7-U5; Sat, 05 Dec 2020 10:43:47 -0700 From: ebiederm@xmission.com (Eric W. Biederman) To: Linus Torvalds Cc: Bernd Edlinger , Linux Kernel Mailing List , Peter Zijlstra , Ingo Molnar , Will Deacon , Jann Horn , Vasiliy Kulikov , Al Viro , Oleg Nesterov , Cyrill Gorcunov , Sargun Dhillon , Christian Brauner , Arnd Bergmann , Arnaldo Carvalho de Melo , Waiman Long , Davidlohr Bueso References: <87tut2bqik.fsf@x220.int.ebiederm.org> <87ft4mbqen.fsf@x220.int.ebiederm.org> <875z5h4b7a.fsf@x220.int.ebiederm.org> Date: Sat, 05 Dec 2020 11:43:11 -0600 In-Reply-To: (Linus Torvalds's message of "Fri, 4 Dec 2020 12:10:58 -0800") Message-ID: <87r1o4174g.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=1klbb0-00GEQ7-U5;;;mid=<87r1o4174g.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+p05mRAYNOc2PihEt9CL8XP0T2aKuv61k= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH 3/3] exec: Transform exec_update_mutex into a rw_semaphore X-SA-Exim-Version: 4.2.1 (built Sat, 08 Feb 2020 21:53:50 +0000) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Linus Torvalds writes: > On Fri, Dec 4, 2020 at 11:35 AM Eric W. Biederman wrote: >> >> From a deadlock perspective the change is strictly better than what we >> have today. The readers will no longer block on each other. > > No, agreed, it's better regardless. > >> For the specific case that syzbot reported it is readers who were >> blocking on each other so that specific case if fixed. > > So the thing is, a reader can still block another reader if a writer > comes in between them. Which is why I was thinking that the same > deadlock could happen if somebody does an execve at just the right > point. >> On the write side of exec_update_lock we have: >> >> cred_guard_mutex -> exec_update_lock >> >> Which means that to get an ABBA deadlock cred_guard_mutex would need to >> be involved > > No, see above: you can get a deadlock with > > - first reader gets exec_update_lock > > - writer on exec_update_lock blocks on first reader (this is exec) > > - second reader of exec_update_lock now blocks on the writer. > > So if that second reader holds something that the first one wants to > get (or is the same thread as the first one), you have a deadlock: the > first reader will never make any progress, will never release the > lock, and the writer will never get it, and the second reader will > forever wait for the writer that is ahead of it in the queue. Oh. I see what you are saying. I knew the writer had to be involved and block, but if the reader comes first all that has to be added to the situation is that an exec happens and attempts to take the lock. Any reader will block the writer. For some reason I was blind to the fact that a reader could block the writer, and I was looking for anything else that might block the writer. > And that deadlock looks very much like what syzcaller detected, in > exactly that scenario: > > Chain exists of: > &sig->exec_update_mutex --> sb_writers#4 --> &p->lock > > Possible unsafe locking scenario: > > CPU0 CPU1 > ---- ---- > lock(&p->lock); > lock(sb_writers#4); > lock(&p->lock); > lock(&sig->exec_update_mutex); > > *** DEADLOCK *** > > No? The only thing that is missing is that writer that causes the > exec_update_mutex readers to block each other - exactly like they did > when it was a mutex. > > But I may be missing something entirely obvious that keeps this from happening. I don't think so. It does look like my change makes it one step more difficult to cause the deadlock, but the deadlock can still happen. :-( Making it a read/writer lock both makes it more difficult to cause a deadlock and makes a lot of sense from a documentation standpoint so I still plan on merging the changes after the weekend. It looks like we do need to have a close look at perf_event_open and everything associated with it. Eric