Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] The patch breaks the fast path when all pages are already on the target pagemap, causing valid no-op migrations to incorrectly fail with `-EBUSY` instead of returning success (0). Pre-existing issues: - [Critical] On the error path of `drm_pagemap_migrate_range`, raw physical frame numbers (PFNs) are left in `migrate.dst` and mistakenly interpreted as formatted `mpfn`s by `migrate_vma_pages()`, leading to memory corruption and mapping uninitialized device memory. - [High] The first and final migration loops incorrectly use `folio_order(page_folio())` to determine the array iteration step, causing them to skip valid entries in `migrate.src` when processing PTE-mapped Transparent Huge Pages (THPs). -- --- Patch [1]: [PATCH] drm/pagemap: no need to recheck own pagemap in migrate_to_devmem --- commit b14c2718135459228d814edbd8f0bdc7589c21d9 Author: Zhaoyu Liu <[email protected]> drm/pagemap: no need to recheck own pagemap in migrate_to_devmem This patch removes the own_pages count and the re-check for the source page being on the same pagemap inside the migration loop. It also removes the subtraction of own_pages from the final validation check. > diff --git a/drivers/gpu/drm/drm_pagemap.c b/drivers/gpu/drm/drm_pagemap.c > index 15c78eca180b7..6842eadee6df9 100644 > --- a/drivers/gpu/drm/drm_pagemap.c > +++ b/drivers/gpu/drm/drm_pagemap.c [ ... ] > @@ -786,7 +779,6 @@ int drm_pagemap_migrate_to_devmem(struct > drm_pagemap_devmem *devmem_allocation, > goto err_finalize; ^^^^^^^^^^^^^^^^^^ [Severity: Critical] This is a pre-existing issue, but does this error path cause memory corruption? If drm_pagemap_migrate_range() fails in the second loop, the local npages variable is truncated (npages = i + 1) right before this goto, but migrate.npages remains unchanged. When migrate_vma_pages(&migrate) is called on the error path, it will iterate over all original entries. For entries beyond the truncated npages, migrate.dst still contains the raw PFNs populated earlier by ops->populate_devmem_pfn(). If a raw PFN is odd, migrate_vma_pages() misinterprets the set bit 0 as MIGRATE_PFN_VALID, shifts the raw PFN right by MIGRATE_PFN_SHIFT (6), and uses the resulting random page pointer, which could cause severe memory corruption. > } > > -next: > i += NR_PAGES(order); > } [ ... ] > @@ -822,7 +812,7 @@ int drm_pagemap_migrate_to_devmem(struct > drm_pagemap_devmem *devmem_allocation, > i += nr_pages; ^^^^^^^^^^^^ [Severity: High] This is a pre-existing issue, but does using folio order here incorrectly skip entries in migrate.src for PTE-mapped Transparent Huge Pages? Because MIGRATE_PFN_COMPOUND is not set for PTE-mapped THPs, the second migration loop correctly increments by 1. However, this final loop and the first counting loop use folio_order(page_folio(page)), which will increment by the folio order (e.g., 512). This inconsistency skips over the remaining 511 PTE entries in migrate.src, inflating migrated_pages and masking migration failures. Userspace could receive a success code even if some subpage migrations actually failed. > } > > - if (!err && migrated_pages < npages - own_pages) { > + if (!err && migrated_pages < npages) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Does this change break the fast path when all pages are already on the target pagemap? If a migration is requested for a range where all device-private pages are already on the target pagemap, the earlier check (own_pages == npages) sets err = 0 and jumps to err_aborted_migration. Before this patch, migrated_pages (0) was not less than (npages - own_pages) because own_pages was equal to npages, so the function successfully returned 0. With the patch removing `- own_pages`, the check evaluates to `0 < npages` (which is true), so the fast path will now falsely abort with -EBUSY and log "Raced while finalizing migration." > drm_dbg(dpagemap->drm, "Raced while finalizing migration.\n"); > err = -EBUSY; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/anR3ksBglj78TJgp@hostpc?part=1
