From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BD1AF2F7F04; Thu, 6 Aug 2026 14:26:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786026397; cv=none; b=gq7RvBttfSURXIU0Zo8NYHlq4Yce+M6SsM4GbdlUI4HL4kNrHAfNaeutUHOYwiu4b3rykaPwHLx0u/ahWB4uyvuKm1G8G/e8EpmAzfOgVsF+xMAYveinjoEtqNenn6yEjoAmVo3czssqD/qMoz/3YmZwuWkSPXgGiuIHb3eYikA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786026397; c=relaxed/simple; bh=2iAMt/GUrF7SBhiuKcwF3ZrURycu86rFZKFulRL3jL8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=feYHJ7Yf0Gk9/GTTbZartbIfl6vGVzXoFiMZbK3Qe/2RUUzlV7QEBlukeXocezkvnoNOONhuvnpCUOQskSHJDSriY/yajjfpCCJRQLlRDXYYL2fXrhurqse7rFFlcGDbnvkaVzrzA67Q9qTml/LsJI3hT9I+HnEhnAF7B8ermIk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=VpD4TNWN; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="VpD4TNWN" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1B3351F00A3A; Thu, 6 Aug 2026 14:26:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1786026395; bh=fZ2eDYGSj2XtYyLf86x9P+NytbC9JWawDjK2akJlovE=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=VpD4TNWN59KnyLiJVal2vHb04rgodpNvBpCOIAWYXFVetxYPhmV2HUsdaIMM182at eyhJLeqf/AUdoYxSTO1Be4wjykrqpSHPIORgCpKaDFR4rirr3ugm7l5eSoQixTcVHZ koWFUAmfd/j7KVGFu4oeA5q7QzjPutCp2IeDaqJOwfGD1Kvw5DQE27bZYtT3oFdgpv MjPJFMvL1pjys9z9KbqsOyeahKWYSuAze9F+uU+8u92dtPCs5ggxoPGz9o3+k6XTLi blcsJr3ljTqGWg8QaxSiEi9XGwyIxL6Oyw6kxPhmEx2AKMAks1vDYvaQNl2nfxha1/ yU4qYnbq7Ppxg== Date: Thu, 6 Aug 2026 15:26:16 +0100 From: "Lorenzo Stoakes (ARM)" To: "David Hildenbrand (Arm)" Cc: Andrew Morton , Zi Yan , Baolin Wang , "Liam R. Howlett" , Nico Pache , Ryan Roberts , Dev Jain , Barry Song , Lance Yang , Usama Arif , Miaohe Lin , Naoya Horiguchi , Wei Yang , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Luis Chamberlain , Li Youhong , stable@vger.kernel.org Subject: Re: [PATCH] mm/memory-failure: fix folio refcount leak and min_order_for_split() locking Message-ID: References: <20260806-try_to_split_thp_page-v1-1-a259e3387e38@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 > >> Closes: https://lore.kernel.org/r/20260804035828.2684059-1-dayou5941@163.com > >> Fixes: 689b8986776c ("mm/memory-failure: improve large block size folio handling") > >> Cc: stable@vger.kernel.org > >> Signed-off-by: David Hildenbrand (Arm) > > > > A typo above and a nit below about a comment but otherwise LGTM so: > > > > Reviewed-by: Lorenzo Stoakes (ARM) > > > >> --- > >> v4 of the last fixing attempts: > >> > >> https://lore.kernel.org/r/20260806031958.677935-1-dayou5941@163.com > >> --- > >> 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