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
