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=-24.8 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_AGENT_SANE_1,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 D24D5C433B4 for ; Wed, 28 Apr 2021 21:03:23 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2B83561418 for ; Wed, 28 Apr 2021 21:03:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2B83561418 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 446406B0036; Wed, 28 Apr 2021 17:03:22 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 41D5B6B006E; Wed, 28 Apr 2021 17:03:22 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2BEE36B0070; Wed, 28 Apr 2021 17:03:22 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0252.hostedemail.com [216.40.44.252]) by kanga.kvack.org (Postfix) with ESMTP id 103476B0036 for ; Wed, 28 Apr 2021 17:03:22 -0400 (EDT) Received: from smtpin35.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id C6C3A3635 for ; Wed, 28 Apr 2021 21:03:21 +0000 (UTC) X-FDA: 78083001402.35.4D04622 Received: from mail-qt1-f173.google.com (mail-qt1-f173.google.com [209.85.160.173]) by imf03.hostedemail.com (Postfix) with ESMTP id 9CF75C0007F7 for ; Wed, 28 Apr 2021 21:03:15 +0000 (UTC) Received: by mail-qt1-f173.google.com with SMTP id o1so6121853qta.1 for ; Wed, 28 Apr 2021 14:03:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:in-reply-to:message-id:references :user-agent:mime-version; bh=WDC8cMj9KQFGN4oBcXzeVKvPTnKsbzd/gp0dCQd3X/Q=; b=PWfyOCz5TRVUzpG1EnvMvWIUqUPjRWGfHRaQDh1gQrU3bm+IA8jET9L2HQs1jsgddu KJMUUzIo0qPq4qZ1QF4Ijkptc1ShjKY0xkgvIJnGoRpxTjka8IINMp2eAwbGqRho8QRN 4g2d4UFc9fY8A8tiNxbklAWVZ+eIKiuYpxiaZ2BXCWKAzRw547K1ezKj+xmWddgegHsu NECnDTscXuhIYqkp7FhZarCow+KRdssgIwz7iTWgxwhxkRc0ubxs/YyN2kjylejK5U/t GHNLe3pUhmcULk7fGyYlOaB2HcxH0hHxEVgpi/8giB7fKphxewl+1cilR+A9ktR8Z+im 0MCQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version; bh=WDC8cMj9KQFGN4oBcXzeVKvPTnKsbzd/gp0dCQd3X/Q=; b=UuL6QSCdaZvUimr6OWz8Nk0q2YRkV0NXVZ+o2oj/Nhq8ctFOWW6+PY2eWNtpgo2e5c djLdyDjfEo416mfUiO4wVr0rby7N0sD1s02azMkFaNzHL3OaWXcr0N7c4s5SDJIbONYw v3J/BU7097o6vN1OuJ35dQ2/24pGAcV7LHbTfKqr3S8pyG8BG3jcRmAQVX2gFrqXSiGP XPbgWrGF6JI8iZ7aLjdKU4kp3IYjEKtnGc+i0ONFlOIBIMDmrUODA/RWgT7ZJNfgr5QX gnm6MHjLQV9QbW8BhdoN/vbzJCKgR8XUG70tkT+GbmQlAw/HsuFDBvOXYrh2w7+9MfzJ kvkA== X-Gm-Message-State: AOAM532dTdPCF3zXC2c7lH4nVamqPJXHZh6KqqJgDg5DzYQwHW7T0e+p hyvdah5tZKSPm2g6WE7vZMcfIw== X-Google-Smtp-Source: ABdhPJxHpUFRnQapCxPAOUFJmZDOlcbrm4ZSAzgXZrqUUfcjf4VuDgreeNK4a6SEKbnoW6EIbypMWg== X-Received: by 2002:a05:622a:6:: with SMTP id x6mr28372136qtw.1.1619643800361; Wed, 28 Apr 2021 14:03:20 -0700 (PDT) Received: from eggly.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id k18sm676219qkg.53.2021.04.28.14.03.19 (version=TLS1 cipher=ECDHE-ECDSA-AES128-SHA bits=128/128); Wed, 28 Apr 2021 14:03:19 -0700 (PDT) Date: Wed, 28 Apr 2021 14:03:05 -0700 (PDT) From: Hugh Dickins X-X-Sender: hugh@eggly.anvils To: Axel Rasmussen cc: Peter Xu , Andrea Arcangeli , Andrew Morton , Hugh Dickins , Lokesh Gidra , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] userfaultfd: release page in error path to avoid BUG_ON In-Reply-To: <20210428183943.GH6584@xz-x1> Message-ID: References: <20210428180109.293606-1-axelrasmussen@google.com> <20210428183943.GH6584@xz-x1> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Rspamd-Queue-Id: 9CF75C0007F7 X-Stat-Signature: etpx9598atykjr5kdxnpfh4hepzawujp X-Rspamd-Server: rspam02 Received-SPF: none (google.com>: No applicable sender policy available) receiver=imf03; identity=mailfrom; envelope-from=""; helo=mail-qt1-f173.google.com; client-ip=209.85.160.173 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1619643795-28431 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, 28 Apr 2021, Peter Xu wrote: > On Wed, Apr 28, 2021 at 11:01:09AM -0700, Axel Rasmussen wrote: > > Consider the following sequence of events (described from the point of > > view of the commit that introduced the bug - see "Fixes:" below): > > > > 1. Userspace issues a UFFD ioctl, which ends up calling into > > shmem_mcopy_atomic_pte(). We successfully account the blocks, we > > shmem_alloc_page(), but then the copy_from_user() fails. We return > > -EFAULT. We don't release the page we allocated. > > 2. Our caller detects this error code, tries the copy_from_user() after > > dropping the mmap_sem, and retries, calling back into > > shmem_mcopy_atomic_pte(). > > 3. Meanwhile, let's say another process filled up the tmpfs being used. > > 4. So shmem_mcopy_atomic_pte() fails to account blocks this time, and > > immediately returns - without releasing the page. This triggers a > > BUG_ON in our caller, which asserts that the page should always be > > consumed, unless -EFAULT is returned. > > > > (Later on in the commit history, -EFAULT became -ENOENT, mmap_sem became > > mmap_lock, and shmem_inode_acct_block() was added.) > > I suggest you do s/EFAULT/ENOENT/ directly in above. I haven't looked into the history, but it would be best for this to describe the situation in v5.12, never mind the details which were different at the time of the commit tagged with Fixes. But we stay alert that when it's backported to stable, we may need to adjust something to suit those releases (which will depend on how much else has been backported to them meanwhile). > > > > > A malicious user (even an unprivileged one) could trigger this > > intentionally without too much trouble. I regret having suggested that. Maybe. Opinions differ as to whether it's helpful to call attention like that. I'd say delete that paragraph. > > > > To fix this, detect if we have a "dangling" page when accounting fails, > > and if so, release it before returning. > > > > Fixes: cb658a453b93 ("userfaultfd: shmem: avoid leaking blocks and used blocks in UFFDIO_COPY") > > Reported-by: Hugh Dickins > > Signed-off-by: Axel Rasmussen Thanks for getting on to this so quickly, Axel. But Peter is right, that unlock_page() needs removing. > > --- > > mm/shmem.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/mm/shmem.c b/mm/shmem.c > > index 26c76b13ad23..46766c9d7151 100644 > > --- a/mm/shmem.c > > +++ b/mm/shmem.c > > @@ -2375,8 +2375,19 @@ static int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, > > pgoff_t offset, max_off; > > > > ret = -ENOMEM; > > - if (!shmem_inode_acct_block(inode, 1)) > > + if (!shmem_inode_acct_block(inode, 1)) { > > + /* > > + * We may have got a page, returned -ENOENT triggering a retry, > > + * and now we find ourselves with -ENOMEM. Release the page, to > > + * avoid a BUG_ON in our caller. > > + */ > > + if (unlikely(*pagep)) { > > + unlock_page(*pagep); > > Not necessary? Worse than not necessary: would trigger a VM_BUG_ON_PAGE(). Delete! > > > + put_page(*pagep); > > + *pagep = NULL; > > + } > > goto out; > > All "goto out" in this functions looks weird as it returns directly... so if > you're touching this after all, I suggest we do "return -ENOMEM" directly and > drop the "ret = -ENOMEM". No strong feeling either way from me on that: whichever looks best to you. But I suspect the "ret = -ENOMEM" cannot be dropped, because it's relied on further down too? > > Thanks, > > > + } > > > > if (!*pagep) { > > page = shmem_alloc_page(gfp, info, pgoff); > > -- > > 2.31.1.498.g6c1eba8ee3d-goog > > > > -- > Peter Xu