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=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,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 D62B1C433FE for ; Fri, 4 Dec 2020 19:35:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9CEBD22C7D for ; Fri, 4 Dec 2020 19:35:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730324AbgLDTfx (ORCPT ); Fri, 4 Dec 2020 14:35:53 -0500 Received: from out01.mta.xmission.com ([166.70.13.231]:51576 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725923AbgLDTfx (ORCPT ); Fri, 4 Dec 2020 14:35:53 -0500 Received: from in02.mta.xmission.com ([166.70.13.52]) by out01.mta.xmission.com with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1klGrE-00DCRa-AU; Fri, 04 Dec 2020 12:35:08 -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 1klGrD-00DghD-79; Fri, 04 Dec 2020 12:35:08 -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> Date: Fri, 04 Dec 2020 13:34:33 -0600 In-Reply-To: (Linus Torvalds's message of "Fri, 4 Dec 2020 09:21:49 -0800") Message-ID: <875z5h4b7a.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=1klGrD-00DghD-79;;;mid=<875z5h4b7a.fsf@x220.int.ebiederm.org>;;;hst=in02.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX19JBdc37j8Jc036PygacSLRGrCDpod6tAE= 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 8:08 AM Bernd Edlinger wrote: >> >> > >> > -static void kcmp_unlock(struct mutex *m1, struct mutex *m2) >> > +static void kcmp_unlock(struct rw_semaphore *l1, struct rw_semaphore *l2) >> > { >> > - if (likely(m2 != m1)) >> > - mutex_unlock(m2); >> > - mutex_unlock(m1); >> > + if (likely(l2 != l1)) >> >> is this still necessary ? >> >> > + up_read(l2); >> > + up_read(l1); >> > } >> > >> > -static int kcmp_lock(struct mutex *m1, struct mutex *m2) >> > +static int kcmp_lock(struct rw_semaphore *l1, struct rw_semaphore *l2) >> > { >> > int err; >> > >> > - if (m2 > m1) >> > - swap(m1, m2); >> > + if (l2 > l1) >> > + swap(l1, l2); >> >> and this is probably also no longer necessary? > > These are still necessary, because even a recursive read lock can > still block on a writer trying to come in between the two read locks > due to fairness guarantees. > > So taking the same read lock twice is still a source of possible deadlocks. > > For the same reason, read locks still have ABBA deadlock and need to > be taken in order. > > So switching from a mutex to a rwlock doesn't really change the > locking rules in this respect. Thinking about the specific case of down_read on two instances of exec_update_lock. If there are two instances of kcmp being called with the sames two pids, but in opposite order running, and the tasks of that both of those pids refer to both exec, you could definitely get a deadlock. So yes. We definitely need to keep the swap as well. > In fact, I'm not convinced this change even fixes the deadlock that > syzbot reported, for the same reason: it just requires a write lock in > between two read locks to deadlock. >From a deadlock perspective the change is strictly better than what we have today. The readers will no longer block on each other. For the specific case that syzbot reported it is readers who were blocking on each other so that specific case if fixed. 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 and it is only acquired in 3 places: ptrace_attach, do_seccomp, and proc_pid_attr_write. In none of the 3 from the syscall entry point until the code that takes cred_guard_mutex can I find anything that takes any locks. Perhaps there is something in io_uring I did not completely trace that write path. So given that the exec path would need to be involved, and the exec path takes exec_update_lock pretty much at the top level. I am not seeing how there is any room for deadlocks after this change. Am I missing something? Eric