Hi Daniel,
daniel.m.jor...@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.jor...@oracle.com>
> Cc: Andrea Arcangeli <aarca...@redhat.com>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: "Aneesh Kumar K.V" <aneesh.ku...@linux.vnet.ibm.com>
> Cc: Gerald Schaefer <gerald.schae...@de.ibm.com>
> Cc: James Morse <james.mo...@arm.com>
> Cc: "Kirill A. Shutemov" <kirill.shute...@linux.intel.com>
> Cc: Michal Hocko <mho...@suse.com>
> Cc: Mike Kravetz <mike.krav...@oracle.com>
> Cc: Naoya Horiguchi <n-horigu...@ah.jp.nec.com>
> Cc: Punit Agrawal <punit.agra...@arm.com>
> Cc: zhong jiang <zhongji...@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.agra...@arm.com>
I was wondering how you hit the issue. Is there a test case that could
have spotted this earlier?
Thanks,
Punit
