Re: [PATCH] drm/pagemap: centralize migrate src base-page counting

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect base-page count calculation for large folios desynchronizes migration loops and skips valid entries.
- [High] Permanent device memory leak on migration error path.
--

commit f9a15fc79a089e7970a0abb86569d6cf4a696964
Author: Junhua Shen <Junhua.Shen@amd.com>

drm/pagemap: centralize migrate src base-page counting

This commit factors out the base-page count calculation for migrate.src
entries into a new drm_pagemap_src_pfn_nr_pages function. It applies this
new helper to the accounting loops to consistently handle compound pages.

> diff --git a/drivers/gpu/drm/drm_pagemap.c b/drivers/gpu/drm/drm_pagemap.c
> index 892b325fa99b8..a31cc4f0af68c 100644
> --- a/drivers/gpu/drm/drm_pagemap.c
> +++ b/drivers/gpu/drm/drm_pagemap.c
> @@ -554,6 +554,37 @@ static int drm_pagemap_migrate_range(struct drm_pagemap_devmem *devmem,
[ ... ]
> +static unsigned long drm_pagemap_src_pfn_nr_pages(unsigned long src_pfn,
> +						   struct page **src_page)
> +{
> +	struct page *page = NULL;
> +	unsigned long nr_pages = 1;
> +
> +	if (src_pfn & MIGRATE_PFN_VALID) {
> +		page = migrate_pfn_to_page(src_pfn);
> +		if (page)
> +			nr_pages = NR_PAGES(folio_order(page_folio(page)));

[Severity: High]
This is a pre-existing issue, but does computing the stride using the backing
folio's size skip independent PTE entries? If a VMA maps a transparent huge
page (THP) via PTEs, migrate_vma_setup produces individual base-page entries.
However, using folio_order(page_folio(page)) returns the full compound order
(e.g., 512). The loops incrementing by this value would then skip the next
511 valid entries in the migrate.src array.

[Severity: High]
This is also a pre-existing issue related to the above, but does this create
a desync between the migration steps? 

In the central loop in drivers/gpu/drm/drm_pagemap.c:drm_pagemap_migrate_to_devmem():

	for (i = 0; i < npages;) {
		unsigned int order = 0;
		...
		if (migrate.src[i] & MIGRATE_PFN_COMPOUND) {
			order = HPAGE_PMD_ORDER;
			...
		}
		...
		i += NR_PAGES(order);
	}

Because order is 0 for PTE-mapped THPs here (MIGRATE_PFN_COMPOUND is false),
this central loop correctly strides by 1. However, the accounting loops that
use the new drm_pagemap_src_pfn_nr_pages() helper will stride by the full
folio size. Will this result in device pages being allocated and processed
here, but skipped in the accounting phases?

[Severity: High]
There is another pre-existing issue on the migration error path. Does
truncating npages cause device pages to be permanently leaked? 

drivers/gpu/drm/drm_pagemap.c:drm_pagemap_migrate_to_devmem() {
	...
	err = ops->populate_devmem_pfn(devmem_allocation, npages, migrate.dst);
	...
	for (i = 0; i < npages;) {
		...
		err = drm_pagemap_migrate_range(...);
		if (err) {
			npages = i + 1;
			goto err_finalize;
		}
		...
	}
	...
err_finalize:
	if (err) {
		drm_pagemap_migration_unlock_put_pages(npages, migrate.dst);
		for (i = npages; i < npages_in_range(start, end); ++i)
			migrate.dst[i] = 0;
	}
	...
}

When drm_pagemap_migrate_range() fails, npages is truncated. The cleanup
code in err_finalize then only handles pages up to this truncated index.
The loop clearing migrate.dst[i] = 0 wipes the only references to the
remaining device pages originally allocated by ops->populate_devmem_pfn(),
ensuring they can never be freed. Could we avoid this by unlocking only
the processed pages while still freeing the remaining unprocessed device
pages?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828083652.40855-1-Junhua.Shen@amd.com?part=1