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, URIBL_BLOCKED,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 56E42C4338F for ; Wed, 4 Aug 2021 14:42:53 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id F3B2860F01 for ; Wed, 4 Aug 2021 14:42:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org F3B2860F01 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 85C648D0007; Wed, 4 Aug 2021 10:42:52 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 80C6C8D0006; Wed, 4 Aug 2021 10:42:52 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 722798D0007; Wed, 4 Aug 2021 10:42:52 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0249.hostedemail.com [216.40.44.249]) by kanga.kvack.org (Postfix) with ESMTP id 558088D0006 for ; Wed, 4 Aug 2021 10:42:52 -0400 (EDT) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 07509252B1 for ; Wed, 4 Aug 2021 14:42:52 +0000 (UTC) X-FDA: 78437664984.09.948835D Received: from mail-lj1-f172.google.com (mail-lj1-f172.google.com [209.85.208.172]) by imf29.hostedemail.com (Postfix) with ESMTP id 76499901E0D7 for ; Wed, 4 Aug 2021 14:42:51 +0000 (UTC) Received: by mail-lj1-f172.google.com with SMTP id x9so900365ljj.2 for ; Wed, 04 Aug 2021 07:42:51 -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=epkSZNOyyPNAWPQz+jmngcd1GrS/+wWE4RVIUtrB7vM=; b=S4cBM6DpryzJ0dCWjdd4PwjY0cPQgQET23yLihuRJ9AyMfKWHeL+VbFZt+9zkGmt0m WzzcB1kb8eF6do2K5Jir9StwEMXk7WITNQiTl4gEmrSYkI0BotdknWsfgKJj+1fJnJf2 neWVuI/A9qzqPOzZ7YT8RH8kB7+bKNwjt1bYT8Gn5zRvNl2k6TrPgzmUhhtoxTj2Oyzv X2Xj1eY0ZFkNxWN9X1RMtU4dJUREUpEmbVVU9kRJHCIAkvA5dSEVYHfoUL5bC9kM650F 0y8JejK6927mdMM2YZkEHTnnyERfsexGghvgU24BYpVhaCR1Mk6bwWvnmAqhMGOaEnho /QYg== 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=epkSZNOyyPNAWPQz+jmngcd1GrS/+wWE4RVIUtrB7vM=; b=QhKZlTXQ2c0KqWYsfxRK/vmLo+h5cwP+Rka8dEEBlPCUi0L9XeMN93Nr4T2+erul2y E0auXKNugOIkxoCVW1GFkqQ4LvXjl2/++fl5nhSHxHYcNDXESzi6pDUE0PlLmejrrzYM 41d3g6Kyeu/pey8yLZiO5zwemg+LeUmU18IPpUp14Xkgi+FfIl64c4nFO0Ce0+ejrCmM xf9mEfSD5Ot4lEHLEAgV4zFxr44KIb70Y7CeYCAoQ8TAQmRJ8SJi0Zvco0JE29aFswsw tTuu7YSDBKxRVaDHmh5Ctuy4KbukhDIP9AWNS2RIjk6SdbT5TOVbbEngLKmCUk3QtP/E 6aCg== X-Gm-Message-State: AOAM530XHjjTP4znO+wb0SBW2AjkyHlc/xhZKMZI7cvIqrLWa31lKZgh p31qB0CaJXZTsv4FbkusdhNYuUCaRq49a4wfisujxg== X-Google-Smtp-Source: ABdhPJyoi1uyaThbAN9PmOfRK/CqCAX1nXZFV8Reju/RQckBL2bo3ds1H0EXbeX+qhFdou5j8BYdO8qJXAHX//s4qr8= X-Received: by 2002:a2e:a817:: with SMTP id l23mr18320916ljq.86.1628088169741; Wed, 04 Aug 2021 07:42:49 -0700 (PDT) MIME-Version: 1.0 References: <20210731175341.3458608-1-lrizzo@google.com> <20210803160803.GG543798@ziepe.ca> <20210803230725.ao3i2emejyyor36n@revolver> In-Reply-To: <20210803230725.ao3i2emejyyor36n@revolver> From: Jann Horn Date: Wed, 4 Aug 2021 16:42:23 +0200 Message-ID: Subject: Re: [PATCH] Add mmap_assert_locked() annotations to find_vma*() To: Liam Howlett Cc: Luigi Rizzo , Jason Gunthorpe , linux-kernel , Andrew Morton , David Rientjes , "linux-mm@kvack.org" Content-Type: text/plain; charset="UTF-8" Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=google.com header.s=20161025 header.b=S4cBM6Dp; spf=pass (imf29.hostedemail.com: domain of jannh@google.com designates 209.85.208.172 as permitted sender) smtp.mailfrom=jannh@google.com; dmarc=pass (policy=reject) header.from=google.com X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 76499901E0D7 X-Stat-Signature: kzn4eqzjbyk7kgr3xsbtrmtyy5xfho75 X-HE-Tag: 1628088171-184077 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:07 AM 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? The call to expand_downwards() takes a VMA pointer as argument, and the mmap lock is the only thing that normally prevents concurrent freeing of VMA structs. Taking a lock there would be of limited utility - either the lock is not necessary because nobody else can access the MM, or the lock is insufficient because someone could have freed the VMA pointer before the lock was taken. So I think that taking a lock around the expand_downwards() call would just be obfuscating things, unless you specifically want to prevent concurrent *reads* while concurrent *writes* are impossible. Since I haven't sent a new version of my old series for almost a year, I think it'd be fine to take Luigi's patch for now, and undo it at a later point when/if we want to actually use proper locking here because we're worried about concurrent access to the MM.