[PATCH] drm/pagemap: Remove unreachable code in drm_pagemap_migrate_to_devmem()

[PATCH] drm/pagemap: Remove unreachable code in drm_pagemap_migrate_to_devmem()

[yaolu]

From: Lu Yao <yaolu@kylinos.cn>

If it can satisfy the condition of "page_pgmap(src_page) == pagemap
&& ! mdetails->can_migrate_same_pagemap", it will be return -EBUSY in
the precondition checks above the loop.

Fixes: 75af93b3f5d0 ("drm/pagemap, drm/xe: Support destination migration over interconnect")
Signed-off-by: Lu Yao <yaolu@kylinos.cn>
---
 drivers/gpu/drm/drm_pagemap.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_pagemap.c b/drivers/gpu/drm/drm_pagemap.c
index 892b325fa99b..23d60a271f17 100644
--- a/drivers/gpu/drm/drm_pagemap.c
+++ b/drivers/gpu/drm/drm_pagemap.c
@@ -746,12 +746,6 @@ 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;
-			}
 			cur.dpagemap = src_zdd->dpagemap;
 			cur.ops = src_zdd->devmem_allocation->ops;
 			cur.device = cur.dpagemap->drm->dev;
@@ -788,7 +782,6 @@ int drm_pagemap_migrate_to_devmem(struct drm_pagemap_devmem *devmem_allocation,
 			goto err_finalize;
 		}
 
-next:
 		i += NR_PAGES(order);
 	}
 
@@ -799,8 +792,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;
 
-- 
2.25.1

Re: [PATCH] drm/pagemap: Remove unreachable code in drm_pagemap_migrate_to_devmem()

[sashiko-bot]

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

New issues:
- [High] The patch removes a critical safeguard against unsupported same-pagemap migrations, erroneously assuming it is unreachable. However, a discrepancy in loop iterations makes the safeguard highly reachable.

Pre-existing issues:
- [Critical] `drm_pagemap_migrate_to_devmem` unconditionally casts device private pages to `drm_pagemap_zdd` without verifying they belong to `drm_pagemap`, leading to type confusion when migrating memory mapped from other drivers.
- [Critical] On error paths, `drm_pagemap_migrate_to_devmem` leaves raw device PFNs uncleared in `migrate.dst`, causing `migrate_vma_pages()` to dereference garbage pointers and panic.
- [Critical] `drm_pagemap_migrate_to_devmem` assumes that `ops->populate_devmem_pfn` returns a contiguous 2MB block when the source page is a THP, leading to severe memory corruption if the device allocator returns fragmented pages.
- [High] Iterating over `migrate.src` using `folio_order(page_folio(src_page))` incorrectly skips array elements for PTE-mapped THPs, ignoring partial migration failures and inflating counters.
--

--- Patch [1]: [PATCH] drm/pagemap: Remove unreachable code in drm_pagemap_migrate_to_devmem() ---
commit bc70ff651e63ccc5d92c40ccbcee5813a9cda5f6
Author: Lu Yao <yaolu@kylinos.cn>

drm/pagemap: Remove unreachable code in drm_pagemap_migrate_to_devmem()

If it can satisfy the condition of "page_pgmap(src_page) == pagemap
&& ! mdetails->can_migrate_same_pagemap", it will be return -EBUSY in
the precondition checks above the loop.

> diff --git a/drivers/gpu/drm/drm_pagemap.c b/drivers/gpu/drm/drm_pagemap.c
> index 15c78eca180b7..b2cade168cedc 100644
> --- a/drivers/gpu/drm/drm_pagemap.c
> +++ b/drivers/gpu/drm/drm_pagemap.c
> @@ -744,12 +744,6 @@ 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);

[Severity: Critical]
This is a pre-existing issue, but does drm_pagemap_migrate_to_devmem() blindly
cast device private pages to drm_pagemap_zdd without verifying they belong to
the drm_pagemap subsystem?

If a user maps device private memory from another driver (like Nouveau or
AMDGPU) and attempts to migrate it to a drm_pagemap-backed device, the page
will have a different pgmap->ops and its zone_device_data will point to a
driver-specific structure. Could dereferencing src_zdd->dpagemap then cause a
type confusion, leading to arbitrary memory reads and kernel panics?

>  
> -			if (page_pgmap(src_page) == pagemap &&
> -			    !mdetails->can_migrate_same_pagemap) {
> -				migrate.dst[i] = 0;
> -				own_pages++;
> -				goto next;
> -			}

[Severity: High]
Is it safe to remove this safeguard?

The commit message claims this condition is caught in the first loop and
returns -EBUSY. However, the first loop calculates:

nr_pages = NR_PAGES(folio_order(page_folio(src_page)))

It then increments i by nr_pages, skipping entries in migrate.src for any
page belonging to a large folio, even if it is PTE-mapped.

This second loop increments i by HPAGE_PMD_ORDER only if
MIGRATE_PFN_COMPOUND is set; otherwise it increments by 1. Because of this
discrepancy, the second loop can iterate over pages that the first loop
completely skipped.

If a skipped page belongs to the same pagemap and
!mdetails->can_migrate_same_pagemap is true, doesn't removing this block
allow unsupported same-pagemap migrations to proceed, potentially causing
hardware state corruption or deadlocks?

[Severity: Critical]
This is a pre-existing issue, but does this loop assume
ops->populate_devmem_pfn returns a contiguous 2MB block when the source
page is a THP?

When MIGRATE_PFN_COMPOUND is set, the code sets order = HPAGE_PMD_ORDER
and overwrites migrate.dst[i + 1] through migrate.dst[i + 511] with 0.
It then calls drm_pagemap_get_devmem_page() which forcefully initializes
the next 511 contiguous physical pages as a compound folio.

If the device allocator returns a fragmented list of smaller pages (e.g.,
non-contiguous 4KB blocks), does this blindly initialize subsequent physical
pages, silently corrupting metadata of unrelated device memory and leaking
the non-contiguous device PFNs that were overwritten with 0?

>  			cur.dpagemap = src_zdd->dpagemap;
>  			cur.ops = src_zdd->devmem_allocation->ops;
>  			cur.device = cur.dpagemap->drm->dev;
> @@ -786,7 +780,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 leave raw device
PFNs uncleared in migrate.dst?

If drm_pagemap_migrate_range() fails at index i, the function truncates
the local npages variable to i + 1 and jumps to err_finalize.
drm_pagemap_migration_unlock_put_pages() clears only up to index i,
leaving elements from i + 1 to the original npages - 1 in migrate.dst
populated with raw device PFNs.

When the code falls through to err_aborted_migration and calls
migrate_vma_pages(&migrate), it iterates up to the original unmodified
migrate.npages. Could migrate_vma_pages() then dereference these garbage
pointers (if the raw PFN is odd), leading to a severe use-after-free and
kernel panic?

>  
> -next:
>  		i += NR_PAGES(order);

[Severity: High]
This is a pre-existing issue, but does iterating over migrate.src using
folio_order(page_folio(src_page)) in the loops incorrectly skip array
elements?

For PTE-mapped THPs, this assumes the next 2^order elements in the array
belong to the same folio, skipping up to 511 elements in the array and
completely missing validation of subsequent pages.

If migrate_vma_pages() fails to migrate one of the skipped pages, won't
the third loop miss this failure and incorrectly tally migrated_pages,
allowing the GPU to access un-migrated pages?

>  	}
>  
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730061635.132619-1-yaolu@kylinos.cn?part=1