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

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

[Lance Yang]

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@syzkaller.appspotmail.com
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@linux.dev>
---
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@gmail.com/

 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;) {
+			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;
+			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();
+	}
+
 	/*
 	 * 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
-- 
2.49.0

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

[Baolin Wang]

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@syzkaller.appspotmail.com
> 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@linux.dev>
> ---

Thanks for the fix.

> 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@gmail.com/
> 
>   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()?

> +			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.

> +			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?

> +
>   	/*
>   	 * 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

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

[Pedro Falcato]

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@syzkaller.appspotmail.com
> > 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@linux.dev>
> > ---
> 
> Thanks for the fix.
> 
> > 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@gmail.com/
> > 
> >   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()?
> 
> > +			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

Yes, you do; a read can "complete" without the IO having completed successfully,
and in those cases PG_uptodate is not set, but the folio is unlocked.

Either way, why are we checking for uptodate here? We only care for starting readahead.
Anyway, I would possibly recommend we do this as post-processing, not pre-processing.
The current loop is a little overhead-happy.

-- 
Pedro

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

[Pedro Falcato]

On Fri, Oct 16, 2026 at 12:00:24PM +0100, Pedro Falcato wrote:
> 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@syzkaller.appspotmail.com
> > > 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@linux.dev>
> > > ---
> > 
> > Thanks for the fix.
> > 
> > > 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@gmail.com/
> > > 
> > >   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()?
> > 
> > > +			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
> 
> Yes, you do; a read can "complete" without the IO having completed successfully,
> and in those cases PG_uptodate is not set, but the folio is unlocked.
> 
> Either way, why are we checking for uptodate here? We only care for starting readahead.
> Anyway, I would possibly recommend we do this as post-processing, not pre-processing.
> The current loop is a little overhead-happy.

To clarify: I would do this kind of RA as an error path if isolation failed.
Namely, at the end of rollback, after unlocking folios. It doesn't make sense
to me to do this kind of checking upfront. I don't think it's particularly
realistic that, starting RA upfront, you'd see uptodate completed folio reads
when doing the actual isolation.

-- 
Pedro

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@syzkaller.appspotmail.com
>> 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@linux.dev>
>> ---
>
>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@gmail.com/
>> 
>>   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
>
>