Re: [PATCH RFC] mm: Fix kernel BUG when userfaultfd_move encounters swapcache

Re: [PATCH RFC] mm: Fix kernel BUG when userfaultfd_move encounters swapcache

[Barry Song]

On Thu, Feb 20, 2025 at 9:40 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 19.02.25 19:58, Suren Baghdasaryan wrote:
> > On Wed, Feb 19, 2025 at 10:30 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 19.02.25 19:26, Suren Baghdasaryan wrote:
> >>> On Wed, Feb 19, 2025 at 3:25 AM Barry Song <21cnbao@gmail.com> wrote:
> >>>>
> >>>> From: Barry Song <v-songbaohua@oppo.com>
> >>>>
> >>>> userfaultfd_move() checks whether the PTE entry is present or a
> >>>> swap entry.
> >>>>
> >>>> - If the PTE entry is present, move_present_pte() handles folio
> >>>>     migration by setting:
> >>>>
> >>>>     src_folio->index = linear_page_index(dst_vma, dst_addr);
> >>>>
> >>>> - If the PTE entry is a swap entry, move_swap_pte() simply copies
> >>>>     the PTE to the new dst_addr.
> >>>>
> >>>> This approach is incorrect because even if the PTE is a swap
> >>>> entry, it can still reference a folio that remains in the swap
> >>>> cache.
> >>>>
> >>>> If do_swap_page() is triggered, it may locate the folio in the
> >>>> swap cache. However, during add_rmap operations, a kernel panic
> >>>> can occur due to:
> >>>>    page_pgoff(folio, page) != linear_page_index(vma, address)
> >>>
> >>> Thanks for the report and reproducer!
> >>>
> >>>>
> >>>> $./a.out > /dev/null
> >>>> [   13.336953] page: refcount:6 mapcount:1 mapping:00000000f43db19c index:0xffffaf150 pfn:0x4667c
> >>>> [   13.337520] head: order:2 mapcount:1 entire_mapcount:0 nr_pages_mapped:1 pincount:0
> >>>> [   13.337716] memcg:ffff00000405f000
> >>>> [   13.337849] anon flags: 0x3fffc0000020459(locked|uptodate|dirty|owner_priv_1|head|swapbacked|node=0|zone=0|lastcpupid=0xffff)
> >>>> [   13.338630] raw: 03fffc0000020459 ffff80008507b538 ffff80008507b538 ffff000006260361
> >>>> [   13.338831] raw: 0000000ffffaf150 0000000000004000 0000000600000000 ffff00000405f000
> >>>> [   13.339031] head: 03fffc0000020459 ffff80008507b538 ffff80008507b538 ffff000006260361
> >>>> [   13.339204] head: 0000000ffffaf150 0000000000004000 0000000600000000 ffff00000405f000
> >>>> [   13.339375] head: 03fffc0000000202 fffffdffc0199f01 ffffffff00000000 0000000000000001
> >>>> [   13.339546] head: 0000000000000004 0000000000000000 00000000ffffffff 0000000000000000
> >>>> [   13.339736] page dumped because: VM_BUG_ON_PAGE(page_pgoff(folio, page) != linear_page_index(vma, address))
> >>>> [   13.340190] ------------[ cut here ]------------
> >>>> [   13.340316] kernel BUG at mm/rmap.c:1380!
> >>>> [   13.340683] Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
> >>>> [   13.340969] Modules linked in:
> >>>> [   13.341257] CPU: 1 UID: 0 PID: 107 Comm: a.out Not tainted 6.14.0-rc3-gcf42737e247a-dirty #299
> >>>> [   13.341470] Hardware name: linux,dummy-virt (DT)
> >>>> [   13.341671] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> >>>> [   13.341815] pc : __page_check_anon_rmap+0xa0/0xb0
> >>>> [   13.341920] lr : __page_check_anon_rmap+0xa0/0xb0
> >>>> [   13.342018] sp : ffff80008752bb20
> >>>> [   13.342093] x29: ffff80008752bb20 x28: fffffdffc0199f00 x27: 0000000000000001
> >>>> [   13.342404] x26: 0000000000000000 x25: 0000000000000001 x24: 0000000000000001
> >>>> [   13.342575] x23: 0000ffffaf0d0000 x22: 0000ffffaf0d0000 x21: fffffdffc0199f00
> >>>> [   13.342731] x20: fffffdffc0199f00 x19: ffff000006210700 x18: 00000000ffffffff
> >>>> [   13.342881] x17: 6c203d2120296567 x16: 6170202c6f696c6f x15: 662866666f67705f
> >>>> [   13.343033] x14: 6567617028454741 x13: 2929737365726464 x12: ffff800083728ab0
> >>>> [   13.343183] x11: ffff800082996bf8 x10: 0000000000000fd7 x9 : ffff80008011bc40
> >>>> [   13.343351] x8 : 0000000000017fe8 x7 : 00000000fffff000 x6 : ffff8000829eebf8
> >>>> [   13.343498] x5 : c0000000fffff000 x4 : 0000000000000000 x3 : 0000000000000000
> >>>> [   13.343645] x2 : 0000000000000000 x1 : ffff0000062db980 x0 : 000000000000005f
> >>>> [   13.343876] Call trace:
> >>>> [   13.344045]  __page_check_anon_rmap+0xa0/0xb0 (P)
> >>>> [   13.344234]  folio_add_anon_rmap_ptes+0x22c/0x320
> >>>> [   13.344333]  do_swap_page+0x1060/0x1400
> >>>> [   13.344417]  __handle_mm_fault+0x61c/0xbc8
> >>>> [   13.344504]  handle_mm_fault+0xd8/0x2e8
> >>>> [   13.344586]  do_page_fault+0x20c/0x770
> >>>> [   13.344673]  do_translation_fault+0xb4/0xf0
> >>>> [   13.344759]  do_mem_abort+0x48/0xa0
> >>>> [   13.344842]  el0_da+0x58/0x130
> >>>> [   13.344914]  el0t_64_sync_handler+0xc4/0x138
> >>>> [   13.345002]  el0t_64_sync+0x1ac/0x1b0
> >>>> [   13.345208] Code: aa1503e0 f000f801 910f6021 97ff5779 (d4210000)
> >>>> [   13.345504] ---[ end trace 0000000000000000 ]---
> >>>> [   13.345715] note: a.out[107] exited with irqs disabled
> >>>> [   13.345954] note: a.out[107] exited with preempt_count 2
> >>>>
> >>>> Fully fixing it would be quite complex, requiring similar handling
> >>>> of folios as done in move_present_pte.
> >>>
> >>> How complex would that be? Is it a matter of adding
> >>> folio_maybe_dma_pinned() checks, doing folio_move_anon_rmap() and
> >>> folio->index = linear_page_index like in move_present_pte() or
> >>> something more?
> >>
> >> If the entry is pte_swp_exclusive(), and the folio is order-0, it cannot
> >> be pinned and we may be able to move it I think.
> >>
> >> So all that's required is to check pte_swp_exclusive() and the folio size.
> >>
> >> ... in theory :) Not sure about the swap details.
> >
> > Looking some more into it, I think we would have to perform all the
> > folio and anon_vma locking and pinning that we do for present pages in
> > move_pages_pte(). If that's correct then maybe treating swapcache
> > pages like a present page inside move_pages_pte() would be simpler?
>
> I'd be more in favor of not doing that. Maybe there are parts we can
> move out into helper functions instead, so we can reuse them?

I actually have a v2 ready. Maybe we can discuss if some of the code can be
extracted as a helper based on the below before I send it formally?

I’d say there are many parts that can be shared with present PTE, but there
are two major differences:

1. Page exclusivity – swapcache doesn’t require it (try_to_unmap_one has remove
Exclusive flag;)
2. src_anon_vma and its lock – swapcache doesn’t require it（folio is not mapped）


Subject: [PATCH v2 Discussing with David] mm: Fix kernel crash when userfaultfd_move encounters
 swapcache

userfaultfd_move() checks whether the PTE entry is present or a
swap entry.

- If the PTE entry is present, move_present_pte() handles folio
  migration by setting:

  src_folio->index = linear_page_index(dst_vma, dst_addr);

- If the PTE entry is a swap entry, move_swap_pte() simply copies
  the PTE to the new dst_addr.

This approach is incorrect because, even if the PTE is a swap entry,
it can still reference a folio that remains in the swap cache.

This exposes a race condition between steps 2 and 4:
 1. add_to_swap: The folio is added to the swapcache.
 2. try_to_unmap: PTEs are converted to swap entries.
 3. pageout: The folio is written back.
 4. Swapcache is cleared.
If userfaultfd_move() happens in the window between step 2 and step 4,
after the swap PTE is moved to the destination, accessing the destination
triggers do_swap_page(), which may locate the folio in the swap cache.
However, during add_rmap operations, a kernel panic can occur due to:

page_pgoff(folio, page) != linear_page_index(vma, address)

This happens because move_swap_pte() has never updated the index to
match dst_vma and dst_addr.

