Re: [PATCH v5 06/12] mm: Remove locking mf_mutex in is_raw_hwpoison_page_in_hugepage()

[Matthew Wilcox <willy@infradead.org>]

Sashiko:
> --- Patch [6]: [PATCH v5 06/12] mm: Remove locking mf_mutex in is_raw_hwpoison_page_in_hugepage() ---
> commit 5aafc5635bbbdae616864594ab6ced23fdde474f
> Author: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> mm: Remove locking mf_mutex in is_raw_hwpoison_page_in_hugepage()
> 
> This commit replaces mf_mutex with a new spinlock to protect the hwpoison
> list in hugetlb folios, preventing unexpected sleeping in predicate functions.
> It also renames the function to hugetlb_page_hwpoison and simplifies it to
> use a standard singly linked list.
> 
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index 9d245b4f2b8fe..e2f50b4aba822 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> [ ... ]
> > +bool hugetlb_page_hwpoison(const struct folio *folio, const struct page *page)
> >  {
> [ ... ]
> >  	/*
> > -	 * When RawHwpUnreliable is set, kernel lost track of which subpages
> > -	 * are HWPOISON. So return as if ALL subpages are HWPOISONed.
> > +	 * When RawHwpUnreliable is set, kernel lost track of which pages
> > +	 * are HWPOISON. So return as if ALL pages are HWPOISONed.
> >  	 */
> >  	if (folio_test_hugetlb_raw_hwp_unreliable(folio))
> >  		return true;
> >
> > -	mutex_lock(&mf_mutex);
> > -
> > -	raw_hwp_head = raw_hwp_list_head(folio);
> > -	llist_for_each_entry(p, raw_hwp_head->first, node) {
> > -		if (page == p->page) {
> > -			ret = true;
> > +	spin_lock_irqsave(&hwp_page_lock, flags);
> 
> This isn't a bug introduced by this patch, but is it possible for a race
> to occur if the unreliable flag is checked without holding the lock?
> 
> If CPU1 calls hugetlb_page_hwpoison() and checks the unreliable flag
> locklessly, it might read false.
> 
> Concurrently, CPU2 executing hugetlb_update_hwpoison() might fail to
> allocate a list entry, acquire hwp_page_lock, set the unreliable flag,
> and clear the hugetlb_hwpoison list:
> 
> hugetlb_update_hwpoison()
>     ...
>     p = kmalloc_obj(*p, GFP_ATOMIC);
>     if (p) {
>         ...
>     } else {
>         folio_set_hugetlb_raw_hwp_unreliable(folio);
>         __folio_free_raw_hwp(folio, false);
>     }
>     spin_unlock_irqrestore(&hwp_page_lock, flags);
> 
> CPU1 would then acquire hwp_page_lock, scan the now-empty list, and
> incorrectly conclude the page is safe. This could potentially allow
> poisoned memory to be accessed.
> 
> Would it be safer to check folio_test_hugetlb_raw_hwp_unreliable() inside
> the hwp_page_lock critical section?

Hm, yes, that's true.  I'll fold the fix into this patch:

@@ -1831,14 +1831,17 @@ bool hugetlb_page_hwpoison(const struct folio *folio, const struct page *page)
        if (!folio_test_has_hwpoisoned(folio))
                return false;

+       spin_lock_irqsave(&hwp_page_lock, flags);
+
        /*
         * When RawHwpUnreliable is set, kernel lost track of which pages
         * are HWPOISON. So return as if ALL pages are HWPOISONed.
         */
-       if (folio_test_hugetlb_raw_hwp_unreliable(folio))
+       if (folio_test_hugetlb_raw_hwp_unreliable(folio)) {
+               spin_unlock_irqrestore(&hwp_page_lock, flags);
                return true;
+       }

-       spin_lock_irqsave(&hwp_page_lock, flags);
        for (p = folio->hugetlb_hwpoison; p; p = p->next) {
                if (page == p->page)
                        break;

> > +	for (p = folio->hugetlb_hwpoison; p; p = p->next) {
> > +		if (page == p->page)
> >  			break;
> > -		}
> >  	}
> > +	spin_unlock_irqrestore(&hwp_page_lock, flags);
> >
> > -	mutex_unlock(&mf_mutex);
> > -
> > -	return ret;
> > +	return p != NULL;
> >  }