lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 29 May 2019 11:44:50 -0700 (PDT)
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Naoya Horiguchi <n-horiguchi@...jp.nec.com>, linux-mm@...ck.org
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...nel.org>,
        xishi.qiuxishi@...baba-inc.com,
        "Chen, Jerry T" <jerry.t.chen@...el.com>,
        "Zhuo, Qiuxu" <qiuxu.zhuo@...el.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] mm: hugetlb: soft-offline: fix wrong return value of
 soft offline

On 5/26/19 11:06 PM, Naoya Horiguchi wrote:
> Soft offline events for hugetlb pages return -EBUSY when page migration
> succeeded and dissolve_free_huge_page() failed, which can happen when
> there're surplus hugepages. We should judge pass/fail of soft offline by
> checking whether the raw error page was finally contained or not (i.e.
> the result of set_hwpoison_free_buddy_page()), so this behavior is wrong.
> 
> This problem was introduced by the following change of commit 6bc9b56433b76
> ("mm: fix race on soft-offlining"):
> 
>                     if (ret > 0)
>                             ret = -EIO;
>             } else {
>     -               if (PageHuge(page))
>     -                       dissolve_free_huge_page(page);
>     +               /*
>     +                * We set PG_hwpoison only when the migration source hugepage
>     +                * was successfully dissolved, because otherwise hwpoisoned
>     +                * hugepage remains on free hugepage list, then userspace will
>     +                * find it as SIGBUS by allocation failure. That's not expected
>     +                * in soft-offlining.
>     +                */
>     +               ret = dissolve_free_huge_page(page);
>     +               if (!ret) {
>     +                       if (set_hwpoison_free_buddy_page(page))
>     +                               num_poisoned_pages_inc();
>     +               }
>             }
>             return ret;
>      }
> 
> , so a simple fix is to restore the PageHuge precheck, but my code
> reading shows that we already have PageHuge check in
> dissolve_free_huge_page() with hugetlb_lock, which is better place to
> check it.  And currently dissolve_free_huge_page() returns -EBUSY for
> !PageHuge but that's simply wrong because that that case should be
> considered as success (meaning that "the given hugetlb was already
> dissolved.")

Hello Naoya,

I am having a little trouble understanding the situation.  The code above is
in the routine soft_offline_huge_page, and occurs immediately after a call to
migrate_pages() with 'page' being the only on the list of pages to be migrated.
In addition, since we are in soft_offline_huge_page, we know that page is
a huge page (PageHuge) before the call to migrate_pages.

IIUC, the issue is that the migrate_pages call results in 'page' being
dissolved into regular base pages.  Therefore, the call to
dissolve_free_huge_page returns -EBUSY and we never end up setting PageHWPoison
on the (base) page which had the error.

It seems that for the original page to be dissolved, it must go through the
free_huge_page routine.  Once that happens, it is possible for the (dissolved)
pages to be allocated again.  Is that just a known race, or am I missing
something?

> This change affects other callers of dissolve_free_huge_page(),
> which are also cleaned up by this patch.

It may just be me, but I am having a hard time separating the fix for this
issue from the change to the dissolve_free_huge_page routine.  Would it be
more clear or possible to create separate patches for these?

-- 
Mike Kravetz

Powered by blists - more mailing lists