$./a.out > /dev/null
[   13.336953] page: refcount:6 mapcount:1 mapping:00000000f43db19c index:0xffffaf150 pfn:0x4667c
[   13.337520] head: order:2 mapcount:1 entire_mapcount:0 nr_pages_mapped:1 pincount:0
[   13.337716] memcg:ffff00000405f000
[   13.337849] anon flags: 0x3fffc0000020459(locked|uptodate|dirty|owner_priv_1|head|swapbacked|node=0|zone=0|lastcpupid=0xffff)
[   13.338630] raw: 03fffc0000020459 ffff80008507b538 ffff80008507b538 ffff000006260361
[   13.338831] raw: 0000000ffffaf150 0000000000004000 0000000600000000 ffff00000405f000
[   13.339031] head: 03fffc0000020459 ffff80008507b538 ffff80008507b538 ffff000006260361
[   13.339204] head: 0000000ffffaf150 0000000000004000 0000000600000000 ffff00000405f000
[   13.339375] head: 03fffc0000000202 fffffdffc0199f01 ffffffff00000000 0000000000000001
[   13.339546] head: 0000000000000004 0000000000000000 00000000ffffffff 0000000000000000
[   13.339736] page dumped because: VM_BUG_ON_PAGE(page_pgoff(folio, page) != linear_page_index(vma, address))
[   13.340190] ------------[ cut here ]------------
[   13.340316] kernel BUG at mm/rmap.c:1380!
[   13.340683] Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
[   13.340969] Modules linked in:
[   13.341257] CPU: 1 UID: 0 PID: 107 Comm: a.out Not tainted 6.14.0-rc3-gcf42737e247a-dirty #299
[   13.341470] Hardware name: linux,dummy-virt (DT)
[   13.341671] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   13.341815] pc : __page_check_anon_rmap+0xa0/0xb0
[   13.341920] lr : __page_check_anon_rmap+0xa0/0xb0
[   13.342018] sp : ffff80008752bb20
[   13.342093] x29: ffff80008752bb20 x28: fffffdffc0199f00 x27: 0000000000000001
[   13.342404] x26: 0000000000000000 x25: 0000000000000001 x24: 0000000000000001
[   13.342575] x23: 0000ffffaf0d0000 x22: 0000ffffaf0d0000 x21: fffffdffc0199f00
[   13.342731] x20: fffffdffc0199f00 x19: ffff000006210700 x18: 00000000ffffffff
[   13.342881] x17: 6c203d2120296567 x16: 6170202c6f696c6f x15: 662866666f67705f
[   13.343033] x14: 6567617028454741 x13: 2929737365726464 x12: ffff800083728ab0
[   13.343183] x11: ffff800082996bf8 x10: 0000000000000fd7 x9 : ffff80008011bc40
[   13.343351] x8 : 0000000000017fe8 x7 : 00000000fffff000 x6 : ffff8000829eebf8
[   13.343498] x5 : c0000000fffff000 x4 : 0000000000000000 x3 : 0000000000000000
[   13.343645] x2 : 0000000000000000 x1 : ffff0000062db980 x0 : 000000000000005f
[   13.343876] Call trace:
[   13.344045]  __page_check_anon_rmap+0xa0/0xb0 (P)
[   13.344234]  folio_add_anon_rmap_ptes+0x22c/0x320
[   13.344333]  do_swap_page+0x1060/0x1400
[   13.344417]  __handle_mm_fault+0x61c/0xbc8
[   13.344504]  handle_mm_fault+0xd8/0x2e8
[   13.344586]  do_page_fault+0x20c/0x770
[   13.344673]  do_translation_fault+0xb4/0xf0
[   13.344759]  do_mem_abort+0x48/0xa0
[   13.344842]  el0_da+0x58/0x130
[   13.344914]  el0t_64_sync_handler+0xc4/0x138
[   13.345002]  el0t_64_sync+0x1ac/0x1b0
[   13.345208] Code: aa1503e0 f000f801 910f6021 97ff5779 (d4210000)
[   13.345504] ---[ end trace 0000000000000000 ]---
[   13.345715] note: a.out[107] exited with irqs disabled
[   13.345954] note: a.out[107] exited with preempt_count 2

This patch also checks the swapcache when handling swap entries. If a
match is found in the swapcache, it processes it similarly to a present
PTE.
However, there are some differences. For example, the folio is no longer
exclusive because folio_try_share_anon_rmap_pte() is performed during
unmapping.
Furthermore, in the case of swapcache, the folio has already been
unmapped, eliminating the risk of concurrent rmap walks and removing the
need to acquire src_folio's anon_vma or lock.

Note that for large folios, in the swapcache handling path, we still
frequently encounter -EBUSY returns because split_folio() returns
-EBUSY when the folio is under writeback.
That is not an urgent fix, so a following patch will address it.

Fixes: adef440691bab ("userfaultfd: UFFDIO_MOVE uABI")
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Axel Rasmussen <axelrasmussen@google.com>
Cc: Brian Geffon <bgeffon@google.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: David Hildenbrand <david@redhat.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Jann Horn <jannh@google.com>
Cc: Kalesh Singh <kaleshsingh@google.com>
Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
Cc: Lokesh Gidra <lokeshgidra@google.com>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Mike Rapoport (IBM) <rppt@kernel.org>
Cc: Nicolas Geoffray <ngeoffray@google.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Ryan Roberts <ryan.roberts@arm.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: ZhangPeng <zhangpeng362@huawei.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Barry Song <v-songbaohua@oppo.com>
---
 mm/userfaultfd.c | 228 +++++++++++++++++++++++++++--------------------
 1 file changed, 133 insertions(+), 95 deletions(-)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 867898c4e30b..e5718835a964 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -18,6 +18,7 @@
 #include <asm/tlbflush.h>
 #include <asm/tlb.h>
 #include "internal.h"
+#include "swap.h"
 
 static __always_inline
 bool validate_dst_vma(struct vm_area_struct *dst_vma, unsigned long dst_end)
@@ -1025,7 +1026,7 @@ static inline bool is_pte_pages_stable(pte_t *dst_pte, pte_t *src_pte,
 	       pmd_same(dst_pmdval, pmdp_get_lockless(dst_pmd));
 }
 
