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 7B6D7C4363D for ; Wed, 30 Sep 2020 23:52:03 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id DD01520C09 for ; Wed, 30 Sep 2020 23:52:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="AELe5H3n" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DD01520C09 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 433496B005C; Wed, 30 Sep 2020 19:52:02 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3EC5C6B0062; Wed, 30 Sep 2020 19:52:02 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2D0A16B0068; Wed, 30 Sep 2020 19:52:02 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0064.hostedemail.com [216.40.44.64]) by kanga.kvack.org (Postfix) with ESMTP id 1386D6B005C for ; Wed, 30 Sep 2020 19:52:02 -0400 (EDT) Received: from smtpin10.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id BD3454410 for ; Wed, 30 Sep 2020 23:52:01 +0000 (UTC) X-FDA: 77321378442.10.news85_230164c27196 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin10.hostedemail.com (Postfix) with ESMTP id 9499B16A06B for ; Wed, 30 Sep 2020 23:52:01 +0000 (UTC) X-HE-Tag: news85_230164c27196 X-Filterd-Recvd-Size: 6014 Received: from mail-ej1-f66.google.com (mail-ej1-f66.google.com [209.85.218.66]) by imf09.hostedemail.com (Postfix) with ESMTP for ; Wed, 30 Sep 2020 23:52:01 +0000 (UTC) Received: by mail-ej1-f66.google.com with SMTP id dd13so5357287ejb.5 for ; Wed, 30 Sep 2020 16:52:00 -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=MY9a7xp4hw7GjU0Q4T5BNjAGiuJEGP50wyCFWUgZk1k=; b=AELe5H3n4BIlWjHXZ8r0jnQ4ThIPG3VpSN0Dy8jgcOZhwowpmOkqepQ+PBeMU8jyIc VlwXwFPyC4nbDVO4YfZgsLK3gKvQzJ3SSb1X/Hx5t/0UcMhkLahc8Uft9DAEusFGCap+ CX2HC8TiovDz5sO/UufY0huVNWGDL5aAuyeBZwYfpHmL1DJTgPuz0hy6etNKObn7Fhfz a0Xu9WDxHFrMgnqOiyX0tbmvXGLB3ilFCon8VJ1WxNWS4M9erfImNmmkWzNutY9TU5iT rDm5YKC0eJr5qK2Oz5W5YhRzq0Bt9JAVYa0sZYI5dpw14H6udz3ch80exedbQtaF1rzp ipKg== 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=MY9a7xp4hw7GjU0Q4T5BNjAGiuJEGP50wyCFWUgZk1k=; b=EeXKqMZwyWDHlMP3J4kCFmFEVAM2IqlCSsU0moqXb3AJjpQn9r33cOJrk/ubK12fzm dz5kq2WlJssNv4UtlOLNTFxYWOD+jBQYHCgoJZZcoTzx9kkeppoGFQzW/VmjSyA2ckMO Yw33hHRuE2QkD82seBXBvFPOLY8oIW2+5FW2dsnMj/JHKKTo2f4ZrvGO26qLoY1FbpZF HN9UZIIKkZDc6rpbXB91CdhToEXkUQ6qIWr78y/Ni0SlRjKTtdtrXOTCnDdhmSgxmNnT zpoWfJQBV1kk6NhbjFFX7dOm+/gSRW9e/QkPIMsPqigcqyUqFViwbcSHiHU3nKMuxc3K 91wg== X-Gm-Message-State: AOAM533rL9o5EINie8yjU6WSakuaCXI6BVGOvT7yOQnGSf9NquDbR8qS X1/MR2MfKTgxjVUtI4HTDrRlTeadXs3mLW6kDtFOew== X-Google-Smtp-Source: ABdhPJxi4XR1rICV8jycnbPWuvIV1E5qABoEjoeptfU443YtnIYKxH/CWIOkKlATnPQlcsu9HWYVvbzX43bL93pJXPc= X-Received: by 2002:a17:906:394:: with SMTP id b20mr4134151eja.513.1601509919666; Wed, 30 Sep 2020 16:51:59 -0700 (PDT) MIME-Version: 1.0 References: <20200930011944.19869-1-jannh@google.com> <20200930123000.GC9916@ziepe.ca> <20200930232655.GE9916@ziepe.ca> In-Reply-To: <20200930232655.GE9916@ziepe.ca> From: Jann Horn Date: Thu, 1 Oct 2020 01:51:33 +0200 Message-ID: Subject: Re: [PATCH 3/4] mmap locking API: Don't check locking if the mm isn't live yet To: Jason Gunthorpe , Michel Lespinasse Cc: 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 Thu, Oct 1, 2020 at 1:26 AM Jason Gunthorpe wrote: > On Wed, Sep 30, 2020 at 10:14:57PM +0200, 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. > > Any chance the mmap lock can just be held from mm_struct allocation > till exec inserts it into the process? Hm... it should work if we define a lockdep subclass for this so that lockdep is happy when we call get_user() on the old mm_struct while holding that mmap lock. > > Does that sound reasonable? > > My only concern is how weird it is to do this with a variable, I've > never seen something like this before It seems clearer to me this way than taking locks when there is no concurrency that we actually need to guard against. But since both you and Michel seem to hate it, I'll go and code up the version with a lockdep subclass. Under protest. :P