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

[Yosry Ahmed]

> > [..]
> >> --- 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/20260410151746.61150-1-kalyazin@xxxxxxxxxx/
> 	[1] https://lore.kernel.org/all/akw1lZDEv8_Ub1zQ@xxxxxxxxxx/
> 
> > 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?