Re: [PATCH v2 7/8] mm/khugepaged: hoist more code into collapse_isolate_folio()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] mm/khugepaged: missing smp_rmb() in collapse_isolate_folio()
--

commit 1a6c61868f8e8d364cc5be77c89e1e39fd7f5a4a
Author: Pedro Falcato <pfalcato@suse.de>

mm/khugepaged: hoist more code into collapse_isolate_folio()

This commit moves the folio reference count and dirtiness checks from
collapse_file() into collapse_isolate_folio(), performing them outside the
i_pages xarray lock.

> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 1e58ae5b3e74d..accd8b4895d30 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2404,13 +2404,53 @@ static enum scan_result collapse_isolate_folio(struct collapse_file_state *state
>  
>  	if (!filemap_release_folio(folio, GFP_KERNEL)) {
>  		result = SCAN_PAGE_HAS_PRIVATE;
> -		folio_putback_lru(folio);
> -		goto out_unlock;
> +		goto out_putback;
>  	}
>  
>  	if (folio_mapped(folio))
>  		try_to_unmap(folio, TTU_IGNORE_MLOCK | TTU_BATCH_FLUSH);
> +	/*
> +	 * We control 2 + nr_pages references to the folio:
> +	 *  - we hold a pin on it;
> +	 *  - nr_pages reference from page cache;
> +	 *  - one from lru_isolate_folio;
> +	 * If those are the only references, then any new usage
> +	 * of the folio will have to fetch it from the page
> +	 * cache. That requires locking the folio to handle
> +	 * truncate, so any new usage will be blocked until we
> +	 * unlock folio after collapse/during rollback.
> +	 */
> +	if (folio_ref_count(folio) != 2 + folio_nr_pages(folio)) {
> +		result = SCAN_PAGE_COUNT;
> +		goto out_putback;
> +	}
> +
> +	/*
> +	 * At this point, the folio is locked and unmapped. If the PTE
> +	 * was dirty, try_to_unmap() has transferred the dirty bit to
> +	 * the folio and we must not collapse it into a clean
> +	 * file-backed folio.
> +	 *
> +	 * If the folio is clean here, no one can write it until we
> +	 * drop the folio lock. A write through a stale TLB entry came
> +	 * from a clean PTE and must fault because the PTE has been
> +	 * cleared; the fault path has to take the folio lock before
> +	 * installing a writable mapping. Buffered write paths also
> +	 * have to take the folio lock before modifying file contents
> +	 * without a mapping, typically via write_begin_get_folio().
> +	 */
> +	if (!state->is_shmem && folio_test_dirty(folio)) {
> +		result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
> +		goto out_putback;
> +	}
>  	return SCAN_SUCCEED;

[Severity: High]
Without xas_lock_irq(&xas), can the folio_test_dirty(folio) load be
satisfied before folio_ref_count(folio) on weakly-ordered architectures?

A concurrent holder of a temporary reference or pin (such as direct I/O
read completion into a file-backed buffer) can mark the folio dirty and
drop its reference without holding the folio lock:

CPU0                                  CPU1
collapse_isolate_folio()
  [speculatively loads folio->flags:
   folio_test_dirty(folio) == false]
                                      folio_mark_dirty(folio)
                                      folio_put(folio)
  folio_ref_count(folio) == 2 + nr
  return SCAN_SUCCEED                 <-- dirty folio collapsed as clean

Previously, holding i_pages.xa_lock across both checks serialized against
__folio_mark_dirty(), as noted in __remove_mapping():

mm/vmscan.c:__remove_mapping() {
	...
	 * Reversing the order of the tests ensures such a situation cannot
	 * escape unnoticed. The smp_rmb is needed to ensure the folio->flags
	 * load is not satisfied before that of folio->_refcount.
	 *
	 * Note that if the dirty flag is always set via folio_mark_dirty,
	 * and thus under the i_pages lock, then this ordering is not required.
	...
}

Because folio_ref_count(folio) and folio_test_dirty(folio) are plain reads
of different struct folio members with no barrier between them, reordering
them allows a newly dirtied file-backed folio to be collapsed into a clean
PMD folio, losing the modified data on reclaim and leaving dirty page
accounting elevated.  Adding smp_rmb() between folio_ref_count(folio) and
folio_test_dirty(folio) (matching ttu_anon_lazyfree_folio() and
__discard_anon_folio_pmd_locked()) would preserve the ordering.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111175121.1328383-1-pfalcato@suse.de?part=7