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 82B8AC43334 for ; Wed, 13 Jul 2022 22:46:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6FF316B017A; Wed, 13 Jul 2022 18:46:44 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6882A940134; Wed, 13 Jul 2022 18:46:44 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 54E486B017C; Wed, 13 Jul 2022 18:46:44 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 4017B6B017A for ; Wed, 13 Jul 2022 18:46:44 -0400 (EDT) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id EB6DC34527 for ; Wed, 13 Jul 2022 22:46:43 +0000 (UTC) X-FDA: 79683562686.20.0152990 Received: from mail-il1-f169.google.com (mail-il1-f169.google.com [209.85.166.169]) by imf02.hostedemail.com (Postfix) with ESMTP id 875E38002F for ; Wed, 13 Jul 2022 22:46:43 +0000 (UTC) Received: by mail-il1-f169.google.com with SMTP id a20so29432ilk.9 for ; Wed, 13 Jul 2022 15:46:43 -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=tUjyOf0yqWbtmoJVmUzPdPx8CgvmqB2wj4J+/5xtVEo=; b=eJ+EKHcT3rbS+tFdvIEqpvHXsfqe+T1C6Y27MaVrAYD0y6pvZn4BETZUgJpq92SNkh ya0/Yt9n3YO6jCtaVeS7UAhiq5Xk3F/4w0Fa8tfGfCpb//cB7C/+tMyRw0+6y8nHOpq8 xBk4TseGtpF+wl2t6Ng3OyQtBEfrhPFPL459Q+DWz0njOuDVH3E+Ns4NG0R8giaatofP bAjWOLqe8UsMfsmft4v818VseaZD8rr76lZzRWytLZlCifHJFD+1vVyfpUOVvFzqbBCO aMViiduLtXJFsW4MK/2sJgtPHR6WEdj2hpn4UQ3TXPlkPIj49u3ktBJT7Ced9/vzSEVE bgrw== 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=tUjyOf0yqWbtmoJVmUzPdPx8CgvmqB2wj4J+/5xtVEo=; b=tGjMt76cxfB1I4iqYN+bd+CgkWcnXTkVk+ksv/f+Bh7j5//skgPS/M+8tT3mcmqj6q w9Nq8JtK3UoBy4+KmV4hKdT4mb6EiNWzD6odc/nO+ll0AsyJ7S2d0L1SSt+HN6uoxdI7 Bv44RaUxY9iPWb2uLo2PhMHyDa5J/IVmCKKyiK/3mdCYEGzd0lI/9v6O438gwP8OQIlO 2+am0ZrCSPJUYVYXMzrGaqA8/lj5DmlGYvUpgstk2A5/h56JvZejuejvxm3JRdtbyIx5 VxLv4RP0OUStgCJRpV0UQlPfXasFq3kHkGvjmpbIPVh/hvgPmvNM/ZASaVoF3yyQhfEZ uVlQ== X-Gm-Message-State: AJIora9WSeZP+TG+hARDlk9BAPZd6UvJJccHXiJPmZY+YcFVkSLFz81x 1IApBrNER6ubox4QciCOZMyG83ol34iXmVOLG+/AFA== X-Google-Smtp-Source: AGRyM1va1xZyVCIOolaP1ogeUVdO3Zk/o5dg4DYT3LF29gUCdDFfYdJj9NmtnAyhMSMjGLkerQY8wNOtwZ8PJ7wHH4k= X-Received: by 2002:a05:6e02:1be9:b0:2dc:7fb2:706e with SMTP id y9-20020a056e021be900b002dc7fb2706emr2948941ilv.239.1657752402648; Wed, 13 Jul 2022 15:46:42 -0700 (PDT) MIME-Version: 1.0 References: <20220712130542.18836-1-linmiaohe@huawei.com> In-Reply-To: From: Axel Rasmussen Date: Wed, 13 Jul 2022 15:46:06 -0700 Message-ID: Subject: Re: [PATCH] mm/hugetlb: avoid corrupting page->mapping in hugetlb_mcopy_atomic_pte To: Peter Xu Cc: Mike Kravetz , Miaohe Lin , Andrew Morton , Muchun Song , Linux MM , LKML Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1657752403; a=rsa-sha256; cv=none; b=Srh23vlQ8ST2e78hfo2/D9yEFE/JoFasW7IYzs45hstlBp51h+ejppp+tVG0GJbzsHVrXV 1HgtszbCrAxa9SzcXwHx39sbKonLJ2A6lOwJRr/p3GzeokOF8f6DKmhdJfwiB4iD53iubA m+rPBxaviGGApL9YlursINYKUKSLuu4= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=eJ+EKHcT; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf02.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.166.169 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=1657752403; 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=tUjyOf0yqWbtmoJVmUzPdPx8CgvmqB2wj4J+/5xtVEo=; b=iquuajPhUSEnRo3M+XGWJkZVQ1knThmHjC6cUMOsoygddlPX1BiCD50Dokh0pvyK/GM+N5 u1bBAhvDURWQ36b5Q12uesf+zncj2OK73VFh4M07ypHrhwMbq0bzJBh8RHh5ZFuqJ1GpJB 9kG2YRA0FjmxzTxeSzfW1ooZz+Fky1w= X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 875E38002F Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=eJ+EKHcT; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf02.hostedemail.com: domain of axelrasmussen@google.com designates 209.85.166.169 as permitted sender) smtp.mailfrom=axelrasmussen@google.com X-Stat-Signature: qn5jirc8opeair3m46dgb346cspwb3hq X-Rspam-User: X-HE-Tag: 1657752403-916379 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: I think there is a small mistake in this patch. Consider the non-minor-fault case. We have this block: /* Add shared, newly allocated pages to the page cache. */ if (vm_shared && !is_continue) { /* ... */ } In here, we've added the newly allocated page to the page cache, and we've set this page_in_pagecache flag to true. But we *do not* setup rmap for this page in this block. I think in this case, the patch will cause us to do the wrong thing: we should hugepage_add_new_anon_rmap() further down, but with this patch we dup instead. On Wed, Jul 13, 2022 at 9:10 AM Peter Xu wrote: > > On Wed, Jul 13, 2022 at 10:24:09AM -0400, Peter Xu wrote: > > On Tue, Jul 12, 2022 at 10:39:20AM -0700, Mike Kravetz wrote: > > > On 07/12/22 21:05, Miaohe Lin wrote: > > > > In MCOPY_ATOMIC_CONTINUE case with a non-shared VMA, pages in the page > > > > cache are installed in the ptes. But hugepage_add_new_anon_rmap is called > > > > for them mistakenly because they're not vm_shared. This will corrupt the > > > > page->mapping used by page cache code. > > > > > > > > Fixes: f619147104c8 ("userfaultfd: add UFFDIO_CONTINUE ioctl") > > > > Signed-off-by: Miaohe Lin > > > > --- > > > > mm/hugetlb.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > This looks correct to me. > > > > > > Reviewed-by: Mike Kravetz > > > > > > However, I am having a hard time wrapping my head around how UFFDIO_CONTINUE > > > should work on non-anon private mappings. For example, a private mapping of > > > a hugetlbfs file. I think we just map the page in the file/cache and do not > > > set the write bit in the pte. So, yes we would want page_dup_file_rmap() > > > in this case as shown below. > > > > > > Adding Axel and Peter on Cc: as they were more involved in adding that code > > > and the design of UFFDIO_CONTINUE. > > > > Yes the change makes sense to me too. There's just one thing to check on > > whether minor mode should support private mappings at all as it's probably > > not in the major goal of when it's proposed. > > > > I don't see why it can't logically, but I think we should have failed the > > uffdio-register already somewhere before when the vma was private and > > registered with minor mode. It's just that I cannot quickly find it in the > > code anywhere.. ideally it should be checked in vma_can_userfault() but it > > seems not. > > > > Axel? > > > > PS: the minor mode man page update seems to be still missing. > > Oh I should have done a pull first on the man-page repo.. > > From the man page indeed I didn't see anything mentioned on not allowing > private mappings. There's the example given on using two mappings for > modifying pages but logically that applies to private mappings too - we > could have mapped the uffd region with private mappings but the other one > shared, then we can modify page caches but later after pte installed it'll > trigger cow for writes. > > So I think we need to confirm whether private mappings are supported. If > no, we should be crystal clear in both the code and man page (we probably > want a follow up patch to man-page to mention that too?). If yes, we'll > need Miaohe's patch and also make sure they're enabled in the current code > path. We'll also want to set test_uffdio_minor=1 for "hugetlb" test case > in the userfaultfd kselftest (currently it's not there). So, originally when I proposed minor fault handling, I was expecting to only support VM_SHARED VMAs. Indeed, I too have a hard time imagining how one might benefit from using it with a private mapping. If my memory serves this restriction was relaxed due to feedback on the original RFC proposal [1], essentially on the basis of: why make it more restrictive than it needs to be? Since all we need for a minor fault to happen is for the pages to be in the page cache, that's the only restriction we should have. I don't see why it shouldn't work in principle though. Imagine a scenario where the VM guest's mapping is private, and the memory manager's mapping is shared. I guess in this case, say for a write from the guest: 1. The guest will generate a minor fault 2. The memory manager can modify the page via its shared mapping, and the guest will see those changes 3. After UFFDIO_CONTINUE resolves the fault, the page is CoW-ed, and the memory manager can no longer see the guest's version of the page I'm not really sure *why* you'd want to do this, but it seems like it should work. [1]: https://patchwork.kernel.org/project/linux-mm/patch/20210107190453.3051110-2-axelrasmussen@google.com/ > > -- > Peter Xu >