From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A043047D470 for ; Fri, 31 Jul 2026 19:28:03 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785526085; cv=none; b=EA2DTB43WzfUX+LZ30SakLJaNeOZkL7HywQLypUJlqDI+9IxQ1kDU9gKuDZp0TatA/QQEhRD6XPBJjoFvtdcrjlSgAx+rjEpBIxJLmiSvaR14myJqdb9VSY0GdYfr8X56wxTyN/+EUiUew646qdwjRkuv2vERHeST+mU08PlEW4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785526085; c=relaxed/simple; bh=TS1oXiTpNPIAwLcL9RfzuBxacuS8t4wUZbmeRSymBYI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=UfizNRn93Jxb8XAzpbE5jm1HzGMiX+SlF6X2tXs2UXy/gbQtic6RzfTyuHRWjK870VxLISzY3dgVU3PuuCav1r30qHjbfdGQs/Rvgx8fsPYbhkp8S7SFfyKzw2MlAvmC5b1G9c5FNg27WU/5BGVgquMt7hfljEpharowIydsWYM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=eTDIwone; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="eTDIwone" Received: by smtp.kernel.org (Postfix) with ESMTPSA id C01641F00AC4; Fri, 31 Jul 2026 19:28:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1785526082; bh=DJcJcfSp8qFT3XBaz1dhInzTEmvps3KjtvVR2urkqlg=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=eTDIwone+FUJc19+BJBx3DJ5lBw74iYtCWJ/DX2Y8Dkd4SlO+H+CSDNFXD8MKl0Ww yzvUNvhH02URDNkaBw2cxcfTKnZ9RGt1gpJ0VedFapUogk8q3AECg0APVudQrlHFMH 47cmer8rNiqkrdtV8UPHNJHVJZdtxZfAyxoJFDYLrNCTrj38Pa3tLCb3YVy/OLpqDO x/vA79EBRHD8JufXVt9loA1C4LWMMsaiA73NylAikqNiY9jkb0LPRyo8sSRaeuz8qQ K2phP+OArfIIpxxPV2+l32a9DC/exLjO6l5LCiLmwDbhSH6AYFeB1pc4lCUxMYq8Gt yN51+GjzCcFpg== Date: Fri, 31 Jul 2026 19:28:00 +0000 From: Yosry Ahmed To: Brendan Jackman Cc: Brendan Jackman , Borislav Petkov , Dave Hansen , Peter Zijlstra , Andrew Morton , David Hildenbrand , Vlastimil Babka , Mike Rapoport , Wei Xu , Johannes Weiner , Zi Yan , Lorenzo Stoakes , linux-mm@kvack.org, linux-kernel@vger.kernel.org, x86@kernel.org, Sumit Garg , Will Deacon , rientjes@google.com, patrick.roy@linux.dev, "Itazuri, Takahiro" , Andy Lutomirski , David Kaplan , Thomas Gleixner , Patrick Bellasi , Reiji Watanabe , Sean Christopherson , Nikita Kalyazin , Ackerley Tng Subject: Re: [PATCH v3 03/26] mm: introduce AS_NO_DIRECT_MAP Message-ID: References: <20260726-page_alloc-unmapped-v3-0-6f5729aa9832@google.com> <20260726-page_alloc-unmapped-v3-3-6f5729aa9832@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: > > [..] > >> --- a/mm/gup.c > >> +++ b/mm/gup.c > >> @@ -11,7 +11,6 @@ > >> #include > >> #include > >> #include > >> -#include > >> > >> #include > >> #include > >> @@ -1216,7 +1215,7 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) > >> if ((gup_flags & FOLL_SPLIT_PMD) && is_vm_hugetlb_page(vma)) > >> return -EOPNOTSUPP; > >> > >> - if (vma_is_secretmem(vma)) > >> + if (vma_has_no_direct_map(vma)) > > > > Same here, and for GUP in general. For example, KVM uses kvm_vcpu_map() > > to map guest memory and access it (e.g. when running nested > > virtualization), which uses GUP under the hood AFAICT. So KVM will want > > GUP to succeed, and probably create an ephemeral mapping as well. > > > > Maybe eventually we will want a GUP flag to handle creating an ephemeral > > mapping, as I imagine multiple GUP users will run into the same issue? > > > > Not sure if this is an ASI-specific issue, or if we also have use cases > > where we need GUP to work guest_memfd pages (with ephemeral mappings). > > Similar to above, this shouldn't affect ASI at all. But outside of ASI, aren't there any cases where the kernel (e.g. KVM) needs to access guest_memfd memory? Maybe Sean or David can help us out here. > >> return -EFAULT; > >> > >> if (write) { > >> @@ -2731,7 +2730,7 @@ EXPORT_SYMBOL(get_user_pages_unlocked); > >> * This call assumes the caller has pinned the folio, that the lowest page table > >> * level still points to this folio, and that interrupts have been disabled. > >> * > >> - * GUP-fast must reject all secretmem folios. > >> + * GUP-fast must reject all folios without direct map entries (such as secretmem). > >> * > >> * Writing to pinned file-backed dirty tracked folios is inherently problematic > >> * (see comment describing the writable_file_mapping_allowed() function). We > >> @@ -2769,7 +2768,7 @@ static bool gup_fast_folio_allowed(struct folio *folio, unsigned int flags) > >> if (WARN_ON_ONCE(folio_test_slab(folio))) > >> return false; > >> > >> - /* hugetlb neither requires dirty-tracking nor can be secretmem. */ > >> + /* hugetlb neither requires dirty-tracking nor can be without direct map. */ Is this necessarily true? I know there were discussions/proposals about using some of the hugetlb infrastructure for guest_memfd. I am not sure if those folios would remain hugetlb folios though. Adding Ackerley here. > >> if (folio_test_hugetlb(folio)) > >> return true; > >> > >> @@ -2812,7 +2811,7 @@ static bool gup_fast_folio_allowed(struct folio *folio, unsigned int flags) > >> * At this point, we know the mapping is non-null and points to an > >> * address_space object. > >> */ > >> - if (check_secretmem && secretmem_mapping(mapping)) > >> + if (mapping_no_direct_map(mapping)) > >> return false; > >> /* The only remaining allowed file system is shmem. */ > >> return !reject_file_backed || shmem_mapping(mapping); > >> diff --git a/mm/mlock.c b/mm/mlock.c > >> index efa6716e4dfbd..045b6779440b1 100644 > >> --- a/mm/mlock.c > >> +++ b/mm/mlock.c > >> @@ -474,7 +474,7 @@ static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma, > >> int ret = 0; > >> > >> if (vma_flags_same_pair(&old_vma_flags, new_vma_flags) || > >> - vma_is_secretmem(vma) || !vma_supports_mlock(vma)) { > >> + vma_has_no_direct_map(vma) || !vma_supports_mlock(vma)) { > > > > I don't think this one is correct. From commit 1507f51255c9 ("mm: > > introduce memfd_secret system call to create "secret" memory areas"): > > > > Since the secretmem mappings are locked in memory they cannot exceed > > RLIMIT_MEMLOCK. Since these mappings are already locked independently > > from mlock(), an attempt to mlock()/munlock() secretmem range would > > fail and mlockall()/munlockall() will ignore secretmem mappings. > > > > Seems like secretmem pages are just mlock()'d by default, hence the > > check here. Maybe this also works for guest_memfd, but I don't think > > it's a generalization that any pages without a direct mapping should > > receive the same treatment here. > > Ack, yeah this sounds correct to me. > > I guess you could argue something like "the reason secretmem is > implicitly mlocked is that it can't be reclaimed, because there's no > direct map". But that doesn't generalise IMO, you could imagine letting > the user say "remove this memory from the direct map, but I trust my > swap system, you can swap it" and then use the mermap to implement > reclaim. Exactly, I don't think no direct mapping implicitly means unreclaimable. I don't think you actually need a direct mapping to read/write from disk to memory? > > > I am aware that perhaps the answer for most these cases is that it works > > for guest_memfd as well as secretmem, but since the main goal of the > > series is setting up ASI, > > [Aside] > Well, the main reason for Google to pay me for it is as a > stepping stone for ASI, but I do actually think > GUEST_MEMFD_FLAG_NO_DIRECT_MAP[0] is valuable and prefer to > think of that as the "main goal" of this patchset. (I didn't > include it here since Sean asked[1] for the KVM bits to be > separate, but it's basically just a repeat of the secretmem.c > changes). Right, I understand this is the goal of the series and it is valuable without ASI. Perhaps "motivation" was the correct word :) > > [0] https://lore.kernel.org/all/20260410151746.61150-1-kalyazin@amazon.com/ > [1] https://lore.kernel.org/all/akw1lZDEv8_Ub1zQ@google.com/ > > > ideally we don't want checks that we know > > will become wrong when ASI is introduced. If the idea is that > > mapping_no_direct_map() and vma_has_no_direct_map() will not be used for > > ASI sensitive mappings, > > (I said this above but just to be clear: that is not the idea). > > > we should document this somewhere, or have > > better localized checks (if at all possible) so that we can side-step > > the whole mixup when ASI mappings come along. > > BUT yes I still totally agree that we should not unnecessarily overload > vma_has_no_direct_map() here. Right, that was essentially what I meant. We shouldn't just current secretmem checks with no direct map checks without thinking them through. > > >> /* > >> * Don't set VMA_LOCKED_BIT or VMA_LOCKONFAULT_BIT and don't > >> * count. For secretmem, don't allow the memory to be unlocked. > >> diff --git a/mm/secretmem.c b/mm/secretmem.c > >> index 4a4934769f8ba..c043c53687d95 100644 > >> --- a/mm/secretmem.c > >> +++ b/mm/secretmem.c > >> @@ -52,49 +52,20 @@ static vm_fault_t secretmem_fault(struct vm_fault *vmf) > >> struct address_space *mapping = vmf->vma->vm_file->f_mapping; > >> struct inode *inode = file_inode(vmf->vma->vm_file); > >> pgoff_t offset = vmf->pgoff; > >> - gfp_t gfp = vmf->gfp_mask; > >> struct folio *folio; > >> vm_fault_t ret; > >> - int err; > >> > >> if (((loff_t)vmf->pgoff << PAGE_SHIFT) >= i_size_read(inode)) > >> return vmf_error(-EINVAL); > >> > >> filemap_invalidate_lock_shared(mapping); > >> > >> -retry: > >> - folio = filemap_lock_folio(mapping, offset); > >> + folio = filemap_grab_folio(mapping, offset); > >> if (IS_ERR(folio)) { > >> - folio = folio_alloc(gfp | __GFP_ZERO, 0); > >> - if (!folio) { > >> - ret = VM_FAULT_OOM; > >> - goto out; > >> - } > >> - > >> - err = folio_zap_direct_map(folio); > >> - if (err) { > >> - folio_put(folio); > >> - ret = vmf_error(err); > >> - goto out; > >> - } > >> - > >> - __folio_mark_uptodate(folio); > >> - err = filemap_add_folio(mapping, folio, offset, gfp); > >> - if (unlikely(err)) { > >> - /* > >> - * If a split of large page was required, it > >> - * already happened when we marked the page invalid > >> - * which guarantees that this call won't fail > >> - */ > >> - folio_restore_direct_map(folio); > >> - folio_put(folio); > >> - if (err == -EEXIST) > >> - goto retry; > >> - > >> - ret = vmf_error(err); > >> - goto out; > >> - } > >> + ret = vmf_error(PTR_ERR(folio)); > >> + goto out; > >> } > >> + folio_mark_uptodate(folio); > > > > This chunk seems like pure refactoring that should be done separately? > > This is the adoption of AS_NO_DIRECT_MAP, i.e. the removal of the > explicit folio_zap_direct_map() call. We could certainly separate out > "create AS_NO_DIRECT_MAP" from "adopt it in secretmem" but the previous > version of this patchset was on v12 and it hadn't split them so I assume > nobody was calling for this split. Oh sorry I wasn't clear. I meant switching from folio_lock_folio() and the rest of the logic to folio_grab_folio(). There are some subtle differences AFAICT so this conversion shouldn't really be part of this patch. I think maybe just drop the folio_zap_direct_map()/folio_restore_direct_map() calls for this patch?