Re: [RFC] a question about reuse hwpoison page in soft_offline_page()

[Xie XiuQi <xiexiuqi@huawei.com>]

Hi Naoya, Xishi,

We have a similar problem.
@zhangfei, could you please describe your problem here.

On 2018/7/6 16:18, Naoya Horiguchi wrote:
> On Fri, Jul 06, 2018 at 11:37:41AM +0800, 裘稀石(稀石) wrote:
>> This patch add05cec
>> (mm: soft-offline: don't free target page in successful page migration) removes
>> set_migratetype_isolate() and unset_migratetype_isolate() in soft_offline_page
>> ().
>>
>> And this patch 243abd5b
>> (mm: hugetlb: prevent reuse of hwpoisoned free hugepages) changes
>> if (!is_migrate_isolate_page(page)) to if (!PageHWPoison(page)), so it could
>> prevent someone
>> reuse the free hugetlb again after set the hwpoison flag
>> in soft_offline_free_page()
>>
>> My question is that if someone reuse the free hugetlb again before 
>> soft_offline_free_page() and
>> after get_any_page(), then it uses the hopoison page, and this may trigger mce
>> kill later, right?
> 
> Hi Xishi,
> 
> Thank you for pointing out the issue. That's nice catch.
> 
> I think that the race condition itself could happen, but it doesn't lead
> to MCE kill because PageHWPoison is not visible to HW which triggers MCE.
> PageHWPoison flag is just a flag in struct page to report the memory error
> from kernel to userspace. So even if a CPU is accessing to the page whose
> struct page has PageHWPoison set, that doesn't cause a MCE unless the page
> is physically broken.
> The type of memory error that soft offline tries to handle is corrected
> one which is not a failure yet although it's starting to wear.
> So such PageHWPoison page can be reused, but that's not critical because
> the page is freed at some point afterword and error containment completes.
> 
> However, I noticed that there's a small pain in free hugetlb case.
> We call dissolve_free_huge_page() in soft_offline_free_page() which moves
> the PageHWPoison flag from the head page to the raw error page.
> If the reported race happens, dissolve_free_huge_page() just return without
> doing any dissolve work because "if (PageHuge(page) && !page_count(page))"
> block is skipped.
> The hugepage is allocated and used as usual, but the contaiment doesn't
> complete as expected in the normal page, because free_huge_pages() doesn't
> call dissolve_free_huge_page() for hwpoison hugepage. This is not critical
> because such error hugepage just reside in free hugepage list. But this
> might looks like a kind of memory leak. And even worse when hugepage pool
> is shrinked and the hwpoison hugepage is freed, the PageHWPoison flag is
> still on the head page which is unlikely to be an actual error page.
> 
> So I think we need improvement here, how about the fix like below?
> 
>   (not tested yet, sorry)
> 
>   diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>   --- a/mm/memory-failure.c
>   +++ b/mm/memory-failure.c
>   @@ -1883,6 +1883,11 @@ static void soft_offline_free_page(struct page *page)
>           struct page *head = compound_head(page);
>   
>           if (!TestSetPageHWPoison(head)) {
>   +               if (page_count(head)) {
>   +                       ClearPageHWPoison(head);
>   +                       return;
>   +               }
>   +
>                   num_poisoned_pages_inc();
>                   if (PageHuge(head))
>                           dissolve_free_huge_page(page);
> 
> Thanks,
> Naoya Horiguchi
> 
> .
> 

-- 
Thanks,
Xie XiuQi