Re: [PATCH] mm/khugepaged: Fix race with folio splitting in hpage_collapse_scan_file()

[David Hildenbrand <david@redhat.com>]

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@syzkaller.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@redhat.com>
>> Signed-off-by: Shivank Garg <shivankg@amd.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