Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1690599 > unrolled thread

[PATCH] mm/hugetlb: __get_user_pages ignores certain follow_hugetlb_page errors

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm/hugetlb: __get_user_pages ignores certain follow_hugetlb_page errors daniel.m.jordan@oracle.com - 2017-07-18 21:50 +0200
    Re: [PATCH] mm/hugetlb: __get_user_pages ignores certain follow_hugetlb_page errors Punit Agrawal <punit.agrawal@arm.com> - 2017-07-21 11:30 +0200
      Re: [PATCH] mm/hugetlb: __get_user_pages ignores certain  follow_hugetlb_page errors Daniel Jordan <daniel.m.jordan@oracle.com> - 2017-07-22 02:10 +0200

#1690599 — [PATCH] mm/hugetlb: __get_user_pages ignores certain follow_hugetlb_page errors

Commit 9a291a7c9428 ("mm/hugetlb: report -EHWPOISON not -EFAULT when
FOLL_HWPOISON is specified") causes __get_user_pages to ignore certain
errors from follow_hugetlb_page.  After such error, __get_user_pages
subsequently calls faultin_page on the same VMA and start address that
follow_hugetlb_page failed on instead of returning the error immediately
as it should.

In follow_hugetlb_page, when hugetlb_fault returns a value covered under
VM_FAULT_ERROR, follow_hugetlb_page returns it without setting nr_pages
to 0 as __get_user_pages expects in this case, which causes the
following to happen in __get_user_pages: the "while (nr_pages)" check
succeeds, we skip the "if (!vma..." check because we got a VMA the last
time around, we find no page with follow_page_mask, and we call
faultin_page, which calls hugetlb_fault for the second time.

This issue also slightly changes how __get_user_pages works.  Before, it
only returned error if it had made no progress (i = 0).  But now,
follow_hugetlb_page can clobber "i" with an error code since its new
return path doesn't check for progress.  So if "i" is nonzero before a
failing call to follow_hugetlb_page, that indication of progress is lost
and __get_user_pages can return error even if some pages were
successfully pinned.

To fix this, change follow_hugetlb_page so that it updates nr_pages,
allowing __get_user_pages to fail immediately and restoring the "error
only if no progress" behavior to __get_user_pages.

Tested that __get_user_pages returns when expected on error from
hugetlb_fault in follow_hugetlb_page.

Fixes: 9a291a7c9428 ("mm/hugetlb: report -EHWPOISON not -EFAULT when FOLL_HWPOISON is specified")
Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Cc: Gerald Schaefer <gerald.schaefer@de.ibm.com>
Cc: James Morse <james.morse@arm.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Punit Agrawal <punit.agrawal@arm.com>
Cc: zhong jiang <zhongjiang@huawei.com>
---
 mm/hugetlb.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 3eedb18..cc28993 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4095,6 +4095,7 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	unsigned long vaddr = *position;
 	unsigned long remainder = *nr_pages;
 	struct hstate *h = hstate_vma(vma);
+	int err = -EFAULT;
 
 	while (vaddr < vma->vm_end && remainder) {
 		pte_t *pte;
@@ -4170,11 +4171,7 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
 			}
 			ret = hugetlb_fault(mm, vma, vaddr, fault_flags);
 			if (ret & VM_FAULT_ERROR) {
-				int err = vm_fault_to_errno(ret, flags);
-
-				if (err)
-					return err;
-
+				err = vm_fault_to_errno(ret, flags);
 				remainder = 0;
 				break;
 			}
@@ -4229,7 +4226,7 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	 */
 	*position = vaddr;
 
-	return i ? i : -EFAULT;
+	return i ? i : err;
 }
 
 #ifndef __HAVE_ARCH_FLUSH_HUGETLB_TLB_RANGE
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1693515

Hi Daniel,

daniel.m.jordan@oracle.com writes:

