Re: [PATCH v3 1/4] mm: Fix invalid page pointer returned with FOLL_PIN gups

[John Hubbard]

On 2/3/22 04:10, Claudio Imbrenda wrote:
> On Thu, 3 Feb 2022 01:32:29 -0800
> John Hubbard <jhubbard@xxxxxxxxxx> wrote:
>
> > From: Peter Xu <peterx@xxxxxxxxxx>
> >
> > Alex reported invalid page pointer returned with pin_user_pages_remote() from
> > vfio after upstream commit 4b6c33b32296 ("vfio/type1: Prepare for batched
> > pinning with struct vfio_batch").  This problem breaks NVIDIA vfio mdev.
> >
> > It turns out that it's not the fault of the vfio commit; however after vfio
> > switches to a full page buffer to store the page pointers it starts to expose
> > the problem easier.
> >
> > The problem is for VM_PFNMAP vmas we should normally fail with an -EFAULT then
> > vfio will carry on to handle the MMIO regions.  However when the bug triggered,
> > follow_page_mask() returned -EEXIST for such a page, which will jump over the
> > current page, leaving that entry in **pages untouched.  However the caller is
> > not aware of it, hence the caller will reference the page as usual even if the
> > pointer data can be anything.
> >
> > We had that -EEXIST logic since commit 1027e4436b6a ("mm: make GUP handle pfn
> > mapping unless FOLL_GET is requested") which seems very reasonable.  It could
> > be that when we reworked GUP with FOLL_PIN we could have overlooked that
> > special path in commit 3faa52c03f44 ("mm/gup: track FOLL_PIN pages"), even if
> > that commit rightfully touched up follow_devmap_pud() on checking FOLL_PIN when
> > it needs to return an -EEXIST.
> >
> > Attaching the Fixes to the FOLL_PIN rework commit, as it happened later than
> > 1027e4436b6a.
> >
> > Cc: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Jan Kara <jack@xxxxxxx>
> > Cc: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
> > Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>
> > Reported-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Debugged-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Tested-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> > Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
>
> you can add
>
> Reviewed-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>

Thanks!

> although maybe this would look better if it were squashed into the next
> patch, as others have also suggested

I was thinking about that. It seems like this patch here cleanly addresses
an oversight, and it is tiny and reasonably suitable for backporting.

Patch 2, on the other hand is less of a fix, and more of a "let's improve
things". And now it is expanding now to cover move_pages() too. So maybe it
is better to leave them separate, after all.


thanks,
--
John Hubbard
NVIDIA

> > ---
> >   mm/gup.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index f0af462ac1e2..65575ae3602f 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -440,7 +440,7 @@ 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)
> > +	if (flags & (FOLL_GET | FOLL_PIN))
> >   		return -EFAULT;
> >   
> >   	if (flags & FOLL_TOUCH) {