Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] Buffer overflow and incorrect PFN advancement in `migrate_device_range()` and `migrate_device_pfns()` when iterating over large folios. - [Critical] Missing `goto next;` in mismatched THP migration error path leads to a Use-After-Free in the page cache. - [High] When splitting a THP during device migration, `folio_free_swap()` is only called on the head sub-folio, leaving tail sub-folios incorrectly residing in the swap cache and causing device private folios to be inserted into the swap cache. - [High] Unconditional folio reference leak in `migrate_vma_split_unmapped_folio()` when `split_huge_pmd_address()` fails. -- --- Patch [1]: [PATCH v2 1/5] mm/migrate_device: Clear MIGRATE_PFN_MIGRATE on all sub-folios of a split THP --- commit 5f556994219f3e728aa37333e780844be8000edb Author: Matthew Brost <[email protected]> mm/migrate_device: Clear MIGRATE_PFN_MIGRATE on all sub-folios of a split THP This commit updates both bailouts in __migrate_device_pages() to clear the MIGRATE_PFN_MIGRATE flag across all sub-folios of a split THP. This ensures the tail pages are properly handled when migration fails. > diff --git a/mm/migrate_device.c b/mm/migrate_device.c > index 18d097c388530..1e24039f62b33 100644 > --- a/mm/migrate_device.c > +++ b/mm/migrate_device.c > @@ -1209,10 +1209,14 @@ static void __migrate_device_pages(unsigned long > *src_pfns, > * device private or coherent memory. > * > * Try to get rid of swap cache if possible. > + * > + * @folio may have been split into @nr folios > + * above, so clear all of them. > */ > if (!folio_test_anon(folio) || > !folio_free_swap(folio)) { [Severity: High] This isn't a bug introduced by this patch, but does this conditional only evaluate the head folio? When migrate_vma_split_unmapped_folio() splits a THP into nr independent order-0 folios, this bailout condition succeeds for the head folio. However, it seems we might proceed without freeing the tail folios from the swap cache. Could this cause the subsequent loop to replace the tail folios with device private folios in the swap cache via __swap_cache_replace_folio(), potentially causing a panic when the swap subsystem interacts with them? > - src_pfns[i] &= ~MIGRATE_PFN_MIGRATE; > + for (j = 0; j < nr && i + j < npages; > j++) > + src_pfns[i+j] &= > ~MIGRATE_PFN_MIGRATE; > goto next; > } > } [ ... ] [Severity: Critical] This is a pre-existing issue, but is there a missing goto next in __migrate_device_pages() during a mismatched THP migration? mm/migrate_device.c:__migrate_device_pages() { ... if (thp_migration_supported()) { ... } else if ((src_pfns[i] & MIGRATE_PFN_MIGRATE) && (dst_pfns[i] & MIGRATE_PFN_COMPOUND) && !(src_pfns[i] & MIGRATE_PFN_COMPOUND)) { src_pfns[i] &= ~MIGRATE_PFN_MIGRATE; } } ... By clearing the migration flag without jumping to next, execution appears to fall through into the mapping migration loop. Could folio_migrate_mapping() successfully replace the source folio with the destination folio in the page cache, and then migrate_vma_finalize() free the destination folio because the flag was cleared? This looks like it might leave a Use-After-Free in the page cache. [Severity: Critical] This isn't a bug introduced by this patch, but does iterating over large folios lack bounds checking in migrate_device_range()? mm/migrate_device.c:migrate_device_range() { ... for (j = 1; j < nr; j++) src_pfns[i+j] = 0; ... When reading nr = folio_nr_pages(folio) for a large folio, there does not appear to be a check against npages. If the large folio extends beyond the requested migration range, could i+j exceed the src_pfns array bounds and corrupt memory? Additionally, advancing the pfn by adding the full folio size seems to bypass the subpage offset. Can this skip unrelated pages? This same pattern appears to exist in migrate_device_pfns(). [Severity: High] This is a pre-existing issue, but does migrate_vma_split_unmapped_folio() leak a folio reference if split_huge_pmd_address() returns early? mm/migrate_device.c:migrate_vma_split_unmapped_folio() { ... folio_get(folio); split_huge_pmd_address(migrate->vma, addr, true); ret = folio_split_unmapped(folio, 0); ... If split_huge_pmd_address() returns early, for instance if the PMD was concurrently unmapped and mm_find_pmd returns NULL, the reference taken by folio_get() does not seem to be dropped. When folio_split_unmapped() later fails with -EAGAIN due to the elevated refcount, does this result in a permanent leak of the large folio? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