> Commit 9a291a7c9428 ("mm/hugetlb: report -EHWPOISON not -EFAULT when
> FOLL_HWPOISON is specified") causes __get_user_pages to ignore certain
> errors from follow_hugetlb_page.  After such error, __get_user_pages
> subsequently calls faultin_page on the same VMA and start address that
> follow_hugetlb_page failed on instead of returning the error immediately
> as it should.
>
> In follow_hugetlb_page, when hugetlb_fault returns a value covered under
> VM_FAULT_ERROR, follow_hugetlb_page returns it without setting nr_pages
> to 0 as __get_user_pages expects in this case, which causes the
> following to happen in __get_user_pages: the "while (nr_pages)" check
> succeeds, we skip the "if (!vma..." check because we got a VMA the last
> time around, we find no page with follow_page_mask, and we call
> faultin_page, which calls hugetlb_fault for the second time.
>
> This issue also slightly changes how __get_user_pages works.  Before, it
> only returned error if it had made no progress (i = 0).  But now,
> follow_hugetlb_page can clobber "i" with an error code since its new
> return path doesn't check for progress.  So if "i" is nonzero before a
> failing call to follow_hugetlb_page, that indication of progress is lost
> and __get_user_pages can return error even if some pages were
> successfully pinned.
>
> To fix this, change follow_hugetlb_page so that it updates nr_pages,
> allowing __get_user_pages to fail immediately and restoring the "error
> only if no progress" behavior to __get_user_pages.
>
> Tested that __get_user_pages returns when expected on error from
> hugetlb_fault in follow_hugetlb_page.
>
> Fixes: 9a291a7c9428 ("mm/hugetlb: report -EHWPOISON not -EFAULT when FOLL_HWPOISON is specified")
> Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> Cc: Gerald Schaefer <gerald.schaefer@de.ibm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Punit Agrawal <punit.agrawal@arm.com>
> Cc: zhong jiang <zhongjiang@huawei.com>
> ---
>  mm/hugetlb.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 3eedb18..cc28993 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4095,6 +4095,7 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  	unsigned long vaddr = *position;
>  	unsigned long remainder = *nr_pages;
>  	struct hstate *h = hstate_vma(vma);
> +	int err = -EFAULT;
>  
>  	while (vaddr < vma->vm_end && remainder) {
>  		pte_t *pte;
> @@ -4170,11 +4171,7 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  			}
>  			ret = hugetlb_fault(mm, vma, vaddr, fault_flags);
>  			if (ret & VM_FAULT_ERROR) {
> -				int err = vm_fault_to_errno(ret, flags);
> -
> -				if (err)
> -					return err;
> -
> +				err = vm_fault_to_errno(ret, flags);
>  				remainder = 0;
>  				break;
>  			}
> @@ -4229,7 +4226,7 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  	 */
>  	*position = vaddr;
>  
> -	return i ? i : -EFAULT;
> +	return i ? i : err;
>  }
>  
>  #ifndef __HAVE_ARCH_FLUSH_HUGETLB_TLB_RANGE

The change makes sense.

FWIW,

Acked-by: Punit Agrawal <punit.agrawal@arm.com>

I was wondering how you hit the issue. Is there a test case that could
have spotted this earlier?

Thanks,
Punit

[toc] | [prev] | [next] | [standalone]

#1694063 — Re: [PATCH] mm/hugetlb: __get_user_pages ignores certain follow_hugetlb_page errors

Hi Punit,

On 07/21/2017 05:20 AM, Punit Agrawal wrote:
> The change makes sense.
> FWIW,
>
> Acked-by: Punit Agrawal <punit.agrawal@arm.com>

Thanks, I appreciate that.

> I was wondering how you hit the issue. Is there a test case that could
> have spotted this earlier?

This was actually just by inspection.

I checked selftests/vm, but there's nothing in there that would have 
come close to spotting this, so unfortunately there's no existing test case.

Thanks,
Daniel

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web