From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753965AbdGUJUz (ORCPT ); Fri, 21 Jul 2017 05:20:55 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:33668 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753881AbdGUJUx (ORCPT ); Fri, 21 Jul 2017 05:20:53 -0400 From: Punit Agrawal To: daniel.m.jordan@oracle.com Cc: linux-mm@kvack.org, aarcange@redhat.com, akpm@linux-foundation.org, aneesh.kumar@linux.vnet.ibm.com, gerald.schaefer@de.ibm.com, james.morse@arm.com, kirill.shutemov@linux.intel.com, mhocko@suse.com, mike.kravetz@oracle.com, n-horiguchi@ah.jp.nec.com, zhongjiang@huawei.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm/hugetlb: __get_user_pages ignores certain follow_hugetlb_page errors References: <1500406795-58462-1-git-send-email-daniel.m.jordan@oracle.com> Date: Fri, 21 Jul 2017 10:20:50 +0100 In-Reply-To: <1500406795-58462-1-git-send-email-daniel.m.jordan@oracle.com> (daniel m. jordan's message of "Tue, 18 Jul 2017 12:39:55 -0700") Message-ID: <87o9sekux9.fsf@e105922-lin.cambridge.arm.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Cc: Andrea Arcangeli > Cc: Andrew Morton > Cc: "Aneesh Kumar K.V" > Cc: Gerald Schaefer > Cc: James Morse > Cc: "Kirill A. Shutemov" > Cc: Michal Hocko > Cc: Mike Kravetz > Cc: Naoya Horiguchi > Cc: Punit Agrawal > Cc: zhong jiang > --- > 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 I was wondering how you hit the issue. Is there a test case that could have spotted this earlier? Thanks, Punit