Re: [PATCH v2 3/5] mm/migrate_device: Apply the fault reference to the correct folio

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Buffer overflow and incorrect PFN skipping in migrate_device_range() and migrate_device_pfns() when processing unaligned large folios.
- [High] Incomplete swap cache cleanup for tail pages of a split THP when migrating to device memory.
- [High] Unchecked destination/source PFN for sub-folios causes NULL pointer dereference.
--

--- Patch [3]: [PATCH v2 3/5] mm/migrate_device: Apply the fault reference to the correct folio ---
commit 8d0ced88bd091cf2d5be9da542139d8ff060a5e1
Author: Matthew Brost <[email protected]>
Subject: mm/migrate_device: Apply the fault reference to the correct folio

This patch fixes an issue where the CPU fault's extra reference count was
incorrectly applied to every sub-folio of a split THP during device migration.
By computing the extra reference count on a per-sub-folio basis, it ensures
only the specific folio containing the fault page receives the extra count.
It also updates the code to use the sub-folio's specific mapping.

> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index 155a182c528f8..cf1f7a3eb1b90 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
[ ... ]
> @@ -1311,13 +1311,25 @@ static void __migrate_device_pages(unsigned long *src_pfns,
>  
>  		BUG_ON(folio_test_writeback(folio));
>  
> -		if (migrate && migrate->fault_page == page)
> -			extra_cnt = 1;
>  		for (j = 0; j < nr && i + j < npages; j++) {
> -			folio = page_folio(migrate_pfn_to_page(src_pfns[i+j]));
> +			struct page *src_page = migrate_pfn_to_page(src_pfns[i+j]);
> +			int extra_cnt = 0;
> +
> +			folio = page_folio(src_page);
>  			newfolio = page_folio(migrate_pfn_to_page(dst_pfns[i+j]));

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for
migrate_pfn_to_page(dst_pfns[i+j]) or src_page to be NULL here?

If a device driver experiences partial allocation failure during a split THP
migration, it might leave some dst_pfns[i+j] entries as 0 (lacking
MIGRATE_PFN_VALID), or clear MIGRATE_PFN_MIGRATE in src_pfns.

Since the loop doesn't check these validity flags for j > 0,
migrate_pfn_to_page() could return NULL. Would calling page_folio() on a
NULL pointer invoke _compound_head(NULL) and lead to a NULL pointer
dereference?

Should we verify that the PFNs are valid before passing them to page_folio()?

[ ... ]

While reviewing this file, I also noticed a couple of other pre-existing
issues.

First, is there an incomplete swap cache cleanup for tail pages of a split THP
when migrating to device memory?

mm/migrate_device.c:__migrate_device_pages() {
    ...
				/*
				 * @folio may have been split into @nr folios
				 * above, so clear all of them.
				 */
				if (!folio_test_anon(folio) ||
				    !folio_free_swap(folio)) {
					for (j = 0; j < nr && i + j < npages; j++)
						src_pfns[i+j] &= ~MIGRATE_PFN_MIGRATE;
					goto next;
				}
    ...
}

[Severity: High]
This is a pre-existing issue, but doesn't this only call folio_free_swap()
once on the head folio?

When migrating to a destination that doesn't support THP, the source THP is
split into independent order-0 folios. The single folio_free_swap() call only
removes the head page from the swap cache, stranding the remaining nr - 1
tail sub-folios.

When the subsequent loop proceeds to migrate all nr sub-folios, does this
improperly insert device private pages into the swap cache? Does this need
a loop to free swap for all sub-folios?


Second, can we overflow the caller-supplied src_pfns array in
migrate_device_range() and migrate_device_pfns() when processing unaligned
large folios?

mm/migrate_device.c:migrate_device_range() {
    ...
		src_pfns[i] = migrate_device_pfn_lock(pfn);
		if (!(src_pfns[i] & MIGRATE_PFN_MIGRATE))
			continue;

		nr = folio_nr_pages(folio);
		if (nr > 1) {
			src_pfns[i] |= MIGRATE_PFN_COMPOUND;
			for (j = 1; j < nr; j++)
				src_pfns[i+j] = 0;
			i += j - 1;
			pfn += j - 1;
		}
    ...
}

[Severity: High]
This is also a pre-existing issue, but this loop zeroes out src_pfns
entries without verifying that i + j < npages.

If the starting PFN belongs to a tail page of a large folio, or if the
npages limit ends mid-folio, nr = folio_nr_pages(folio) retrieves the full
folio size. Since the loop then writes 0 to src_pfns[i+j] unconditionally,
can this overflow the bounds of the caller's array?

Additionally, for tail pages, wouldn't pfn += j - 1 incorrectly advance
the PFN iterator as if it started from the head page, leading to skipped
pages?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3