Re: [PATCH] mm/memory-failure: fix folio refcount leak and min_order_for_split() locking

[Lorenzo Stoakes (ARM)]

On Thu, Aug 06, 2026 at 04:17:21PM +0200, David Hildenbrand (Arm) wrote:
> On 8/6/26 16:03, Lorenzo Stoakes (ARM) wrote:
> > On Thu, Aug 06, 2026 at 01:14:38PM +0200, David Hildenbrand (Arm) wrote:
> >> hwpoison code can end up calling min_order_for_split() without holding
> >> the folio lock. There isn't really something that would prevent
> >> concurrent folio split. Consequently folio->mapping can get set to
> >> NULL after checking for "!folio->mapping", and if the compiler
> >> reloads folio->mapping, mapping_min_folio_order() would try to
> >> dereference NULL.
> >>
> >> While very unlikely to happen in practice, let's just enforce that
> >> min_order_for_split() is called with the folio lock held. We can
> >> significantly cleanup the calling hwpoison code, and just get rid
> >> of try_to_split_thp_page() to hold the folio lock for a bit longer.
> >>
> >> Just work on folios now, which further cleans up the code. We just
> >> have to be careful about doing the page_folio() after splitting, which
> >> we have to do already either way. Do not change the way we split for
> >> now, this needs more thought and should be done separately.
> >>
> >> Cleaning this up we fix another issue: in soft_offline_in_use_page(), we
> >> would currently have leaked a folio reference.
> >
> > Yikes...
> >
> >>
> >> In folio_split(), document and assert that we need the folio lock.
> >
> > You mean min_order_for_split()?
>
> Very right :)
>
> >
> >> Drop the questionable VM_BUG_ON_PAGE(!page_count(p), p) check entirely.
> >>
> >> The folio->mapping problem was identified by Sashiko, and Li Youhong
> >> reported it by sending a proposal fix.
> >>
> >> This likely does not really warrant CCing stable, but I expect little
> >> conflicts when doing the backport, so let's just CC stable because of
> >> the refcount leak.
> >>
> >> Reported-by: Li Youhong <liyouhong@xxxxxxxxxx>
> >> Closes: https://lore.kernel.org/r/20260804035828.2684059-1-dayou5941@xxxxxxx
> >> Fixes: 689b8986776c ("mm/memory-failure: improve large block size folio handling")
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> >
> > A typo above and a nit below about a comment but otherwise LGTM so:
> >
> > Reviewed-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>
> >
> >> ---
> >> v4 of the last fixing attempts:
> >>
> >> https://lore.kernel.org/r/20260806031958.677935-1-dayou5941@xxxxxxx
> >> ---
> >>  mm/huge_memory.c    |  4 ++++
> >>  mm/memory-failure.c | 56 ++++++++++++++++++-----------------------------------
> >>  2 files changed, 23 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >> index 9b1f3b24f7e0d..a00df56a68b57 100644
> >> --- a/mm/huge_memory.c
> >> +++ b/mm/huge_memory.c
> >> @@ -4362,10 +4362,14 @@ int folio_split(struct folio *folio, unsigned int new_order,
> >>   * If a file-backed folio is truncated, 0 will be returned. Any subsequent
> >>   * split attempt should get -EBUSY from split checking code.
> >>   *
> >> + * Context: @folio must be locked.
> >> + *
> >>   * Return: @folio's minimum order for split
> >>   */
> >>  unsigned int min_order_for_split(struct folio *folio)
> >>  {
> >> +	VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
> >> +
> >>  	if (folio_test_anon(folio))
> >>  		return 0;
> >>
> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> >> index aaf14608b30e2..f4f3f1fc9eaff 100644
> >> --- a/mm/memory-failure.c
> >> +++ b/mm/memory-failure.c
> >> @@ -1705,26 +1705,6 @@ static int identify_page_state(unsigned long pfn, struct page *p,
> >>  	return page_action(ps, p, pfn);
> >>  }
> >>
> >> -/*
> >> - * When 'release' is 'false', it means that if thp split has failed,
> >> - * there is still more to do, hence the page refcount we took earlier
> >> - * is still needed.
> >> - */
> >> -static int try_to_split_thp_page(struct page *page, unsigned int new_order,
> >> -		bool release)
> >> -{
> >> -	int ret;
> >> -
> >> -	lock_page(page);
> >> -	ret = split_huge_page_to_order(page, new_order);
> >> -	unlock_page(page);
> >> -
> >> -	if (ret && release)
> >> -		put_page(page);
> >> -
> >> -	return ret;
> >> -}
> >> -
> >>  static void unmap_and_kill(struct list_head *to_kill, unsigned long pfn,
> >>  		struct address_space *mapping, pgoff_t index, int flags)
> >>  {
> >> @@ -2509,9 +2489,6 @@ int memory_failure(unsigned long pfn, int flags)
> >>  	folio_unlock(folio);
> >>
> >>  	if (folio_test_large(folio)) {
> >> -		const int new_order = min_order_for_split(folio);
> >> -		int err;
> >> -
> >>  		/*
> >>  		 * The flag must be set after the refcount is bumped
> >>  		 * otherwise it may race with THP split.
> >> @@ -2526,24 +2503,25 @@ int memory_failure(unsigned long pfn, int flags)
> >>  		 * page is a valid handlable page.
> >>  		 */
> >>  		folio_set_has_hwpoisoned(folio);
> >> -		err = try_to_split_thp_page(p, new_order, /* release= */ false);
> >> +
> >> +		folio_lock(folio);
> >> +		split_huge_page_to_order(p, min_order_for_split(folio));
> >> +		folio = page_folio(p);
> >> +		folio_unlock(folio);
> >> +
> >>  		/*
> >>  		 * If splitting a folio to order-0 fails, kill the process.
> >>  		 * Split the folio regardless to minimize unusable pages.
> >>  		 * Because the memory failure code cannot handle large
> >>  		 * folios, this split is always treated as if it failed.
> >>  		 */
> >> -		if (err || new_order) {
> >> -			/* get folio again in case the original one is split */
> >> -			folio = page_folio(p);
> >> +		if (folio_test_large(folio)) {
> >>  			res = -EHWPOISON;
> >>  			kill_procs_now(p, pfn, flags, folio);
> >> -			put_page(p);
> >> +			folio_put(folio);
> >>  			action_result(pfn, MF_MSG_UNSPLIT_THP, MF_FAILED);
> >>  			goto unlock_mutex;
> >>  		}
> >> -		VM_BUG_ON_PAGE(!page_count(p), p);
> >> -		folio = page_folio(p);
> >>  	}
> >>
> >>  	/*
> >> @@ -2861,9 +2839,8 @@ static int soft_offline_in_use_page(struct page *page)
> >>  		.reason = MR_MEMORY_FAILURE,
> >>  	};
> >>
> >> +	folio_lock(folio);
> >>  	if (!huge && folio_test_large(folio)) {
> >> -		const int new_order = min_order_for_split(folio);
> >> -
> >>  		/*
> >>  		 * If new_order (target split order) is not 0, do not split the
> >>  		 * folio at all to retain the still accessible large folio.
> >> @@ -2871,15 +2848,20 @@ static int soft_offline_in_use_page(struct page *page)
> >>  		 * preferred, split it to non-zero new_order like it is done in
> >>  		 * memory_failure().
> >>  		 */
> >
> > This comment is:
> >
> > 		/*
> > 		 * If new_order (target split order) is not 0, do not split the
> > 		 * folio at all to retain the still accessible large folio.
> > 		 * NOTE: if minimizing the number of soft offline pages is
> > 		 * preferred, split it to non-zero new_order like it is done in
> > 		 * memory_failure().
> > 		 */
> >
> > Which references 'new_order' twice - probably need to update this.
>
> Agreed, thanks!
>
> While at it, I'll just drop the NOTE that is not of any value, really?

Yeah rip that out.

>
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index f4f3f1fc9eaff..9f1e547e7b472 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2842,11 +2842,8 @@ static int soft_offline_in_use_page(struct page *page)
>         folio_lock(folio);
>         if (!huge && folio_test_large(folio)) {
>                 /*
> -                * If new_order (target split order) is not 0, do not split the
> -                * folio at all to retain the still accessible large folio.
> -                * NOTE: if minimizing the number of soft offline pages is
> -                * preferred, split it to non-zero new_order like it is done in
> -                * memory_failure().
> +                * If we cannot split to order-0, do not split the folio at all
> +                * to retain the still accessible large folio.
>                  */
>                 if (!min_order_for_split(folio))
>                         ret = split_huge_page_to_order(page, 0) ? -EBUSY : 0;
>
> >
> >> -		if (new_order || try_to_split_thp_page(page, /* new_order= */ 0,
> >> -						       /* release= */ true)) {
> >> +		if (!min_order_for_split(folio))
> >> +			ret = split_huge_page_to_order(page, 0) ? -EBUSY : 0;
> >> +		else
> >> +			ret = -EBUSY;
> >> +		folio = page_folio(page);
> >> +
> >> +		if (ret) {
> >> +			folio_unlock(folio);
> >> +			folio_put(folio);
> >
> > Wow, incredible that we had a refcount leak here for ages and didn't know. Ugh
> > what a mess this hw posion crap is!
>
> I mean, soft-offline handling is not what runs on ... everyone's box. And the
> memory will effectively be dead soon either way (hardware tells us that this
> memory is likely going to get hwpoison soon) in most cases ... :)

Good. I think we should burn the whole thing down tbh.

>
> --
> Cheers,
>
> David

--
Cheers, Lorenzo