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
