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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1F6E5C43334 for ; Fri, 10 Jun 2022 17:32:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 635318D00CE; Fri, 10 Jun 2022 13:32:32 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5BEB08D00CB; Fri, 10 Jun 2022 13:32:32 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 460AC8D00CE; Fri, 10 Jun 2022 13:32:32 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 30B678D00CB for ; Fri, 10 Jun 2022 13:32:32 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 0A69B1745 for ; Fri, 10 Jun 2022 17:32:32 +0000 (UTC) X-FDA: 79563020544.12.9FD4954 Received: from mail-io1-f47.google.com (mail-io1-f47.google.com [209.85.166.47]) by imf18.hostedemail.com (Postfix) with ESMTP id B30D91C0011 for ; Fri, 10 Jun 2022 17:32:31 +0000 (UTC) Received: by mail-io1-f47.google.com with SMTP id y79so7428750iof.2 for ; Fri, 10 Jun 2022 10:32:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=mYaarscCDbiYRQpS283kg18dErtB3WbPTISYZjeM1fg=; b=XI5SlOSdJe1INtlhWj4qWgSBvxqioEZ1C1eHjSENKnAALpsMLQnmHIl7ofJ8ddr82+ AKcCrGbmvjkIEmkpO85nUOhBth3ZUYlKOMv2CzXt/rY5gQ0rTl/5bKaZl2/RoXyI9iGb 5WaFMaUgqZ7YZseM5sE9+mfJ+F35Pgto9VjZb2JjEVY4D/MYDEJZQRl4Fh39a5tzZZ4V fCRkC6BdLFm0ZjmJpSilx7QznCErM5109JNzVOQx4E2bc150u8Q7aclLOmXCfqHPGbWM aoNQdALMTQYeaqJBNxJ5yQQMEREbtUs2xupMRo4QCDv2hZw5nEIS33TeV17Kdus9/het bJ4w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mYaarscCDbiYRQpS283kg18dErtB3WbPTISYZjeM1fg=; b=buioo8FAwG1TI5zCa3eiUZTDOlHUhHDbGALezUgraoXaDMW8cktjvnqHHvD91sNTQ0 xE611IuEj3jI0svgJiGcidWF3fW4nEZGa2pAUfN4BhtDtr9qDwoWJOPka18LlNpgFiWs rObABfFKeKb44sGoZ73DqyebQ5u1VMbVn1z+2xYPO7PFRUbe5+1IM825wFRJi9qdntCA wILLk0VL5+izgE1rUVcnrNeVSZJVgxgRSlQo5JMQ9Vt+OO8Vk+CtuztUCnQno3vqSXuO PV0Dyk3oIOK2PW1S1/rhy9Bfm+rcbE9+UYk2XdONCXy2bdghzXYAcwd/uLHHXvF2b9l2 gYvA== X-Gm-Message-State: AOAM530/6K6GwSq8jg9ahQ/PHM/OfbOEhCb2qU/ZX13yvggNg3ZgHskD FBHPaHpyLIdMNHC2qKsM4QK8AHwB1TFJ2NBnzzA4oQ== X-Google-Smtp-Source: ABdhPJzi4n+BiEugpCfRZawhd4LlJudPgFHb/9fD3ahN+VmylkhLwh3Qhr5k4K2Z/P7/U2b/mPvy1OVGt3qs4TagwNo= X-Received: by 2002:a6b:bac1:0:b0:669:b1fe:58e4 with SMTP id k184-20020a6bbac1000000b00669b1fe58e4mr4354104iof.171.1654882350815; Fri, 10 Jun 2022 10:32:30 -0700 (PDT) MIME-Version: 1.0 References: <20220603205741.12888-1-axelrasmussen@google.com> In-Reply-To: From: Axel Rasmussen Date: Fri, 10 Jun 2022 10:31:53 -0700 Message-ID: Subject: Re: [PATCH] mm: userfaultfd: fix UFFDIO_CONTINUE on fallocated shmem pages To: Peter Xu Cc: Andrew Morton , Hugh Dickins , Linux MM , LKML , stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1654882351; a=rsa-sha256; cv=none; b=lEMMPJRX4z+ZPx4H238AJQaIaQCPcjYQktjoSMo3toVNH4bU0oMEs/x8iO3Fyns5WwR3uW ioUzGzqoQ9Ks+itFeeG2f+3CFvughCMGHi90MZe4YMEZRneELnD3ZXH2HfGowxbNWNl/TU /VzoJP9fvLbCzFLarRyJonWPV5q5Rzo= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=XI5SlOSd; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf18.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.166.47 as permitted sender) smtp.mailfrom=axelrasmussen@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1654882351; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=mYaarscCDbiYRQpS283kg18dErtB3WbPTISYZjeM1fg=; b=5q0AEbmUdmHQYgoPZY0luzn4p5xdmvt2Q5+FQ/FobJQlHr+2yyHZONziacs2auuUFsohz6 xm+DHBtLGzbGbv7o7pz6GzMtRsbdjBR0hqB+JqZL3VES53uSKUyvEDiGEXVVoaDffM4zpu P5qsUW2BH1iYupiUebk+GqSLLk06B78= X-Rspamd-Queue-Id: B30D91C0011 X-Rspam-User: Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=XI5SlOSd; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf18.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.166.47 as permitted sender) smtp.mailfrom=axelrasmussen@google.com X-Stat-Signature: 59ni5t9819mxhr8i5tmq4scemi8r44q3 X-Rspamd-Server: rspam02 X-HE-Tag: 1654882351-601252 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: Ah, thanks for pointing this out Peter, it is definitely something I missed. You're right that elsewhere in mm/userfaultfd.c we use -ENOENT for some special case. I think for mcontinue_atomic_pte, we don't want to change the status quo - if we fail to lookup an existing page, we should just return -EFAULT just like we were doing before. We certainly shouldn't return -ENOENT, as that causes us to take a wrong, unrelated code path a couple of callers up, as you mentioned. I'll send a v2 with this small modification. On Thu, Jun 9, 2022 at 2:44 PM Peter Xu wrote: > > Hi, Axel, > > Sorry to read this late. > > On Fri, Jun 03, 2022 at 01:57:41PM -0700, Axel Rasmussen wrote: > > When fallocate() is used on a shmem file, the pages we allocate can end > > up with !PageUptodate. > > > > Since UFFDIO_CONTINUE tries to find the existing page the user wants to > > map with SGP_READ, we would fail to find such a page, since > > shmem_getpage_gfp returns with a "NULL" pagep for SGP_READ if it > > discovers !PageUptodate. As a result, UFFDIO_CONTINUE returns -EFAULT, > > as it would do if the page wasn't found in the page cache at all. > > > > This isn't the intended behavior. UFFDIO_CONTINUE is just trying to find > > if a page exists, and doesn't care whether it still needs to be cleared > > or not. So, instead of SGP_READ, pass in SGP_NOALLOC. This is the same, > > except for one critical difference: in the !PageUptodate case, > > SGP_NOALLOC will clear the page and then return it. With this change, > > UFFDIO_CONTINUE works properly (succeeds) on a shmem file which has been > > fallocated, but otherwise not modified. > > > > Fixes: 153132571f02 ("userfaultfd/shmem: support UFFDIO_CONTINUE for shmem") > > Cc: stable@vger.kernel.org > > Signed-off-by: Axel Rasmussen > > --- > > mm/userfaultfd.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > > index 4f4892a5f767..c156f7f5b854 100644 > > --- a/mm/userfaultfd.c > > +++ b/mm/userfaultfd.c > > @@ -246,7 +246,7 @@ static int mcontinue_atomic_pte(struct mm_struct *dst_mm, > > struct page *page; > > int ret; > > > > - ret = shmem_getpage(inode, pgoff, &page, SGP_READ); > > + ret = shmem_getpage(inode, pgoff, &page, SGP_NOALLOC); > > if (ret) > > goto out; > > if (!page) { > > It all looks sane if the page is !uptodate as you described. Though I've a > question on what'll happen if the page is actually missing rather than just > !PageUptodate(). > > My reading is previously it'll keep returning 0 on shmem_getpage_gfp() for > both cases, but now for the missing page shmem_getpage_gfp() will return > -ENOENT instead. > > This reminded me on whether this will errornously let __mcopy_atomic() go > into the special path to copy the page without mmap lock, please see this > commit: > > b6ebaedb4cb1 ("userfaultfd: avoid mmap_sem read recursion in mcopy_atomic", 2015-09-04) > > Would that be a problem? Or could I read it wrong? > > This also reminded me that whether we'd better need some protection in the > -ENOENT handling in __mcopy_atomic() to be always safe. > > Thanks, > > -- > Peter Xu >