Re: [PATCH v3 2/4] mm/gup: clean up follow_pfn_pte() slightly

[Jason Gunthorpe]

On Thu, Feb 03, 2022 at 02:53:52PM +0100, Jan Kara wrote:
> On Thu 03-02-22 01:32:30, John Hubbard wrote:
> > Regardless of any FOLL_* flags, get_user_pages() and its variants should
> > handle PFN-only entries by stopping early, if the caller expected
> > **pages to be filled in.
> > 
> > This makes for a more reliable API, as compared to the previous approach
> > of skipping over such entries (and thus leaving them silently
> > unwritten).
> > 
> > Cc: Peter Xu <peterx@xxxxxxxxxx>
> > Cc: Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx>
> > Suggested-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
> >  mm/gup.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 65575ae3602f..cad3f28492e3 100644
> > +++ b/mm/gup.c
> > @@ -439,10 +439,6 @@ static struct page *no_page_table(struct vm_area_struct *vma,
> >  static int follow_pfn_pte(struct vm_area_struct *vma, unsigned long address,
> >  		pte_t *pte, unsigned int flags)
> >  {
> > -	/* No page to get reference */
> > -	if (flags & (FOLL_GET | FOLL_PIN))
> > -		return -EFAULT;
> > -
> >  	if (flags & FOLL_TOUCH) {
> >  		pte_t entry = *pte;
> >  
> 
> This will also modify the error code returned from follow_page(). 

Er, but isn't that the whole point of this entire design? It is what
the commit that added it says:

commit 1027e4436b6a5c413c95d95e50d0f26348a602ac
Author: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
Date:   Fri Sep 4 15:47:55 2015 -0700

    mm: make GUP handle pfn mapping unless FOLL_GET is requested
    
    With DAX, pfn mapping becoming more common.  The patch adjusts GUP code to
    cover pfn mapping for cases when we don't need struct page to proceed.
    
    To make it possible, let's change follow_page() code to return -EEXIST
    error code if proper page table entry exists, but no corresponding struct
    page.  __get_user_page() would ignore the error code and move to the next
    page frame.
    
    The immediate effect of the change is working MAP_POPULATE and mlock() on
    DAX mappings.

> A quick audit shows that at least the user in mm/migrate.c will
> propagate this error code to userspace and I'm not sure the change
> in error code will not break something... EEXIST is a bit strange
> error code to get from move_pages(2).

That makes sense, maybe move_pages should squash the return codes to
EEXIST?

Jason