linux-kernel - Re: [PATCH] mm/khugepaged: Fix race with folio splitting in hpage_collapse_scan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a3e9e11e-1e8e-42a5-bcc6-e5ad97b4e4da@redhat.com>
Date: Thu, 22 May 2025 13:59:22 +0200
From: David Hildenbrand <david@...hat.com>
To: Baolin Wang <baolin.wang@...ux.alibaba.com>,
 Shivank Garg <shivankg@....com>, akpm@...ux-foundation.org,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org
Cc: ziy@...dia.com, lorenzo.stoakes@...cle.com, Liam.Howlett@...cle.com,
 npache@...hat.com, ryan.roberts@....com, dev.jain@....com,
 fengwei.yin@...el.com, bharata@....com,
 syzbot+2b99589e33edbe9475ca@...kaller.appspotmail.com
Subject: Re: [PATCH] mm/khugepaged: Fix race with folio splitting in
 hpage_collapse_scan_file()

On 22.05.25 12:01, Baolin Wang wrote:
> 
> 
> On 2025/5/22 17:34, Shivank Garg wrote:
>> folio_mapcount() checks folio_test_large() before proceeding to
>> folio_large_mapcount(), but there exists a race window where a folio
>> could be split between these checks which triggered the
>> VM_WARN_ON_FOLIO(!folio_test_large(folio), folio) in
>> folio_large_mapcount().
>>
>> Take a temporary folio reference in hpage_collapse_scan_file() to prevent
>> races with concurrent folio splitting/freeing. This prevent potential
>> incorrect large folio detection.
>>
>> Reported-by: syzbot+2b99589e33edbe9475ca@...kaller.appspotmail.com
>> Closes: https://lore.kernel.org/all/6828470d.a70a0220.38f255.000c.GAE@google.com
>> Fixes: 05c5323b2a34 ("mm: track mapcount of large folios in single value")
>> Suggested-by: David Hildenbrand <david@...hat.com>
>> Signed-off-by: Shivank Garg <shivankg@....com>
>> ---
>>    mm/khugepaged.c | 16 ++++++++++++++++
>>    1 file changed, 16 insertions(+)
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index cc945c6ab3bd..6e8902f9d88c 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -2295,6 +2295,17 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>>    			continue;
>>    		}
>>    
>> +		if (!folio_try_get(folio)) {
>> +			xas_reset(&xas);
>> +			continue;
>> +		}
>> +
>> +		if (unlikely(folio != xas_reload(&xas))) {
>> +			folio_put(folio);
>> +			xas_reset(&xas);
>> +			continue;
>> +		}
>> +
>>    		if (folio_order(folio) == HPAGE_PMD_ORDER &&
>>    		    folio->index == start) {
>>    			/* Maybe PMD-mapped */
>> @@ -2305,23 +2316,27 @@ static int hpage_collapse_scan_file(struct mm_struct *mm, unsigned long addr,
>>    			 * it's safe to skip LRU and refcount checks before
>>    			 * returning.
>>    			 */
>> +			folio_put(folio);
>>    			break;
>>    		}
>>    
>>    		node = folio_nid(folio);
>>    		if (hpage_collapse_scan_abort(node, cc)) {
>>    			result = SCAN_SCAN_ABORT;
>> +			folio_put(folio);
>>    			break;
>>    		}
>>    		cc->node_load[node]++;
>>    
>>    		if (!folio_test_lru(folio)) {
>>    			result = SCAN_PAGE_LRU;
>> +			folio_put(folio);
>>    			break;
>>    		}
>>    
>>    		if (!is_refcount_suitable(folio)) {
> 
> You add a temporary refcnt for the folio, then the
> is_refcount_suitable() will always fail, right?

Indeed. Would one of our MADV_COLLAPSE selftests catch that?

We should also be converting that code to use folio_expected_ref_count() 
-- either directly or wrapped in is_refcount_suitable().

Likely just here through

if (folio_expected_ref_count(folio) + 1 != folio_ref_count(folio))
	...

-- 
Cheers,

David / dhildenb