Re: [PATCH v2] mm/hwpoison: set PageHWPoison after taking page lock in memory_failure_hugetlb()

[HORIGUCHI NAOYA(堀口 直也)]

On Tue, Mar 15, 2022 at 05:33:43PM -0700, Mike Kravetz wrote:
> On 3/15/22 07:00, Miaohe Lin wrote:
> > On 2022/3/15 13:49, HORIGUCHI NAOYA(堀口 直也) wrote:
> >> On Mon, Mar 14, 2022 at 03:10:25PM +0800, Miaohe Lin wrote:
> >>> On 2022/3/14 10:13, Naoya Horiguchi wrote:
> >>>> From: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>
> >>>>
> >>>> There is a race condition between memory_failure_hugetlb() and hugetlb
> >>>> free/demotion, which causes setting PageHWPoison flag on the wrong page
> >>>> (which was a hugetlb when memory_failure() was called, but was removed
> >>>> or demoted when memory_failure_hugetlb() is called).  This results in
> >>>> killing wrong processes.  So set PageHWPoison flag with holding page lock,
> >>>
> >>> It seems hold page lock could not help solve this race condition as hugetlb
> >>> page demotion is not required to hold the page lock. Could you please explain
> >>> this a bit more?
> >>
> >> Sorry, the last line in the paragraph need change. What prevents the current
> >> race is hugetlb_lock, not page lock.  The page lock is here to prevent the
> >> race with hugepage allocation (not directly related to the current issue,
> >> but it's still necessary).
> > 
> > Many thanks for clarifying this.
> > 
> >>
> >>>
> >>> BTW:Is there some words missing or here should be 'page lock.' instead of 'page lock,' ?
> >>
> >> I should use a period here, I'll fix it.
> >>
> >> [...]
> >>
> >>>> @@ -1503,24 +1502,11 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
> >>>>  	int res;
> >>>>  	unsigned long page_flags;
> >>>>  
> >>>> -	if (TestSetPageHWPoison(head)) {
> >>>> -		pr_err("Memory failure: %#lx: already hardware poisoned\n",
> >>>> -		       pfn);
> >>>> -		res = -EHWPOISON;
> >>>> -		if (flags & MF_ACTION_REQUIRED)
> >>>> -			res = kill_accessing_process(current, page_to_pfn(head), flags);
> >>>> -		return res;
> >>>> -	}
> >>>> -
> >>>> -	num_poisoned_pages_inc();
> >>>> -
> >>>>  	if (!(flags & MF_COUNT_INCREASED)) {
> >>>>  		res = get_hwpoison_page(p, flags);
> >>>>  		if (!res) {
> >>>
> >>> In this (res == 0) case, hugetlb page could be dissolved via __page_handle_poison.
> >>> But since PageHWPoison is not set yet, we can't set the PageHWPoison to the correct
> >>> page. Think about the below code in dissolve_free_huge_page:
> >>> 	/*
> >>> 	 * 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);
> >>> 	}
> >>>
> >>> SetPageHWPoison won't be called for the error page. Or am I miss something?
> >>
> >> No, you're right.  We need call page_handle_poison() instead of
> >> __page_handle_poison().
> >>
> >> @@ -1512,7 +1512,7 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
> >>  			}
> >>  			unlock_page(head);
> >>  			res = MF_FAILED;
> >> -			if (__page_handle_poison(p)) {
> >> +			if (page_handle_poison(p, true, false)) {
> >>  				page_ref_inc(p);
> >>  				res = MF_RECOVERED;
> >>  			}
> >>
> > 
> > This one looks good to me.
> 
> I must be missing something.  It seems page_handle_poison() calls
> __page_handle_poison and thus dissolve_free_huge_page before
> SetPageHWPoison.
> 
> I could easily be missing some patches, but that is the order of calls
> in the code I am looking at.

Sorry for my lack of words, maybe the situation is a little complicated.
page_handle_poison() can be called both before and after SetPageHWPoison.
"before SetPageHWPoison" case is when called from memory_failure() for
in-use pages. "after SetPageHWPoison" case is when called from soft offline.
So the above change is intended to move "memory_failure() for free hugepage"
from "before SetPageHWPoison" case to "after SetPageHWPoison" case.

BTW, I found myself the issue in the above diff, I have to remove the
page_ref_inc() in it because page_handle_poison() calls it inside it.
I'll share the updated patch soon.

Thanks,
Naoya Horiguchi