Re: [PATCH v4 02/14] memory-failure: Test the page is hwpoison before taking the mutex

[Matthew Wilcox <willy@infradead.org>]

Sashiko review:
> --- Patch [2]: [PATCH v4 02/14] memory-failure: Test the page is hwpoison before taking the mutex ---
> commit 1cca344401408425fcfd4ca129165f85229f9227
> Author: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> memory-failure: Test the page is hwpoison before taking the mutex
> 
> This patch moves the is_page_hwpoison() check in unpoison_memory() outside
> of mf_mutex. This avoids synchronizing with memory failure just to filter
> out pages which are not hwpoisoned.
> 
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index 944e6e1d49719..3615ced0a11bc 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> > @@ -2677,7 +2677,12 @@ int unpoison_memory(unsigned long pfn)
> >  	p = pfn_to_online_page(pfn);
> >  	if (!p)
> >  		return -EIO;
> > -	folio = page_folio(p);
> > +
> > +	if (!is_page_hwpoison(p)) {
> > +		unpoison_pr_info("%#lx: page is not poisoned\n",
> > +				 pfn, &unpoison_rs);
> > +		return -EBUSY;
> > +	}
> >
> >  	mutex_lock(&mf_mutex);
> >
> > @@ -2688,6 +2693,8 @@ int unpoison_memory(unsigned long pfn)
> >  		goto unlock_mutex;
> >  	}
> >
> > +	folio = page_folio(p);
> > +
> >  	if (is_huge_zero_folio(folio)) {
> >  		unpoison_pr_info("%#lx: huge zero page is not supported\n",
> >  				 pfn, &unpoison_rs);
> > @@ -2695,12 +2702,6 @@ int unpoison_memory(unsigned long pfn)
> >  		goto unlock_mutex;
> >  	}
> >
> > -	if (!is_page_hwpoison(p)) {
> > -		unpoison_pr_info("%#lx: page is not poisoned\n",
> > -				 pfn, &unpoison_rs);
> > -		goto unlock_mutex;
> > -	}
> > -
> 
> Does removing this check from inside the mutex introduce a time-of-check to
> time-of-use race?
> 
> If two threads call unpoison_memory() concurrently on the same PFN, both could
> pass the lockless check. Thread A takes the lock, unpoisons the page, and the
> page is returned to the buddy allocator.
> 
> When Thread B acquires the lock, it will now proceed without re-checking
> is_page_hwpoison(). If the page was concurrently reallocated as a compound tail
> page, the speculative folio_ref_count() or folio_test_slab() checks below
> could trigger a VM_BUG_ON_PGFLAGS crash.
> 
> >  	if (folio_ref_count(folio) > 1) {
> >  		unpoison_pr_info("%#lx: someone grabs the hwpoison page\n",
> >  				 pfn, &unpoison_rs);

Uh.  Yeah.  I wasn't thinking about two simultaneous calls to
unpoison_memory().  I'll just drop this patch; it wasn't important.