[PATCH] drm/pagemap: no need to recheck own pagemap in migrate_to_devmem

[PATCH] drm/pagemap: no need to recheck own pagemap in migrate_to_devmem

[Zhaoyu Liu]

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.

Signed-off-by: Zhaoyu Liu <liuzhaoyu.zackary@picoheart.com>
---
 drivers/gpu/drm/drm_pagemap.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_pagemap.c b/drivers/gpu/drm/drm_pagemap.c
index 15c78eca180b..6842eadee6df 100644
--- a/drivers/gpu/drm/drm_pagemap.c
+++ b/drivers/gpu/drm/drm_pagemap.c
@@ -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;
 	}
-- 
2.34.1

Re: [PATCH] drm/pagemap: no need to recheck own pagemap in migrate_to_devmem

[sashiko-bot]

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 <liuzhaoyu.zackary@picoheart.com>

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