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 <[email protected]> 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/[email protected]?part=1
