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=-23.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=ham 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 96D88C4320A for ; Tue, 3 Aug 2021 23:58:03 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 34B9961050 for ; Tue, 3 Aug 2021 23:58:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 34B9961050 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 452C78D0012; Tue, 3 Aug 2021 19:58:02 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4037E8D000F; Tue, 3 Aug 2021 19:58:02 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2F15C8D0012; Tue, 3 Aug 2021 19:58:02 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0153.hostedemail.com [216.40.44.153]) by kanga.kvack.org (Postfix) with ESMTP id 11E5B8D000F for ; Tue, 3 Aug 2021 19:58:02 -0400 (EDT) Received: from smtpin34.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id A349C23E5B for ; Tue, 3 Aug 2021 23:58:01 +0000 (UTC) X-FDA: 78435435162.34.C1DFD2C Received: from mail-ed1-f47.google.com (mail-ed1-f47.google.com [209.85.208.47]) by imf16.hostedemail.com (Postfix) with ESMTP id 5C0F0F006826 for ; Tue, 3 Aug 2021 23:58:01 +0000 (UTC) Received: by mail-ed1-f47.google.com with SMTP id ec13so1320185edb.0 for ; Tue, 03 Aug 2021 16:58:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=bHZZMg7dTux+Z0i/Dg0mZ8wPTSufzEow/lzNd5aKgII=; b=lS2pf2FOoLcPkgGCvkEtLEM/9DG+hPeghG4h6jEBSwLmzkQxo8ZgFV54B/DlgES3r2 fzV2D2xQIDQ+eP75tWnFBP5WTldUf3AbkjCY232SzSLMs+NyOdqewL85RyHoajBn/WM0 6/upOJ02QzwdX02hXwihrkqpmNsaoYPE2/T9DE7wW9Vs7OLM2/Nm5V4YVuvEpfi6uKHT TIw5tm0hpi9OH5fn8kYSlAknobo8CoAKADVoDJ3+q7Q4bIScSKI+SmUfRBkWLQkfGSlN LeREEIhMWgheTZCZhyXoOatI1yHCm3S5NqTKwsTjxHR2XvZRh5aKY9kdUWG007PORGCL P18g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=bHZZMg7dTux+Z0i/Dg0mZ8wPTSufzEow/lzNd5aKgII=; b=oSp6FzVWt5WWVIQkNTKixhpkZ687ufGAr5thT4PdZPuzgCwgXdqgL+xYb2fWG4Zsui X2pv7r/1XQouzMkOnhdCjoVIQhpX5l34NkTEYkbBe59/zt0ybDOKSO0xE/CJPGV2ApN4 fzksckYzakDdVrnnoj0gKh2Lkzl3ima45jDZp5szTn/2knJ5fdBehPis4A5gtrTYyzfq amnY0+RCQJhvzPM2zqBF7pMogNIuMGjDGTslOIq/OJup+56WF2t1l51V0qky0MEnXzhe FB/AqrLrggc07dVtsPhw4AcvQVgmSa6oXw/6WHPFQTCuuo13bpslQQTsDV4hsoEfh9wQ Py9w== X-Gm-Message-State: AOAM532HZqFTrjG5n8N8TwxwrLK6mQ5+d5u1WJFGKKdnL0WHde4fO/uo fZCwdmp1SeAprQli4WdhBjSDbYKvAJuMRv4GT9VsHw== X-Google-Smtp-Source: ABdhPJzW6Sl9104AQe1TikPz2V06k4GI9p6VRrI44rLorYi/V7/mB7jZJtwj8TH2QusD972PA3jytgSSHTeBJVOrZ2g= X-Received: by 2002:a05:6402:40d4:: with SMTP id z20mr21700623edb.89.1628035079883; Tue, 03 Aug 2021 16:57:59 -0700 (PDT) MIME-Version: 1.0 References: <20210731175341.3458608-1-lrizzo@google.com> <20210803160803.GG543798@ziepe.ca> <20210803230725.ao3i2emejyyor36n@revolver> <20210803233542.GH543798@ziepe.ca> In-Reply-To: <20210803233542.GH543798@ziepe.ca> From: Luigi Rizzo Date: Wed, 4 Aug 2021 01:57:48 +0200 Message-ID: Subject: Re: [PATCH] Add mmap_assert_locked() annotations to find_vma*() To: Jason Gunthorpe Cc: Liam Howlett , Jann Horn , linux-kernel , Andrew Morton , David Rientjes , "linux-mm@kvack.org" Content-Type: text/plain; charset="UTF-8" Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=google.com header.s=20161025 header.b=lS2pf2FO; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf16.hostedemail.com: domain of lrizzo@google.com designates 209.85.208.47 as permitted sender) smtp.mailfrom=lrizzo@google.com X-Stat-Signature: rjhqdj3jx8qw61i3yawkb11iyty9kx4a X-Rspamd-Queue-Id: 5C0F0F006826 X-Rspamd-Server: rspam01 X-HE-Tag: 1628035081-319699 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: On Wed, Aug 4, 2021 at 1:35 AM Jason Gunthorpe wrote: > > On Tue, Aug 03, 2021 at 11:07:35PM +0000, Liam Howlett wrote: > > * Luigi Rizzo [210803 17:49]: > > > On Tue, Aug 3, 2021 at 6:08 PM Jason Gunthorpe wrote: > > > > > > > > On Sat, Jul 31, 2021 at 10:53:41AM -0700, Luigi Rizzo wrote: > > > > > find_vma() and variants need protection when used. > > > > > This patch adds mmap_assert_lock() calls in the functions. > > > > > > > > > > To make sure the invariant is satisfied, we also need to add a > > > > > mmap_read_loc() around the get_user_pages_remote() call in > > > > > get_arg_page(). The lock is not strictly necessary because the mm > > > > > has been newly created, but the extra cost is limited because > > > > > the same mutex was also acquired shortly before in __bprm_mm_init(), > > > > > so it is hot and uncontended. > > > > > > > > > > Signed-off-by: Luigi Rizzo > > > > > fs/exec.c | 2 ++ > > > > > mm/mmap.c | 2 ++ > > > > > 2 files changed, 4 insertions(+) > > > > > > > > > > diff --git a/fs/exec.c b/fs/exec.c > > > > > index 38f63451b928..ac7603e985b4 100644 > > > > > +++ b/fs/exec.c > > > > > @@ -217,8 +217,10 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, > > > > > * We are doing an exec(). 'current' is the process > > > > > * doing the exec and bprm->mm is the new process's mm. > > > > > */ > > > > > + mmap_read_lock(bprm->mm); > > > > > ret = get_user_pages_remote(bprm->mm, pos, 1, gup_flags, > > > > > &page, NULL, NULL); > > > > > + mmap_read_unlock(bprm->mm); > > > > > if (ret <= 0) > > > > > return NULL; > > > > > > > > Wasn't Jann Horn working on something like this too? > > > > > > > > https://lore.kernel.org/linux-mm/20201016225713.1971256-1-jannh@google.com/ > > > > > > > > IIRC it was very tricky here, are you sure it is OK to obtain this lock > > > > here? > > > > > > I cannot comment on Jann's patch series but no other thread knows > > > about this mm at this point in the code so the lock is definitely > > > safe to acquire (shortly before there was also a write lock acquired > > > on the same mm, in the same conditions). > > > > If there is no other code that knows about this mm, then does one need > > the lock at all? Is this just to satisfy the new check you added? > > > > If you want to make this change, I would suggest writing it in a way to > > ensure the call to expand_downwards() in the same function also holds > > the lock. I believe this is technically required as well? What do you > > think? > > This is essentially what Jann was doing. Since the mm is newly created > we can create it write locked and then we can add proper locking tests > to many of the functions called along this path. > > Adding useless locks around each troublesome callsite just seems > really confusing to me. Uhm... by that reasoning, even creating the mm locked (and unlocking at the end) is equally unnecessary. My goal was to add asserts and invariants that are easy to understand and get right, rather than optimize a path that does not appear to be critical. Adding one read lock pair around the one function we annotate is easy to understand and clearly a leaf lock. Having alloc_bprm return a locked object is a bit unconventional, and also passing it to other methods raises the question of whether they take other lock possibly causing lock order reversals in the future. cheers luigi