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ä <[email protected]> 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/[email protected]?part=5
