Re: [PATCH v14 06/12] mm/hmm: migrate collection in HMM pagewalk - pte level

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm: hmm: double unlock of caller-locked fault_folio on rmap share failure
- [High] mm: hmm: livelock in large folio split due to refcount inflation before locking
- [High] mm: hmm: kernel crash due to split_folio() on non-folio compound pages
--

commit 5d5a85160b145735bf86c066185b4f70abea7ca4
Author: Mika Penttilä <mpenttil@redhat.com>

mm/hmm: migrate collection in HMM pagewalk - pte level
    
Implement hmm_vma_handle_migrate_prepare() and migrate_vma_split_folio() for
HMM pagewalk-based migration. The hmm_vma_handle_migrate_prepare() function
is responsible for migration decisions, possibly splitting folios, and
inserting migration entries.

> diff --git a/mm/hmm.c b/mm/hmm.c
> index e6469ef4ae6ad..b3b79d13c7978 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
[ ... ]
> +static int migrate_vma_split_folio(struct folio *folio,
> +				   struct page *fault_page,
> +				   struct hmm_vma_walk *hmm_vma_walk,
> +				   pte_t *ptep)
> +{
> +	int ret;
> +	struct folio *fault_folio = fault_page ? page_folio(fault_page) : NULL;
> +	struct folio *new_fault_folio = NULL;
> +
> +	if (folio != fault_folio)
> +		folio_get(folio);
> +
> +	pte_unmap_unlock(ptep, hmm_vma_walk->ptl);
> +	hmm_vma_walk->ptelocked = false;
> +
> +	if (folio != fault_folio)
> +		folio_lock(folio);
> +
> +	ret = split_folio(folio);

[Severity: High]
Does taking the reference via folio_get() before blocking on folio_lock()
create a livelock when multiple threads fault on the same large folio?

If multiple threads execute migrate_vma_split_folio() simultaneously, they
would all elevate the refcount and block on the lock. When the lock is finally
acquired, split_folio() expects the refcount to be exactly
folio_expected_ref_count() + 1. The extra references held by the waiting
threads would cause split_folio() to return -EAGAIN repeatedly, preventing
forward progress.

[ ... ]
> +static int hmm_vma_handle_migrate_prepare(const struct mm_walk *walk,
> +					  pmd_t *pmdp,
> +					  pte_t *ptep,
> +					  unsigned long addr,
> +					  unsigned long *hmm_pfn,
> +					  bool *unmapped)
> +{
[ ... ]
> +	} else {
> +		pfn = pte_pfn(pte);
> +		if (is_zero_pfn(pfn) &&
> +		    (minfo & MIGRATE_VMA_SELECT_SYSTEM)) {
> +			*hmm_pfn = HMM_PFN_MIGRATE;
> +			goto out;
> +		}
> +		page = vm_normal_page(walk->vma, addr, pte);
> +		if (page && !is_zone_device_page(page) &&
> +		    !(minfo & MIGRATE_VMA_SELECT_SYSTEM)) {
> +			goto out;
> +		} else if (page && is_device_coherent_page(page)) {
> +			pgmap = page_pgmap(page);
> +
> +			if (!(minfo &
> +			      MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
> +			    pgmap->owner != migrate->pgmap_owner)
> +				goto out;
> +		}
> +
> +		folio = page ? page_folio(page) : NULL;
> +		if (folio && folio_test_large(folio)) {
> +			int ret;
> +
> +			ret = migrate_vma_split_folio(folio,
> +						      migrate->fault_page,
> +						      hmm_vma_walk,
> +						      ptep);

[Severity: High]
Can this split_folio() crash if a driver mapped a raw compound page into the
VMA using vm_insert_page()?

The large folio check here uses folio_test_large() which returns true for any
compound page (since PG_head is set). However, page_folio() blindly casts
compound pages. If the compound page lacks initialized folio metadata (like a
valid mapping or LRU state), splitting it will corrupt memory. The structural
validation gate checking for a valid mapping (!page || !page->mapping) is only
performed after this split attempt.

> +			if (ret)
> +				goto out_error;
> +			return -EAGAIN;
> +		}
> +
> +		writable = pte_write(pte);
> +	}
> +
> +	if (!page || !page->mapping)
> +		goto out;
[ ... ]
> +	if (fault_folio == folio || folio_trylock(folio)) {
> +		anon_exclusive = folio_test_anon(folio) &&
> +			PageAnonExclusive(page);
> +
> +		if (pte_present(pte))
> +			flush_cache_page(walk->vma, addr, pfn);
> +
> +		if (anon_exclusive) {
> +			pte = ptep_clear_flush(walk->vma, addr, ptep);
> +
> +			if (folio_try_share_anon_rmap_pte(folio, page)) {
> +				set_pte_at(mm, addr, ptep, pte);
> +				folio_unlock(folio);

[Severity: High]
Does this error path unconditionally unlock a folio that the caller might hold
the lock for?

When entering this block, if fault_folio == folio, the code bypasses calling
folio_trylock(folio) because the caller already owns the lock. If
folio_try_share_anon_rmap_pte() subsequently fails, calling folio_unlock()
here would destroy the caller's lock state, causing a double-unlock crash
when the caller attempts to unlock it later.

> +				folio_put(folio);
> +				goto out;
> +			}
> +		} else {
> +			pte = ptep_get_and_clear(mm, addr, ptep);
> +		}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922053421.4092027-1-mpenttil@redhat.com?part=6