* Re: [PATCH] drm/pagemap: Remove unreachable code in drm_pagemap_migrate_to_devmem()
2026-07-30 6:16 [PATCH] drm/pagemap: Remove unreachable code in drm_pagemap_migrate_to_devmem() yaolu
@ 2026-07-30 6:30 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-07-30 6:30 UTC (permalink / raw)
To: yaolu; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 2+ messages in thread