linux-kernel - Re: [PATCH v1] mm: hugetlb: soft-offline: fix wrong return value of soft offline

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 linux-hardening linux-cve-announce 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