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

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

New issues:
- [High] VMA virtual address changes (e.g., via mremap) break the retirement bitmap indexing, leading to massive memory allocation failures or infinite page fault loops.
- [Medium] High-order kzalloc scaling with user-controlled VMA size is highly susceptible to fragmentation failures.

Pre-existing issues:
- [High] Accessing folio metadata after dropping the page reference in the error path.
- [High] DMA mappings are leaked on error paths because the destination page array is cleared before unmapping.
--

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

A device page migrated to system memory by a CPU fault can remain
referenced after migration completes. During this window, raw-PFN eviction
can collect the same device PFN and migrate it again.

> 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
> @@ -78,29 +85,38 @@ struct drm_pagemap_zdd {
>  static struct drm_pagemap_zdd *
> -drm_pagemap_zdd_alloc(struct drm_pagemap *dpagemap)
> +drm_pagemap_zdd_alloc(struct drm_pagemap *dpagemap, unsigned long start,
> +		      unsigned long npages)
>  {
>  	struct drm_pagemap_zdd *zdd;
>  
> -	zdd = kmalloc_obj(*zdd);
> +	zdd = kzalloc(struct_size(zdd, retire_map, BITS_TO_LONGS(npages)),
> +		      GFP_KERNEL);

[Severity: Medium]
Does this allocation scale linearly with a user-supplied VMA size?

If a user maps a large region of device memory, this kzalloc request in
drm_pagemap_zdd_alloc() might require a high-order contiguous allocation.
This could fail easily on a fragmented system.

Would it be safer to use kvzalloc here instead?

[ ... ]

> @@ -875,32 +875,25 @@ static int drm_pagemap_migrate_populate_ram_pfn
>  	return 0;
>  
>  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));

[Severity: High]
This is a pre-existing issue, but can this dereference freed memory?

By the time folio_order() is called in the error path of
drm_pagemap_migrate_populate_ram_pfn(), the reference to page has already
been dropped via put_page(). This means the underlying folio metadata could
be freed or reused before it is read.

Should the order be cached before dropping the reference?

[ ... ]

> @@ -1295,10 +1295,12 @@ int drm_pagemap_evict_to_ram(struct drm_pagemap_devmem *devmem_allocation)
>  err_finalize:
>  	if (err)
>  		drm_pagemap_migration_unlock_put_pages(npages, dst);
>  	migrate_device_pages(src, dst, npages);
> +	/* Raw-PFN eviction: array starts at allocation offset zero. */
> +	drm_pagemap_retire_migrated_pages(src, npages, 0);
>  	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 does this error path permanently leak DMA
mappings?

Looking at drm_pagemap_evict_to_ram(), if there is an error it calls
drm_pagemap_migration_unlock_put_pages(), which sets the entries in the dst
array to 0. When drm_pagemap_migrate_unmap_pages() subsequently runs using
the same dst array, it skips unmapping because the entries are now null.

Should the unmap occur before the array is zeroed?

[ ... ]

> @@ -1369,6 +1373,10 @@ static int __drm_pagemap_migrate_to_ram(struct vm_area_struct *vas,
>  	if (end > vas->vm_end)
>  		end = vas->vm_end;
>  
> +	/* Keep the range within the ZDD allocation so retirement offsets stay valid. */
> +	start = max(start, zdd->range_start);
> +	end = min(end, zdd->range_start + (zdd->range_npages << PAGE_SHIFT));
> +
>  	migrate.start = start;
>  	migrate.end = end;
>  	npages = npages_in_range(start, end);

[Severity: High]
What happens if userspace moves the VMA to a different virtual address via
mremap?

If the VMA is moved to a higher address, clipping against the static
zdd->range_start in __drm_pagemap_migrate_to_ram() could result in start
being greater than end. This would cause an underflow in npages_in_range(),
leading to an extremely large kvcalloc attempt. If kvcalloc succeeds, the
migrate_vma_setup() loop immediately exits and returns 0 migrated pages,
which could result in an infinite page fault loop.

How should we handle moved VMAs here?

[ ... ]

> @@ -1433,6 +1435,8 @@ static int __drm_pagemap_migrate_to_ram(struct vm_area_struct *vas,
>  err_finalize:
>  	if (err)
>  		drm_pagemap_migration_unlock_put_pages(npages, migrate.dst);
>  	migrate_vma_pages(&migrate);
> +	drm_pagemap_retire_migrated_pages(migrate.src, npages,
> +					  (start - zdd->range_start) >> PAGE_SHIFT);
>  	migrate_vma_finalize(&migrate);
>  	if (dev)
>  		drm_pagemap_migrate_unmap_pages(dev, pagemap_addr, migrate.dst,
>  						npages, DMA_FROM_DEVICE,
>  						&state);

[Severity: High]
This is a pre-existing issue, but does this error path permanently leak DMA
mappings?

Looking at __drm_pagemap_migrate_to_ram(), similar to the issue above, if
there is an error it calls drm_pagemap_migration_unlock_put_pages() which
zeroes the migrate.dst array. The subsequent call to
drm_pagemap_migrate_unmap_pages() then skips unmapping because the entries
are null.

Should the unmap occur before the array is zeroed?

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