Re: [PATCH v4 08/14] mm: Remove locking mf_mutex in is_raw_hwpoison_page_in_hugepage()

[Matthew Wilcox <willy@infradead.org>]

Sashiko review:
> --- Patch [8]: [PATCH v4 08/14] mm: Remove locking mf_mutex in is_raw_hwpoison_page_in_hugepage() ---
> commit c0f68557b399585852271ea0bc6a590caa47b3ad
> Author: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> mm: Remove locking mf_mutex in is_raw_hwpoison_page_in_hugepage()
> 
> This commit removes the mf_mutex protection from the raw hwpoison list
> iteration in hugetlb_page_hwpoison(), relying on lockless llist traversal
> using READ_ONCE(). It also renames the function and modifies it to take a
> folio.
> 
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index 4200deab2c3c4..5076ee5d4788d 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> [ ... ]
> > @@ -1817,37 +1817,30 @@ struct raw_hwp_page {
> >  	struct page *page;
> >  };
> >  
> > -bool is_raw_hwpoison_page_in_hugepage(struct page *page)
> > +/*
> > + * Check if a given @page in a hugetlb folio is HWPOISON.
> > + */
> > +bool hugetlb_page_hwpoison(const struct folio *folio, const struct page *page)
> >  {
> [ ... ]
> > -	mutex_lock(&mf_mutex);
> > -
> > -	llist_for_each_entry(p, folio->hugetlb_hwpoison.first, node) {
> > -		if (page == p->page) {
> > -			ret = true;
> > -			break;
> > -		}
> > +	llist_for_each_entry(p, READ_ONCE(list->first), node) {
> > +		if (page == p->page)
> > +			return true;
> >  	}
> 
> Is the assumption in the commit message that updates only add to the head of
> the list accurate?
> 
> Looking at __folio_free_raw_hwp() in mm/memory-failure.c, the entire list can
> be concurrently detached and its nodes freed via kfree() without an RCU grace
> period:
> 
>     head = llist_del_all(&folio->hugetlb_hwpoison);
>     llist_for_each_entry_safe(p, next, head, node) {
>         ...
>         kfree(p);
>     }
> 
> This freeing can happen while the folio is still in active use. For example,
> if a subsequent memory failure on the same folio fails a GFP_ATOMIC allocation
> in hugetlb_update_hwpoison():
> 
>     } else {
>         folio_set_hugetlb_raw_hwp_unreliable(folio);
>         __folio_free_raw_hwp(folio, false);
>     }
> 
> Since callers like hugetlbfs_read_iter() explicitly drop the folio lock before
> calling adjust_range_hwpoison() (which triggers this lockless traversal),
> could a concurrent memory failure free these list nodes while this loop is
> still accessing them, leading to a use-after-free?
> 
> > -	mutex_unlock(&mf_mutex);
> > -
> > -	return ret;
> > +	return false;
> >  }

argh.  I give up on this cleverness.  I'm going with the new spinlock
approach (like I did in v3) but retaining the per-folio list of pages
rather than having a global hash table.