Re: [PATCH RFC v3 4/6] mm/gup: add get_user_page_vma() to fault in a page under a held lock

[Rik van Riel <riel@surriel.com>]

On Mon, 2026-07-20 at 05:35 -0700, Usama Arif wrote:
> On Fri, 17 Jul 2026 13:00:34 -0400 Rik van Riel <riel@surriel.com>
> wrote:
> 
> > 
> > +	/*
> > +	 * Validate the VMA up front, like __get_user_pages(). A
> > VM_IO/VM_PFNMAP
> > +	 * VMA is not rejected outright: it can hold COWed pages
> > that have a
> > +	 * struct page, so let follow_page_mask() look for one,
> > and treat only
> > +	 * its struct-page-less PFNs as unreachable. Any other
> > rejection
> > +	 * (secretmem, bad permissions, ...) is final.
> > +	 */
> > +	ret = check_vma_flags(vma, gup_flags);
> > +	if (ret && !(vma->vm_flags & (VM_IO | VM_PFNMAP)))
> 
> Do you need to somehow restructure check_vma_flags()?
> 
> The first thing that check_vma_flags() does is:
> 
> 	if (vm_flags & (VM_IO | VM_PFNMAP))
> 		return -EFAULT;
> 
> check_vma_flags() returns before evaluating VM_READ,
> FOLL_FORCE, FOLL_ANON or the architecture permission check.
> 
> The code then ignores that early error and may return a COWed
> page through follow_page_mask(). A non-FOLL_FORCE caller
> can therefore read a COWed page from a PROT_NONE PFNMAP VMA.

In this code path, if we return an error to
__access_remote_vm(), we end up falling back
to the ->access() hook, and will end up reading
the memory that's "under" that COW, instead of
the COWed page that is currently in the process
we are accessing.

In other words, I think the behavior of this
code is the way we want, even though the
implementation should be cleaned up.

How about we change check_vma_flags() so it
ignores specified flags?

check_vma_flags(vma, gup_flags, ignore_flags)

Then this one call to check_vma_flags can have
it ignore VM_IO | VM_PFNMAP, since we want to
access the COWed pages in those VMAs in this
code path, but fall back to ->access when
there are no COW pages.

> 
> > +		goto fail;
> > +	pfnmap = ret;
> > +
> > +	for (;;) {
> > +		if (fatal_signal_pending(current)) {
> > +			ret = -EINTR;
> > +			goto fail;
> > +		}
> > +		cond_resched();
> > +
> > +		page = follow_page_mask(vma, addr,
> > +					gup_flags | FOLL_TOUCH |
> > FOLL_GET,
> > +					&page_mask);
> > +		if (!IS_ERR_OR_NULL(page))
> > +			return page;
> 
> __get_user_pages() does
> 
> 	flush_anon_page(vma, subpage, start + j * PAGE_SIZE);
> 	flush_dcache_page(subpage);
> 
> before returning the page. Do you need to that here as well above?

I suppose on systems with virtually indexed caches
we do need that.

Thanks for spotting that!

Now I also wonder if I could restructure get_user_page_vma_remote()
to do the lookup before the get_user_pages_remote(), and have
it then use the new get_user_page_vma() function, to avoid
the double VMA lookup (why are we doing that, anyway?)
> 

-- 
All Rights Reversed.