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=-12.1 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_IN_DEF_DKIM_WL 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 F19E3C4741F for ; Wed, 30 Sep 2020 23:42:38 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 738212158C for ; Wed, 30 Sep 2020 23:42:38 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="sIW19YFF" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 738212158C Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B27E86B005C; Wed, 30 Sep 2020 19:42:37 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AD9156B0062; Wed, 30 Sep 2020 19:42:37 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A163D6B0068; Wed, 30 Sep 2020 19:42:37 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0033.hostedemail.com [216.40.44.33]) by kanga.kvack.org (Postfix) with ESMTP id 8AD586B005C for ; Wed, 30 Sep 2020 19:42:37 -0400 (EDT) Received: from smtpin28.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 50B188249980 for ; Wed, 30 Sep 2020 23:42:37 +0000 (UTC) X-FDA: 77321354754.28.start15_180f19527196 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin28.hostedemail.com (Postfix) with ESMTP id 354C86C04 for ; Wed, 30 Sep 2020 23:42:37 +0000 (UTC) X-HE-Tag: start15_180f19527196 X-Filterd-Recvd-Size: 6239 Received: from mail-yb1-f194.google.com (mail-yb1-f194.google.com [209.85.219.194]) by imf33.hostedemail.com (Postfix) with ESMTP for ; Wed, 30 Sep 2020 23:42:36 +0000 (UTC) Received: by mail-yb1-f194.google.com with SMTP id k2so2603949ybp.7 for ; Wed, 30 Sep 2020 16:42:36 -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=luDVp57wMcfpdftNDbDKSeVu1a5GQyNIzBHQaw4nI84=; b=sIW19YFFz+zsmEro8C1rNIpiF39LuiwMUcIGYtUS7Lu/+X/MqVJk6EXLGsFyk69z08 dIfCxKpTgTmIEDuF/zowUKJxBeJHpPzPcavxn1oW2a0d9htzi7MzFXYHBbs4a+rSn9HV 2wDwtTwPflyJ0D+bqBAo2wBJDV4VPW6TAqkt2zWieAOhFySixWDxf3XpYSmdDBXKmjUs fOokQkaHh7sZoEN+TD5rpMcCcnXbjTH7DqPfKOJqRTJGCaVDJNzKLleMfYhjM26ZrlOf twz26tv0h8W8qBOUO0lJl1nH8SJaYyVJvounETnWTzOAICmNSHfbJwM8gdDj+PNW/GIg p2JQ== 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=luDVp57wMcfpdftNDbDKSeVu1a5GQyNIzBHQaw4nI84=; b=MbsvGNJzZo3ELhpF7CQ9p5u1Bh5FkooGd9Evm87XzBwxdYeLF26yAkKfQh662Is8yn /9PnFWKBgLd9taqjypbhjKR6W1IDCS8YlUNV2sJjKZ2UjsaiXaJvEDERWa9OrSAVaxYX 44T3vj9dFSBONeK3OwV3KLX4gMX990xYzP+vFFIHZKzOKmVI1wsxw6gKRDgdeIvLvcti tfBAnzr8zke/5hstf7yBJcRWkK2QCZF38/wiSizk13djDvcnR6EP3vXqVWr45IM0NFTi mVzv+zDiTxq8Lge3jRo/E4+LBdqi/Z5rIY+oMjqC3E/cqQ4RlQpE60QR5w2LAENZD012 YMww== X-Gm-Message-State: AOAM533XV79/4QaIbk0TNze0Yez4k8dpFZmHC10FQXhWwYUwHBdHvGF7 VE/xtH/br0QE9VWE4UqMFrWVBu3Udk+lrDYrsPph+vJ9I+fKb0xq X-Google-Smtp-Source: ABdhPJyjKYPRns8jRytqvCWE8ZRfS7jx6rNxLXa2BRBQRCGLDzj42tWa7l9AT04SW0GPR2MfdO/2wHEWMG0/6tpHKLs= X-Received: by 2002:a25:5056:: with SMTP id e83mr6637772ybb.287.1601509355833; Wed, 30 Sep 2020 16:42:35 -0700 (PDT) MIME-Version: 1.0 References: <20200930011944.19869-1-jannh@google.com> <20200930123000.GC9916@ziepe.ca> In-Reply-To: From: Michel Lespinasse Date: Wed, 30 Sep 2020 16:42:22 -0700 Message-ID: Subject: Re: [PATCH 3/4] mmap locking API: Don't check locking if the mm isn't live yet To: Jann Horn Cc: Jason Gunthorpe , Andrew Morton , Linux-MM , kernel list , "Eric W . Biederman" , Mauro Carvalho Chehab , Sakari Ailus Content-Type: text/plain; charset="UTF-8" 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, Sep 30, 2020 at 1:15 PM Jann Horn wrote: > On Wed, Sep 30, 2020 at 2:50 PM Jann Horn wrote: > > On Wed, Sep 30, 2020 at 2:30 PM Jason Gunthorpe wrote: > > > On Tue, Sep 29, 2020 at 06:20:00PM -0700, Jann Horn wrote: > > > > In preparation for adding a mmap_assert_locked() check in > > > > __get_user_pages(), teach the mmap_assert_*locked() helpers that it's fine > > > > to operate on an mm without locking in the middle of execve() as long as > > > > it hasn't been installed on a process yet. > > > > > > I'm happy to see lockdep being added here, but can you elaborate on > > > why add this mmap_locked_required instead of obtaining the lock in the > > > execv path? > > > > My thinking was: At that point, we're logically still in the > > single-owner initialization phase of the mm_struct. Almost any object > > has initialization and teardown steps that occur in a context where > > the object only has a single owner, and therefore no locking is > > required. It seems to me that adding locking in places like > > get_arg_page() would be confusing because it would suggest the > > existence of concurrency where there is no actual concurrency, and it > > might be annoying in terms of lockdep if someone tries to use > > something like get_arg_page() while holding the mmap_sem of the > > calling process. It would also mean that we'd be doing extra locking > > in normal kernel builds that isn't actually logically required. > > > > Hmm, on the other hand, dup_mmap() already locks the child mm (with > > mmap_write_lock_nested()), so I guess it wouldn't be too bad to also > > do it in get_arg_page() and tomoyo_dump_page(), with comments that > > note that we're doing this for lockdep consistency... I guess I can go > > change this in v2. > > Actually, I'm taking that back. There's an extra problem: > get_arg_page() accesses bprm->vma, which is set all the way back in > __bprm_mm_init(). We really shouldn't be pretending that we're > properly taking the mmap_sem when actually, we keep reusing a > vm_area_struct pointer. > > So for that reason I prefer the approach in the existing patch, where > we make it clear that mm_struct has two different lifetime phases in > which GUP works, and that those lifetime phases have very different > locking requirements. > > Does that sound reasonable? I'm really not a fan of adding such exceptions; I think it's both unusual and adds complexity that is not strictly contained into the init paths. I don't really understand the concern with the bprm vma in get_arg_page(); I'm not super familiar with this code but isn't it a normal vma within the process that __do_execve_file() is creating ? I received Jason's last email while I was composing this one, but I think I have the same concern/approach as him, i.e. I think it would be simplest to keep the new MM locked through the __do_execve_file() call and avoid adding the mmap_lock_required exception to the mmap_assert_locked rule. -- Michel "Walken" Lespinasse A program is never fully debugged until the last user dies.