drm/pagemap: no need to recheck own pagemap in migrate_to_devmem
Subsystems (3)
PATCH 1/1 drm/pagemap: no need to recheck own pagemap in migrate_to_devmem
The second loop in drm_pagemap_migrate_to_devmem() checks page_pgmap(src_page) == pagemap && !can_migrate_same_pagemap to skip pages already on the target pagemap. However, this condition is always false at this point: if can_migrate_same_pagemap is false and any own pages exist, the first loop already returns -EBUSY before reaching here.
--- drivers/gpu/drm/drm_pagemap.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)
drivers/gpu/drm/drm_pagemap.c +2 -12
@@ -730,8 +730,6 @@ int drm_pagemap_migrate_to_devmem(struct drm_pagemap_devmem *devmem_allocation,if (err) goto err_aborted_migration; - own_pages =0 ; -for (i =0 ; i < npages;) {unsigned long j;struct page *page =pfn_to_page (migrate.dst[i]);
@@ -744,12 +742,7 @@ int drm_pagemap_migrate_to_devmem(struct drm_pagemap_devmem *devmem_allocation,struct drm_pagemap_zdd *src_zdd =drm_pagemap_page_zone_device_data (src_page); -if (page_pgmap (src_page) == pagemap && - !mdetails->can_migrate_same_pagemap) { - migrate.dst[i] =0 ; - own_pages++; - goto next; - } +/* Own or peer device private page */ cur.dpagemap = src_zdd->dpagemap; cur.ops = src_zdd->devmem_allocation->ops; cur.device = cur.dpagemap->drm->dev;
@@ -786,7 +779,6 @@ int drm_pagemap_migrate_to_devmem(struct drm_pagemap_devmem *devmem_allocation, goto err_finalize; } -next: i +=NR_PAGES (order); }
@@ -797,8 +789,6 @@ int drm_pagemap_migrate_to_devmem(struct drm_pagemap_devmem *devmem_allocation,if (err) goto err_finalize; -drm_WARN_ON (dpagemap->drm, !!own_pages); -dma_fence_put (devmem_allocation->pre_migrate_fence); devmem_allocation->pre_migrate_fence =NULL ;
@@ -822,7 +812,7 @@ int drm_pagemap_migrate_to_devmem(struct drm_pagemap_devmem *devmem_allocation, i += nr_pages; } -if (!err && migrated_pages < npages - own_pages) { +if (!err && migrated_pages < npages) {drm_dbg (dpagemap->drm,"Raced while finalizing migration.\n" ); err = -EBUSY ; }
Signature · 1 line
2.34.1
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
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 mpfns 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.
Quoted 4 lines
drivers/gpu/drm/drm_pagemap.c +0
-0
[ ... ]
drivers/gpu/drm/drm_pagemap.c int drm_pagemap_migrate_to_devmem(struct drm_pagemap_devmem *devmem_allocation, · line 779
Quoted 2 lines
drivers/gpu/drm/drm_pagemap.c +0
-0@@ -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.
Quoted 5 lines
drivers/gpu/drm/drm_pagemap.c +0
-1} -next: i +=NR_PAGES (order); }
[ ... ]
drivers/gpu/drm/drm_pagemap.c int drm_pagemap_migrate_to_devmem(struct drm_pagemap_devmem *devmem_allocation, · line 812
Quoted 2 lines
drivers/gpu/drm/drm_pagemap.c +0
-0@@ -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.
drivers/gpu/drm/drm_pagemap.c int drm_pagemap_migrate_to_devmem(struct drm_pagemap_devmem *devmem_allocation, · line 815
Quoted 4 lines
drivers/gpu/drm/drm_pagemap.c +1
-1} -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."
Quoted 3 lines
drivers/gpu/drm/drm_pagemap.c +0
-0drm_dbg (dpagemap->drm,"Raced while finalizing migration.\n" ); err = -EBUSY ; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/anR3ksBglj78TJgp@hostpc?part=1