Re: [PATCH v14 05/12] mm/hmm: implement folio split for migrate needs in HMM pagewalk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm: livelock in hmm_vma_walk_split() due to refcount elevation before trylock
- [High] mm: lock leak and VM_BUG_ON in hmm_vma_walk_split() for tail fault_page
- [Medium] mm: spurious migration failure in hmm_vma_walk_split() from TOCTOU split
--

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

mm/hmm: implement folio split for migrate needs in HMM pagewalk

Implement the helper to split PMD size folios during pagewalk.
Splitting is needed if the start and end addresses are
not PMD aligned, or setting up pmd migration entry fails.

> diff --git a/mm/hmm.c b/mm/hmm.c
> index 4805c88347e67..e6469ef4ae6ad 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -514,10 +514,51 @@ static int hmm_vma_walk_split(pmd_t *pmdp,

[ ... ]

> +	folio = pmd_folio(*pmdp);
> +	if (is_huge_zero_folio(folio)) {
> +		spin_unlock(ptl);
> +		split_huge_pmd(walk->vma, pmdp, addr);
> +	} else {
> +		folio_get(folio);
> +		spin_unlock(ptl);
> +
> +		if (folio != fault_folio) {
> +			if (unlikely(!folio_trylock(folio))) {

[Severity: High]
Does this create a livelock under contention?

If multiple threads race in hmm_vma_walk_split(), Thread B might elevate the
refcount here before Thread A (which already holds the lock) calls
split_folio(). When Thread A calls split_folio(), the inflated refcount will
cause split_folio() to fail with -EAGAIN. Thread A returns -EAGAIN and drops
the lock, while Thread B fails folio_trylock() and returns -EBUSY. Both can
then retry infinitely.

Could we attempt folio_trylock() before elevating the refcount?

> +				folio_put(folio);
> +				ret = -EBUSY;
> +				goto out;
> +			}
> +		}  else {
> +			folio_put(folio);
> +		}
> +
> +		ret = split_folio(folio);

[Severity: Medium]
Will this spuriously fail migration if the folio is split concurrently?

If another thread concurrently splits the THP during the lockless window
before we acquire the lock, split_folio() will return -EINVAL. Passing this
error up to hmm_vma_walk_pmd() aborts the pagewalk and marks the range with
HMM_PFN_ERROR instead of treating it as a condition to retry the walk with
the new PTEs.

[Severity: High]
Does this correctly handle cases where fault_page was a tail page?

If hmm_vma_walk_split() is called with migrate->fault_page as a tail page,
fault_folio is resolved as the head page. Before the split,
fault_folio == folio evaluates to true, so we bypass unlocking the folio on
the assumption the caller owns the lock.

However, split_folio() leaves the new order-0 tail pages unlocked.
When the caller subsequently uses the now order-0 fault_page and calls
folio_unlock(page_folio(fault_page)), it will crash with
VM_BUG_ON_FOLIO(!folio_test_locked(folio)) because the tail page is
unlocked. In addition, the original head page lock will be permanently
leaked.

> +		if (fault_folio != folio) {
> +			folio_unlock(folio);
> +			folio_put(folio);
> +		}
> +	}
> +out:
> +	return ret;
> +}

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