| Message ID | 20220203093232.572380-2-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/gup: some cleanups | expand |
On Thu, 3 Feb 2022 01:32:29 -0800 John Hubbard <jhubbard@nvidia.com> wrote: > From: Peter Xu <peterx@redhat.com> > > 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@redhat.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Jan Kara <jack@suse.cz> > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages") > Reviewed-by: John Hubbard <jhubbard@nvidia.com> > Reported-by: Alex Williamson <alex.williamson@redhat.com> > Debugged-by: Alex Williamson <alex.williamson@redhat.com> > Tested-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > Signed-off-by: John Hubbard <jhubbard@nvidia.com> you can add Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> although maybe this would look better if it were squashed into the next patch, as others have also suggested > --- > 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) {
On Thu, Feb 03, 2022 at 01:32:29AM -0800, John Hubbard wrote: > From: Peter Xu <peterx@redhat.com> > > 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. There still isn't any nvidia vfio mdev driver in the tree, so this changelog stilldoesn't make sense.
On 2/3/22 06:00, Christoph Hellwig wrote: > On Thu, Feb 03, 2022 at 01:32:29AM -0800, John Hubbard wrote: >> From: Peter Xu <peterx@redhat.com> >> >> 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. > > There still isn't any nvidia vfio mdev driver in the tree, so this > changelog stilldoesn't make sense. I'll remove that last sentence (and put in a tiny note that I'm scribbling on Peter's commit description a little bit more), in order to avoid any references to out of tree things. thanks,
On 2/3/22 04:10, Claudio Imbrenda wrote: > On Thu, 3 Feb 2022 01:32:29 -0800 > John Hubbard <jhubbard@nvidia.com> wrote: > >> From: Peter Xu <peterx@redhat.com> >> >> 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@redhat.com> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: Jan Kara <jack@suse.cz> >> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >> Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages") >> Reviewed-by: John Hubbard <jhubbard@nvidia.com> >> Reported-by: Alex Williamson <alex.williamson@redhat.com> >> Debugged-by: Alex Williamson <alex.williamson@redhat.com> >> Tested-by: Alex Williamson <alex.williamson@redhat.com> >> Signed-off-by: Peter Xu <peterx@redhat.com> >> Signed-off-by: John Hubbard <jhubbard@nvidia.com> > > you can add > > Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > 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,
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) {