lkml.org 
[lkml]   [2021]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] mm: hugetlb: fix a race between memory-failure/soft_offline and gather_surplus_pages
    Date
    On Thu, Apr 22, 2021 at 08:27:46AM +0000, HORIGUCHI NAOYA(堀口 直也) wrote:
    > On Wed, Apr 21, 2021 at 11:03:24AM -0700, Mike Kravetz wrote:
    > > On 4/21/21 1:33 AM, HORIGUCHI NAOYA(堀口 直也) wrote:
    > > > On Wed, Apr 21, 2021 at 10:03:34AM +0200, Michal Hocko wrote:
    > > >> [Cc Naoya]
    > > >>
    > > >> On Wed 21-04-21 14:02:59, Muchun Song wrote:
    > > >>> The possible bad scenario:
    > > >>>
    > > >>> CPU0: CPU1:
    > > >>>
    > > >>> gather_surplus_pages()
    > > >>> page = alloc_surplus_huge_page()
    > > >>> memory_failure_hugetlb()
    > > >>> get_hwpoison_page(page)
    > > >>> __get_hwpoison_page(page)
    > > >>> get_page_unless_zero(page)
    > > >>> zero = put_page_testzero(page)
    > > >>> VM_BUG_ON_PAGE(!zero, page)
    > > >>> enqueue_huge_page(h, page)
    > > >>> put_page(page)
    > > >>>
    > > >>> The refcount can possibly be increased by memory-failure or soft_offline
    > > >>> handlers, we can trigger VM_BUG_ON_PAGE and wrongly add the page to the
    > > >>> hugetlb pool list.
    > > >>
    > > >> The hwpoison side of this looks really suspicious to me. It shouldn't
    > > >> really touch the reference count of hugetlb pages without being very
    > > >> careful (and having hugetlb_lock held).
    > > >
    > > > I have the same feeling, there is a window where a hugepage is refcounted
    > > > during converting from buddy free pages into free hugepage, so refcount
    > > > alone is not enough to prevent the race. hugetlb_lock is retaken after
    > > > alloc_surplus_huge_page returns, so simply holding hugetlb_lock in
    > > > get_hwpoison_page() seems not work. Is there any status bit to show that a
    > > > hugepage is just being initialized (not in free hugepage pool or in use)?
    > > >
    > >
    > > It seems we can also race with the code that makes a compound page a
    > > hugetlb page. The memory failure code could be called after allocating
    > > pages from buddy and before setting compound page DTOR. So, the memory
    > > handling code will process it as a compound page.
    >
    > Yes, so get_hwpoison_page() has to call get_page_unless_zero()
    > only when memory_failure() can surely handle the error.
    >
    > >
    > > Just thinking that this may not be limited to the hugetlb specific memory
    > > failure handling?
    >
    > Currently hugetlb page is the only type of compound page supported by memory
    > failure. But I agree with you that other types of compound pages have the
    > same race window, and judging only with get_page_unless_zero() is dangerous.
    > So I think that __get_hwpoison_page() should have the following structure:
    >
    > if (PageCompound) {
    > if (PageHuge) {
    > if (PageHugeFreed || PageHugeActive) {
    > if (get_page_unless_zero)
    > return 0; // path for in-use hugetlb page
    > else
    > return 1; // path for free hugetlb page
    > } else {
    > return -EBUSY; // any transient hugetlb page
    > }
    > } else {
    > ... // any other compound page (like thp, slab, ...)
    > }
    > } else {
    > ... // any non-compound page
    > }

    The above pseudo code was wrong, so let me update my thought.
    I'm now trying to solve the reported issue by changing __get_hwpoison_page()
    like below:

    static int __get_hwpoison_page(struct page *page)
    {
    struct page *head = compound_head(page);

    if (PageCompound(page)) {
    if (PageSlab(page)) {
    return get_page_unless_zero(page);
    } else if (PageHuge(head)) {
    if (HPageFreed(head) || HPageMigratable(head))
    return get_page_unless_zero(head);
    } else if (PageTransHuge(head)) {
    /*
    * Non anonymous thp exists only in allocation/free time. We
    * can't handle such a case correctly, so let's give it up.
    * This should be better than triggering BUG_ON when kernel
    * tries to touch the "partially handled" page.
    */
    if (!PageAnon(head)) {
    pr_err("Memory failure: %#lx: non anonymous thp\n",
    page_to_pfn(page));
    return 0;
    }
    if (get_page_unless_zero(head)) {
    if (head == compound_head(page))
    return 1;
    pr_info("Memory failure: %#lx cannot catch tail\n",
    page_to_pfn(page));
    put_page(head);
    }
    }
    return 0;
    }

    return get_page_unless_zero(page);
    }

    Some notes:

    - in hugetlb path, new HPage* checks should avoid the reported race,
    but I still need more testing to confirm it,
    - PageSlab check is added because otherwise I found that "non anonymous thp"
    path is chosen, that's obviously wrong,
    - thp's branch has a known issue unrelated to the current issue, which
    will/should be improved later.

    I'll send a patch next week.

    Thanks,
    Naoya Horiguchi
    \
     
     \ /
      Last update: 2021-04-23 10:03    [W:8.886 / U:0.360 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site