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