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

[HORIGUCHI NAOYA(堀口 直也)]

On Fri, Apr 22, 2022 at 02:53:48PM +0800, Miaohe Lin wrote:
> On 2022/4/22 6:04, Mike Kravetz wrote:
> > 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@xxxxxxxxxx>
> >>> ---
> >>>  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 :)
> 
> Me too. ;)
> 
> > 
> > What 'state' could a poisoned hugetlb page be in here?
> > - Mapped into a process address space?
> > - On the hugetlb free lists?

There seems at least 2 cases where a hwpoisoned hugetlb page keeps undissolved.
  (1) Hwpoisoned hugepage in free hugepage list
  (2) Hwpoisoned hugepage belonging to a hugetlbfs file

> > 
> > 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?

Yes, (1) is made by failure in error handling, but (2) can be made even
when memory error on in-use hugepage is successfully handled.

> 
> I think you're right.
> 
> BTW: Now that PageHWPoison is set under hugetlb_lock and HPageFreed is also checked
> under that lock, it should be more likely to handle the hugetlb page on the free lists
> successfully because the possible race with hugetlb page allocation or demotion can be
> prevented by PageHWPoison flag. So it is more likely to be a mapped hugetlb page here.
> But that doesn't matter.
> 
> > 
> > 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?
> > 
> 
> IIRC, Naoya once proposed to store the poisoned page info into subpage field.

Yes, it's important to keep the offset of raw page.  I have a "storing raw
error info" patch for this, but it's still immature to submit.

> 
> > 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'm sorry. I missed that case. :(

OK, so the above check should not be called for hugetlb pages.

> 
> > 
> > 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 ...

That seems depends on whether the hugepage is anonymous or file.
If we consider anonymous hugepage, the above statement is correct.

As for what we can do, for (1) we can simply skip the hwpoisoned hugepage
in do_migrate_range() (i.e. no need to call isolate_huge_page()), because in
the outer while-loop in offline_pages() we call dissolve_free_huge_pages()
so free hugepage can be handled here.  The above "storing raw error info"
patch will also enable dissolve_free_huge_pages() to handle hwpoisoned free
hugepage (by moving PG_hwpoison flag to the raw error page), so I assume
to have the patch.

As for (2), I think that making such hugepages unmovable (by clearing
HPageMigratable) could work.  With that change, scan_movable_pages() detects
unmovable pages so do_migrate_range is not called for the hugepages.
Moreover, scan_movable_pages() should return -EBUSY if an unmovable page is found,
so offline_pages() fails without trying to touch the hwpoisoned hugepage.
Anyway the reported problem (try to migrate hwpoisoned hugepage) would to be solved.

# Maybe we might optionally try to free the hwpoisoned hugepage by removing
# the affected hugetlbfs file, but it's not unclear to me taht that's acceptable
# (a file is gone when doing memory hotremove?).

> 
> Might hugetlb page still be in the pagecache? But yes, the last unmap could put
> the hugetlb page on the free list.
> 
> > 
> > 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.
> 
> Yes, this is possible. It's really a pity. Could we just reject to dissolve the
> hugetlb page and keep the hugetlb page around? This will waste some healthy subpages
> but can do the right things?

Could you let me try writing patches in my approach?
I'll try to submit the fix with the "storing raw error info" patch.

> 
> > 
> > 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.
> > 
> 
> IIUC, a (hugetlb) page with poison could be offlined anytime even if it's still in the
> pagecache, swapcache, i.e. still be referenced by somewhere, because memory offline will
> just offline the poisoned page while ignoring the page refcnt totally. I can't figure
> out a easy way to fix it yet. :(

Yes, that's finally expected behavior, but unlike normal pagecache we need
pay extra attention for hugetlbfs file because it's not storage-backed.

Thank you for valuable comments, everyone.

- Naoya Horiguchi