Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] mm: set_pmd_migration_entry() uses wrong subpage for >2MB folios - [High] mm: pmd_folio() dereferences bogus pointer for non-present PMD leaf entries - [High] mm: unreachable device-private PMD branch causes infinite fault loops --
commit 191891c0dfbd0707d81f83e9fa9b79a00e1693ac Author: Mika Penttilä <[email protected]> mm/hmm: migrate collection in HMM pagewalk - pmd level Implement hmm_vma_handle_migrate_prepare_pmd() for HMM pagewalk migration. It makes migration decisions, possibly splits folios, and inserts migration pmds. > diff --git a/mm/hmm.c b/mm/hmm.c > index b3b79d13c7978..a1459bd34c04e 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -552,8 +552,100 @@ static int hmm_vma_handle_migrate_prepare_pmd(const > struct mm_walk *walk, [ ... ] > + if (pmd_trans_huge(*pmdp)) { > + if (!(minfo & MIGRATE_VMA_SELECT_SYSTEM)) > + goto out; > + > + folio = pmd_folio(*pmdp); [Severity: High] Does this code handle non-present PMD leaf entries correctly? When pmd_trans_huge() is called, it can return true for non-present PMD leaf entries like migration or device-private swap entries because they have the huge bit set. If that happens, this path will unconditionally call pmd_folio(), which can produce a bogus struct page pointer because swap metadata is interpreted as a PFN. Could this lead to an invalid memory access if userspace triggers HMM migration on memory regions with these entries? > + if (is_huge_zero_folio(folio)) > + return hmm_pfns_fill(start, end, hmm_vma_walk, 0); > + > + } else if (!pmd_present(*pmdp)) { [Severity: High] Is this branch actually reachable for device-private PMDs? According to the MM subsystem guide, pmd_present() returns true for non-present PMD leaf entries, which includes device-private entries. That would make !pmd_present(*pmdp) evaluate to false. If device-private PMDs bypass the huge PMD check and fail this condition, they will fall through to the final else block and return -EBUSY. Returning -EBUSY causes the outer walk_page_range() loop in hmm_range_fault_locked() to retry the same address indefinitely. Can this cause a denial of service via an infinite loop? > + const softleaf_t entry = softleaf_from_pmd(*pmdp); [ ... ] > + if (thp_migration_supported() && > + (migrate->flags & MIGRATE_VMA_SELECT_COMPOUND) && > + (IS_ALIGNED(start, HPAGE_PMD_SIZE) && > + IS_ALIGNED(end, HPAGE_PMD_SIZE))) { > + struct page_vma_mapped_walk pvmw = { > + .ptl = hmm_vma_walk->ptl, > + .address = start, > + .pmd = pmdp, > + .vma = walk->vma, > + }; > + > + hmm_pfn[0] |= HMM_PFN_MIGRATE | HMM_PFN_COMPOUND; > + > + r = set_pmd_migration_entry(&pvmw, folio_page(folio, 0)); [Severity: Critical] Will this use the correct subpage for folios larger than 2MB? For file-backed folios that are larger than PMD size (e.g., 1GB), they can be mapped by multiple PMDs. Using folio_page(folio, 0) always evaluates to the head page of the large folio, regardless of which 2MB chunk the PMD actually maps. Since set_pmd_migration_entry() uses page_to_pfn() on the provided page, all PMD migration entries for the large folio might encode the PFN of the head chunk. Upon migration restoration, could this silently corrupt data by mapping all PMDs to the first 2MB chunk's physical memory? Would it be safer to use pmd_page(*pmdp) here to get the specific subpage? > + if (r) { > + hmm_pfn[0] &= ~(HMM_PFN_MIGRATE | HMM_PFN_COMPOUND); > + r = -ENOENT; // fallback -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
