Re: [PATCH v3 09/12] mm/rmap: Mark folios mapped into crash_memaction-marked VMAs

[sashiko-bot@kernel.org]

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 <linux@jaseg.de>
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-e511e9ee2329@jaseg.de?part=9