Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Incorrect PFN array iteration when encountering large folios causes unrelated folios to be skipped, and leaves tail pages improperly processed. Pre-existing issues: - [High] Out-of-bounds array write in `migrate_device_pfns()` when processing compound pages. -- --- Patch [1]: [PATCH v5] drm/pagemap: Prevent double migration of device pages --- commit e82d028d2739c02084b327b5c44a4c4cb949a76c Author: Arvind Yadav <[email protected]> Subject: drm/pagemap: Prevent double migration of device pages This patch aims to prevent soft lockups caused by double migration of device-private folios. It marks successfully migrated folios in their zone_device_data and makes both CPU-fault and raw-PFN migration paths skip folios carrying this flag. > 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_skip_retired_pages(unsigned long *src_pfns, > + unsigned long npages) > +{ > + unsigned long i = 0; > + > + while (i < npages) { > + struct page *page = migrate_pfn_to_page(src_pfns[i]); > + > + if (!page) { > + i++; > + continue; > + } > + > + if ((src_pfns[i] & MIGRATE_PFN_MIGRATE) && > + is_device_private_page(page) && > + (drm_pagemap_page_get_flags(page) & > + DRM_PAGEMAP_ZDD_FLAG_MIGRATED)) > + src_pfns[i] &= ~MIGRATE_PFN_MIGRATE; > + [Severity: High] Does advancing by folio_nr_pages() here leave tail pages improperly processed? By only clearing the MIGRATE_PFN_MIGRATE bit for src_pfns[i] (the head page) and then skipping ahead, any subsequent tail pages in the src_pfns array will remain flagged. Downstream functions like migrate_vma_pages() process every array entry, so they might see the un-cleared tail pages and attempt to migrate them individually. Could this defeat the fix and trigger the double migration lockups again? Also, if the PFN array starts on a tail page due to VMA boundary clamping, will advancing by the full folio_nr_pages() overshoot the folio's actual footprint in the array and skip completely unrelated folios? This same pattern appears in drm_pagemap_retire_migrated_pages() just above. There are also pre-existing loops in drm_pagemap_evict_to_ram(), __drm_pagemap_migrate_to_ram(), and drm_pagemap_migrate_populate_ram_pfn() that advance using i += NR_PAGES(order) and might suffer from this same tail-page stepping issue. > + i += folio_nr_pages(page_folio(page)); > + } > +} [ ... ] > @@ -1143,6 +1240,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); > + [Severity: High] This is a pre-existing issue, but does migrate_device_pfns() (called just above this in drm_pagemap_evict_to_ram) lack bounds checking when processing compound pages? When the following loop in mm/migrate_device.c:migrate_device_pfns() encounters a compound page, it zeroes out subsequent entries in the src_pfns array up to the full size of the compound page: mm/migrate_device.c:migrate_device_pfns() { ... for (j = 1; j < nr; j++) src_pfns[i+j] = 0; ... } If a large folio extends beyond the npages limit, such as when the caller passes a clamped range or an unaligned array size, this loop will write zeros past the end of the src_pfns allocation. Could this corrupt adjacent memory, like the uninitialized dst array allocated concurrently? > err = drm_pagemap_migrate_populate_ram_pfn(NULL, NULL, npages, &mpages, > src, dst, 0); > if (err || !mpages) [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
