Re: [PATCH 2/4] mm/migrate_device: Apply the fault reference to the correct folio

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

Pre-existing issues:
- [Critical] Error paths in `__migrate_device_pages` after a successful THP split fail to clear `MIGRATE_PFN_MIGRATE` on tail pages, leading to uninitialized device memory being mapped into userspace.
- [High] `folio_free_swap()` is only called on the head folio after splitting a swap-backed THP, leaving tail folios improperly in the swap cache.
--

--- Patch [2]: [PATCH 2/4] mm/migrate_device: Apply the fault reference to the correct folio ---
commit 4d89d00fc35f8db7c498dd86850a797d2e8c3d2b
Author: Matthew Brost <[email protected]>

mm/migrate_device: Apply the fault reference to the correct folio

This commit fixes a bug in __migrate_device_pages() where extra_cnt was
calculated once for the head page and incorrectly applied to all sub-folios
during a THP migration split. It moves the extra_cnt calculation inside the
sub-folio loop so that the CPU fault reference is correctly applied only to
the sub-folio containing the fault page.

> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index bf8d21857d380..5b078fcbec792 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
> @@ -1201,7 +1201,7 @@ static void __migrate_device_pages(unsigned long *src_pfns,
>  		struct page *page = migrate_pfn_to_page(src_pfns[i]);
>  		struct address_space *mapping;
>  		struct folio *newfolio, *folio;
> -		int r, extra_cnt = 0;
> +		int r;
>  		unsigned long nr = 1;
>  
>  		if (!newpage) {

[ ... ]

> @@ -1306,13 +1306,25 @@ static void __migrate_device_pages(unsigned long *src_pfns,
>  
>  		BUG_ON(folio_test_writeback(folio));
>  

[Severity: High]
This is a pre-existing issue, but does calling folio_free_swap() only on the
head page leave the tail pages in the swap cache?

If a swap-backed THP is split into order-0 folios by
migrate_vma_split_unmapped_folio() in __migrate_device_pages(), the local
"folio" variable still points to the head page. The code just before this
BUG_ON() calls:

			if (mapping) {
				if (!folio_test_anon(folio) ||
				    !folio_free_swap(folio)) {

Because this is outside the subpage loop, it only removes the head page from
the swap cache. Later in the loop, when folio_migrate_mapping() is called
for the tail pages, could they still be marked as swap cache and incorrectly
replace their swap cache entries with device private pages?

[Severity: Critical]
This is a pre-existing issue, but do the error paths after a THP split fail to
clear MIGRATE_PFN_MIGRATE on tail pages?

If the destination only allocates order-0 pages and the THP is successfully
split in __migrate_device_pages(), the MIGRATE_PFN_MIGRATE flag is active on
all tail pages in src_pfns. If a subsequent check fails, the code just before
this BUG_ON() does:

				if (!folio_test_anon(folio) ||
				    !folio_free_swap(folio)) {
					src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
					goto next;
				}
		} else if (folio_is_zone_device(newfolio)) {
			src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
			goto next;
		}

This clears the flag for the head page (src_pfns[i]), but not the tail pages.
Since the loop advances by "nr" at the "next" label, the tail pages keep their
migration flag. Could __migrate_device_finalize() then mistakenly assume they
migrated successfully and map uninitialized device memory into userspace?

> -		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]));
>  
> -			r = folio_migrate_mapping(mapping, newfolio, folio, extra_cnt);
> +			/*
> +			 * The CPU fault holds an extra reference on the folio
> +			 * containing the fault page. @folio may have been
> +			 * split above, so the fault page only accounts for an
> +			 * extra reference on the folio it actually ended up
> +			 * in, not on every folio of the original THP.
> +			 */
> +			if (migrate && migrate->fault_page == src_page)
> +				extra_cnt = 1;
> +
> +			r = folio_migrate_mapping(folio_mapping(folio), newfolio,
> +						  folio, extra_cnt);
>  			if (r)
>  				src_pfns[i+j] &= ~MIGRATE_PFN_MIGRATE;
>  			else

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