From: Peter Xu <[email protected]>
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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Kirill A. Shutemov <[email protected]>
Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
Reviewed-by: John Hubbard <[email protected]>
Reported-by: Alex Williamson <[email protected]>
Debugged-by: Alex Williamson <[email protected]>
Tested-by: Alex Williamson <[email protected]>
Signed-off-by: Peter Xu <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
---
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) {
--
2.35.1
On Thu, 3 Feb 2022 01:32:29 -0800
John Hubbard <[email protected]> wrote:
> From: Peter Xu <[email protected]>
>
> 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 <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Jan Kara <[email protected]>
> Cc: Kirill A. Shutemov <[email protected]>
> Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
> Reviewed-by: John Hubbard <[email protected]>
> Reported-by: Alex Williamson <[email protected]>
> Debugged-by: Alex Williamson <[email protected]>
> Tested-by: Alex Williamson <[email protected]>
> Signed-off-by: Peter Xu <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
you can add
Reviewed-by: Claudio Imbrenda <[email protected]>
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 2/3/22 06:00, Christoph Hellwig wrote:
> On Thu, Feb 03, 2022 at 01:32:29AM -0800, John Hubbard wrote:
>> From: Peter Xu <[email protected]>
>>
>> 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,
--
John Hubbard
NVIDIA
On 2/3/22 04:10, Claudio Imbrenda wrote:
> On Thu, 3 Feb 2022 01:32:29 -0800
> John Hubbard <[email protected]> wrote:
>
>> From: Peter Xu <[email protected]>
>>
>> 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 <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: Jan Kara <[email protected]>
>> Cc: Kirill A. Shutemov <[email protected]>
>> Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
>> Reviewed-by: John Hubbard <[email protected]>
>> Reported-by: Alex Williamson <[email protected]>
>> Debugged-by: Alex Williamson <[email protected]>
>> Tested-by: Alex Williamson <[email protected]>
>> Signed-off-by: Peter Xu <[email protected]>
>> Signed-off-by: John Hubbard <[email protected]>
>
> you can add
>
> Reviewed-by: Claudio Imbrenda <[email protected]>
>
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) {
>
>
On Thu, Feb 03, 2022 at 01:32:29AM -0800, John Hubbard wrote:
> From: Peter Xu <[email protected]>
>
> 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.