Re: [PATCH v3 1/1] khugepaged: fix readahead lock inversion in collapse_file()

[Lance Yang]

+Cc: Pedro

On Fri, Oct 16, 2026 at 05:57:48PM +0800, Baolin Wang wrote:
>
>
>On 10/16/26 2:59 PM, Lance Yang wrote:
>> collapse_file() can end up calling page_cache_sync_readahead() with
>> source folios from earlier in the range still locked. If truncate already
>> holds mapping->invalidate_lock exclusively and is waiting for one of
>> those folios, readahead blocks on the same invalidate_lock and neither
>> side can make progress.
>> 
>> Do readahead in a separate pass before we start holding source folios
>> locked for collapse. Keep page_cache_sync_readahead() and its existing
>> policy, and drop each folio's lock and reference after checking that it
>> is uptodate. If truncate or reclaim removes a folio between the two
>> passes, roll back instead of doing readahead again with source folios
>> locked.
>> 
>> Shmem handles swapin and fallocated folios through shmem_get_folio()
>> without taking mapping->invalidate_lock, so it isn't affected by this
>> lock inversion.
>> 
>> Reported-by: syzbot+16bf7cd0ebeb1de93aa5@xxxxxxxxxxxxxxxxxxxxxxxxx
>> Closes: https://syzkaller.appspot.com/bug?extid=16bf7cd0ebeb1de93aa5
>> Fixes: 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")
>> Signed-off-by: Lance Yang <lance.yang@xxxxxxxxx>
>> ---
>
>Thanks for the fix.

Cheers!

>> Thanks to Nguyen Ngoc Thang for v2. David asked for volunteers, so I'm
>> happy to take this over.
>> 
>> Some older stable trees lack the folio infrastructure used here, and the
>> code will differ for each stable tree, so each backport will need quite a
>> bit of rework and its own review. I'm not sure whether to Cc stable here.
>> Once we agree on the fix, I'll send a separate backport for each maintained
>> stable tree except 5.10, which isn't affected.
>> 
>> v2 -> v3:
>>   - Replace the scan-wide invalidate_lock and page_cache_ra_unbounded() with
>>     a preparation pass using page_cache_sync_readahead().
>>   - Roll back on a new page cache miss during collapse.
>>   - https://lore.kernel.org/all/20260913163644.122133-1-ngocthang2710.1999@xxxxxxxxx/
>> 
>>   mm/khugepaged.c | 42 +++++++++++++++++++++++++++++++-----------
>>   1 file changed, 31 insertions(+), 11 deletions(-)
>> 
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index ef5cd6fda34f..b3f67617caf0 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -2262,6 +2262,34 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
>>   	new_folio->index = start;
>>   	new_folio->mapping = mapping;
>>   
>> +	if (!is_shmem) {
>> +		/*
>> +		 * Populate the range before accumulating locked source folios:
>> +		 * readahead may block on invalidate_lock.
>> +		 */
>> +		for (index = start; index < end;) {
>
>why drop xas_unlock_irq()?

Hmm ... we haven't taken the lock here yet, so there's just nothing to
unlock, no?

>> +			folio = filemap_lock_folio(mapping, index);
>> +			if (IS_ERR(folio)) {
>> +				page_cache_sync_readahead(mapping, &file->f_ra,
>> +							  file, index, end - index);
>> +				folio = filemap_lock_folio(mapping, index);
>> +				if (IS_ERR(folio)) {
>> +					result = SCAN_FAIL;
>> +					goto rollback;
>> +				}
>> +			}
>> +			if (!folio_test_uptodate(folio))
>> +				result = SCAN_FAIL;
>
>Do we need to check PG_uptodate after locking the folio? If we 
>successfully lock the folio, doesn't that mean the read I/O has 
>completed? The following code also checks the writeback flag, so the 
>uptodate check seems unnecessary to me.

IIRC, the folio gets unlocked even if the read fails. I just wanted to
catch read errors early. And yeah, we already check uptodate later,
so this one can go, thanks!

>> +			index = folio_next_index(folio);
>> +			folio_unlock(folio);
>> +			folio_put(folio);
>> +			if (result != SCAN_SUCCEED)
>> +				goto rollback;
>> +		}
>> +		/* drain lru cache to help folio_isolate_lru() */
>> +		lru_add_drain();
>> +	}
>
>Could we factor this code out into a small helper?

Okay, let me play with it a bit :) Might pull it out if it reads cleaner.

Thanks, Lance

>> +
>>   	/*
>>   	 * Ensure we have slots for all the pages in the range.  This is
>>   	 * almost certainly a no-op because most of the pages must be present
>> @@ -2320,17 +2348,9 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
>>   			}
>>   		} else {	/* !is_shmem */
>>   			if (!folio || xa_is_value(folio)) {
>> -				xas_unlock_irq(&xas);
>> -				page_cache_sync_readahead(mapping, &file->f_ra,
>> -							  file, index,
>> -							  end - index);
>> -				/* drain lru cache to help folio_isolate_lru() */
>> -				lru_add_drain();
>> -				folio = filemap_lock_folio(mapping, index);
>> -				if (IS_ERR(folio)) {
>> -					result = SCAN_FAIL;
>> -					goto xa_unlocked;
>> -				}
>> +				/* The range changed after the preparation pass. */
>> +				result = SCAN_PAGE_LOCK;
>> +				goto xa_locked;
>>   			} else if (folio_test_dirty(folio)) {
>>   				/*
>>   				 * This page is dirty because it hasn't
>
>