Re: [PATCH v3 2/6] mm/migrate_device: Do not write past the end of the src_pfns array

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

Pre-existing issues:
- [High] Unsafe speculative read of `folio_nr_pages(folio)` without lock success check leads to array corruption and PFN iterator overshoot.
- [High] `migrate_device_pfns()` corrupts non-contiguous caller arrays by indiscriminately zeroing elements when encountering a compound page.
--

--- Patch [2]: [PATCH v3 2/6] mm/migrate_device: Do not write past the end of the src_pfns array ---
commit f29e7afad950b0af66d7edea5f1c3b8203675195
Author: Matthew Brost <[email protected]>

mm/migrate_device: Do not write past the end of the src_pfns array

migrate_device_range() and migrate_device_pfns() zero the tail entries
of a large folio without checking them against @npages. This commit bounds
the loop by @npages to prevent corrupting whatever follows the array.

> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index 9a346162c6881..e1a18d291b9c1 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
> @@ -1425,7 +1425,7 @@ int migrate_device_range(unsigned long *src_pfns, unsigned long start,

[ ... ]

>  		src_pfns[i] = migrate_device_pfn_lock(pfn);
>  		nr = folio_nr_pages(folio);
>  		if (nr > 1) {

[Severity: High]
This isn't a bug introduced by this patch, but does this code speculatively
read the folio size even if the lock fails?

If migrate_device_pfn_lock() fails (e.g., due to a concurrent free or split),
folio_nr_pages(folio) accesses unstable memory, yielding a garbage nr. This
can cause unbounded iterator advancement and arbitrary zeroing of src_pfns.

Additionally, if the PFN is a tail page, the lock correctly fails, but
folio_nr_pages() reads the head page's size and advances pfn += nr - 1,
overshooting the end of the folio and silently skipping unrelated PFNs in the
migration range.

Should there be a check to ensure src_pfns[i] indicates a successful lock
before reading the folio size?

>  			src_pfns[i] |= MIGRATE_PFN_COMPOUND;
> -			for (j = 1; j < nr; j++)
> +			for (j = 1; j < nr && (i + j) < npages; j++)
>  				src_pfns[i+j] = 0;
>  			i += j - 1;
>  			pfn += j - 1;
> @@ -1459,7 +1459,7 @@ int migrate_device_pfns(unsigned long *src_pfns, unsigned long npages)

[ ... ]

>  		src_pfns[i] = migrate_device_pfn_lock(src_pfns[i]);
>  		nr = folio_nr_pages(folio);
>  		if (nr > 1) {

[Severity: High]
This is also a pre-existing issue, but is it safe to read the folio size
here without confirming that migrate_device_pfn_lock() succeeded, similar to
the issue in migrate_device_range() above?

>  			src_pfns[i] |= MIGRATE_PFN_COMPOUND;
> -			for (j = 1; j < nr; j++)
> +			for (j = 1; j < nr && (i + j) < npages; j++)
>  				src_pfns[i+j] = 0;

[Severity: High]
This is a pre-existing issue, but does this zeroing logic break support for
non-contiguous PFN arrays?

migrate_device_pfns() documents that it supports non-contiguous pre-populated
arrays of device pages to migrate. However, when it encounters a compound
page, it reuses the large-folio iteration logic and zeroes out the next
nr - 1 elements.

In a non-contiguous array, these subsequent elements are independent,
unrelated PFNs provided by the caller, which would be overwritten
and excluded from migration.

>  			i += j - 1;
>  		}

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