-static int move_present_pte(struct mm_struct *mm,
+static int move_pte_and_folio(struct mm_struct *mm,
 			    struct vm_area_struct *dst_vma,
 			    struct vm_area_struct *src_vma,
 			    unsigned long dst_addr, unsigned long src_addr,
@@ -1046,7 +1047,7 @@ static int move_present_pte(struct mm_struct *mm,
 	}
 	if (folio_test_large(src_folio) ||
 	    folio_maybe_dma_pinned(src_folio) ||
-	    !PageAnonExclusive(&src_folio->page)) {
+	    (pte_present(orig_src_pte) && !PageAnonExclusive(&src_folio->page))) {
 		err = -EBUSY;
 		goto out;
 	}
@@ -1062,10 +1063,13 @@ static int move_present_pte(struct mm_struct *mm,
 	folio_move_anon_rmap(src_folio, dst_vma);
 	src_folio->index = linear_page_index(dst_vma, dst_addr);
 
-	orig_dst_pte = mk_pte(&src_folio->page, dst_vma->vm_page_prot);
-	/* Follow mremap() behavior and treat the entry dirty after the move */
-	orig_dst_pte = pte_mkwrite(pte_mkdirty(orig_dst_pte), dst_vma);
-
+	if (pte_present(orig_src_pte)) {
+		orig_dst_pte = mk_pte(&src_folio->page, dst_vma->vm_page_prot);
+		/* Follow mremap() behavior and treat the entry dirty after the move */
+		orig_dst_pte = pte_mkwrite(pte_mkdirty(orig_dst_pte), dst_vma);
+	} else { /* swap entry */
+		orig_dst_pte = orig_src_pte;
+	}
 	set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
 out:
 	double_pt_unlock(dst_ptl, src_ptl);
@@ -1079,9 +1083,6 @@ static int move_swap_pte(struct mm_struct *mm,
 			 pmd_t *dst_pmd, pmd_t dst_pmdval,
 			 spinlock_t *dst_ptl, spinlock_t *src_ptl)
 {
-	if (!pte_swp_exclusive(orig_src_pte))
-		return -EBUSY;
-
 	double_pt_lock(dst_ptl, src_ptl);
 
 	if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
@@ -1137,6 +1138,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
 			  __u64 mode)
 {
 	swp_entry_t entry;
+	struct swap_info_struct *si = NULL;
 	pte_t orig_src_pte, orig_dst_pte;
 	pte_t src_folio_pte;
 	spinlock_t *src_ptl, *dst_ptl;
@@ -1220,122 +1222,156 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
 		goto out;
 	}
 
-	if (pte_present(orig_src_pte)) {
-		if (is_zero_pfn(pte_pfn(orig_src_pte))) {
-			err = move_zeropage_pte(mm, dst_vma, src_vma,
-					       dst_addr, src_addr, dst_pte, src_pte,
-					       orig_dst_pte, orig_src_pte,
-					       dst_pmd, dst_pmdval, dst_ptl, src_ptl);
+	if (!pte_present(orig_src_pte)) {
+		entry = pte_to_swp_entry(orig_src_pte);
+		if (is_migration_entry(entry)) {
+			pte_unmap(&orig_src_pte);
+			pte_unmap(&orig_dst_pte);
+			src_pte = dst_pte = NULL;
+			migration_entry_wait(mm, src_pmd, src_addr);
+			err = -EAGAIN;
+			goto out;
+		}
+
+		if (non_swap_entry(entry)) {
+			err = -EFAULT;
+			goto out;
+		}
+
+		if (!pte_swp_exclusive(orig_src_pte)) {
+			err = -EBUSY;
+			goto out;
+		}
+		/* Prevent swapoff from happening to us. */
+		if (!si)
+			si = get_swap_device(entry);
+		if (unlikely(!si)) {
+			err = -EAGAIN;
 			goto out;
 		}
+	}
+
+	if (pte_present(orig_src_pte) && is_zero_pfn(pte_pfn(orig_src_pte))) {
+		err = move_zeropage_pte(mm, dst_vma, src_vma,
+				dst_addr, src_addr, dst_pte, src_pte,
+				orig_dst_pte, orig_src_pte,
+				dst_pmd, dst_pmdval, dst_ptl, src_ptl);
+		goto out;
+	}
+
+	/*
+	 * Pin and lock both source folio and anon_vma. Since we are in
+	 * RCU read section, we can't block, so on contention have to
+	 * unmap the ptes, obtain the lock and retry.
+	 */
+	if (!src_folio) {
+		struct folio *folio;
 
 		/*
-		 * Pin and lock both source folio and anon_vma. Since we are in
-		 * RCU read section, we can't block, so on contention have to
-		 * unmap the ptes, obtain the lock and retry.
+		 * Pin the page while holding the lock to be sure the
+		 * page isn't freed under us
 		 */
-		if (!src_folio) {
-			struct folio *folio;
+		spin_lock(src_ptl);
+		if (!pte_same(orig_src_pte, ptep_get(src_pte))) {
+			spin_unlock(src_ptl);
+			err = -EAGAIN;
+			goto out;
+		}
 
-			/*
-			 * Pin the page while holding the lock to be sure the
-			 * page isn't freed under us
-			 */
-			spin_lock(src_ptl);
-			if (!pte_same(orig_src_pte, ptep_get(src_pte))) {
+		if (pte_present(orig_src_pte)) {
+			folio = vm_normal_folio(src_vma, src_addr, orig_src_pte);
+			if (!folio) {
 				spin_unlock(src_ptl);
-				err = -EAGAIN;
+				err = -EBUSY;
 				goto out;
 			}
-
-			folio = vm_normal_folio(src_vma, src_addr, orig_src_pte);
-			if (!folio || !PageAnonExclusive(&folio->page)) {
+			if (!PageAnonExclusive(&folio->page)) {
 				spin_unlock(src_ptl);
 				err = -EBUSY;
 				goto out;
 			}
-
 			folio_get(folio);
-			src_folio = folio;
-			src_folio_pte = orig_src_pte;
-			spin_unlock(src_ptl);
-
-			if (!folio_trylock(src_folio)) {
-				pte_unmap(&orig_src_pte);
-				pte_unmap(&orig_dst_pte);
-				src_pte = dst_pte = NULL;
-				/* now we can block and wait */
-				folio_lock(src_folio);
-				goto retry;
-			}
-
-			if (WARN_ON_ONCE(!folio_test_anon(src_folio))) {
-				err = -EBUSY;
+		} else {
+			/*
+			 * Check if swapcache exists.
+			 * If it does, we need to move the folio
+			 * even if the PTE is a swap entry.
+			 */
+			folio = filemap_get_folio(swap_address_space(entry),
+					swap_cache_index(entry));
+			if (IS_ERR(folio)) {
+				spin_unlock(src_ptl);
+				err = move_swap_pte(mm, dst_addr, src_addr, dst_pte, src_pte,
+						orig_dst_pte, orig_src_pte, dst_pmd,
+						dst_pmdval, dst_ptl, src_ptl);
 				goto out;
 			}
 		}
 
-		/* at this point we have src_folio locked */
-		if (folio_test_large(src_folio)) {
-			/* split_folio() can block */
+		src_folio = folio;
+		src_folio_pte = orig_src_pte;
+		spin_unlock(src_ptl);
+
+		if (!folio_trylock(src_folio)) {
 			pte_unmap(&orig_src_pte);
 			pte_unmap(&orig_dst_pte);
 			src_pte = dst_pte = NULL;
-			err = split_folio(src_folio);
-			if (err)
-				goto out;
-			/* have to reacquire the folio after it got split */
-			folio_unlock(src_folio);
-			folio_put(src_folio);
-			src_folio = NULL;
+			/* now we can block and wait */
+			folio_lock(src_folio);
 			goto retry;
 		}
 
-		if (!src_anon_vma) {
-			/*
-			 * folio_referenced walks the anon_vma chain
-			 * without the folio lock. Serialize against it with
-			 * the anon_vma lock, the folio lock is not enough.
-			 */
-			src_anon_vma = folio_get_anon_vma(src_folio);
-			if (!src_anon_vma) {
-				/* page was unmapped from under us */
-				err = -EAGAIN;
-				goto out;
-			}
-			if (!anon_vma_trylock_write(src_anon_vma)) {
-				pte_unmap(&orig_src_pte);
-				pte_unmap(&orig_dst_pte);
-				src_pte = dst_pte = NULL;
-				/* now we can block and wait */
-				anon_vma_lock_write(src_anon_vma);
-				goto retry;
-			}
+		if (WARN_ON_ONCE(!folio_test_anon(src_folio))) {
+			err = -EBUSY;
+			goto out;
 		}
+	}
 
-		err = move_present_pte(mm,  dst_vma, src_vma,
-				       dst_addr, src_addr, dst_pte, src_pte,
-				       orig_dst_pte, orig_src_pte, dst_pmd,
-				       dst_pmdval, dst_ptl, src_ptl, src_folio);
-	} else {
-		entry = pte_to_swp_entry(orig_src_pte);
-		if (non_swap_entry(entry)) {
-			if (is_migration_entry(entry)) {
-				pte_unmap(&orig_src_pte);
-				pte_unmap(&orig_dst_pte);
-				src_pte = dst_pte = NULL;
-				migration_entry_wait(mm, src_pmd, src_addr);
-				err = -EAGAIN;
-			} else
-				err = -EFAULT;
+	/* at this point we have src_folio locked */
+	if (folio_test_large(src_folio)) {
+		/* split_folio() can block */
+		pte_unmap(&orig_src_pte);
+		pte_unmap(&orig_dst_pte);
+		src_pte = dst_pte = NULL;
+		err = split_folio(src_folio);
+		if (err)
 			goto out;
-		}
+		/* have to reacquire the folio after it got split */
+		folio_unlock(src_folio);
+		folio_put(src_folio);
+		src_folio = NULL;
+		goto retry;
+	}
 
-		err = move_swap_pte(mm, dst_addr, src_addr, dst_pte, src_pte,
-				    orig_dst_pte, orig_src_pte, dst_pmd,
-				    dst_pmdval, dst_ptl, src_ptl);
+	if (!src_anon_vma && pte_present(orig_src_pte)) {
+		/*
+		 * folio_referenced walks the anon_vma chain
+		 * without the folio lock. Serialize against it with
+		 * the anon_vma lock, the folio lock is not enough.
+		 * In the swapcache case, the folio has been unmapped,
+		 * so there is no concurrent rmap walk.
+		 */
+		src_anon_vma = folio_get_anon_vma(src_folio);
+		if (!src_anon_vma) {
+			/* page was unmapped from under us */
+			err = -EAGAIN;
+			goto out;
+		}
+		if (!anon_vma_trylock_write(src_anon_vma)) {
+			pte_unmap(&orig_src_pte);
+			pte_unmap(&orig_dst_pte);
+			src_pte = dst_pte = NULL;
+			/* now we can block and wait */
+			anon_vma_lock_write(src_anon_vma);
+			goto retry;
+		}
 	}
 
+	err = move_pte_and_folio(mm,  dst_vma, src_vma,
+			dst_addr, src_addr, dst_pte, src_pte,
+			orig_dst_pte, orig_src_pte, dst_pmd,
+			dst_pmdval, dst_ptl, src_ptl, src_folio);
+
 out:
 	if (src_anon_vma) {
 		anon_vma_unlock_write(src_anon_vma);
@@ -1351,6 +1387,8 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
 		pte_unmap(src_pte);
 	mmu_notifier_invalidate_range_end(&range);
 
+	if (si)
+		put_swap_device(si);
 	return err;
 }
 
-- 
2.39.3 (Apple Git-146)


>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH RFC] mm: Fix kernel BUG when userfaultfd_move encounters swapcache

[David Hildenbrand]

On 20.02.25 10:21, Barry Song wrote:
> On Thu, Feb 20, 2025 at 9:40 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 19.02.25 19:58, Suren Baghdasaryan wrote:
>>> On Wed, Feb 19, 2025 at 10:30 AM David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 19.02.25 19:26, Suren Baghdasaryan wrote:
>>>>> On Wed, Feb 19, 2025 at 3:25 AM Barry Song <21cnbao@gmail.com> wrote:
>>>>>>
>>>>>> From: Barry Song <v-songbaohua@oppo.com>
>>>>>>
>>>>>> userfaultfd_move() checks whether the PTE entry is present or a
>>>>>> swap entry.
>>>>>>
>>>>>> - If the PTE entry is present, move_present_pte() handles folio
>>>>>>      migration by setting:
>>>>>>
>>>>>>      src_folio->index = linear_page_index(dst_vma, dst_addr);
>>>>>>
>>>>>> - If the PTE entry is a swap entry, move_swap_pte() simply copies
>>>>>>      the PTE to the new dst_addr.
>>>>>>
>>>>>> This approach is incorrect because even if the PTE is a swap
>>>>>> entry, it can still reference a folio that remains in the swap
>>>>>> cache.
>>>>>>
>>>>>> If do_swap_page() is triggered, it may locate the folio in the
>>>>>> swap cache. However, during add_rmap operations, a kernel panic
>>>>>> can occur due to:
>>>>>>     page_pgoff(folio, page) != linear_page_index(vma, address)
>>>>>
>>>>> Thanks for the report and reproducer!
>>>>>
>>>>>>
>>>>>> $./a.out > /dev/null
>>>>>> [   13.336953] page: refcount:6 mapcount:1 mapping:00000000f43db19c index:0xffffaf150 pfn:0x4667c
>>>>>> [   13.337520] head: order:2 mapcount:1 entire_mapcount:0 nr_pages_mapped:1 pincount:0
>>>>>> [   13.337716] memcg:ffff00000405f000
>>>>>> [   13.337849] anon flags: 0x3fffc0000020459(locked|uptodate|dirty|owner_priv_1|head|swapbacked|node=0|zone=0|lastcpupid=0xffff)
>>>>>> [   13.338630] raw: 03fffc0000020459 ffff80008507b538 ffff80008507b538 ffff000006260361
>>>>>> [   13.338831] raw: 0000000ffffaf150 0000000000004000 0000000600000000 ffff00000405f000
>>>>>> [   13.339031] head: 03fffc0000020459 ffff80008507b538 ffff80008507b538 ffff000006260361
>>>>>> [   13.339204] head: 0000000ffffaf150 0000000000004000 0000000600000000 ffff00000405f000
>>>>>> [   13.339375] head: 03fffc0000000202 fffffdffc0199f01 ffffffff00000000 0000000000000001
>>>>>> [   13.339546] head: 0000000000000004 0000000000000000 00000000ffffffff 0000000000000000
>>>>>> [   13.339736] page dumped because: VM_BUG_ON_PAGE(page_pgoff(folio, page) != linear_page_index(vma, address))
>>>>>> [   13.340190] ------------[ cut here ]------------
>>>>>> [   13.340316] kernel BUG at mm/rmap.c:1380!
>>>>>> [   13.340683] Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
>>>>>> [   13.340969] Modules linked in:
>>>>>> [   13.341257] CPU: 1 UID: 0 PID: 107 Comm: a.out Not tainted 6.14.0-rc3-gcf42737e247a-dirty #299
>>>>>> [   13.341470] Hardware name: linux,dummy-virt (DT)
>>>>>> [   13.341671] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>>>>> [   13.341815] pc : __page_check_anon_rmap+0xa0/0xb0
>>>>>> [   13.341920] lr : __page_check_anon_rmap+0xa0/0xb0
>>>>>> [   13.342018] sp : ffff80008752bb20
>>>>>> [   13.342093] x29: ffff80008752bb20 x28: fffffdffc0199f00 x27: 0000000000000001
>>>>>> [   13.342404] x26: 0000000000000000 x25: 0000000000000001 x24: 0000000000000001
>>>>>> [   13.342575] x23: 0000ffffaf0d0000 x22: 0000ffffaf0d0000 x21: fffffdffc0199f00
>>>>>> [   13.342731] x20: fffffdffc0199f00 x19: ffff000006210700 x18: 00000000ffffffff
>>>>>> [   13.342881] x17: 6c203d2120296567 x16: 6170202c6f696c6f x15: 662866666f67705f
>>>>>> [   13.343033] x14: 6567617028454741 x13: 2929737365726464 x12: ffff800083728ab0
>>>>>> [   13.343183] x11: ffff800082996bf8 x10: 0000000000000fd7 x9 : ffff80008011bc40
>>>>>> [   13.343351] x8 : 0000000000017fe8 x7 : 00000000fffff000 x6 : ffff8000829eebf8
>>>>>> [   13.343498] x5 : c0000000fffff000 x4 : 0000000000000000 x3 : 0000000000000000
>>>>>> [   13.343645] x2 : 0000000000000000 x1 : ffff0000062db980 x0 : 000000000000005f
>>>>>> [   13.343876] Call trace:
>>>>>> [   13.344045]  __page_check_anon_rmap+0xa0/0xb0 (P)
>>>>>> [   13.344234]  folio_add_anon_rmap_ptes+0x22c/0x320
>>>>>> [   13.344333]  do_swap_page+0x1060/0x1400
>>>>>> [   13.344417]  __handle_mm_fault+0x61c/0xbc8
>>>>>> [   13.344504]  handle_mm_fault+0xd8/0x2e8
>>>>>> [   13.344586]  do_page_fault+0x20c/0x770
>>>>>> [   13.344673]  do_translation_fault+0xb4/0xf0
>>>>>> [   13.344759]  do_mem_abort+0x48/0xa0
>>>>>> [   13.344842]  el0_da+0x58/0x130
>>>>>> [   13.344914]  el0t_64_sync_handler+0xc4/0x138
>>>>>> [   13.345002]  el0t_64_sync+0x1ac/0x1b0
>>>>>> [   13.345208] Code: aa1503e0 f000f801 910f6021 97ff5779 (d4210000)
>>>>>> [   13.345504] ---[ end trace 0000000000000000 ]---
>>>>>> [   13.345715] note: a.out[107] exited with irqs disabled
>>>>>> [   13.345954] note: a.out[107] exited with preempt_count 2
>>>>>>
>>>>>> Fully fixing it would be quite complex, requiring similar handling
>>>>>> of folios as done in move_present_pte.
>>>>>
>>>>> How complex would that be? Is it a matter of adding
>>>>> folio_maybe_dma_pinned() checks, doing folio_move_anon_rmap() and
>>>>> folio->index = linear_page_index like in move_present_pte() or
>>>>> something more?
>>>>
>>>> If the entry is pte_swp_exclusive(), and the folio is order-0, it cannot
>>>> be pinned and we may be able to move it I think.
>>>>
>>>> So all that's required is to check pte_swp_exclusive() and the folio size.
>>>>
>>>> ... in theory :) Not sure about the swap details.
>>>
>>> Looking some more into it, I think we would have to perform all the
>>> folio and anon_vma locking and pinning that we do for present pages in
>>> move_pages_pte(). If that's correct then maybe treating swapcache
>>> pages like a present page inside move_pages_pte() would be simpler?
>>
>> I'd be more in favor of not doing that. Maybe there are parts we can
>> move out into helper functions instead, so we can reuse them?
> 
> I actually have a v2 ready. Maybe we can discuss if some of the code can be
> extracted as a helper based on the below before I send it formally?
> 
> I’d say there are many parts that can be shared with present PTE, but there
> are two major differences:
> 
> 1. Page exclusivity – swapcache doesn’t require it (try_to_unmap_one has remove
> Exclusive flag;)
> 2. src_anon_vma and its lock – swapcache doesn’t require it（folio is not mapped）
> 

That's a lot of complicated code you have there (not your fault, it's 
complicated stuff ... ) :)

Some of it might be compressed/simplified by the use of "else if".

I'll try to take a closer look later (will have to apply it to see the 
context better). Just one independent comment because I stumbled over 
this recently:

[...]

> @@ -1062,10 +1063,13 @@ static int move_present_pte(struct mm_struct *mm,
>   	folio_move_anon_rmap(src_folio, dst_vma);
>   	src_folio->index = linear_page_index(dst_vma, dst_addr);
>   
> -	orig_dst_pte = mk_pte(&src_folio->page, dst_vma->vm_page_prot);
> -	/* Follow mremap() behavior and treat the entry dirty after the move */
> -	orig_dst_pte = pte_mkwrite(pte_mkdirty(orig_dst_pte), dst_vma);
> -
> +	if (pte_present(orig_src_pte)) {
> +		orig_dst_pte = mk_pte(&src_folio->page, dst_vma->vm_page_prot);
> +		/* Follow mremap() behavior and treat the entry dirty after the move */
> +		orig_dst_pte = pte_mkwrite(pte_mkdirty(orig_dst_pte), dst_vma);

I'll note that the comment and mkdirty is misleading/wrong. It's 
softdirty that we care about only. But that is something independent of 
this change.

For swp PTEs, we maybe also would want to set softdirty.

See move_soft_dirty_pte() on what is actually done on the mremap path.

-- 
Cheers,

David / dhildenb

Re: [PATCH RFC] mm: Fix kernel BUG when userfaultfd_move encounters swapcache

[Barry Song]

On Thu, Feb 20, 2025 at 11:24 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 20.02.25 10:21, Barry Song wrote:
> > On Thu, Feb 20, 2025 at 9:40 PM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 19.02.25 19:58, Suren Baghdasaryan wrote:
> >>> On Wed, Feb 19, 2025 at 10:30 AM David Hildenbrand <david@redhat.com> wrote:
> >>>>
> >>>> On 19.02.25 19:26, Suren Baghdasaryan wrote:
> >>>>> On Wed, Feb 19, 2025 at 3:25 AM Barry Song <21cnbao@gmail.com> wrote:
> >>>>>>
> >>>>>> From: Barry Song <v-songbaohua@oppo.com>
> >>>>>>
> >>>>>> userfaultfd_move() checks whether the PTE entry is present or a
> >>>>>> swap entry.
> >>>>>>
> >>>>>> - If the PTE entry is present, move_present_pte() handles folio
> >>>>>>      migration by setting:
> >>>>>>
> >>>>>>      src_folio->index = linear_page_index(dst_vma, dst_addr);
> >>>>>>
> >>>>>> - If the PTE entry is a swap entry, move_swap_pte() simply copies
> >>>>>>      the PTE to the new dst_addr.
> >>>>>>
> >>>>>> This approach is incorrect because even if the PTE is a swap
> >>>>>> entry, it can still reference a folio that remains in the swap
> >>>>>> cache.
> >>>>>>
> >>>>>> If do_swap_page() is triggered, it may locate the folio in the
> >>>>>> swap cache. However, during add_rmap operations, a kernel panic
> >>>>>> can occur due to:
> >>>>>>     page_pgoff(folio, page) != linear_page_index(vma, address)
> >>>>>
> >>>>> Thanks for the report and reproducer!
> >>>>>
> >>>>>>
> >>>>>> $./a.out > /dev/null
> >>>>>> [   13.336953] page: refcount:6 mapcount:1 mapping:00000000f43db19c index:0xffffaf150 pfn:0x4667c
> >>>>>> [   13.337520] head: order:2 mapcount:1 entire_mapcount:0 nr_pages_mapped:1 pincount:0
> >>>>>> [   13.337716] memcg:ffff00000405f000
> >>>>>> [   13.337849] anon flags: 0x3fffc0000020459(locked|uptodate|dirty|owner_priv_1|head|swapbacked|node=0|zone=0|lastcpupid=0xffff)
> >>>>>> [   13.338630] raw: 03fffc0000020459 ffff80008507b538 ffff80008507b538 ffff000006260361
> >>>>>> [   13.338831] raw: 0000000ffffaf150 0000000000004000 0000000600000000 ffff00000405f000
> >>>>>> [   13.339031] head: 03fffc0000020459 ffff80008507b538 ffff80008507b538 ffff000006260361
> >>>>>> [   13.339204] head: 0000000ffffaf150 0000000000004000 0000000600000000 ffff00000405f000
> >>>>>> [   13.339375] head: 03fffc0000000202 fffffdffc0199f01 ffffffff00000000 0000000000000001
> >>>>>> [   13.339546] head: 0000000000000004 0000000000000000 00000000ffffffff 0000000000000000
> >>>>>> [   13.339736] page dumped because: VM_BUG_ON_PAGE(page_pgoff(folio, page) != linear_page_index(vma, address))
> >>>>>> [   13.340190] ------------[ cut here ]------------
> >>>>>> [   13.340316] kernel BUG at mm/rmap.c:1380!
> >>>>>> [   13.340683] Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
> >>>>>> [   13.340969] Modules linked in:
> >>>>>> [   13.341257] CPU: 1 UID: 0 PID: 107 Comm: a.out Not tainted 6.14.0-rc3-gcf42737e247a-dirty #299
> >>>>>> [   13.341470] Hardware name: linux,dummy-virt (DT)
> >>>>>> [   13.341671] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> >>>>>> [   13.341815] pc : __page_check_anon_rmap+0xa0/0xb0
> >>>>>> [   13.341920] lr : __page_check_anon_rmap+0xa0/0xb0
> >>>>>> [   13.342018] sp : ffff80008752bb20
> >>>>>> [   13.342093] x29: ffff80008752bb20 x28: fffffdffc0199f00 x27: 0000000000000001
> >>>>>> [   13.342404] x26: 0000000000000000 x25: 0000000000000001 x24: 0000000000000001
> >>>>>> [   13.342575] x23: 0000ffffaf0d0000 x22: 0000ffffaf0d0000 x21: fffffdffc0199f00
> >>>>>> [   13.342731] x20: fffffdffc0199f00 x19: ffff000006210700 x18: 00000000ffffffff
> >>>>>> [   13.342881] x17: 6c203d2120296567 x16: 6170202c6f696c6f x15: 662866666f67705f
> >>>>>> [   13.343033] x14: 6567617028454741 x13: 2929737365726464 x12: ffff800083728ab0
> >>>>>> [   13.343183] x11: ffff800082996bf8 x10: 0000000000000fd7 x9 : ffff80008011bc40
> >>>>>> [   13.343351] x8 : 0000000000017fe8 x7 : 00000000fffff000 x6 : ffff8000829eebf8
> >>>>>> [   13.343498] x5 : c0000000fffff000 x4 : 0000000000000000 x3 : 0000000000000000
> >>>>>> [   13.343645] x2 : 0000000000000000 x1 : ffff0000062db980 x0 : 000000000000005f
> >>>>>> [   13.343876] Call trace:
> >>>>>> [   13.344045]  __page_check_anon_rmap+0xa0/0xb0 (P)
> >>>>>> [   13.344234]  folio_add_anon_rmap_ptes+0x22c/0x320
> >>>>>> [   13.344333]  do_swap_page+0x1060/0x1400
> >>>>>> [   13.344417]  __handle_mm_fault+0x61c/0xbc8
> >>>>>> [   13.344504]  handle_mm_fault+0xd8/0x2e8
> >>>>>> [   13.344586]  do_page_fault+0x20c/0x770
> >>>>>> [   13.344673]  do_translation_fault+0xb4/0xf0
> >>>>>> [   13.344759]  do_mem_abort+0x48/0xa0
> >>>>>> [   13.344842]  el0_da+0x58/0x130
> >>>>>> [   13.344914]  el0t_64_sync_handler+0xc4/0x138
> >>>>>> [   13.345002]  el0t_64_sync+0x1ac/0x1b0
> >>>>>> [   13.345208] Code: aa1503e0 f000f801 910f6021 97ff5779 (d4210000)
> >>>>>> [   13.345504] ---[ end trace 0000000000000000 ]---
> >>>>>> [   13.345715] note: a.out[107] exited with irqs disabled
> >>>>>> [   13.345954] note: a.out[107] exited with preempt_count 2
> >>>>>>
> >>>>>> Fully fixing it would be quite complex, requiring similar handling
> >>>>>> of folios as done in move_present_pte.
> >>>>>
> >>>>> How complex would that be? Is it a matter of adding
> >>>>> folio_maybe_dma_pinned() checks, doing folio_move_anon_rmap() and
> >>>>> folio->index = linear_page_index like in move_present_pte() or
> >>>>> something more?
> >>>>
> >>>> If the entry is pte_swp_exclusive(), and the folio is order-0, it cannot
> >>>> be pinned and we may be able to move it I think.
> >>>>
> >>>> So all that's required is to check pte_swp_exclusive() and the folio size.
> >>>>
> >>>> ... in theory :) Not sure about the swap details.
> >>>
> >>> Looking some more into it, I think we would have to perform all the
> >>> folio and anon_vma locking and pinning that we do for present pages in
> >>> move_pages_pte(). If that's correct then maybe treating swapcache
> >>> pages like a present page inside move_pages_pte() would be simpler?
> >>
> >> I'd be more in favor of not doing that. Maybe there are parts we can
> >> move out into helper functions instead, so we can reuse them?
> >
> > I actually have a v2 ready. Maybe we can discuss if some of the code can be
> > extracted as a helper based on the below before I send it formally?
> >
> > I’d say there are many parts that can be shared with present PTE, but there
> > are two major differences:
> >
> > 1. Page exclusivity – swapcache doesn’t require it (try_to_unmap_one has remove
> > Exclusive flag;)
> > 2. src_anon_vma and its lock – swapcache doesn’t require it（folio is not mapped）
> >
>
> That's a lot of complicated code you have there (not your fault, it's
> complicated stuff ... ) :)
>
> Some of it might be compressed/simplified by the use of "else if".
>
> I'll try to take a closer look later (will have to apply it to see the
> context better). Just one independent comment because I stumbled over
> this recently:
>
> [...]
>
> > @@ -1062,10 +1063,13 @@ static int move_present_pte(struct mm_struct *mm,
> >       folio_move_anon_rmap(src_folio, dst_vma);
> >       src_folio->index = linear_page_index(dst_vma, dst_addr);
> >
> > -     orig_dst_pte = mk_pte(&src_folio->page, dst_vma->vm_page_prot);
> > -     /* Follow mremap() behavior and treat the entry dirty after the move */
> > -     orig_dst_pte = pte_mkwrite(pte_mkdirty(orig_dst_pte), dst_vma);
> > -
> > +     if (pte_present(orig_src_pte)) {
> > +             orig_dst_pte = mk_pte(&src_folio->page, dst_vma->vm_page_prot);
> > +             /* Follow mremap() behavior and treat the entry dirty after the move */
> > +             orig_dst_pte = pte_mkwrite(pte_mkdirty(orig_dst_pte), dst_vma);
>
> I'll note that the comment and mkdirty is misleading/wrong. It's
> softdirty that we care about only. But that is something independent of
> this change.
>
> For swp PTEs, we maybe also would want to set softdirty.
>
> See move_soft_dirty_pte() on what is actually done on the mremap path.

I actually don't quite understand the changelog in  commit 0f8975ec4db2
(" mm: soft-dirty bits for user memory changes tracking").

"    Another thing to note, is that when mremap moves PTEs they are marked
    with soft-dirty as well, since from the user perspective mremap modifies
    the virtual memory at mremap's new address."

Why is the hardware-dirty bit not relevant? From the user's perspective,
the memory at the destination virtual address of mremap/userfaultfd_move
has changed.

For systems where CONFIG_HAVE_ARCH_SOFT_DIRTY is false, how can the dirty status
be determined?

Or is the answer that we only care about soft-dirty changes?

For the hardware-dirty bit, do we only care about actual modifications to the
physical page content rather than changes at the virtual address level?

>
> --
> Cheers,
>
> David / dhildenb
>

Thanks
Barry

Re: [PATCH RFC] mm: Fix kernel BUG when userfaultfd_move encounters swapcache

[David Hildenbrand]

On 26.02.25 06:37, Barry Song wrote:
> On Thu, Feb 20, 2025 at 11:24 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 20.02.25 10:21, Barry Song wrote:
>>> On Thu, Feb 20, 2025 at 9:40 PM David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 19.02.25 19:58, Suren Baghdasaryan wrote:
>>>>> On Wed, Feb 19, 2025 at 10:30 AM David Hildenbrand <david@redhat.com> wrote:
>>>>>>
>>>>>> On 19.02.25 19:26, Suren Baghdasaryan wrote:
>>>>>>> On Wed, Feb 19, 2025 at 3:25 AM Barry Song <21cnbao@gmail.com> wrote:
>>>>>>>>
>>>>>>>> From: Barry Song <v-songbaohua@oppo.com>
>>>>>>>>
>>>>>>>> userfaultfd_move() checks whether the PTE entry is present or a
>>>>>>>> swap entry.
>>>>>>>>
>>>>>>>> - If the PTE entry is present, move_present_pte() handles folio
>>>>>>>>       migration by setting:
>>>>>>>>
>>>>>>>>       src_folio->index = linear_page_index(dst_vma, dst_addr);
>>>>>>>>
>>>>>>>> - If the PTE entry is a swap entry, move_swap_pte() simply copies
>>>>>>>>       the PTE to the new dst_addr.
>>>>>>>>
>>>>>>>> This approach is incorrect because even if the PTE is a swap
>>>>>>>> entry, it can still reference a folio that remains in the swap
>>>>>>>> cache.
>>>>>>>>
>>>>>>>> If do_swap_page() is triggered, it may locate the folio in the
>>>>>>>> swap cache. However, during add_rmap operations, a kernel panic
>>>>>>>> can occur due to:
>>>>>>>>      page_pgoff(folio, page) != linear_page_index(vma, address)
>>>>>>>
>>>>>>> Thanks for the report and reproducer!
>>>>>>>
>>>>>>>>
>>>>>>>> $./a.out > /dev/null
>>>>>>>> [   13.336953] page: refcount:6 mapcount:1 mapping:00000000f43db19c index:0xffffaf150 pfn:0x4667c
>>>>>>>> [   13.337520] head: order:2 mapcount:1 entire_mapcount:0 nr_pages_mapped:1 pincount:0
>>>>>>>> [   13.337716] memcg:ffff00000405f000
>>>>>>>> [   13.337849] anon flags: 0x3fffc0000020459(locked|uptodate|dirty|owner_priv_1|head|swapbacked|node=0|zone=0|lastcpupid=0xffff)
>>>>>>>> [   13.338630] raw: 03fffc0000020459 ffff80008507b538 ffff80008507b538 ffff000006260361
>>>>>>>> [   13.338831] raw: 0000000ffffaf150 0000000000004000 0000000600000000 ffff00000405f000
>>>>>>>> [   13.339031] head: 03fffc0000020459 ffff80008507b538 ffff80008507b538 ffff000006260361
>>>>>>>> [   13.339204] head: 0000000ffffaf150 0000000000004000 0000000600000000 ffff00000405f000
>>>>>>>> [   13.339375] head: 03fffc0000000202 fffffdffc0199f01 ffffffff00000000 0000000000000001
>>>>>>>> [   13.339546] head: 0000000000000004 0000000000000000 00000000ffffffff 0000000000000000
>>>>>>>> [   13.339736] page dumped because: VM_BUG_ON_PAGE(page_pgoff(folio, page) != linear_page_index(vma, address))
>>>>>>>> [   13.340190] ------------[ cut here ]------------
>>>>>>>> [   13.340316] kernel BUG at mm/rmap.c:1380!
>>>>>>>> [   13.340683] Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
>>>>>>>> [   13.340969] Modules linked in:
>>>>>>>> [   13.341257] CPU: 1 UID: 0 PID: 107 Comm: a.out Not tainted 6.14.0-rc3-gcf42737e247a-dirty #299
>>>>>>>> [   13.341470] Hardware name: linux,dummy-virt (DT)
>>>>>>>> [   13.341671] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>>>>>>> [   13.341815] pc : __page_check_anon_rmap+0xa0/0xb0
>>>>>>>> [   13.341920] lr : __page_check_anon_rmap+0xa0/0xb0
>>>>>>>> [   13.342018] sp : ffff80008752bb20
>>>>>>>> [   13.342093] x29: ffff80008752bb20 x28: fffffdffc0199f00 x27: 0000000000000001
>>>>>>>> [   13.342404] x26: 0000000000000000 x25: 0000000000000001 x24: 0000000000000001
>>>>>>>> [   13.342575] x23: 0000ffffaf0d0000 x22: 0000ffffaf0d0000 x21: fffffdffc0199f00
>>>>>>>> [   13.342731] x20: fffffdffc0199f00 x19: ffff000006210700 x18: 00000000ffffffff
>>>>>>>> [   13.342881] x17: 6c203d2120296567 x16: 6170202c6f696c6f x15: 662866666f67705f
>>>>>>>> [   13.343033] x14: 6567617028454741 x13: 2929737365726464 x12: ffff800083728ab0
>>>>>>>> [   13.343183] x11: ffff800082996bf8 x10: 0000000000000fd7 x9 : ffff80008011bc40
>>>>>>>> [   13.343351] x8 : 0000000000017fe8 x7 : 00000000fffff000 x6 : ffff8000829eebf8
>>>>>>>> [   13.343498] x5 : c0000000fffff000 x4 : 0000000000000000 x3 : 0000000000000000
>>>>>>>> [   13.343645] x2 : 0000000000000000 x1 : ffff0000062db980 x0 : 000000000000005f
>>>>>>>> [   13.343876] Call trace:
>>>>>>>> [   13.344045]  __page_check_anon_rmap+0xa0/0xb0 (P)
>>>>>>>> [   13.344234]  folio_add_anon_rmap_ptes+0x22c/0x320
>>>>>>>> [   13.344333]  do_swap_page+0x1060/0x1400
>>>>>>>> [   13.344417]  __handle_mm_fault+0x61c/0xbc8
>>>>>>>> [   13.344504]  handle_mm_fault+0xd8/0x2e8
>>>>>>>> [   13.344586]  do_page_fault+0x20c/0x770
>>>>>>>> [   13.344673]  do_translation_fault+0xb4/0xf0
>>>>>>>> [   13.344759]  do_mem_abort+0x48/0xa0
>>>>>>>> [   13.344842]  el0_da+0x58/0x130
>>>>>>>> [   13.344914]  el0t_64_sync_handler+0xc4/0x138
>>>>>>>> [   13.345002]  el0t_64_sync+0x1ac/0x1b0
>>>>>>>> [   13.345208] Code: aa1503e0 f000f801 910f6021 97ff5779 (d4210000)
>>>>>>>> [   13.345504] ---[ end trace 0000000000000000 ]---
>>>>>>>> [   13.345715] note: a.out[107] exited with irqs disabled
>>>>>>>> [   13.345954] note: a.out[107] exited with preempt_count 2
>>>>>>>>
>>>>>>>> Fully fixing it would be quite complex, requiring similar handling
>>>>>>>> of folios as done in move_present_pte.
>>>>>>>
>>>>>>> How complex would that be? Is it a matter of adding
>>>>>>> folio_maybe_dma_pinned() checks, doing folio_move_anon_rmap() and
>>>>>>> folio->index = linear_page_index like in move_present_pte() or
>>>>>>> something more?
>>>>>>
>>>>>> If the entry is pte_swp_exclusive(), and the folio is order-0, it cannot
>>>>>> be pinned and we may be able to move it I think.
>>>>>>
>>>>>> So all that's required is to check pte_swp_exclusive() and the folio size.
>>>>>>
>>>>>> ... in theory :) Not sure about the swap details.
>>>>>
>>>>> Looking some more into it, I think we would have to perform all the
>>>>> folio and anon_vma locking and pinning that we do for present pages in
>>>>> move_pages_pte(). If that's correct then maybe treating swapcache
>>>>> pages like a present page inside move_pages_pte() would be simpler?
>>>>
>>>> I'd be more in favor of not doing that. Maybe there are parts we can
>>>> move out into helper functions instead, so we can reuse them?
>>>
>>> I actually have a v2 ready. Maybe we can discuss if some of the code can be
>>> extracted as a helper based on the below before I send it formally?
>>>
>>> I’d say there are many parts that can be shared with present PTE, but there
>>> are two major differences:
>>>
>>> 1. Page exclusivity – swapcache doesn’t require it (try_to_unmap_one has remove
>>> Exclusive flag;)
>>> 2. src_anon_vma and its lock – swapcache doesn’t require it（folio is not mapped）
>>>
>>
>> That's a lot of complicated code you have there (not your fault, it's
>> complicated stuff ... ) :)
>>
>> Some of it might be compressed/simplified by the use of "else if".
>>
>> I'll try to take a closer look later (will have to apply it to see the
>> context better). Just one independent comment because I stumbled over
>> this recently:
>>
>> [...]
>>
>>> @@ -1062,10 +1063,13 @@ static int move_present_pte(struct mm_struct *mm,
>>>        folio_move_anon_rmap(src_folio, dst_vma);
>>>        src_folio->index = linear_page_index(dst_vma, dst_addr);
>>>
>>> -     orig_dst_pte = mk_pte(&src_folio->page, dst_vma->vm_page_prot);
>>> -     /* Follow mremap() behavior and treat the entry dirty after the move */
>>> -     orig_dst_pte = pte_mkwrite(pte_mkdirty(orig_dst_pte), dst_vma);
>>> -
>>> +     if (pte_present(orig_src_pte)) {
>>> +             orig_dst_pte = mk_pte(&src_folio->page, dst_vma->vm_page_prot);
>>> +             /* Follow mremap() behavior and treat the entry dirty after the move */
>>> +             orig_dst_pte = pte_mkwrite(pte_mkdirty(orig_dst_pte), dst_vma);
>>
>> I'll note that the comment and mkdirty is misleading/wrong. It's
>> softdirty that we care about only. But that is something independent of
>> this change.
>>
>> For swp PTEs, we maybe also would want to set softdirty.
>>
>> See move_soft_dirty_pte() on what is actually done on the mremap path.
> 
> I actually don't quite understand the changelog in  commit 0f8975ec4db2
> (" mm: soft-dirty bits for user memory changes tracking").
> 
> "    Another thing to note, is that when mremap moves PTEs they are marked
>      with soft-dirty as well, since from the user perspective mremap modifies
>      the virtual memory at mremap's new address."
> 
> Why is the hardware-dirty bit not relevant? From the user's perspective,
> the memory at the destination virtual address of mremap/userfaultfd_move
> has changed.

Yes, but it did not change from the system POV. For example, if the page 
was R/O clean and we moved it, why should it suddenly be R/O dirty and 
e.g., require writeback again.

Nobody modified *page content*, but from a user perspective the memory 
at that *virtual memory location* (dst) changed, for example, for 
logical zero (no page mapped) to non-zero (page mapped). That's what 
soft-dirty is about.


> 
> For systems where CONFIG_HAVE_ARCH_SOFT_DIRTY is false, how can the dirty status
> be determined?

No soft-dirty tracking, so nothing to maintain.

> 
> Or is the answer that we only care about soft-dirty changes?
 > > For the hardware-dirty bit, do we only care about actual 
modifications to the
> physical page content rather than changes at the virtual address level?

Exactly!

-- 
Cheers,

David / dhildenb

Re: [PATCH RFC] mm: Fix kernel BUG when userfaultfd_move encounters swapcache

[Peter Xu]

On Thu, Feb 20, 2025 at 10:21:01PM +1300, Barry Song wrote:
> 2. src_anon_vma and its lock – swapcache doesn’t require it（folio is not mapped）

Could you help explain what guarantees the rmap walk not happen on a
swapcache page?

I'm not familiar with this path, though at least I see damon can start a
rmap walk on PageAnon almost with no locking..  some explanations would be
appreciated.

-- 
Peter Xu

Re: [PATCH RFC] mm: Fix kernel BUG when userfaultfd_move encounters swapcache

[Barry Song]

On Fri, Feb 21, 2025 at 12:32 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, Feb 20, 2025 at 10:21:01PM +1300, Barry Song wrote:
> > 2. src_anon_vma and its lock – swapcache doesn’t require it（folio is not mapped）
>
> Could you help explain what guarantees the rmap walk not happen on a
> swapcache page?
>
> I'm not familiar with this path, though at least I see damon can start a
> rmap walk on PageAnon almost with no locking..  some explanations would be
> appreciated.

I am observing the following in folio_referenced(), which the anon_vma lock
was originally intended to protect.

        if (!pra.mapcount)
                return 0;

I assume all other rmap walks should do the same?

int folio_referenced(struct folio *folio, int is_locked,
                     struct mem_cgroup *memcg, unsigned long *vm_flags)
{

        bool we_locked = false;
        struct folio_referenced_arg pra = {
                .mapcount = folio_mapcount(folio),
                .memcg = memcg,
        };

        struct rmap_walk_control rwc = {
                .rmap_one = folio_referenced_one,
                .arg = (void *)&pra,
                .anon_lock = folio_lock_anon_vma_read,
                .try_lock = true,
                .invalid_vma = invalid_folio_referenced_vma,
        };

        *vm_flags = 0;
        if (!pra.mapcount)
                return 0;
        ...
}

By the way, since the folio has been under reclamation in this case and
isn't in the lru, this should also prevent the rmap walk, right?

>
> --
> Peter Xu
>

Thanks
Barry

Re: [PATCH RFC] mm: Fix kernel BUG when userfaultfd_move encounters swapcache

[Peter Xu]

On Fri, Feb 21, 2025 at 01:07:24PM +1300, Barry Song wrote:
> On Fri, Feb 21, 2025 at 12:32 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Thu, Feb 20, 2025 at 10:21:01PM +1300, Barry Song wrote:
> > > 2. src_anon_vma and its lock – swapcache doesn’t require it（folio is not mapped）
> >
> > Could you help explain what guarantees the rmap walk not happen on a
> > swapcache page?
> >
> > I'm not familiar with this path, though at least I see damon can start a
> > rmap walk on PageAnon almost with no locking..  some explanations would be
> > appreciated.
> 
> I am observing the following in folio_referenced(), which the anon_vma lock
> was originally intended to protect.
> 
>         if (!pra.mapcount)
>                 return 0;
> 
> I assume all other rmap walks should do the same?

Yes normally there'll be a folio_mapcount() check, however..

> 
> int folio_referenced(struct folio *folio, int is_locked,
>                      struct mem_cgroup *memcg, unsigned long *vm_flags)
> {
> 
>         bool we_locked = false;
>         struct folio_referenced_arg pra = {
>                 .mapcount = folio_mapcount(folio),
>                 .memcg = memcg,
>         };
> 
>         struct rmap_walk_control rwc = {
>                 .rmap_one = folio_referenced_one,
>                 .arg = (void *)&pra,
>                 .anon_lock = folio_lock_anon_vma_read,
>                 .try_lock = true,
>                 .invalid_vma = invalid_folio_referenced_vma,
>         };
> 
>         *vm_flags = 0;
>         if (!pra.mapcount)
>                 return 0;
>         ...
> }
> 
> By the way, since the folio has been under reclamation in this case and
> isn't in the lru, this should also prevent the rmap walk, right?

.. I'm not sure whether it's always working.

The thing is anon doesn't even require folio lock held during (1) checking
mapcount and (2) doing the rmap walk, in all similar cases as above.  I see
nothing blocks it from a concurrent thread zapping that last mapcount:

               thread 1                         thread 2
               --------                         --------
        [whatever scanner] 
           check folio_mapcount(), non-zero
                                                zap the last map.. then mapcount==0
           rmap_walk()

Not sure if I missed something.

The other thing is IIUC swapcache page can also have chance to be faulted
in but only if a read not write.  I actually had a feeling that your
reproducer triggered that exact path, causing a read swap in, reusing the
swapcache page, and hit the sanity check there somehow (even as mentioned
in the other reply, I don't yet know why the 1st check didn't seem to
work.. as we do check folio->index twice..).

Said that, I'm not sure if above concern will happen in this specific case,
as UIFFDIO_MOVE is pretty special, that we check exclusive bit first in swp
entry so we know it's definitely not mapped elsewhere, meanwhile if we hold
pgtable lock so maybe it can't get mapped back.. it is just still tricky,
at least we do some dances all over releasing and retaking locks.

We could either justify that's safe, or maybe still ok and simpler if we
could take anon_vma write lock, making sure nobody will be able to read the
folio->index when it's prone to an update.

Thanks,

-- 
Peter Xu

Re: [PATCH RFC] mm: Fix kernel BUG when userfaultfd_move encounters swapcache

[Barry Song]

On Fri, Feb 21, 2025 at 2:49 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Feb 21, 2025 at 01:07:24PM +1300, Barry Song wrote:
> > On Fri, Feb 21, 2025 at 12:32 PM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Thu, Feb 20, 2025 at 10:21:01PM +1300, Barry Song wrote:
> > > > 2. src_anon_vma and its lock – swapcache doesn’t require it（folio is not mapped）
> > >
> > > Could you help explain what guarantees the rmap walk not happen on a
> > > swapcache page?
> > >
> > > I'm not familiar with this path, though at least I see damon can start a
> > > rmap walk on PageAnon almost with no locking..  some explanations would be
> > > appreciated.
> >
> > I am observing the following in folio_referenced(), which the anon_vma lock
> > was originally intended to protect.
> >
> >         if (!pra.mapcount)
> >                 return 0;
> >
> > I assume all other rmap walks should do the same?
>
> Yes normally there'll be a folio_mapcount() check, however..
>
> >
> > int folio_referenced(struct folio *folio, int is_locked,
> >                      struct mem_cgroup *memcg, unsigned long *vm_flags)
> > {
> >
> >         bool we_locked = false;
> >         struct folio_referenced_arg pra = {
> >                 .mapcount = folio_mapcount(folio),
> >                 .memcg = memcg,
> >         };
> >
> >         struct rmap_walk_control rwc = {
> >                 .rmap_one = folio_referenced_one,
> >                 .arg = (void *)&pra,
> >                 .anon_lock = folio_lock_anon_vma_read,
> >                 .try_lock = true,
> >                 .invalid_vma = invalid_folio_referenced_vma,
> >         };
> >
> >         *vm_flags = 0;
> >         if (!pra.mapcount)
> >                 return 0;
> >         ...
> > }
> >
> > By the way, since the folio has been under reclamation in this case and
> > isn't in the lru, this should also prevent the rmap walk, right?
>
> .. I'm not sure whether it's always working.
>
> The thing is anon doesn't even require folio lock held during (1) checking
> mapcount and (2) doing the rmap walk, in all similar cases as above.  I see
> nothing blocks it from a concurrent thread zapping that last mapcount:
>
>                thread 1                         thread 2
>                --------                         --------
>         [whatever scanner]
>            check folio_mapcount(), non-zero
>                                                 zap the last map.. then mapcount==0
>            rmap_walk()
>
> Not sure if I missed something.
>
> The other thing is IIUC swapcache page can also have chance to be faulted
> in but only if a read not write.  I actually had a feeling that your
> reproducer triggered that exact path, causing a read swap in, reusing the
> swapcache page, and hit the sanity check there somehow (even as mentioned
> in the other reply, I don't yet know why the 1st check didn't seem to
> work.. as we do check folio->index twice..).
>
> Said that, I'm not sure if above concern will happen in this specific case,
> as UIFFDIO_MOVE is pretty special, that we check exclusive bit first in swp
> entry so we know it's definitely not mapped elsewhere, meanwhile if we hold
> pgtable lock so maybe it can't get mapped back.. it is just still tricky,
> at least we do some dances all over releasing and retaking locks.
>
> We could either justify that's safe, or maybe still ok and simpler if we
> could take anon_vma write lock, making sure nobody will be able to read the
> folio->index when it's prone to an update.

What prompted me to do the former is that folio_get_anon_vma() returns
NULL for an unmapped folio. As for the latter, we need to carefully evaluate
whether the change below is safe.

--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -505,7 +505,7 @@ struct anon_vma *folio_get_anon_vma(const struct
folio *folio)
        anon_mapping = (unsigned long)READ_ONCE(folio->mapping);
        if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
                goto out;

-       if (!folio_mapped(folio))
+       if (!folio_mapped(folio) && !folio_test_swapcache(folio))
                goto out;

        anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON);
@@ -521,7 +521,7 @@ struct anon_vma *folio_get_anon_vma(const struct
folio *folio)
         * SLAB_TYPESAFE_BY_RCU guarantees that - so the atomic_inc_not_zero()
         * above cannot corrupt).
         */

-       if (!folio_mapped(folio)) {
+       if (!folio_mapped(folio) && !folio_test_swapcache(folio)) {
                rcu_read_unlock();
                put_anon_vma(anon_vma);
                return NULL;


The above change, combined with the change below, has also resolved the mTHP
-EBUSY issue.

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index e5718835a964..1ef991b5c225 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -1333,6 +1333,7 @@ static int move_pages_pte(struct mm_struct *mm,
pmd_t *dst_pmd, pmd_t *src_pmd,
                pte_unmap(&orig_src_pte);
                pte_unmap(&orig_dst_pte);
                src_pte = dst_pte = NULL;
+               folio_wait_writeback(src_folio);
                err = split_folio(src_folio);

                if (err)
                        goto out;
@@ -1343,7 +1344,7 @@ static int move_pages_pte(struct mm_struct *mm,
pmd_t *dst_pmd, pmd_t *src_pmd,
                goto retry;
        }

-       if (!src_anon_vma && pte_present(orig_src_pte)) {
+       if (!src_anon_vma) {
                /*
                 * folio_referenced walks the anon_vma chain
                 * without the folio lock. Serialize against it with


split_folio() returns -EBUSY if the folio is under writeback or if
folio_get_anon_vma() returns NULL.

I have no issues with the latter, provided the change in folio_get_anon_vma()
is safe, as it also resolves the mTHP -EBUSY issue.

We need to carefully consider the five places where folio_get_anon_vma() is
called, as this patch will also be backported to stable.

  1   2618  mm/huge_memory.c <<move_pages_huge_pmd>>
             src_anon_vma = folio_get_anon_vma(src_folio);

   2   3765  mm/huge_memory.c <<__folio_split>>
             anon_vma = folio_get_anon_vma(folio);

   3   1280  mm/migrate.c <<migrate_folio_unmap>>
             anon_vma = folio_get_anon_vma(src);

   4   1485  mm/migrate.c <<unmap_and_move_huge_page>>
             anon_vma = folio_get_anon_vma(src);

   5   1354  mm/userfaultfd.c <<move_pages_pte>>
             src_anon_vma = folio_get_anon_vma(src_folio);

>
> Thanks,
>
> --
> Peter Xu
>

Thanks
barry

Re: [PATCH RFC] mm: Fix kernel BUG when userfaultfd_move encounters swapcache

[Peter Xu]

On Sun, Feb 23, 2025 at 10:31:37AM +1300, Barry Song wrote:
> On Fri, Feb 21, 2025 at 2:49 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Fri, Feb 21, 2025 at 01:07:24PM +1300, Barry Song wrote:
> > > On Fri, Feb 21, 2025 at 12:32 PM Peter Xu <peterx@redhat.com> wrote:
> > > >
> > > > On Thu, Feb 20, 2025 at 10:21:01PM +1300, Barry Song wrote:
> > > > > 2. src_anon_vma and its lock – swapcache doesn’t require it（folio is not mapped）
> > > >
> > > > Could you help explain what guarantees the rmap walk not happen on a
> > > > swapcache page?
> > > >
> > > > I'm not familiar with this path, though at least I see damon can start a
> > > > rmap walk on PageAnon almost with no locking..  some explanations would be
> > > > appreciated.
> > >
> > > I am observing the following in folio_referenced(), which the anon_vma lock
> > > was originally intended to protect.
> > >
> > >         if (!pra.mapcount)
> > >                 return 0;
> > >
> > > I assume all other rmap walks should do the same?
> >
> > Yes normally there'll be a folio_mapcount() check, however..
> >
> > >
> > > int folio_referenced(struct folio *folio, int is_locked,
> > >                      struct mem_cgroup *memcg, unsigned long *vm_flags)
> > > {
> > >
> > >         bool we_locked = false;
> > >         struct folio_referenced_arg pra = {
> > >                 .mapcount = folio_mapcount(folio),
> > >                 .memcg = memcg,
> > >         };
> > >
> > >         struct rmap_walk_control rwc = {
> > >                 .rmap_one = folio_referenced_one,
> > >                 .arg = (void *)&pra,
> > >                 .anon_lock = folio_lock_anon_vma_read,
> > >                 .try_lock = true,
> > >                 .invalid_vma = invalid_folio_referenced_vma,
> > >         };
> > >
> > >         *vm_flags = 0;
> > >         if (!pra.mapcount)
> > >                 return 0;
> > >         ...
> > > }
> > >
> > > By the way, since the folio has been under reclamation in this case and
> > > isn't in the lru, this should also prevent the rmap walk, right?
> >
> > .. I'm not sure whether it's always working.
> >
> > The thing is anon doesn't even require folio lock held during (1) checking
> > mapcount and (2) doing the rmap walk, in all similar cases as above.  I see
> > nothing blocks it from a concurrent thread zapping that last mapcount:
> >
> >                thread 1                         thread 2
> >                --------                         --------
> >         [whatever scanner]
> >            check folio_mapcount(), non-zero
> >                                                 zap the last map.. then mapcount==0
> >            rmap_walk()
> >
> > Not sure if I missed something.
> >
> > The other thing is IIUC swapcache page can also have chance to be faulted
> > in but only if a read not write.  I actually had a feeling that your
> > reproducer triggered that exact path, causing a read swap in, reusing the
> > swapcache page, and hit the sanity check there somehow (even as mentioned
> > in the other reply, I don't yet know why the 1st check didn't seem to
> > work.. as we do check folio->index twice..).
> >
> > Said that, I'm not sure if above concern will happen in this specific case,
> > as UIFFDIO_MOVE is pretty special, that we check exclusive bit first in swp
> > entry so we know it's definitely not mapped elsewhere, meanwhile if we hold
> > pgtable lock so maybe it can't get mapped back.. it is just still tricky,
> > at least we do some dances all over releasing and retaking locks.
> >
> > We could either justify that's safe, or maybe still ok and simpler if we
> > could take anon_vma write lock, making sure nobody will be able to read the
> > folio->index when it's prone to an update.
> 
> What prompted me to do the former is that folio_get_anon_vma() returns
> NULL for an unmapped folio. As for the latter, we need to carefully evaluate
> whether the change below is safe.
> 
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -505,7 +505,7 @@ struct anon_vma *folio_get_anon_vma(const struct
> folio *folio)
>         anon_mapping = (unsigned long)READ_ONCE(folio->mapping);
>         if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
>                 goto out;
> 
> -       if (!folio_mapped(folio))
> +       if (!folio_mapped(folio) && !folio_test_swapcache(folio))
>                 goto out;
> 
>         anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON);
> @@ -521,7 +521,7 @@ struct anon_vma *folio_get_anon_vma(const struct
> folio *folio)
>          * SLAB_TYPESAFE_BY_RCU guarantees that - so the atomic_inc_not_zero()
>          * above cannot corrupt).
>          */

[1]

> 
> -       if (!folio_mapped(folio)) {
> +       if (!folio_mapped(folio) && !folio_test_swapcache(folio)) {
>                 rcu_read_unlock();
>                 put_anon_vma(anon_vma);
>                 return NULL;

Hmm, this let me go back read again on how we manage anon_vma lifespan,
then I just noticed this may not work.

See the comment right above [1], here's a full version:

	/*
	 * If this folio is still mapped, then its anon_vma cannot have been
	 * freed.  But if it has been unmapped, we have no security against the
	 * anon_vma structure being freed and reused (for another anon_vma:
	 * SLAB_TYPESAFE_BY_RCU guarantees that - so the atomic_inc_not_zero()
	 * above cannot corrupt).
	 */

So afaiu that means we pretty much very rely upon folio_mapped() check to
make sure anon_vma being valid at all that we fetched from folio->mapping,
not to mention the rmap walk later afterwards.

Then above diff in folio_get_anon_vma() should be problematic, as when
"folio_mapped()==false && folio_test_swapcache()==true", above change will
start to return anon_vma pointer even if the anon_vma could have been freed
and reused by other VMAs.

> 
> 
> The above change, combined with the change below, has also resolved the mTHP
> -EBUSY issue.
> 
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index e5718835a964..1ef991b5c225 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1333,6 +1333,7 @@ static int move_pages_pte(struct mm_struct *mm,
> pmd_t *dst_pmd, pmd_t *src_pmd,
>                 pte_unmap(&orig_src_pte);
>                 pte_unmap(&orig_dst_pte);
>                 src_pte = dst_pte = NULL;
> +               folio_wait_writeback(src_folio);
>                 err = split_folio(src_folio);
> 
>                 if (err)
>                         goto out;
> @@ -1343,7 +1344,7 @@ static int move_pages_pte(struct mm_struct *mm,
> pmd_t *dst_pmd, pmd_t *src_pmd,
>                 goto retry;
>         }
> 
> -       if (!src_anon_vma && pte_present(orig_src_pte)) {
> +       if (!src_anon_vma) {
>                 /*
>                  * folio_referenced walks the anon_vma chain
>                  * without the folio lock. Serialize against it with
> 
> 
> split_folio() returns -EBUSY if the folio is under writeback or if
> folio_get_anon_vma() returns NULL.
> 
> I have no issues with the latter, provided the change in folio_get_anon_vma()
> is safe, as it also resolves the mTHP -EBUSY issue.
> 
> We need to carefully consider the five places where folio_get_anon_vma() is
> called, as this patch will also be backported to stable.
> 
>   1   2618  mm/huge_memory.c <<move_pages_huge_pmd>>
>              src_anon_vma = folio_get_anon_vma(src_folio);
> 
>    2   3765  mm/huge_memory.c <<__folio_split>>
>              anon_vma = folio_get_anon_vma(folio);
> 
>    3   1280  mm/migrate.c <<migrate_folio_unmap>>
>              anon_vma = folio_get_anon_vma(src);
> 
>    4   1485  mm/migrate.c <<unmap_and_move_huge_page>>
>              anon_vma = folio_get_anon_vma(src);
> 
>    5   1354  mm/userfaultfd.c <<move_pages_pte>>
>              src_anon_vma = folio_get_anon_vma(src_folio);

If my above understanding is correct, we may indeed need alternative plans.
Not sure whether others have thoughts, but two things I am thinking out
loud..

  - Justify rmap on the swap cache folio not possible: I think if
    folio_mapped() is required for any anon rmap walk (which I didn't
    notice previously..), and we know this is exclusive swap entry and
    keeps true (so nobody will be able to race and make the swapcache
    mapped during the whole process), maybe we are able to justify any rmap
    won't happen at all, because they should fail at folio_mapped() check.
    Then we update the folio->mapping & folio->index directly without
    anon_vma locking.  A rich comment would be helpful in this case..

  - I wonder if it's possible we free the swap cache if it's already
    writeback to backend storage and clean.  Then moving the swp entry
    alone looks safe when without the swapcache present.

Thanks,

-- 
Peter Xu

Re: [PATCH RFC] mm: Fix kernel BUG when userfaultfd_move encounters swapcache

[David Hildenbrand]

On 24.02.25 18:50, Peter Xu wrote:
> On Sun, Feb 23, 2025 at 10:31:37AM +1300, Barry Song wrote:
>> On Fri, Feb 21, 2025 at 2:49 PM Peter Xu <peterx@redhat.com> wrote:
>>>
>>> On Fri, Feb 21, 2025 at 01:07:24PM +1300, Barry Song wrote:
>>>> On Fri, Feb 21, 2025 at 12:32 PM Peter Xu <peterx@redhat.com> wrote:
>>>>>
>>>>> On Thu, Feb 20, 2025 at 10:21:01PM +1300, Barry Song wrote:
>>>>>> 2. src_anon_vma and its lock – swapcache doesn’t require it（folio is not mapped）
>>>>>
>>>>> Could you help explain what guarantees the rmap walk not happen on a
>>>>> swapcache page?
>>>>>
>>>>> I'm not familiar with this path, though at least I see damon can start a
>>>>> rmap walk on PageAnon almost with no locking..  some explanations would be
>>>>> appreciated.
>>>>
>>>> I am observing the following in folio_referenced(), which the anon_vma lock
>>>> was originally intended to protect.
>>>>
>>>>          if (!pra.mapcount)
>>>>                  return 0;
>>>>
>>>> I assume all other rmap walks should do the same?
>>>
>>> Yes normally there'll be a folio_mapcount() check, however..
>>>
>>>>
>>>> int folio_referenced(struct folio *folio, int is_locked,
>>>>                       struct mem_cgroup *memcg, unsigned long *vm_flags)
>>>> {
>>>>
>>>>          bool we_locked = false;
>>>>          struct folio_referenced_arg pra = {
>>>>                  .mapcount = folio_mapcount(folio),
>>>>                  .memcg = memcg,
>>>>          };
>>>>
>>>>          struct rmap_walk_control rwc = {
>>>>                  .rmap_one = folio_referenced_one,
>>>>                  .arg = (void *)&pra,
>>>>                  .anon_lock = folio_lock_anon_vma_read,
>>>>                  .try_lock = true,
>>>>                  .invalid_vma = invalid_folio_referenced_vma,
>>>>          };
>>>>
>>>>          *vm_flags = 0;
>>>>          if (!pra.mapcount)
>>>>                  return 0;
>>>>          ...
>>>> }
>>>>
>>>> By the way, since the folio has been under reclamation in this case and
>>>> isn't in the lru, this should also prevent the rmap walk, right?
>>>
>>> .. I'm not sure whether it's always working.
>>>
>>> The thing is anon doesn't even require folio lock held during (1) checking
>>> mapcount and (2) doing the rmap walk, in all similar cases as above.  I see
>>> nothing blocks it from a concurrent thread zapping that last mapcount:
>>>
>>>                 thread 1                         thread 2
>>>                 --------                         --------
>>>          [whatever scanner]
>>>             check folio_mapcount(), non-zero
>>>                                                  zap the last map.. then mapcount==0
>>>             rmap_walk()
>>>
>>> Not sure if I missed something.
>>>
>>> The other thing is IIUC swapcache page can also have chance to be faulted
>>> in but only if a read not write.  I actually had a feeling that your
>>> reproducer triggered that exact path, causing a read swap in, reusing the
>>> swapcache page, and hit the sanity check there somehow (even as mentioned
>>> in the other reply, I don't yet know why the 1st check didn't seem to
>>> work.. as we do check folio->index twice..).
>>>
>>> Said that, I'm not sure if above concern will happen in this specific case,
>>> as UIFFDIO_MOVE is pretty special, that we check exclusive bit first in swp
>>> entry so we know it's definitely not mapped elsewhere, meanwhile if we hold
>>> pgtable lock so maybe it can't get mapped back.. it is just still tricky,
>>> at least we do some dances all over releasing and retaking locks.
>>>
>>> We could either justify that's safe, or maybe still ok and simpler if we
>>> could take anon_vma write lock, making sure nobody will be able to read the
>>> folio->index when it's prone to an update.
>>
>> What prompted me to do the former is that folio_get_anon_vma() returns
>> NULL for an unmapped folio. As for the latter, we need to carefully evaluate
>> whether the change below is safe.
>>
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -505,7 +505,7 @@ struct anon_vma *folio_get_anon_vma(const struct
>> folio *folio)
>>          anon_mapping = (unsigned long)READ_ONCE(folio->mapping);
>>          if ((anon_mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
>>                  goto out;
>>
>> -       if (!folio_mapped(folio))
>> +       if (!folio_mapped(folio) && !folio_test_swapcache(folio))
>>                  goto out;
>>
>>          anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON);
>> @@ -521,7 +521,7 @@ struct anon_vma *folio_get_anon_vma(const struct
>> folio *folio)
>>           * SLAB_TYPESAFE_BY_RCU guarantees that - so the atomic_inc_not_zero()
>>           * above cannot corrupt).
>>           */
> 
> [1]
> 
>>
>> -       if (!folio_mapped(folio)) {
>> +       if (!folio_mapped(folio) && !folio_test_swapcache(folio)) {
>>                  rcu_read_unlock();
>>                  put_anon_vma(anon_vma);
>>                  return NULL;
> 
> Hmm, this let me go back read again on how we manage anon_vma lifespan,
> then I just noticed this may not work.
> 
> See the comment right above [1], here's a full version:
> 
> 	/*
> 	 * If this folio is still mapped, then its anon_vma cannot have been
> 	 * freed.  But if it has been unmapped, we have no security against the
> 	 * anon_vma structure being freed and reused (for another anon_vma:
> 	 * SLAB_TYPESAFE_BY_RCU guarantees that - so the atomic_inc_not_zero()
> 	 * above cannot corrupt).
> 	 */
> 
> So afaiu that means we pretty much very rely upon folio_mapped() check to
> make sure anon_vma being valid at all that we fetched from folio->mapping,
> not to mention the rmap walk later afterwards.
> 
> Then above diff in folio_get_anon_vma() should be problematic, as when
> "folio_mapped()==false && folio_test_swapcache()==true", above change will
> start to return anon_vma pointer even if the anon_vma could have been freed
> and reused by other VMAs.

When splitting a folio, we use folio_get_anon_vma(). That seems to work 
as long as we have the folio locked.

-- 
Cheers,

David / dhildenb