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

[PATCH v2] 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.

v2:
  - Skip the no-op migrate_vma_pages() when all pages already reside on
    the target pagemap.

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

diff --git a/drivers/gpu/drm/drm_pagemap.c b/drivers/gpu/drm/drm_pagemap.c
index 15c78eca180b..64922b944092 100644
--- a/drivers/gpu/drm/drm_pagemap.c
+++ b/drivers/gpu/drm/drm_pagemap.c
@@ -698,7 +698,7 @@ int drm_pagemap_migrate_to_devmem(struct drm_pagemap_devmem *devmem_allocation,
 		 * nothing. Raced or unknown device pages.
 		 */
 		err = -EBUSY;
-		goto err_aborted_migration;
+		goto direct_finalize;
 	}
 
 	/* Count device-private pages to migrate */
@@ -719,19 +719,17 @@ int drm_pagemap_migrate_to_devmem(struct drm_pagemap_devmem *devmem_allocation,
 	if (own_pages == npages) {
 		err = 0;
 		drm_dbg(dpagemap->drm, "Migration wasn't necessary.\n");
-		goto err_aborted_migration;
+		goto direct_finalize;
 	} else if (own_pages && !mdetails->can_migrate_same_pagemap) {
 		err = -EBUSY;
 		drm_dbg(dpagemap->drm, "Migration aborted due to fragmentation.\n");
-		goto err_aborted_migration;
+		goto direct_finalize;
 	}
 
 	err = ops->populate_devmem_pfn(devmem_allocation, npages, migrate.dst);
 	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,11 +812,12 @@ 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;
 	}
 
+direct_finalize:
 	migrate_vma_finalize(&migrate);
 err_free:
 	drm_pagemap_zdd_put(zdd);
-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Migration error paths pass unencoded raw PFNs to migrate_vma_pages(), causing out-of-bounds dereferences or mapping arbitrary physical memory into userspace.
--

--- Patch [1]: [PATCH v2] drm/pagemap: no need to recheck own pagemap in migrate_to_devmem ---
commit f37b1a946d737409d8a543e854c0c96738771e75
Author: Zhaoyu Liu <liuzhaoyu.zackary@picoheart.com>

drm/pagemap: no need to recheck own pagemap in migrate_to_devmem
    
The second loop in drm_pagemap_migrate_to_devmem() checks to skip pages
already on the target pagemap. However, this condition is always false at
this point since the first loop already aborts if necessary. This commit
removes the dead code and simplifies the error jumping paths.

> diff --git a/drivers/gpu/drm/drm_pagemap.c b/drivers/gpu/drm/drm_pagemap.c
> index 15c78eca180b7..64922b944092c 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;
>  		}
>  
> -next:
>  		i += NR_PAGES(order);
>  	}

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential problem in the
error handling path during the migration loop.

If drm_pagemap_migrate_range() fails in the middle of this loop, the local
npages variable is truncated to i + 1 before jumping to err_finalize. 
However, err_finalize falls through to err_aborted_migration:

err_finalize:
	if (err)
		drm_pagemap_migration_unlock_put_pages(npages, migrate.dst);
err_aborted_migration:
	migrate_vma_pages(&migrate);

Because migrate_vma_pages() uses the struct field migrate.npages, it will
process the entire original array length rather than the truncated local
npages value. The remaining elements in migrate.dst still contain unencoded
raw PFNs previously populated by ops->populate_devmem_pfn().

If one of these raw PFNs happens to be odd (bit 0 set), it matches
MIGRATE_PFN_VALID. This could cause the kernel to shift it by 6 and use it
as a struct page pointer, leading to an out-of-bounds dereference or mapping
arbitrary physical memory into userspace.

Could this error path be updated to avoid passing unencoded PFNs to
migrate_vma_pages() when the migration loop aborts early?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/anSKZabjuhl5Ojl4@hostpc?part=1