Re: [PATCH] mm/memory_hotplug: avoid consuming corrupted data when offline pages

[Mike Kravetz <mike.kravetz@oracle.com>]

On 4/21/22 07:20, David Hildenbrand wrote:
> On 21.04.22 15:51, Miaohe Lin wrote:
>> When trying to offline pages, HWPoisoned hugepage is migrated without
>> checking PageHWPoison first. So corrupted data could be consumed. Fix
>> it by deferring isolate_huge_page until PageHWPoison is handled.
>>
> 
> CCing Oscar, Mike and Naoya
> 
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/memory_hotplug.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 4c6065e5d274..093f85ec5c5c 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1600,11 +1600,9 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>  		folio = page_folio(page);
>>  		head = &folio->page;
>>  
>> -		if (PageHuge(page)) {
>> +		if (PageHuge(page))
>>  			pfn = page_to_pfn(head) + compound_nr(head) - 1;
>> -			isolate_huge_page(head, &source);
>> -			continue;
>> -		} else if (PageTransHuge(page))
>> +		else if (PageTransHuge(page))
>>  			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
>>  
>>  		/*
>> @@ -1622,6 +1620,11 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>  			continue;
>>  		}
>>  
>> +		if (PageHuge(page)) {
>> +			isolate_huge_page(head, &source);
>> +			continue;
>> +		}
>> +
>>  		if (!get_page_unless_zero(page))
>>  			continue;
>>  		/*
> 
> The problem statement makes sense to me but I am not sure about the
> details if we run into the "PageHWPoison" path with a huge page. I have
> the gut feeling that we have to do more for huge pages in the
> PageHWPoison() path, because we might be dealing with a free huge page
> after unmap succeeds. I might be wrong.
> 

Thinking about memory errors always makes my head hurt :)

What 'state' could a poisoned hugetlb page be in here?
- Mapped into a process address space?
- On the hugetlb free lists?

IIUC, when poisoning a hugetlb page we try to dissolve those that are
free and unmap those which are mapped.  So, this means those operations
must have failed for some reason.  Is that correct?

Now, IF the above is true this implies there is a poisoned page somewhere
within the hugetlb page.  But, poison is only marked in the head page.
So, we do not really know where within the page the actual error exists.
Is my reasoning still correct?

If my reasoning is correct, then I am not sure what we can do here.
The code to handle poison is:

                 if (PageHWPoison(page)) {
                        if (WARN_ON(folio_test_lru(folio)))
                                folio_isolate_lru(folio);
                        if (folio_mapped(folio))
                                try_to_unmap(folio, TTU_IGNORE_MLOCK);
                        continue;
                }

My first observation is that if a hugetlb page is passed to try_to_unmap
as above we may BUG.  This is because we need to hold i_mmap_rwsem in
write mode because of the possibility of calling huge_pmd_unshare.  :(

I 'think' try_to_unmap could succeed on a poisoned hugetlb page once we
add correct locking.  So, the page would be unmapped.  I do not think anyone
is holding a ref, so the last unmap should put the hugetlb page on the
free list.  Correct?  We will not migrate the page, but ...

After the call to do_migrate_range() in offline_pages, we will call
dissolve_free_huge_pages.  For each hugetlb page, dissolve_free_huge_pages
will call dissolve_free_huge_page likely passing in the 'head' page.
When dissolve_free_huge_page is called for poisoned hugetlb pages from
the memory error handling code, it passes in the sub page which contains
the memory error.  Before freeing the hugetlb page to buddy, there is this
code:

                        /*
                         * Move PageHWPoison flag from head page to the raw
                         * error page, which makes any subpages rather than
                         * the error page reusable.
                         */
                        if (PageHWPoison(head) && page != head) {
                                SetPageHWPoison(page);
                                ClearPageHWPoison(head);
                        }
                        update_and_free_page(h, head, false)

As previously mentioned, outside the memory error handling code we do
not know what page within the hugetlb page contains poison.  So, this
will likely result with a page with errors on the free list and an OK
page marked with error.

In other places, we 'bail' if we encounter a hugetlb page with poison.
It would be unfortunate to prevent memory offline if the range contains
a hugetlb page with poison.  After all, offlining a section with poison
make sense.
-- 
Mike Kravetz