Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Data race on folio->page.zone_device_data due to non-atomic read-modify-write. Pre-existing issues: - [Critical] Out-of-bounds write in migrate_device_pfns and migrate_device_range. - [High] Use-after-free when accessing page metadata after dropping the reference. - [High] DMA mappings are leaked in error paths because unmapping is skipped when the PFN array is zeroed. -- --- Patch [1]: [PATCH v6] drm/pagemap: Prevent double migration of device pages --- commit 8398dc4cfadc132c4a2fb375e9dd9f211e6bb842 Author: Arvind Yadav <[email protected]> drm/pagemap: Prevent double migration of device pages A device-private folio migrated to system memory by a CPU fault can remain reachable through the raw-PFN eviction path until migration finalization drops the source reference. > diff --git a/drivers/gpu/drm/drm_pagemap.c b/drivers/gpu/drm/drm_pagemap.c > --- a/drivers/gpu/drm/drm_pagemap.c > +++ b/drivers/gpu/drm/drm_pagemap.c [ ... ] > +static void drm_pagemap_page_set_flags(struct page *page, > + unsigned long flags) > +{ > + struct folio *folio = page_folio(page); > + unsigned long old; > + > + if (WARN_ON_ONCE(flags & ~DRM_PAGEMAP_ZDD_FLAG_MASK)) > + return; > + > + old = (unsigned long)folio_zone_device_data(folio); > + folio_set_zone_device_data(folio, (void *)(old | flags)); > +} [Severity: High] Can this non-atomic read-modify-write race with concurrent lockless readers? If a CPU fault handler calls __drm_pagemap_migrate_to_ram() and reads this pointer locklessly via drm_pagemap_page_zone_device_data(), and this function updates it concurrently, could the lack of READ_ONCE()/WRITE_ONCE() protection result in a torn pointer read and subsequent pointer corruption? [ ... ] > @@ -1148,6 +1253,8 @@ int drm_pagemap_evict_to_ram(struct drm_pagemap_devmem > *devmem_allocation) > if (err) > goto err_free; > > + drm_pagemap_skip_retired_pages(src, npages); > + > err = drm_pagemap_migrate_populate_ram_pfn(NULL, NULL, npages, &mpages, > src, dst, 0); [Severity: Critical] This is a pre-existing issue, but does migrate_device_pfns() (called just before this snippet) have an out-of-bounds write if the migrated memory range ends midway through a large folio? Looking at migrate_device_pfns() in mm/migrate_device.c: for (j = 1; j < nr; j++) src_pfns[i+j] = 0; Since the inner loop doesn't check if i + j < npages, if a caller passes an array of pfns where the last elements belong to a large folio, this clears subsequent elements past the end of the src_pfns array, potentially corrupting adjacent memory like the dst array. [Severity: High] This is also a pre-existing issue, but is there a use-after-free in the error cleanup path of drm_pagemap_migrate_populate_ram_pfn() called here? When cleaning up after a failure, the code drops the page reference and then immediately accesses its metadata to determine the loop advancement step: drm_pagemap_migrate_populate_ram_pfn() { ... free_pages: ... put_page(page); mpfn[i] = 0; order = folio_order(page_folio(page)); ... } If put_page() drops the last reference and frees the page, isn't the subsequent call to folio_order(page_folio(page)) accessing freed memory? [ ... ] > @@ -1308,6 +1418,7 @@ static int __drm_pagemap_migrate_to_ram(struct > vm_area_struct *vas, > if (err) > drm_pagemap_migration_unlock_put_pages(npages, migrate.dst); > migrate_vma_pages(&migrate); > + drm_pagemap_retire_migrated_pages(migrate.src, npages); > migrate_vma_finalize(&migrate); > if (dev) > drm_pagemap_migrate_unmap_pages(dev, pagemap_addr, migrate.dst, [Severity: High] This is a pre-existing issue, but are DMA mappings leaked in this error path? If an error occurs, drm_pagemap_migration_unlock_put_pages() is called which zeroes out the migrate.dst array: drm_pagemap_migration_unlock_put_pages() { ... drm_pagemap_migration_unlock_put_folio(folio); migrate_pfn[i] = 0; ... } Subsequently, drm_pagemap_migrate_unmap_pages() is called to unmap the DMA addresses, but it explicitly skips entries where the page is NULL: drm_pagemap_migrate_unmap_pages() { ... if (!page || !pagemap_addr[i].addr || dma_mapping_error(dev, pagemap_addr[i].addr)) goto next; ... } Since all pages in the array were just zeroed, migrate_pfn_to_page() will return NULL for all of them, bypassing the unmap and leaving the DMA mappings orphaned. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
