Re: [PATCH v3 03/26] mm: introduce AS_NO_DIRECT_MAP

On Fri Jul 31, 2026 at 9:28 PM CEST, Yosry Ahmed wrote:
>> > [..]
>> >> --- a/mm/gup.c
>> >> +++ b/mm/gup.c
>> >> @@ -11,7 +11,6 @@
>> >>  #include <linux/rmap.h>
>> >>  #include <linux/swap.h>
>> >>  #include <linux/swapops.h>
>> >> -#include <linux/secretmem.h>
>> >>  
>> >>  #include <linux/sched/signal.h>
>> >>  #include <linux/rwsem.h>
>> >> @@ -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/[email protected]/
>> 	[1] https://lore.kernel.org/all/[email protected]/
>> 
>> > 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?