Re: [PATCH v4] drm/pagemap: Prevent double migration of device pages

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

Pre-existing issues:
- [High] Double migration of device pages due to missing flag in peer-to-peer migration.
- [High] DMA mappings are leaked on the error path because the destination PFN array is zeroed out before unmapping.
- [High] Use-after-free of folio metadata in the error path of `drm_pagemap_migrate_populate_ram_pfn`.
--

--- Patch [1]: [PATCH v4] drm/pagemap: Prevent double migration of device pages ---
commit ddf1406cd4fcd17693eecb3abf05b87068425942
Author: Arvind Yadav <[email protected]>
Subject: drm/pagemap: Prevent double migration of device pages

This patch addresses a double migration bug involving device-private folios.
It flags successfully migrated device folios using a low bit in their
zone_device_data before migration finalization, ensuring that raw-PFN
eviction paths skip them.

> --- a/drivers/gpu/drm/drm_pagemap.c
> +++ b/drivers/gpu/drm/drm_pagemap.c
[ ... ]
> @@ -1143,6 +1250,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: High]
This is a pre-existing issue, but does this call to
drm_pagemap_migrate_populate_ram_pfn() trigger a use-after-free on its error
path?

If page allocation fails inside drm_pagemap_migrate_populate_ram_pfn(), the
cleanup path jumps to the free_pages label:

free_pages:
	for (i = 0; i < npages;) {
		struct page *page = migrate_pfn_to_page(mpfn[i]);
		unsigned int order = 0;

		if (!page)
			goto next_put;

		put_page(page);
		mpfn[i] = 0;

		order = folio_order(page_folio(page));

Is it safe to read the folio_order metadata after put_page() has dropped the
reference and potentially freed the page? It looks like this could result in
garbage order values, leading to incorrect loop increments and missed cleanups.

>  	if (err || !mpages)
>  		goto err_finalize;
[ ... ]
> @@ -1173,6 +1282,7 @@ int drm_pagemap_evict_to_ram(struct drm_pagemap_devmem *devmem_allocation)
>  	if (err)
>  		drm_pagemap_migration_unlock_put_pages(npages, dst);
>  	migrate_device_pages(src, dst, npages);
> +	drm_pagemap_retire_migrated_pages(src, npages);
>  	migrate_device_finalize(src, dst, npages);
>  	drm_pagemap_migrate_unmap_pages(devmem_allocation->dev, pagemap_addr, dst, npages,
>  					DMA_FROM_DEVICE, &state);

[Severity: High]
This is a pre-existing issue, but I am wondering if the peer-to-peer migration
path is also vulnerable to this same double-migration bug?

Looking at drm_pagemap_migrate_to_devmem(), it finalizes the migration
without calling drm_pagemap_retire_migrated_pages() to set the flag on the
source folio:

err_aborted_migration:
	migrate_vma_pages(&migrate);

	for (i = 0; !err && i < npages;) {
...
	migrate_vma_finalize(&migrate);

Could a concurrent eviction select the same device folio, fail to skip it
since the migrated flag is missing, and attempt to migrate it again?

[ ... ]
> @@ -1303,6 +1413,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 does this error path permanently leak DMA
mappings?

If an error occurs, drm_pagemap_migration_unlock_put_pages() is called, which
zeroes out the migrate.dst array internally:

static void drm_pagemap_migration_unlock_put_pages(...)
{
...
		drm_pagemap_migration_unlock_put_folio(folio);
		migrate_pfn[i] = 0;
...
}

Then shortly after, we pass that same zeroed array to
drm_pagemap_migrate_unmap_pages().

Inside drm_pagemap_migrate_unmap_pages(), it retrieves the page:

		struct page *page = migrate_pfn_to_page(migrate_pfn[i]);

		if (!page || !pagemap_addr[i].addr || dma_mapping_error(...))
			goto next;

Since the array entries were zeroed, migrate_pfn_to_page(0) returns NULL,
causing the loop to bypass dma_unmap_page() entirely.

>  						npages, DMA_FROM_DEVICE,
>  						&state);
>  err_free:
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1