Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] mm/userfaultfd: UFFDIO_MOVE misses crash_memaction registration for swap cache - [Medium] mm: stale crash_memaction marks when moving pages to unmarked VMAs - [Low] mm/rmap: data race on vma->vm_flags in crash_mark_pages() --
commit 78115d9186907359bdb879efc1ef14d161aac39a Author: Jan Sebastian Götte <[email protected]> Subject: mm/rmap: Mark folios mapped into crash_memaction-marked VMAs This patch propagates crash_memaction marks set through madvise() to the crash_memaction registry. It registers folios of a VMA carrying VM_CRASH_MARK as they are mapped, and updates the registry when a folio moves. It also hooks into the free path to deregister pages and avoid tracking stale data. > diff --git a/include/linux/crash_memaction.h b/include/linux/crash_memaction.h > index 5f3e114c60ad7..2de60bc12dd9f 100644 > --- a/include/linux/crash_memaction.h > +++ b/include/linux/crash_memaction.h [ ... ] > @@ -50,6 +51,18 @@ static inline void crash_memaction_unmark_pfns(unsigned > long pfn, > __crash_memaction_unmark_pfns(pfn, nr_pages); > } > > +static inline void crash_mark_pages(struct page *page, int nr_pages, > + struct vm_area_struct *vma) > +{ > + if (!static_branch_unlikely(&crash_memaction_active)) > + return; > + > + if (likely(!(vma->vm_flags & VM_CRASH_MARK))) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: Low] Should this lockless read of vma->vm_flags use READ_ONCE()? The crash_mark_pages() function is called via folio_add_anon_rmap_ptes() during migration (such as in remove_migration_pte()) holding only the anon_vma lock. At the same time, mprotect() can update vma->vm_flags while holding the VMA lock, but not the anon_vma lock. Could this concurrent modification lead to a data race or torn reads during migration? > + return; > + > + __crash_memaction_mark_pfns(page_to_pfn(page), nr_pages); > +} > + > void crash_memaction_mark(void *addr, size_t size, int types); > void crash_memaction_unmark(void *addr, size_t size); > [ ... ] > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 3fb9504dff7a9..adbda968d58ef 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -2966,6 +2966,8 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t > *dst_pmd, pmd_t *src_pmd, pm > > folio_move_anon_rmap(src_folio, dst_vma); > src_folio->index = linear_anon_page_index(dst_vma, dst_addr); > + /* No rmap add, and the two VMAs need not agree on the flag. */ > + crash_mark_pages(&src_folio->page, HPAGE_PMD_NR, dst_vma); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: Medium] How are stale crash_memaction marks handled when moving to an unmarked VMA? If a process marks a VMA with MADV_CRASH_CACHE, then moves an exclusive anonymous page to an unmarked VMA via mremap() here in move_pages_huge_pmd(), the destination VMA is checked but the source VMA is not unmarked. Because the new VMA lacks the mark, a subsequent madvise(MADV_CRASH_RESET) will short-circuit and fail to unmark the pages. Will these pages remain permanently marked and excluded from kdump dumps until freed, potentially risking data loss? > > _dst_pmd = folio_mk_pmd(src_folio, dst_vma->vm_page_prot); > /* Follow mremap() behavior and treat the entry dirty after the > move */ [ ... ] > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > index b242fa8b22c8c..bb0a35d59daa9 100644 > --- a/mm/userfaultfd.c > +++ b/mm/userfaultfd.c > @@ -1326,6 +1326,7 @@ static long move_present_ptes(struct mm_struct *mm, > > folio_move_anon_rmap(src_folio, dst_vma); > src_folio->index = linear_anon_page_index(dst_vma, dst_addr); > + crash_mark_pages(&src_folio->page, 1, dst_vma); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: Medium] Similar to the mremap() case above, does move_present_ptes() fail to unmark the PFN if the page came from a marked src_vma but is moving to an unmarked dst_vma? [Severity: Medium] Are we missing crash_memaction registration for swap cache folios during UFFDIO_MOVE? While the rmap is updated for present PTEs with a crash_mark_pages() call here, the parallel move_swap_pte() path in mm/userfaultfd.c seems to omit it: if (src_folio) { folio_move_anon_rmap(src_folio, dst_vma); src_folio->index = linear_anon_page_index(dst_vma, dst_addr); } else { If a process marks a VMA with MADV_CRASH_SECRET, and then uses UFFDIO_MOVE to move a swapped-out but swapcache-resident page into this VMA, the page registration is skipped. If the system crashes before the page is faulted in, is there a risk that the kdump kernel will dump the unmarked RAM and leak the secret data? > > orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot); > /* Set soft dirty bit so userspace can notice the pte was moved > */ -- Sashiko AI review · https://sashiko.dev/#/patchset/20260928-crash-memaction-upstream-20260921-v3-0-e511e9ee2...@jaseg.de?part=9
