Re: [PATCH v2 4/4] mm/mlock: migrate folios out of CMA when mlocking a range

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The `is_migrate_cma_page` condition is inverted in the PTE loop, isolating non-CMA pages instead of CMA pages.
- [High] Setting `ACTION_AGAIN` on `pte_offset_map_lock()` failure creates an infinite loop.
- [High] Folios are isolated and added to a list without elevating their refcount, causing Use-After-Free.
- [High] Recursive `mmap_read_lock` acquisition creates an ABBA self-deadlock risk.
- [High] Locking order inversion between `pte_unmap` and `spin_unlock` causes a Use-After-Free of the spinlock.
- [Medium] Unbounded folio isolation aggregates too many pages off the LRU, leading to resource exhaustion.
--

--- Patch [4]: [PATCH v2 4/4] mm/mlock: migrate folios out of CMA when mlocking a range ---
commit 95a15b2b2b9bbf36b5f4e3a9dd716ec837e02900
Author: Wandun Chen <chenwandun@xxxxxxxxxxx>
mm/mlock: migrate folios out of CMA when mlocking a range

This commit attempts to reduce latency spikes on RT kernels by migrating
CMA pages out of mlocked regions during the mlock setup path. It introduces
a page walker to collect migratable CMA pages and migrates them using
migrate_pages().

Link: https://lore.kernel.org/all/CAPTztWZpnX1j8-7yeppVUsxE=O9hbVeqricDjZt8_pnN7a-kBQ@xxxxxxxxxxxxxx/#t

> diff --git a/mm/mlock.c b/mm/mlock.c
> index ac65de40b22b9..f56c685533f55 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
[ ... ]
> +static int mlock_collect_migratable_pte_range(pmd_t *pmd, unsigned long addr,
> +			unsigned long end, struct mm_walk *walk)
> +{
[ ... ]
> +	ptl = pmd_trans_huge_lock(pmd, vma);
> +	if (ptl) {
[ ... ]
> +		folio = pmd_folio(*pmd);
> +		if (folio_is_zone_device(folio))
> +			goto out;
> +		if (is_migrate_cma_page(&folio->page))
> +			isolate_folio_to_list(folio, folio_list);

[Severity: High]
Does this code risk a Use-After-Free on the folio? In the PMD path of
mlock_collect_migratable_pte_range(), isolate_folio_to_list() is called
without first obtaining a reference via folio_get(). Once the page table lock
is dropped at the end of the function, a concurrent thread could unmap the
region, dropping the mapcount and freeing the folio while it is still on
folio_list. When migrate_pages() iterates over the list later, it might
access freed memory.

> +		goto out;
> +	}
> +
> +	start_pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
> +	if (!start_pte) {
> +		walk->action = ACTION_AGAIN;
> +		return 0;
> +	}

[Severity: High]
Could this create an infinite loop? When pte_offset_map_lock() returns NULL,
which happens non-transiently for migration entries or empty PMDs, setting
ACTION_AGAIN unconditionally tells the page walker to retry the exact same
PMD forever. This could hang the kernel thread if mlock() races with another
operation that installs migration entries or zaps the PMD.

> +
> +	for (pte = start_pte; addr != end; pte += step, addr += step * PAGE_SIZE) {
[ ... ]
> +		folio = vm_normal_folio(vma, addr, ptent);
> +		if (!folio || folio_is_zone_device(folio))
> +			continue;
> +		step = folio_mlock_step(folio, pte, addr, end);
> +		if (is_migrate_cma_page(&folio->page))
> +			continue;
> +		isolate_folio_to_list(folio, folio_list);

[Severity: High]
Is the logic inverted here? The condition causes CMA pages to be skipped
while erroneously isolating non-CMA pages for migration. Any mlock() call
touching regular memory might isolate and migrate those pages unnecessarily,
while skipping the CMA pages this patch was intended to target.

[Severity: High]
Can this lead to a Use-After-Free? Just like in the PMD path, the folio here
is isolated without a preceding folio_get() to ensure it survives outside the
page table lock.

> +	}
> +	pte_unmap(start_pte);
> +out:
> +	spin_unlock(ptl);

[Severity: High]
Is there a lock ordering inversion here? Calling pte_unmap() drops the RCU
read lock, which means the page table page could immediately be freed by
concurrent teardown like tlb_remove_table_rcu(). The subsequent
spin_unlock(ptl) might then access the spinlock residing within the freed
page table page on architectures with split PMD/PTE locks.

> +	cond_resched();
> +	return 0;
> +}
> +
> +static const struct mm_walk_ops mlock_collect_migratable_ops = {
> +	.pmd_entry	= mlock_collect_migratable_pte_range,
> +	.walk_lock	= PGWALK_RDLOCK,
> +};
> +
> +static void mlock_migrate_cma_range(unsigned long start, unsigned long len)
> +{
[ ... ]
> +	if (mmap_read_lock_killable(mm))
> +		goto out;
> +
> +	walk_page_range(mm, start, end, &mlock_collect_migratable_ops,
> +			&folio_list);

[Severity: High]
Does this introduce a recursive mmap_read_lock acquisition that could result
in a deadlock? mlock_migrate_cma_range() explicitly acquires the
mmap_read_lock. It then calls walk_page_range() using the
mlock_collect_migratable_ops, which specifies PGWALK_RDLOCK. The walker will
internally attempt to acquire the mmap_read_lock again. If a concurrent
writer attempts to acquire mmap_write_lock between these two read-lock
acquisitions, it will block waiting for the first read lock to be released,
and the second read lock will block waiting for the writer, creating an ABBA
self-deadlock.

[Severity: Medium]
Is it safe to accumulate an unbounded number of folios into a single list
during this page walk? For example, mlockall() invokes this for the entire
TASK_SIZE range. Millions of folios could be isolated into folio_list,
removing them from the LRU and hiding them from memory reclaim, potentially
leading to soft lockups or OOM situations.

> +	mmap_read_unlock(mm);
> +
> +	if (list_empty(&folio_list))
> +		goto out;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707125925.3725177-1-chenwandun1@xxxxxxxxx?part=4