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
