mm, swap: introduce swap table

From: Kairui Song <[email protected]>

mm/userfaultfd.c +0 -0

index e5a0db7f3331..5b4f01aecf35 100644

Quoted 8 lines

From: Kairui Song <[email protected]>

mm/userfaultfd.c +0 -0

index e5a0db7f3331..5b4f01aecf35 100644

mm/userfaultfd.c static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, · line 1415

Quoted 7 lines

mm/userfaultfd.c +4 -0

@@ -1409,6 +1409,10 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
                              goto retry;
                      }
              }
+             if (!folio_swap_contains(src_folio, entry)) {
+                     err = -EBUSY;
+                     goto out;
+             }

It seems we don't need this. In move_swap_pte(), we have been checking pte pages are stable:

        if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
                                 dst_pmd, dst_pmdval)) {
                double_pt_unlock(dst_ptl, src_ptl);
                return -EAGAIN;
        }

Also, -EBUSY is somehow incorrect error code.

mm/userfaultfd.c static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, · line 1418

Quoted 5 lines

mm/userfaultfd.c +0 -0

              err = move_swap_pte(mm, dst_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);

Thanks
Barry

Quoted 19 lines · Barry Song

Quoted 8 lines

From: Kairui Song <[email protected]>

mm/userfaultfd.c +0 -0

index e5a0db7f3331..5b4f01aecf35 100644

mm/userfaultfd.c static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, · line 1415

Quoted 7 lines

mm/userfaultfd.c +4 -0

@@ -1409,6 +1409,10 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
                              goto retry;
                      }
              }
+             if (!folio_swap_contains(src_folio, entry)) {
+                     err = -EBUSY;
+                     goto out;
+             }

It seems we don't need this. In move_swap_pte(), we have been checking pte pages are stable:

Quoted 5 lines

        if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
                                 dst_pmd, dst_pmdval)) {
                double_pt_unlock(dst_ptl, src_ptl);
                return -EAGAIN;
        }

The tricky part is when swap_cache_get_folio returns the folio, both folio and ptes are unlocked. So is it possible that someone else swapped in the entries, then swapped them out again using the same entries?

The folio will be different here but PTEs are still the same value to they will pass the is_pte_pages_stable check, we previously saw similar races with anon fault or shmem. I think more strict checking won't hurt here.

Quoted 3 lines

Also, -EBUSY is somehow incorrect error code.

Yes, thanks, I'll use EAGAIN here just like move_swap_pte.

Quoted 6 lines

Quoted 5 lines

mm/userfaultfd.c +0 -0

              err = move_swap_pte(mm, dst_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);

Quoted 32 lines · Kairui Song

Quoted 19 lines · Barry Song

Quoted 8 lines

From: Kairui Song <[email protected]>

mm/userfaultfd.c +0 -0

index e5a0db7f3331..5b4f01aecf35 100644

mm/userfaultfd.c static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, · line 1415

Quoted 7 lines

mm/userfaultfd.c +4 -0

@@ -1409,6 +1409,10 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
                              goto retry;
                      }
              }
+             if (!folio_swap_contains(src_folio, entry)) {
+                     err = -EBUSY;
+                     goto out;
+             }

It seems we don't need this. In move_swap_pte(), we have been checking pte pages are stable:

Quoted 5 lines

        if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
                                 dst_pmd, dst_pmdval)) {
                double_pt_unlock(dst_ptl, src_ptl);
                return -EAGAIN;
        }

The tricky part is when swap_cache_get_folio returns the folio, both folio and ptes are unlocked. So is it possible that someone else swapped in the entries, then swapped them out again using the same entries?

Quoted 4 lines

The folio will be different here but PTEs are still the same value to they will pass the is_pte_pages_stable check, we previously saw similar races with anon fault or shmem. I think more strict checking won't hurt here.

This doesn't seem to be the same case as the one you fixed in do_swap_page(). Here, we're hitting the swap cache, whereas in that case, there was no one hitting the swap cache, and you used swap_prepare() to set up the cache to fix the issue.

By the way, if we're not hitting the swap cache, src_folio will be NULL. Also, it seems that folio_swap_contains(src_folio, entry) does not guard against that case either.

But I suspect we won't have a problem, since we're not swapping in — we didn't read any stale data, right? Swap-in will only occur after we move the PTEs.

Quoted 14 lines

Quoted 4 lines

Also, -EBUSY is somehow incorrect error code.

Yes, thanks, I'll use EAGAIN here just like move_swap_pte.

Quoted 6 lines

Quoted 5 lines

mm/userfaultfd.c +0 -0

              err = move_swap_pte(mm, dst_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);

Thanks
Barry

Quoted 39 lines · Barry Song

Quoted 32 lines · Kairui Song

Quoted 19 lines · Barry Song

Quoted 8 lines

From: Kairui Song <[email protected]>

mm/userfaultfd.c +0 -0

index e5a0db7f3331..5b4f01aecf35 100644

mm/userfaultfd.c static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, · line 1415

Quoted 7 lines

mm/userfaultfd.c +4 -0

@@ -1409,6 +1409,10 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
                              goto retry;
                      }
              }
+             if (!folio_swap_contains(src_folio, entry)) {
+                     err = -EBUSY;
+                     goto out;
+             }

It seems we don't need this. In move_swap_pte(), we have been checking pte pages are stable:

Quoted 5 lines

        if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
                                 dst_pmd, dst_pmdval)) {
                double_pt_unlock(dst_ptl, src_ptl);
                return -EAGAIN;
        }

The tricky part is when swap_cache_get_folio returns the folio, both folio and ptes are unlocked. So is it possible that someone else swapped in the entries, then swapped them out again using the same entries?

Quoted 4 lines

The folio will be different here but PTEs are still the same value to they will pass the is_pte_pages_stable check, we previously saw similar races with anon fault or shmem. I think more strict checking won't hurt here.

Quoted 8 lines

This doesn't seem to be the same case as the one you fixed in do_swap_page(). Here, we're hitting the swap cache, whereas in that case, there was no one hitting the swap cache, and you used swap_prepare() to set up the cache to fix the issue.

By the way, if we're not hitting the swap cache, src_folio will be NULL. Also, it seems that folio_swap_contains(src_folio, entry) does not guard against that case either.

Ah, that's true, it should be moved inside the if (folio) {...} block above. Thanks for catching this!

Quoted 3 lines

But I suspect we won't have a problem, since we're not swapping in — we didn't read any stale data, right? Swap-in will only occur after we move the PTEs.

My concern is that a parallel swapin / swapout could result in the folio to be a completely irrelevant or invalid folio.

It's not about the dst, but in the move src side, something like:

CPU1 CPU2
move_pages_pte
folio = swap_cache_get_folio(...)
| Got folio A here
move_swap_pte
<swapin src_pte, using folio A> <swapout src_pte, put folio A>
| Now folio A is no longer valid. | It's very unlikely but here SWAP | could reuse the same entry as above.

double_pt_lock
is_pte_pages_stable
| Passed because of entry reuse.
folio_move_anon_rmap(...)
| Moved invalid folio A.

And could it be possible that the swap_cache_get_folio returns NULL here, but later right before the double_pt_lock, a folio is added to swap cache? Maybe we better check the swap cache after clear and releasing dst lock, but before releasing src lock?

Quoted 11 lines

Quoted 11 lines

Quoted 6 lines

Also, -EBUSY is somehow incorrect error code.

Yes, thanks, I'll use EAGAIN here just like move_swap_pte.

Quoted 1 line

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

mm/userfaultfd.c static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, · line 1418

Quoted 4 lines

mm/userfaultfd.c +0 -0

              err = move_swap_pte(mm, dst_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);

Quoted 62 lines · Kairui Song

Quoted 39 lines · Barry Song

Quoted 32 lines · Kairui Song

Quoted 19 lines · Barry Song

Quoted 8 lines

From: Kairui Song <[email protected]>

mm/userfaultfd.c +0 -0

index e5a0db7f3331..5b4f01aecf35 100644

mm/userfaultfd.c static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, · line 1415

Quoted 7 lines

mm/userfaultfd.c +4 -0

@@ -1409,6 +1409,10 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
                              goto retry;
                      }
              }
+             if (!folio_swap_contains(src_folio, entry)) {
+                     err = -EBUSY;
+                     goto out;
+             }

It seems we don't need this. In move_swap_pte(), we have been checking pte pages are stable:

Quoted 5 lines

        if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
                                 dst_pmd, dst_pmdval)) {
                double_pt_unlock(dst_ptl, src_ptl);
                return -EAGAIN;
        }

The tricky part is when swap_cache_get_folio returns the folio, both folio and ptes are unlocked. So is it possible that someone else swapped in the entries, then swapped them out again using the same entries?

Quoted 4 lines

The folio will be different here but PTEs are still the same value to they will pass the is_pte_pages_stable check, we previously saw similar races with anon fault or shmem. I think more strict checking won't hurt here.

Quoted 8 lines

This doesn't seem to be the same case as the one you fixed in do_swap_page(). Here, we're hitting the swap cache, whereas in that case, there was no one hitting the swap cache, and you used swap_prepare() to set up the cache to fix the issue.

By the way, if we're not hitting the swap cache, src_folio will be NULL. Also, it seems that folio_swap_contains(src_folio, entry) does not guard against that case either.

Ah, that's true, it should be moved inside the if (folio) {...} block above. Thanks for catching this!

Quoted 3 lines

But I suspect we won't have a problem, since we're not swapping in — we didn't read any stale data, right? Swap-in will only occur after we move the PTEs.

My concern is that a parallel swapin / swapout could result in the folio to be a completely irrelevant or invalid folio.

It's not about the dst, but in the move src side, something like:

Quoted 10 lines

CPU1 CPU2
move_pages_pte
folio = swap_cache_get_folio(...)
| Got folio A here
move_swap_pte
<swapin src_pte, using folio A> <swapout src_pte, put folio A>
| Now folio A is no longer valid. | It's very unlikely but here SWAP | could reuse the same entry as above.

swap_cache_get_folio() does increment the folio's refcount, but it seems this doesn't prevent do_swap_page() from freeing the swap entry after swapping in src_pte with folio A, if it's a read fault.
for write fault, folio_ref_count(folio) == (1 + folio_nr_pages(folio)) will be false:

static inline bool should_try_to_free_swap(struct folio *folio,
struct vm_area_struct *vma, unsigned int fault_flags)

{
       ...

        /*
         * If we want to map a page that's in the swapcache writable, we
         * have to detect via the refcount if we're really the exclusive
         * user. Try freeing the swapcache to get rid of the swapcache
         * reference only in case it's likely that we'll be the exlusive user.
         */
        return (fault_flags & FAULT_FLAG_WRITE) && !folio_test_ksm(folio) &&
                folio_ref_count(folio) == (1 + folio_nr_pages(folio));
}

and for swapout, __removing_mapping does check refcount as well:

static int __remove_mapping(struct address_space *mapping, struct folio *folio,
bool reclaimed, struct mem_cgroup *target_memcg)

{
        refcount = 1 + folio_nr_pages(folio);
        if (!folio_ref_freeze(folio, refcount))
                goto cannot_free;

}

However, since __remove_mapping() occurs after pageout(), it seems this also doesn't prevent swapout from allocating a new swap entry to fill src_pte.

It seems your concern is valid—unless I'm missing something. Do you have a reproducer? If so, this will likely need a separate fix patch rather than being hidden in this patchset.

Quoted 11 lines

double_pt_lock
is_pte_pages_stable
| Passed because of entry reuse.
folio_move_anon_rmap(...)
| Moved invalid folio A.

And could it be possible that the swap_cache_get_folio returns NULL here, but later right before the double_pt_lock, a folio is added to swap cache? Maybe we better check the swap cache after clear and releasing dst lock, but before releasing src lock?

It seems you're suggesting that a parallel swap-in allocates and adds a folio to the swap cache, but the PTE has not yet been updated from a swap entry to a present mapping?

As long as do_swap_page() adds the folio to the swap cache before updating the PTE to present, this scenario seems possible.

It seems we need to double-check:

mm/userfaultfd.c +7 -1

index bc473ad21202..976053bd2bf1 100644
@@ -1102,8 +1102,14 @@ static int move_swap_pte(struct mm_struct *mm,
 struct vm_area_struct *dst_vma,
        if (src_folio) {
                folio_move_anon_rmap(src_folio, dst_vma);
                src_folio->index = linear_page_index(dst_vma, dst_addr);
+       } else {
+               struct folio *folio =
 filemap_get_folio(swap_address_space(entry),
+                                       swap_cache_index(entry));
+               if (!IS_ERR_OR_NULL(folio)) {
+                       double_pt_unlock(dst_ptl, src_ptl);
+                       return -EAGAIN;
+               }
        }
-
        orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);

 #ifdef CONFIG_MEM_SOFT_DIRTY
        orig_src_pte = pte_swp_mksoft_dirty(orig_src_pte);

Let me run test case [1] to check whether this ever happens. I guess I need to hack kernel a bit to always add folio to swapcache even for SYNC IO.

[1] https://lore.kernel.org/linux-mm/[email protected]/

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 7 lines

Also, -EBUSY is somehow incorrect error code.

Yes, thanks, I'll use EAGAIN here just like move_swap_pte.

Quoted 1 line

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

mm/userfaultfd.c static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, · line 1418

Quoted 4 lines

mm/userfaultfd.c +0 -0

              err = move_swap_pte(mm, dst_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);

Thanks
Barry

Quoted 109 lines · Barry Song <[email protected]> 于 2025年5月21日周三 06:33写道：

Quoted 62 lines · Kairui Song

Quoted 39 lines · Barry Song

Quoted 32 lines · Kairui Song

Quoted 19 lines · Barry Song

Quoted 8 lines

From: Kairui Song <[email protected]>

mm/userfaultfd.c +0 -0

index e5a0db7f3331..5b4f01aecf35 100644

mm/userfaultfd.c static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, · line 1415

Quoted 7 lines

mm/userfaultfd.c +4 -0

@@ -1409,6 +1409,10 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
                              goto retry;
                      }
              }
+             if (!folio_swap_contains(src_folio, entry)) {
+                     err = -EBUSY;
+                     goto out;
+             }

It seems we don't need this. In move_swap_pte(), we have been checking pte pages are stable:

Quoted 5 lines

        if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
                                 dst_pmd, dst_pmdval)) {
                double_pt_unlock(dst_ptl, src_ptl);
                return -EAGAIN;
        }

The tricky part is when swap_cache_get_folio returns the folio, both folio and ptes are unlocked. So is it possible that someone else swapped in the entries, then swapped them out again using the same entries?

Quoted 4 lines

The folio will be different here but PTEs are still the same value to they will pass the is_pte_pages_stable check, we previously saw similar races with anon fault or shmem. I think more strict checking won't hurt here.

Quoted 8 lines

This doesn't seem to be the same case as the one you fixed in do_swap_page(). Here, we're hitting the swap cache, whereas in that case, there was no one hitting the swap cache, and you used swap_prepare() to set up the cache to fix the issue.

By the way, if we're not hitting the swap cache, src_folio will be NULL. Also, it seems that folio_swap_contains(src_folio, entry) does not guard against that case either.

Ah, that's true, it should be moved inside the if (folio) {...} block above. Thanks for catching this!

Quoted 3 lines

But I suspect we won't have a problem, since we're not swapping in — we didn't read any stale data, right? Swap-in will only occur after we move the PTEs.

My concern is that a parallel swapin / swapout could result in the folio to be a completely irrelevant or invalid folio.

It's not about the dst, but in the move src side, something like:

Quoted 10 lines

CPU1 CPU2
move_pages_pte
folio = swap_cache_get_folio(...)
| Got folio A here
move_swap_pte
<swapin src_pte, using folio A> <swapout src_pte, put folio A>
| Now folio A is no longer valid. | It's very unlikely but here SWAP | could reuse the same entry as above.

swap_cache_get_folio() does increment the folio's refcount, but it seems this doesn't prevent do_swap_page() from freeing the swap entry after swapping in src_pte with folio A, if it's a read fault.
for write fault, folio_ref_count(folio) == (1 + folio_nr_pages(folio)) will be false:

static inline bool should_try_to_free_swap(struct folio *folio,
struct vm_area_struct *vma, unsigned int fault_flags)

{
       ...

        /*
         * If we want to map a page that's in the swapcache writable, we
         * have to detect via the refcount if we're really the exclusive
         * user. Try freeing the swapcache to get rid of the swapcache
         * reference only in case it's likely that we'll be the exlusive user.
         */
        return (fault_flags & FAULT_FLAG_WRITE) && !folio_test_ksm(folio) &&
                folio_ref_count(folio) == (1 + folio_nr_pages(folio));
}

and for swapout, __removing_mapping does check refcount as well:

static int __remove_mapping(struct address_space *mapping, struct folio *folio,
bool reclaimed, struct mem_cgroup *target_memcg)

{
        refcount = 1 + folio_nr_pages(folio);
        if (!folio_ref_freeze(folio, refcount))
                goto cannot_free;

}

Quoted 7 lines

However, since __remove_mapping() occurs after pageout(), it seems this also doesn't prevent swapout from allocating a new swap entry to fill src_pte.

It seems your concern is valid—unless I'm missing something. Do you have a reproducer? If so, this will likely need a separate fix patch rather than being hidden in this patchset.

Thanks for the analysis. I don't have a reproducer yet, I did some local experiments and that seems possible, but the race window is so tiny and it's very difficult to make the swap entry reuse to collide with that, I'll try more but in theory this seems possible, or at least looks very fragile.

And yeah, let's patch the kernel first if that's a real issue.

Quoted 12 lines

Quoted 11 lines

double_pt_lock
is_pte_pages_stable
| Passed because of entry reuse.
folio_move_anon_rmap(...)
| Moved invalid folio A.

And could it be possible that the swap_cache_get_folio returns NULL here, but later right before the double_pt_lock, a folio is added to swap cache? Maybe we better check the swap cache after clear and releasing dst lock, but before releasing src lock?

Quoted 6 lines

It seems you're suggesting that a parallel swap-in allocates and adds a folio to the swap cache, but the PTE has not yet been updated from a swap entry to a present mapping?

As long as do_swap_page() adds the folio to the swap cache before updating the PTE to present, this scenario seems possible.

Yes, that's two kinds of problems here. I suspected there could be an ABA problem while working on the series, but wasn't certain. And just realised there could be another missed cache read here thanks to your review and discussion :)

Quoted 23 lines

It seems we need to double-check:

mm/userfaultfd.c +7 -1

index bc473ad21202..976053bd2bf1 100644
@@ -1102,8 +1102,14 @@ static int move_swap_pte(struct mm_struct *mm,
 struct vm_area_struct *dst_vma,
        if (src_folio) {
                folio_move_anon_rmap(src_folio, dst_vma);
                src_folio->index = linear_page_index(dst_vma, dst_addr);
+       } else {
+               struct folio *folio =
 filemap_get_folio(swap_address_space(entry),
+                                       swap_cache_index(entry));
+               if (!IS_ERR_OR_NULL(folio)) {
+                       double_pt_unlock(dst_ptl, src_ptl);
+                       return -EAGAIN;
+               }
        }
-
        orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);

Quoted 2 lines

 #ifdef CONFIG_MEM_SOFT_DIRTY
        orig_src_pte = pte_swp_mksoft_dirty(orig_src_pte);

Maybe it has to get even dirtier here to call swapcache_prepare too to cover the SYNC_IO case?

Quoted 3 lines

Let me run test case [1] to check whether this ever happens. I guess I need to hack kernel a bit to always add folio to swapcache even for SYNC IO.

That will cause quite a performance regression I think. Good thing is, that's exactly the problem this series is solving by dropping the SYNC IO swapin path and never bypassing the swap cache, while improving the performance, eliminating things like this. One more reason to justify the approach :)

Quoted 1 line

Quoted 1 line

[1] https://lore.kernel.org/linux-mm/[email protected]/

I'll try this too.

Quoted 19 lines

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

Quoted 7 lines

Also, -EBUSY is somehow incorrect error code.

Yes, thanks, I'll use EAGAIN here just like move_swap_pte.

Quoted 6 lines

Quoted 5 lines

mm/userfaultfd.c +0 -0

              err = move_swap_pte(mm, dst_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);

Quoted 109 lines · Barry Song <[email protected]> 于 2025年5月21日周三 06:33写道：

Quoted 62 lines · Kairui Song

Quoted 39 lines · Barry Song

Quoted 32 lines · Kairui Song

Quoted 19 lines · Barry Song

Quoted 8 lines

From: Kairui Song <[email protected]>

mm/userfaultfd.c +0 -0

index e5a0db7f3331..5b4f01aecf35 100644

mm/userfaultfd.c static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, · line 1415

Quoted 7 lines

mm/userfaultfd.c +4 -0

@@ -1409,6 +1409,10 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
                              goto retry;
                      }
              }
+             if (!folio_swap_contains(src_folio, entry)) {
+                     err = -EBUSY;
+                     goto out;
+             }

It seems we don't need this. In move_swap_pte(), we have been checking pte pages are stable:

Quoted 5 lines

        if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
                                 dst_pmd, dst_pmdval)) {
                double_pt_unlock(dst_ptl, src_ptl);
                return -EAGAIN;
        }

The tricky part is when swap_cache_get_folio returns the folio, both folio and ptes are unlocked. So is it possible that someone else swapped in the entries, then swapped them out again using the same entries?

Quoted 4 lines

The folio will be different here but PTEs are still the same value to they will pass the is_pte_pages_stable check, we previously saw similar races with anon fault or shmem. I think more strict checking won't hurt here.

Quoted 8 lines

This doesn't seem to be the same case as the one you fixed in do_swap_page(). Here, we're hitting the swap cache, whereas in that case, there was no one hitting the swap cache, and you used swap_prepare() to set up the cache to fix the issue.

By the way, if we're not hitting the swap cache, src_folio will be NULL. Also, it seems that folio_swap_contains(src_folio, entry) does not guard against that case either.

Ah, that's true, it should be moved inside the if (folio) {...} block above. Thanks for catching this!

Quoted 3 lines

But I suspect we won't have a problem, since we're not swapping in — we didn't read any stale data, right? Swap-in will only occur after we move the PTEs.

My concern is that a parallel swapin / swapout could result in the folio to be a completely irrelevant or invalid folio.

It's not about the dst, but in the move src side, something like:

Quoted 10 lines

CPU1 CPU2
move_pages_pte
folio = swap_cache_get_folio(...)
| Got folio A here
move_swap_pte
<swapin src_pte, using folio A> <swapout src_pte, put folio A>
| Now folio A is no longer valid. | It's very unlikely but here SWAP | could reuse the same entry as above.

swap_cache_get_folio() does increment the folio's refcount, but it seems this doesn't prevent do_swap_page() from freeing the swap entry after swapping in src_pte with folio A, if it's a read fault.
for write fault, folio_ref_count(folio) == (1 + folio_nr_pages(folio)) will be false:

static inline bool should_try_to_free_swap(struct folio *folio,
struct vm_area_struct *vma, unsigned int fault_flags)

{
       ...

        /*
         * If we want to map a page that's in the swapcache writable, we
         * have to detect via the refcount if we're really the exclusive
         * user. Try freeing the swapcache to get rid of the swapcache
         * reference only in case it's likely that we'll be the exlusive user.
         */
        return (fault_flags & FAULT_FLAG_WRITE) && !folio_test_ksm(folio) &&
                folio_ref_count(folio) == (1 + folio_nr_pages(folio));
}

and for swapout, __removing_mapping does check refcount as well:

static int __remove_mapping(struct address_space *mapping, struct folio *folio,
bool reclaimed, struct mem_cgroup *target_memcg)

{
        refcount = 1 + folio_nr_pages(folio);
        if (!folio_ref_freeze(folio, refcount))
                goto cannot_free;

}

Quoted 7 lines

However, since __remove_mapping() occurs after pageout(), it seems this also doesn't prevent swapout from allocating a new swap entry to fill src_pte.

It seems your concern is valid—unless I'm missing something. Do you have a reproducer? If so, this will likely need a separate fix patch rather than being hidden in this patchset.

Thanks for the analysis. I don't have a reproducer yet, I did some local experiments and that seems possible, but the race window is so tiny and it's very difficult to make the swap entry reuse to collide with that, I'll try more but in theory this seems possible, or at least looks very fragile.

And yeah, let's patch the kernel first if that's a real issue.

Quoted 12 lines

Quoted 11 lines

double_pt_lock
is_pte_pages_stable
| Passed because of entry reuse.
folio_move_anon_rmap(...)
| Moved invalid folio A.

And could it be possible that the swap_cache_get_folio returns NULL here, but later right before the double_pt_lock, a folio is added to swap cache? Maybe we better check the swap cache after clear and releasing dst lock, but before releasing src lock?

Quoted 6 lines

It seems you're suggesting that a parallel swap-in allocates and adds a folio to the swap cache, but the PTE has not yet been updated from a swap entry to a present mapping?

As long as do_swap_page() adds the folio to the swap cache before updating the PTE to present, this scenario seems possible.

Yes, that's two kinds of problems here. I suspected there could be an ABA problem while working on the series, but wasn't certain. And just realised there could be another missed cache read here thanks to your review and discussion :)

Quoted 23 lines

It seems we need to double-check:

mm/userfaultfd.c +7 -1

index bc473ad21202..976053bd2bf1 100644
@@ -1102,8 +1102,14 @@ static int move_swap_pte(struct mm_struct *mm,
 struct vm_area_struct *dst_vma,
        if (src_folio) {
                folio_move_anon_rmap(src_folio, dst_vma);
                src_folio->index = linear_page_index(dst_vma, dst_addr);
+       } else {
+               struct folio *folio =
 filemap_get_folio(swap_address_space(entry),
+                                       swap_cache_index(entry));
+               if (!IS_ERR_OR_NULL(folio)) {
+                       double_pt_unlock(dst_ptl, src_ptl);
+                       return -EAGAIN;
+               }
        }
-
        orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);

Quoted 2 lines

 #ifdef CONFIG_MEM_SOFT_DIRTY
        orig_src_pte = pte_swp_mksoft_dirty(orig_src_pte);

Maybe it has to get even dirtier here to call swapcache_prepare too to cover the SYNC_IO case?

Quoted 1 line

Quoted 2 lines

Let me run test case [1] to check whether this ever happens. I guess I need to hack kernel a bit to always add folio to swapcache even for SYNC IO.

That will cause quite a performance regression I think. Good thing is, that's exactly the problem this series is solving by dropping the SYNC IO swapin path and never bypassing the swap cache, while improving the performance, eliminating things like this. One more reason to justify the approach :)

Quoted 3 lines

[1] https://lore.kernel.org/linux-mm/[email protected]/

I'll try this too.

Quoted 19 lines

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

Quoted 7 lines

Also, -EBUSY is somehow incorrect error code.

Yes, thanks, I'll use EAGAIN here just like move_swap_pte.

Quoted 6 lines

Quoted 5 lines

mm/userfaultfd.c +0 -0

              err = move_swap_pte(mm, dst_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);

Quoted 181 lines · Kairui Song

Quoted 109 lines · Barry Song <[email protected]> 于 2025年5月21日周三 06:33写道：

Quoted 62 lines · Kairui Song

Quoted 39 lines · Barry Song

Quoted 32 lines · Kairui Song

Quoted 19 lines · Barry Song

Quoted 8 lines

From: Kairui Song <[email protected]>

mm/userfaultfd.c +0 -0

index e5a0db7f3331..5b4f01aecf35 100644

mm/userfaultfd.c static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, · line 1415

Quoted 7 lines

mm/userfaultfd.c +4 -0

@@ -1409,6 +1409,10 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
                              goto retry;
                      }
              }
+             if (!folio_swap_contains(src_folio, entry)) {
+                     err = -EBUSY;
+                     goto out;
+             }

It seems we don't need this. In move_swap_pte(), we have been checking pte pages are stable:

Quoted 5 lines

        if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
                                 dst_pmd, dst_pmdval)) {
                double_pt_unlock(dst_ptl, src_ptl);
                return -EAGAIN;
        }

The tricky part is when swap_cache_get_folio returns the folio, both folio and ptes are unlocked. So is it possible that someone else swapped in the entries, then swapped them out again using the same entries?

Quoted 4 lines

The folio will be different here but PTEs are still the same value to they will pass the is_pte_pages_stable check, we previously saw similar races with anon fault or shmem. I think more strict checking won't hurt here.

Quoted 8 lines

This doesn't seem to be the same case as the one you fixed in do_swap_page(). Here, we're hitting the swap cache, whereas in that case, there was no one hitting the swap cache, and you used swap_prepare() to set up the cache to fix the issue.

By the way, if we're not hitting the swap cache, src_folio will be NULL. Also, it seems that folio_swap_contains(src_folio, entry) does not guard against that case either.

Ah, that's true, it should be moved inside the if (folio) {...} block above. Thanks for catching this!

Quoted 3 lines

But I suspect we won't have a problem, since we're not swapping in — we didn't read any stale data, right? Swap-in will only occur after we move the PTEs.

My concern is that a parallel swapin / swapout could result in the folio to be a completely irrelevant or invalid folio.

It's not about the dst, but in the move src side, something like:

Quoted 10 lines

CPU1 CPU2
move_pages_pte
folio = swap_cache_get_folio(...)
| Got folio A here
move_swap_pte
<swapin src_pte, using folio A> <swapout src_pte, put folio A>
| Now folio A is no longer valid. | It's very unlikely but here SWAP | could reuse the same entry as above.

swap_cache_get_folio() does increment the folio's refcount, but it seems this doesn't prevent do_swap_page() from freeing the swap entry after swapping in src_pte with folio A, if it's a read fault.
for write fault, folio_ref_count(folio) == (1 + folio_nr_pages(folio)) will be false:

static inline bool should_try_to_free_swap(struct folio *folio,
struct vm_area_struct *vma, unsigned int fault_flags)

{
       ...

        /*
         * If we want to map a page that's in the swapcache writable, we
         * have to detect via the refcount if we're really the exclusive
         * user. Try freeing the swapcache to get rid of the swapcache
         * reference only in case it's likely that we'll be the exlusive user.
         */
        return (fault_flags & FAULT_FLAG_WRITE) && !folio_test_ksm(folio) &&
                folio_ref_count(folio) == (1 + folio_nr_pages(folio));
}

and for swapout, __removing_mapping does check refcount as well:

static int __remove_mapping(struct address_space *mapping, struct folio *folio,
bool reclaimed, struct mem_cgroup *target_memcg)

{
        refcount = 1 + folio_nr_pages(folio);
        if (!folio_ref_freeze(folio, refcount))
                goto cannot_free;

}

Quoted 7 lines

However, since __remove_mapping() occurs after pageout(), it seems this also doesn't prevent swapout from allocating a new swap entry to fill src_pte.

It seems your concern is valid—unless I'm missing something. Do you have a reproducer? If so, this will likely need a separate fix patch rather than being hidden in this patchset.

Thanks for the analysis. I don't have a reproducer yet, I did some local experiments and that seems possible, but the race window is so tiny and it's very difficult to make the swap entry reuse to collide with that, I'll try more but in theory this seems possible, or at least looks very fragile.

And yeah, let's patch the kernel first if that's a real issue.

Quoted 12 lines

Quoted 11 lines

double_pt_lock
is_pte_pages_stable
| Passed because of entry reuse.
folio_move_anon_rmap(...)
| Moved invalid folio A.

And could it be possible that the swap_cache_get_folio returns NULL here, but later right before the double_pt_lock, a folio is added to swap cache? Maybe we better check the swap cache after clear and releasing dst lock, but before releasing src lock?

Quoted 6 lines

It seems you're suggesting that a parallel swap-in allocates and adds a folio to the swap cache, but the PTE has not yet been updated from a swap entry to a present mapping?

As long as do_swap_page() adds the folio to the swap cache before updating the PTE to present, this scenario seems possible.

Yes, that's two kinds of problems here. I suspected there could be an ABA problem while working on the series, but wasn't certain. And just realised there could be another missed cache read here thanks to your review and discussion :)

Quoted 23 lines

It seems we need to double-check:

mm/userfaultfd.c +7 -1

index bc473ad21202..976053bd2bf1 100644
@@ -1102,8 +1102,14 @@ static int move_swap_pte(struct mm_struct *mm,
 struct vm_area_struct *dst_vma,
        if (src_folio) {
                folio_move_anon_rmap(src_folio, dst_vma);
                src_folio->index = linear_page_index(dst_vma, dst_addr);
+       } else {
+               struct folio *folio =
 filemap_get_folio(swap_address_space(entry),
+                                       swap_cache_index(entry));
+               if (!IS_ERR_OR_NULL(folio)) {
+                       double_pt_unlock(dst_ptl, src_ptl);
+                       return -EAGAIN;
+               }
        }
-
        orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);

Quoted 2 lines

 #ifdef CONFIG_MEM_SOFT_DIRTY
        orig_src_pte = pte_swp_mksoft_dirty(orig_src_pte);

Maybe it has to get even dirtier here to call swapcache_prepare too to cover the SYNC_IO case?

Quoted 1 line

Quoted 8 lines

Quoted 2 lines

Let me run test case [1] to check whether this ever happens. I guess I need to hack kernel a bit to always add folio to swapcache even for SYNC IO.

That will cause quite a performance regression I think. Good thing is, that's exactly the problem this series is solving by dropping the SYNC IO swapin path and never bypassing the swap cache, while improving the performance, eliminating things like this. One more reason to justify the approach :)

I attempted to reproduce the scenario where a folio is added to the swapcache after filemap_get_folio() returns NULL but before move_swap_pte() moves the swap PTE
using non-synchronized I/O. Technically, this seems possible; however, I was unable
to reproduce it, likely because the time window between swapin_readahead and taking the page table lock within do_swap_page() is too short.

Quoted 183 lines · Barry Song

Quoted 181 lines · Kairui Song

Quoted 109 lines · Barry Song <[email protected]> 于 2025年5月21日周三 06:33写道：

Quoted 62 lines · Kairui Song

Quoted 39 lines · Barry Song

Quoted 32 lines · Kairui Song

Quoted 19 lines · Barry Song

Quoted 8 lines

From: Kairui Song <[email protected]>

mm/userfaultfd.c +0 -0

index e5a0db7f3331..5b4f01aecf35 100644

mm/userfaultfd.c static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, · line 1415

Quoted 7 lines

mm/userfaultfd.c +4 -0

@@ -1409,6 +1409,10 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
                              goto retry;
                      }
              }
+             if (!folio_swap_contains(src_folio, entry)) {
+                     err = -EBUSY;
+                     goto out;
+             }

It seems we don't need this. In move_swap_pte(), we have been checking pte pages are stable:

Quoted 5 lines

        if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
                                 dst_pmd, dst_pmdval)) {
                double_pt_unlock(dst_ptl, src_ptl);
                return -EAGAIN;
        }

The tricky part is when swap_cache_get_folio returns the folio, both folio and ptes are unlocked. So is it possible that someone else swapped in the entries, then swapped them out again using the same entries?

Quoted 4 lines

The folio will be different here but PTEs are still the same value to they will pass the is_pte_pages_stable check, we previously saw similar races with anon fault or shmem. I think more strict checking won't hurt here.

Quoted 8 lines

This doesn't seem to be the same case as the one you fixed in do_swap_page(). Here, we're hitting the swap cache, whereas in that case, there was no one hitting the swap cache, and you used swap_prepare() to set up the cache to fix the issue.

By the way, if we're not hitting the swap cache, src_folio will be NULL. Also, it seems that folio_swap_contains(src_folio, entry) does not guard against that case either.

Ah, that's true, it should be moved inside the if (folio) {...} block above. Thanks for catching this!

Quoted 3 lines

But I suspect we won't have a problem, since we're not swapping in — we didn't read any stale data, right? Swap-in will only occur after we move the PTEs.

My concern is that a parallel swapin / swapout could result in the folio to be a completely irrelevant or invalid folio.

It's not about the dst, but in the move src side, something like:

Quoted 10 lines

CPU1 CPU2
move_pages_pte
folio = swap_cache_get_folio(...)
| Got folio A here
move_swap_pte
<swapin src_pte, using folio A> <swapout src_pte, put folio A>
| Now folio A is no longer valid. | It's very unlikely but here SWAP | could reuse the same entry as above.

swap_cache_get_folio() does increment the folio's refcount, but it seems this doesn't prevent do_swap_page() from freeing the swap entry after swapping in src_pte with folio A, if it's a read fault.
for write fault, folio_ref_count(folio) == (1 + folio_nr_pages(folio)) will be false:

static inline bool should_try_to_free_swap(struct folio *folio,
struct vm_area_struct *vma, unsigned int fault_flags)

{
       ...

        /*
         * If we want to map a page that's in the swapcache writable, we
         * have to detect via the refcount if we're really the exclusive
         * user. Try freeing the swapcache to get rid of the swapcache
         * reference only in case it's likely that we'll be the exlusive user.
         */
        return (fault_flags & FAULT_FLAG_WRITE) && !folio_test_ksm(folio) &&
                folio_ref_count(folio) == (1 + folio_nr_pages(folio));
}

and for swapout, __removing_mapping does check refcount as well:

static int __remove_mapping(struct address_space *mapping, struct folio *folio,
bool reclaimed, struct mem_cgroup *target_memcg)

{
        refcount = 1 + folio_nr_pages(folio);
        if (!folio_ref_freeze(folio, refcount))
                goto cannot_free;

}

Quoted 7 lines

However, since __remove_mapping() occurs after pageout(), it seems this also doesn't prevent swapout from allocating a new swap entry to fill src_pte.

It seems your concern is valid—unless I'm missing something. Do you have a reproducer? If so, this will likely need a separate fix patch rather than being hidden in this patchset.

Thanks for the analysis. I don't have a reproducer yet, I did some local experiments and that seems possible, but the race window is so tiny and it's very difficult to make the swap entry reuse to collide with that, I'll try more but in theory this seems possible, or at least looks very fragile.

And yeah, let's patch the kernel first if that's a real issue.

Quoted 12 lines

Quoted 11 lines

double_pt_lock
is_pte_pages_stable
| Passed because of entry reuse.
folio_move_anon_rmap(...)
| Moved invalid folio A.

And could it be possible that the swap_cache_get_folio returns NULL here, but later right before the double_pt_lock, a folio is added to swap cache? Maybe we better check the swap cache after clear and releasing dst lock, but before releasing src lock?

Quoted 6 lines

It seems you're suggesting that a parallel swap-in allocates and adds a folio to the swap cache, but the PTE has not yet been updated from a swap entry to a present mapping?

As long as do_swap_page() adds the folio to the swap cache before updating the PTE to present, this scenario seems possible.

Yes, that's two kinds of problems here. I suspected there could be an ABA problem while working on the series, but wasn't certain. And just realised there could be another missed cache read here thanks to your review and discussion :)

Quoted 23 lines

It seems we need to double-check:

mm/userfaultfd.c +7 -1

index bc473ad21202..976053bd2bf1 100644
@@ -1102,8 +1102,14 @@ static int move_swap_pte(struct mm_struct *mm,
 struct vm_area_struct *dst_vma,
        if (src_folio) {
                folio_move_anon_rmap(src_folio, dst_vma);
                src_folio->index = linear_page_index(dst_vma, dst_addr);
+       } else {
+               struct folio *folio =
 filemap_get_folio(swap_address_space(entry),
+                                       swap_cache_index(entry));
+               if (!IS_ERR_OR_NULL(folio)) {
+                       double_pt_unlock(dst_ptl, src_ptl);
+                       return -EAGAIN;
+               }
        }
-
        orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);

Quoted 2 lines

 #ifdef CONFIG_MEM_SOFT_DIRTY
        orig_src_pte = pte_swp_mksoft_dirty(orig_src_pte);

Maybe it has to get even dirtier here to call swapcache_prepare too to cover the SYNC_IO case?

Quoted 1 line

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Let me run test case [1] to check whether this ever happens. I guess I need to hack kernel a bit to always add folio to swapcache even for SYNC IO.

That will cause quite a performance regression I think. Good thing is, that's exactly the problem this series is solving by dropping the SYNC IO swapin path and never bypassing the swap cache, while improving the performance, eliminating things like this. One more reason to justify the approach :)

Hi Barry,

Quoted 8 lines

I attempted to reproduce the scenario where a folio is added to the swapcache after filemap_get_folio() returns NULL but before move_swap_pte() moves the swap PTE
using non-synchronized I/O. Technically, this seems possible; however, I was unable
to reproduce it, likely because the time window between swapin_readahead and taking the page table lock within do_swap_page() is too short.

Thank you so much for trying this!

I have been trying to reproduce it too, and so far I didn't observe any crash or warn. I added following debug code:

static __always_inline
bool validate_dst_vma(struct vm_area_struct *dst_vma, unsigned long dst_end)

(quoted diff) +1 -0

@@ -1163,6 +1167,7 @@ static int move_pages_pte(struct mm_struct *mm,
 pmd_t *dst_pmd, pmd_t *src_pmd,
        pmd_t dummy_pmdval;
        pmd_t dst_pmdval;
        struct folio *src_folio = NULL;
+       struct folio *tmp_folio = NULL;
        struct anon_vma *src_anon_vma = NULL;
        struct mmu_notifier_range range;

        int err = 0;

(quoted diff) +9 -0

@@ -1391,6 +1396,15 @@ static int move_pages_pte(struct mm_struct *mm,
 pmd_t *dst_pmd, pmd_t *src_pmd,
                if (!src_folio)
                        folio = filemap_get_folio(swap_address_space(entry),
                                        swap_cache_index(entry));
+               udelay(get_random_u32_below(1000));
+               tmp_folio = filemap_get_folio(swap_address_space(entry),
+                                       swap_cache_index(entry));
+               if (!IS_ERR_OR_NULL(tmp_folio)) {
+                       if (!IS_ERR_OR_NULL(folio) && tmp_folio != folio) {
+                               pr_err("UFFDIO_MOVE: UNSTABLE folio
 %lx (%lx) -> %lx (%lx)\n", folio, folio->swap.val, tmp_folio,
 tmp_folio->swap.val);
+                       }
+                       folio_put(tmp_folio);
+               }

                if (!IS_ERR_OR_NULL(folio)) {
                        if (folio_test_large(folio)) {
                                err = -EBUSY;

(quoted diff) +2 -0

@@ -1413,6 +1427,8 @@ static int move_pages_pte(struct mm_struct *mm,
 pmd_t *dst_pmd, pmd_t *src_pmd,
                err = move_swap_pte(mm, dst_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);
+               if (tmp_folio != folio && !err)
+                       pr_err("UFFDIO_MOVE: UNSTABLE folio passed
 check: %lx -> %lx\n", folio, tmp_folio);

        }

And I saw these two prints are getting triggered like this (not a real issue though, just help to understand the problem)
...

[ 3127.632791] UFFDIO_MOVE: UNSTABLE folio fffffdffc334cd00 (0) ->
fffffdffc7ccac80 (51)
[ 3172.033269] UFFDIO_MOVE: UNSTABLE folio fffffdffc343bb40 (0) ->
fffffdffc3435e00 (3b)
[ 3194.425213] UFFDIO_MOVE: UNSTABLE folio fffffdffc7d481c0 (0) ->
fffffdffc34ab8c0 (76)
[ 3194.991318] UFFDIO_MOVE: UNSTABLE folio fffffdffc34f95c0 (0) ->
fffffdffc34ab8c0 (6d)
[ 3203.467212] UFFDIO_MOVE: UNSTABLE folio fffffdffc34b13c0 (0) ->
fffffdffc34eda80 (32)
[ 3206.217820] UFFDIO_MOVE: UNSTABLE folio fffffdffc7d297c0 (0) ->
fffffdffc38cedc0 (b)
[ 3214.913039] UFFDIO_MOVE: UNSTABLE folio passed check: 0 -> fffffdffc34db140
[ 3217.066972] UFFDIO_MOVE: UNSTABLE folio fffffdffc342b5c0 (0) ->

fffffdffc3465cc0 (21)
...

The "UFFDIO_MOVE: UNSTABLE folio fffffdffc3435180 (0) -> fffffdffc3853540 (53)" worries me at first. On first look it seems the folio is indeed freed completely from the swap cache after the first lookup, so another swapout can reuse the entry. But as you mentioned __remove_mapping won't release a folio if the refcount check fails, so they must be freed by folio_free_swap or __try_to_reclaim_swap, there are many places that can happen. But these two helpers won't free a folio from swap cache if its swap count is not zero. And the folio will either be swapped out (swap count non zero), or mapped (freeing it is fine, PTE is non_swap, and another swapout will still use the same folio).

So after more investigation and dumping the pages, it's actually the second lookup (tmp_folio) seeing the entry being reused by another page table entry, after the first folio is swapped back and released. So the page table check below will always fail just fine.

But this also proves the first look up can see a completely irrelevant folio too: If the src folio is swapped out, but got swapped back and freed, then another folio B shortly got added to swap cache reuse the src folio's old swap entry, then the folio B got seen by the look up here and get freed from swap cache, then src folio got swapped out again also reusing the same entry, then we have a problem as PTE seems untouched indeed but we grabbed a wrong folio. Seems possible if I'm not wrong:

Something like this:
CPU1 CPU2
move_pages_pte
entry = pte_to_swp_entry(orig_src_pte);
| Got Swap Entry S1 from src_pte
...
<swapin src_pte, using folio A>

<free folio A from swap cache freeing S1> <someone else try swap out folio B > <put folio B to swap cache using S1 >
...
folio = swap_cache_get_folio(S1)
| Got folio B here !!!
move_swap_pte
<free folio B from swap cache>
| Holding a reference doesn't pin the cache | as we have demonstrated
<Swapout folio A also using S1>
double_pt_lock
is_pte_pages_stable
| Passed because of S1 is reused
folio_move_anon_rmap(...)
| Moved invalid folio B here !!!

But this is extremely hard to reproduce though, even if doing it deliberately...

So I think a "folio_swap_contains" or equivalent check here is a good thing to have, to make it more robust and easier to understand. The checking after locking a folio has very tiny overhead and can definitely ensure the folio's swap entry is valid and stable.

The "UFFDIO_MOVE: UNSTABLE folio passed check: 0 -> fffffdffc385fb00" here might seem problematic, but it's still not a real problem. That's the case where the swapin in src region happens after the lookup, and before the PTE lock. It will pass the PTE check without moving the folio. But the folio is guaranteed to be a completely new folio here because the folio can't be added back to the page table without holding the PTE lock, and if that happens the following PTE check here will fail.

So I think we should patch the current kernel only adding a "folio_swap_contains" equivalent check here, and maybe more comments, how do you think?

Quoted 118 lines · Kairui Song

Quoted 4 lines · Barry Song

Quoted 2 lines · Kairui Song

Barry Song <[email protected]> 于 2025年5月21日周三 06:33写道：

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Let me run test case [1] to check whether this ever happens. I guess I need to hack kernel a bit to always add folio to swapcache even for SYNC IO.

That will cause quite a performance regression I think. Good thing is, that's exactly the problem this series is solving by dropping the SYNC IO swapin path and never bypassing the swap cache, while improving the performance, eliminating things like this. One more reason to justify the approach :)

Hi Barry,

Quoted 8 lines

I attempted to reproduce the scenario where a folio is added to the swapcache after filemap_get_folio() returns NULL but before move_swap_pte() moves the swap PTE
using non-synchronized I/O. Technically, this seems possible; however, I was unable
to reproduce it, likely because the time window between swapin_readahead and taking the page table lock within do_swap_page() is too short.

Thank you so much for trying this!

I have been trying to reproduce it too, and so far I didn't observe any crash or warn. I added following debug code:

static __always_inline
bool validate_dst_vma(struct vm_area_struct *dst_vma, unsigned long dst_end)

(quoted diff) +1 -0

@@ -1163,6 +1167,7 @@ static int move_pages_pte(struct mm_struct *mm,
 pmd_t *dst_pmd, pmd_t *src_pmd,
        pmd_t dummy_pmdval;
        pmd_t dst_pmdval;
        struct folio *src_folio = NULL;
+       struct folio *tmp_folio = NULL;
        struct anon_vma *src_anon_vma = NULL;
        struct mmu_notifier_range range;

        int err = 0;

(quoted diff) +9 -0

@@ -1391,6 +1396,15 @@ static int move_pages_pte(struct mm_struct *mm,
 pmd_t *dst_pmd, pmd_t *src_pmd,
                if (!src_folio)
                        folio = filemap_get_folio(swap_address_space(entry),
                                        swap_cache_index(entry));
+               udelay(get_random_u32_below(1000));
+               tmp_folio = filemap_get_folio(swap_address_space(entry),
+                                       swap_cache_index(entry));
+               if (!IS_ERR_OR_NULL(tmp_folio)) {
+                       if (!IS_ERR_OR_NULL(folio) && tmp_folio != folio) {
+                               pr_err("UFFDIO_MOVE: UNSTABLE folio
 %lx (%lx) -> %lx (%lx)\n", folio, folio->swap.val, tmp_folio,
 tmp_folio->swap.val);
+                       }
+                       folio_put(tmp_folio);
+               }

                if (!IS_ERR_OR_NULL(folio)) {
                        if (folio_test_large(folio)) {
                                err = -EBUSY;

(quoted diff) +2 -0

@@ -1413,6 +1427,8 @@ static int move_pages_pte(struct mm_struct *mm,
 pmd_t *dst_pmd, pmd_t *src_pmd,
                err = move_swap_pte(mm, dst_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);
+               if (tmp_folio != folio && !err)
+                       pr_err("UFFDIO_MOVE: UNSTABLE folio passed
 check: %lx -> %lx\n", folio, tmp_folio);

        }

And I saw these two prints are getting triggered like this (not a real issue though, just help to understand the problem)
...

[ 3127.632791] UFFDIO_MOVE: UNSTABLE folio fffffdffc334cd00 (0) ->
fffffdffc7ccac80 (51)
[ 3172.033269] UFFDIO_MOVE: UNSTABLE folio fffffdffc343bb40 (0) ->
fffffdffc3435e00 (3b)
[ 3194.425213] UFFDIO_MOVE: UNSTABLE folio fffffdffc7d481c0 (0) ->
fffffdffc34ab8c0 (76)
[ 3194.991318] UFFDIO_MOVE: UNSTABLE folio fffffdffc34f95c0 (0) ->
fffffdffc34ab8c0 (6d)
[ 3203.467212] UFFDIO_MOVE: UNSTABLE folio fffffdffc34b13c0 (0) ->
fffffdffc34eda80 (32)
[ 3206.217820] UFFDIO_MOVE: UNSTABLE folio fffffdffc7d297c0 (0) ->
fffffdffc38cedc0 (b)
[ 3214.913039] UFFDIO_MOVE: UNSTABLE folio passed check: 0 -> fffffdffc34db140
[ 3217.066972] UFFDIO_MOVE: UNSTABLE folio fffffdffc342b5c0 (0) ->

fffffdffc3465cc0 (21)
...

The "UFFDIO_MOVE: UNSTABLE folio fffffdffc3435180 (0) -> fffffdffc3853540 (53)" worries me at first. On first look it seems the folio is indeed freed completely from the swap cache after the first lookup, so another swapout can reuse the entry. But as you mentioned __remove_mapping won't release a folio if the refcount check fails, so they must be freed by folio_free_swap or __try_to_reclaim_swap, there are many places that can happen. But these two helpers won't free a folio from swap cache if its swap count is not zero. And the folio will either be swapped out (swap count non zero), or mapped (freeing it is fine, PTE is non_swap, and another swapout will still use the same folio).

So after more investigation and dumping the pages, it's actually the second lookup (tmp_folio) seeing the entry being reused by another page table entry, after the first folio is swapped back and released. So the page table check below will always fail just fine.

But this also proves the first look up can see a completely irrelevant folio too: If the src folio is swapped out, but got swapped back and freed, then another folio B shortly got added to swap cache reuse the src folio's old swap entry, then the folio B got seen by the look up here and get freed from swap cache, then src folio got swapped out again also reusing the same entry, then we have a problem as PTE seems untouched indeed but we grabbed a wrong folio. Seems possible if I'm not wrong:

Quoted 7 lines

Something like this:
CPU1 CPU2
move_pages_pte
entry = pte_to_swp_entry(orig_src_pte);
| Got Swap Entry S1 from src_pte
...
<swapin src_pte, using folio A>

I’m assuming you mean <swapin src_pte, using folio B>, since I’m not sure where folio B comes from in the statement <someone else tried to swap out folio B>.

If that assumption is correct, and folio A is still in the swapcache, how could someone swap in folio B without hitting folio A? That would suggest folio A must have been removed from the swapcache earlier—right?

Quoted 2 lines

<free folio A from swap cache freeing S1> <someone else try swap out folio B >

Quoted 32 lines

<put folio B to swap cache using S1 >
...
folio = swap_cache_get_folio(S1)
| Got folio B here !!!
move_swap_pte
<free folio B from swap cache>
| Holding a reference doesn't pin the cache | as we have demonstrated
<Swapout folio A also using S1>
double_pt_lock
is_pte_pages_stable
| Passed because of S1 is reused
folio_move_anon_rmap(...)
| Moved invalid folio B here !!!

But this is extremely hard to reproduce though, even if doing it deliberately...

So I think a "folio_swap_contains" or equivalent check here is a good thing to have, to make it more robust and easier to understand. The checking after locking a folio has very tiny overhead and can definitely ensure the folio's swap entry is valid and stable.

The "UFFDIO_MOVE: UNSTABLE folio passed check: 0 -> fffffdffc385fb00" here might seem problematic, but it's still not a real problem. That's the case where the swapin in src region happens after the lookup, and before the PTE lock. It will pass the PTE check without moving the folio. But the folio is guaranteed to be a completely new folio here because the folio can't be added back to the page table without holding the PTE lock, and if that happens the following PTE check here will fail.

Quoted 3 lines

So I think we should patch the current kernel only adding a "folio_swap_contains" equivalent check here, and maybe more comments, how do you think?

The description appears to have some inconsistencies. Would you mind rephrasing it?

Thanks
barry

Quoted 132 lines · Barry Song

Quoted 118 lines · Kairui Song

Quoted 4 lines · Barry Song

Quoted 2 lines · Kairui Song

Barry Song <[email protected]> 于 2025年5月21日周三 06:33写道：

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Let me run test case [1] to check whether this ever happens. I guess I need to hack kernel a bit to always add folio to swapcache even for SYNC IO.

That will cause quite a performance regression I think. Good thing is, that's exactly the problem this series is solving by dropping the SYNC IO swapin path and never bypassing the swap cache, while improving the performance, eliminating things like this. One more reason to justify the approach :)

Hi Barry,

Quoted 8 lines

I attempted to reproduce the scenario where a folio is added to the swapcache after filemap_get_folio() returns NULL but before move_swap_pte() moves the swap PTE
using non-synchronized I/O. Technically, this seems possible; however, I was unable
to reproduce it, likely because the time window between swapin_readahead and taking the page table lock within do_swap_page() is too short.

Thank you so much for trying this!

I have been trying to reproduce it too, and so far I didn't observe any crash or warn. I added following debug code:

static __always_inline
bool validate_dst_vma(struct vm_area_struct *dst_vma, unsigned long dst_end)

(quoted diff) +1 -0

@@ -1163,6 +1167,7 @@ static int move_pages_pte(struct mm_struct *mm,
 pmd_t *dst_pmd, pmd_t *src_pmd,
        pmd_t dummy_pmdval;
        pmd_t dst_pmdval;
        struct folio *src_folio = NULL;
+       struct folio *tmp_folio = NULL;
        struct anon_vma *src_anon_vma = NULL;
        struct mmu_notifier_range range;

        int err = 0;

(quoted diff) +9 -0

@@ -1391,6 +1396,15 @@ static int move_pages_pte(struct mm_struct *mm,
 pmd_t *dst_pmd, pmd_t *src_pmd,
                if (!src_folio)
                        folio = filemap_get_folio(swap_address_space(entry),
                                        swap_cache_index(entry));
+               udelay(get_random_u32_below(1000));
+               tmp_folio = filemap_get_folio(swap_address_space(entry),
+                                       swap_cache_index(entry));
+               if (!IS_ERR_OR_NULL(tmp_folio)) {
+                       if (!IS_ERR_OR_NULL(folio) && tmp_folio != folio) {
+                               pr_err("UFFDIO_MOVE: UNSTABLE folio
 %lx (%lx) -> %lx (%lx)\n", folio, folio->swap.val, tmp_folio,
 tmp_folio->swap.val);
+                       }
+                       folio_put(tmp_folio);
+               }

                if (!IS_ERR_OR_NULL(folio)) {
                        if (folio_test_large(folio)) {
                                err = -EBUSY;

(quoted diff) +2 -0

@@ -1413,6 +1427,8 @@ static int move_pages_pte(struct mm_struct *mm,
 pmd_t *dst_pmd, pmd_t *src_pmd,
                err = move_swap_pte(mm, dst_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);
+               if (tmp_folio != folio && !err)
+                       pr_err("UFFDIO_MOVE: UNSTABLE folio passed
 check: %lx -> %lx\n", folio, tmp_folio);

        }

And I saw these two prints are getting triggered like this (not a real issue though, just help to understand the problem)
...

[ 3127.632791] UFFDIO_MOVE: UNSTABLE folio fffffdffc334cd00 (0) ->
fffffdffc7ccac80 (51)
[ 3172.033269] UFFDIO_MOVE: UNSTABLE folio fffffdffc343bb40 (0) ->
fffffdffc3435e00 (3b)
[ 3194.425213] UFFDIO_MOVE: UNSTABLE folio fffffdffc7d481c0 (0) ->
fffffdffc34ab8c0 (76)
[ 3194.991318] UFFDIO_MOVE: UNSTABLE folio fffffdffc34f95c0 (0) ->
fffffdffc34ab8c0 (6d)
[ 3203.467212] UFFDIO_MOVE: UNSTABLE folio fffffdffc34b13c0 (0) ->
fffffdffc34eda80 (32)
[ 3206.217820] UFFDIO_MOVE: UNSTABLE folio fffffdffc7d297c0 (0) ->
fffffdffc38cedc0 (b)
[ 3214.913039] UFFDIO_MOVE: UNSTABLE folio passed check: 0 -> fffffdffc34db140
[ 3217.066972] UFFDIO_MOVE: UNSTABLE folio fffffdffc342b5c0 (0) ->

fffffdffc3465cc0 (21)
...

The "UFFDIO_MOVE: UNSTABLE folio fffffdffc3435180 (0) -> fffffdffc3853540 (53)" worries me at first. On first look it seems the folio is indeed freed completely from the swap cache after the first lookup, so another swapout can reuse the entry. But as you mentioned __remove_mapping won't release a folio if the refcount check fails, so they must be freed by folio_free_swap or __try_to_reclaim_swap, there are many places that can happen. But these two helpers won't free a folio from swap cache if its swap count is not zero. And the folio will either be swapped out (swap count non zero), or mapped (freeing it is fine, PTE is non_swap, and another swapout will still use the same folio).

So after more investigation and dumping the pages, it's actually the second lookup (tmp_folio) seeing the entry being reused by another page table entry, after the first folio is swapped back and released. So the page table check below will always fail just fine.

But this also proves the first look up can see a completely irrelevant folio too: If the src folio is swapped out, but got swapped back and freed, then another folio B shortly got added to swap cache reuse the src folio's old swap entry, then the folio B got seen by the look up here and get freed from swap cache, then src folio got swapped out again also reusing the same entry, then we have a problem as PTE seems untouched indeed but we grabbed a wrong folio. Seems possible if I'm not wrong:

Quoted 7 lines

Something like this:
CPU1 CPU2
move_pages_pte
entry = pte_to_swp_entry(orig_src_pte);
| Got Swap Entry S1 from src_pte
...
<swapin src_pte, using folio A>

I’m assuming you mean <swapin src_pte, using folio B>, since I’m not sure where folio B comes from in the statement <someone else tried to swap out folio B>.

Quoted 6 lines

If that assumption is correct, and folio A is still in the swapcache, how could someone swap in folio B without hitting folio A? That would suggest folio A must have been removed from the swapcache earlier—right?

Quoted 2 lines

<free folio A from swap cache freeing S1> <someone else try swap out folio B >

Sorry my bad, I think I made people think folio B is related to src_pte at this point. What I actually mean is that: Another random folio B, unrelated to src_pte, could got swapped out, and using the swap entry S1.

Quoted 32 lines

Quoted 32 lines

<put folio B to swap cache using S1 >
...
folio = swap_cache_get_folio(S1)
| Got folio B here !!!
move_swap_pte
<free folio B from swap cache>
| Holding a reference doesn't pin the cache | as we have demonstrated
<Swapout folio A also using S1>
double_pt_lock
is_pte_pages_stable
| Passed because of S1 is reused
folio_move_anon_rmap(...)
| Moved invalid folio B here !!!

But this is extremely hard to reproduce though, even if doing it deliberately...

So I think a "folio_swap_contains" or equivalent check here is a good thing to have, to make it more robust and easier to understand. The checking after locking a folio has very tiny overhead and can definitely ensure the folio's swap entry is valid and stable.

The "UFFDIO_MOVE: UNSTABLE folio passed check: 0 -> fffffdffc385fb00" here might seem problematic, but it's still not a real problem. That's the case where the swapin in src region happens after the lookup, and before the PTE lock. It will pass the PTE check without moving the folio. But the folio is guaranteed to be a completely new folio here because the folio can't be added back to the page table without holding the PTE lock, and if that happens the following PTE check here will fail.

Quoted 6 lines

Quoted 3 lines

So I think we should patch the current kernel only adding a "folio_swap_contains" equivalent check here, and maybe more comments, how do you think?

The description appears to have some inconsistencies. Would you mind rephrasing it?

Yeah, let's ignore the "UFFDIO_MOVE: UNSTABLE folio passed check: 0 -> fffffdffc385fb00" part first, as both you and me have come into a conclusion that "filemap_get_folio() returns NULL before move_swap_pte, but a folio was added to swap cache" is OK, and this output only proves that happens.

So the problematic race is:

Here move_pages_pte is moving src_pte to dst_pte, and it begins with src_pte == swap entry S1, and S1 isn't cached.

CPU1 CPU2
move_pages_pte()

  entry = pte_to_swp_entry(orig_src_pte);
    | src_pte is absent, and got entry == S1
  ... < Somehow interrupted> ...
                                 <swapin src_pte, using folio A>
                                   | folio A is just a new allocated folio
                                   | for resolving the swap in fault.
                                 <free folio A from swap cache freeing S1>
                                   | swap in fault is resolved, src_pte
                                   | now points to folio A, so folio A
                                   | can get freed just fine.
                                   | And now S1 is free to be used
                                   | by anyone.
                                 <someone else try swap out another folio B >
                                   | Folio B is a completely unrelated
                                   | folio swapped out by random process.
                                   | (has nothing to do with src_pte)
                                   | But S1 is freed so it may use S1
                                   | as its swap entry.
                                 <put folio B to swap cache with index S1 >
                                 ...
  folio = filemap_get_folio(S1)
    | The lookup is using S1, so it
    | got folio B here !!!
  ... < Somehow interrupted> ...
                                 <free folio B from swap cache>
                                   | Folio B could fail to be swapped out
                                   | or got swapped in again, so it can
                                   | be freed by folio_free_swap or
                                   | swap cache reclaim.
                                   | CPU1 is holding a reference but it
                                   | doesn't pin the swap cache folio
                                   | as I have demonstrated with the
                                   | test C program previously.
                                   | New S1 is free to be used again.
                                 <Swapout src_pte again using S1>
                                   | No thing blocks this from happening
                                   | The swapout is still using folio A,
                                   | and src_pte == S1.
  folio_trylock(folio)
  move_swap_pte
    double_pt_lock
    is_pte_pages_stable
      | Passed because of S1 is reused so src_pte == S1.
    folio_move_anon_rmap(...)
      | Moved invalid folio B here !!!

It's a long and complex one, I don't think it's practically possible to happen in reality but in theory doable, once in a million maybe... Still we have to fix it, or did I got anything wrong here?

Quoted 244 lines · Kairui Song

Quoted 132 lines · Barry Song

Quoted 118 lines · Kairui Song

Quoted 4 lines · Barry Song

Quoted 2 lines · Kairui Song

Barry Song <[email protected]> 于 2025年5月21日周三 06:33写道：

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Let me run test case [1] to check whether this ever happens. I guess I need to hack kernel a bit to always add folio to swapcache even for SYNC IO.

That will cause quite a performance regression I think. Good thing is, that's exactly the problem this series is solving by dropping the SYNC IO swapin path and never bypassing the swap cache, while improving the performance, eliminating things like this. One more reason to justify the approach :)

Hi Barry,

Quoted 8 lines

I attempted to reproduce the scenario where a folio is added to the swapcache after filemap_get_folio() returns NULL but before move_swap_pte() moves the swap PTE
using non-synchronized I/O. Technically, this seems possible; however, I was unable
to reproduce it, likely because the time window between swapin_readahead and taking the page table lock within do_swap_page() is too short.

Thank you so much for trying this!

I have been trying to reproduce it too, and so far I didn't observe any crash or warn. I added following debug code:

static __always_inline
bool validate_dst_vma(struct vm_area_struct *dst_vma, unsigned long dst_end)

(quoted diff) +1 -0

@@ -1163,6 +1167,7 @@ static int move_pages_pte(struct mm_struct *mm,
 pmd_t *dst_pmd, pmd_t *src_pmd,
        pmd_t dummy_pmdval;
        pmd_t dst_pmdval;
        struct folio *src_folio = NULL;
+       struct folio *tmp_folio = NULL;
        struct anon_vma *src_anon_vma = NULL;
        struct mmu_notifier_range range;

        int err = 0;

(quoted diff) +9 -0

@@ -1391,6 +1396,15 @@ static int move_pages_pte(struct mm_struct *mm,
 pmd_t *dst_pmd, pmd_t *src_pmd,
                if (!src_folio)
                        folio = filemap_get_folio(swap_address_space(entry),
                                        swap_cache_index(entry));
+               udelay(get_random_u32_below(1000));
+               tmp_folio = filemap_get_folio(swap_address_space(entry),
+                                       swap_cache_index(entry));
+               if (!IS_ERR_OR_NULL(tmp_folio)) {
+                       if (!IS_ERR_OR_NULL(folio) && tmp_folio != folio) {
+                               pr_err("UFFDIO_MOVE: UNSTABLE folio
 %lx (%lx) -> %lx (%lx)\n", folio, folio->swap.val, tmp_folio,
 tmp_folio->swap.val);
+                       }
+                       folio_put(tmp_folio);
+               }

                if (!IS_ERR_OR_NULL(folio)) {
                        if (folio_test_large(folio)) {
                                err = -EBUSY;

(quoted diff) +2 -0

@@ -1413,6 +1427,8 @@ static int move_pages_pte(struct mm_struct *mm,
 pmd_t *dst_pmd, pmd_t *src_pmd,
                err = move_swap_pte(mm, dst_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);
+               if (tmp_folio != folio && !err)
+                       pr_err("UFFDIO_MOVE: UNSTABLE folio passed
 check: %lx -> %lx\n", folio, tmp_folio);

        }

And I saw these two prints are getting triggered like this (not a real issue though, just help to understand the problem)
...

[ 3127.632791] UFFDIO_MOVE: UNSTABLE folio fffffdffc334cd00 (0) ->
fffffdffc7ccac80 (51)
[ 3172.033269] UFFDIO_MOVE: UNSTABLE folio fffffdffc343bb40 (0) ->
fffffdffc3435e00 (3b)
[ 3194.425213] UFFDIO_MOVE: UNSTABLE folio fffffdffc7d481c0 (0) ->
fffffdffc34ab8c0 (76)
[ 3194.991318] UFFDIO_MOVE: UNSTABLE folio fffffdffc34f95c0 (0) ->
fffffdffc34ab8c0 (6d)
[ 3203.467212] UFFDIO_MOVE: UNSTABLE folio fffffdffc34b13c0 (0) ->
fffffdffc34eda80 (32)
[ 3206.217820] UFFDIO_MOVE: UNSTABLE folio fffffdffc7d297c0 (0) ->
fffffdffc38cedc0 (b)
[ 3214.913039] UFFDIO_MOVE: UNSTABLE folio passed check: 0 -> fffffdffc34db140
[ 3217.066972] UFFDIO_MOVE: UNSTABLE folio fffffdffc342b5c0 (0) ->

fffffdffc3465cc0 (21)
...

The "UFFDIO_MOVE: UNSTABLE folio fffffdffc3435180 (0) -> fffffdffc3853540 (53)" worries me at first. On first look it seems the folio is indeed freed completely from the swap cache after the first lookup, so another swapout can reuse the entry. But as you mentioned __remove_mapping won't release a folio if the refcount check fails, so they must be freed by folio_free_swap or __try_to_reclaim_swap, there are many places that can happen. But these two helpers won't free a folio from swap cache if its swap count is not zero. And the folio will either be swapped out (swap count non zero), or mapped (freeing it is fine, PTE is non_swap, and another swapout will still use the same folio).

So after more investigation and dumping the pages, it's actually the second lookup (tmp_folio) seeing the entry being reused by another page table entry, after the first folio is swapped back and released. So the page table check below will always fail just fine.

But this also proves the first look up can see a completely irrelevant folio too: If the src folio is swapped out, but got swapped back and freed, then another folio B shortly got added to swap cache reuse the src folio's old swap entry, then the folio B got seen by the look up here and get freed from swap cache, then src folio got swapped out again also reusing the same entry, then we have a problem as PTE seems untouched indeed but we grabbed a wrong folio. Seems possible if I'm not wrong:

Quoted 7 lines

Something like this:
CPU1 CPU2
move_pages_pte
entry = pte_to_swp_entry(orig_src_pte);
| Got Swap Entry S1 from src_pte
...
<swapin src_pte, using folio A>

I’m assuming you mean <swapin src_pte, using folio B>, since I’m not sure where folio B comes from in the statement <someone else tried to swap out folio B>.

Quoted 6 lines

If that assumption is correct, and folio A is still in the swapcache, how could someone swap in folio B without hitting folio A? That would suggest folio A must have been removed from the swapcache earlier—right?

Quoted 2 lines

<free folio A from swap cache freeing S1> <someone else try swap out folio B >

Sorry my bad, I think I made people think folio B is related to src_pte at this point. What I actually mean is that: Another random folio B, unrelated to src_pte, could got swapped out, and using the swap entry S1.

Quoted 32 lines

Quoted 32 lines

<put folio B to swap cache using S1 >
...
folio = swap_cache_get_folio(S1)
| Got folio B here !!!
move_swap_pte
<free folio B from swap cache>
| Holding a reference doesn't pin the cache | as we have demonstrated
<Swapout folio A also using S1>
double_pt_lock
is_pte_pages_stable
| Passed because of S1 is reused
folio_move_anon_rmap(...)
| Moved invalid folio B here !!!

But this is extremely hard to reproduce though, even if doing it deliberately...

So I think a "folio_swap_contains" or equivalent check here is a good thing to have, to make it more robust and easier to understand. The checking after locking a folio has very tiny overhead and can definitely ensure the folio's swap entry is valid and stable.

The "UFFDIO_MOVE: UNSTABLE folio passed check: 0 -> fffffdffc385fb00" here might seem problematic, but it's still not a real problem. That's the case where the swapin in src region happens after the lookup, and before the PTE lock. It will pass the PTE check without moving the folio. But the folio is guaranteed to be a completely new folio here because the folio can't be added back to the page table without holding the PTE lock, and if that happens the following PTE check here will fail.

Quoted 6 lines

Quoted 3 lines

So I think we should patch the current kernel only adding a "folio_swap_contains" equivalent check here, and maybe more comments, how do you think?

The description appears to have some inconsistencies. Would you mind rephrasing it?

Yeah, let's ignore the "UFFDIO_MOVE: UNSTABLE folio passed check: 0 -> fffffdffc385fb00" part first, as both you and me have come into a conclusion that "filemap_get_folio() returns NULL before move_swap_pte, but a folio was added to swap cache" is OK, and this output only proves that happens.

So the problematic race is:

Here move_pages_pte is moving src_pte to dst_pte, and it begins with src_pte == swap entry S1, and S1 isn't cached.

CPU1 CPU2
move_pages_pte()

  entry = pte_to_swp_entry(orig_src_pte);
    | src_pte is absent, and got entry == S1
  ... < Somehow interrupted> ...
                                 <swapin src_pte, using folio A>
                                   | folio A is just a new allocated folio
                                   | for resolving the swap in fault.
                                 <free folio A from swap cache freeing S1>
                                   | swap in fault is resolved, src_pte
                                   | now points to folio A, so folio A
                                   | can get freed just fine.
                                   | And now S1 is free to be used
                                   | by anyone.
                                 <someone else try swap out another folio B >
                                   | Folio B is a completely unrelated
                                   | folio swapped out by random process.
                                   | (has nothing to do with src_pte)
                                   | But S1 is freed so it may use S1
                                   | as its swap entry.
                                 <put folio B to swap cache with index S1 >
                                 ...
  folio = filemap_get_folio(S1)
    | The lookup is using S1, so it
    | got folio B here !!!
  ... < Somehow interrupted> ...
                                 <free folio B from swap cache>
                                   | Folio B could fail to be swapped out
                                   | or got swapped in again, so it can
                                   | be freed by folio_free_swap or
                                   | swap cache reclaim.
                                   | CPU1 is holding a reference but it
                                   | doesn't pin the swap cache folio
                                   | as I have demonstrated with the
                                   | test C program previously.
                                   | New S1 is free to be used again.
                                 <Swapout src_pte again using S1>
                                   | No thing blocks this from happening
                                   | The swapout is still using folio A,
                                   | and src_pte == S1.
  folio_trylock(folio)
  move_swap_pte
    double_pt_lock
    is_pte_pages_stable
      | Passed because of S1 is reused so src_pte == S1.
    folio_move_anon_rmap(...)
      | Moved invalid folio B here !!!

Quoted 3 lines

It's a long and complex one, I don't think it's practically possible to happen in reality but in theory doable, once in a million maybe... Still we have to fix it, or did I got anything wrong here?

Hi Barry,

I managed to reproduce this issue, by hacking the kernel a bit (Only adding only delay to increase the race window, and adding a WARN to indicate the problem)

1. Applying following patch for kernel:
===

mm/userfaultfd.c +5 -0

index bc473ad21202..1d710adf9839 100644
@@ -15,6 +15,7 @@
 #include <linux/mmu_notifier.h>
 #include <linux/hugetlb.h>
 #include <linux/shmem_fs.h>
+#include <linux/delay.h>
 #include <asm/tlbflush.h>
 #include <asm/tlb.h>
 #include "internal.h"

@@ -1100,6 +1101,10 @@ static int move_swap_pte(struct mm_struct *mm,
 struct vm_area_struct *dst_vma,
         * occur and hit the swapcache after moving the PTE.
         */
        if (src_folio) {
+               if (WARN_ON(src_folio->swap.val !=
 pte_to_swp_entry(orig_src_pte).val))
+                       pr_err("Moving folio %lx (folio->swap = %lx),
 orig_src_pte = %lx\n",
+                              (unsigned long)src_folio, src_folio->swap.val,
+                              pte_to_swp_entry(orig_src_pte).val);

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

mm/userfaultfd.c +4 -0

@@ -1388,9 +1393,13 @@ static int move_pages_pte(struct mm_struct *mm,
 pmd_t *dst_pmd, pmd_t *src_pmd,
                 * folios in the swapcache. This issue needs to be resolved
                 * separately to allow proper handling.
                 */
+               pr_err("DEBUG: Will do the lookup using entry %lx,
 wait 3s...\n", entry.val);
+               mdelay(1000 * 3);
                if (!src_folio)
                        folio = filemap_get_folio(swap_address_space(entry),
                                        swap_cache_index(entry));
+               pr_err("DEBUG: Got folio value %lx, wait 3s...\n",
 (unsigned long)folio);
+               mdelay(1000 * 3);

                if (!IS_ERR_OR_NULL(folio)) {
                        if (folio_test_large(folio)) {
                                err = -EBUSY;

2. Save following program in userspace (didn't bother with error check for simpler code):
===
#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
#include <sys/ioctl.h>
#include <sys/syscall.h>
#include <linux/userfaultfd.h>
#include <fcntl.h>
#include <pthread.h>
#include <unistd.h>
#include <poll.h>
#include <errno.h>

#define PAGE_SIZE 4096
/* Need to consume all slots so define the swap device size here */ #define SWAP_DEVICE_SIZE (PAGE_SIZE * 1024 - 1)

char *src, *race, *dst, *place_holder;
int uffd;

void read_in(char *p) {
    /* This test program initials memory with 0xAA to bypass zeromap */
    while (*((volatile char*)p) != 0xAA);
}

void *reader_thread(void *arg) {
    /* Test requires kernel to wait upon uffd move */
    read_in(dst);
    return NULL;
}

void *fault_handler_thread(void *arg) {
    int ret;
    struct uffd_msg msg;
    struct uffdio_move move;
    struct pollfd pollfd = { .fd = uffd, .events = POLLIN };
    pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL);
    pthread_setcanceltype(PTHREAD_CANCEL_DEFERRED, NULL);

    while (1) {
        poll(&pollfd, 1, -1);
        read(uffd, &msg, sizeof(msg));

        move.src = (unsigned long)src + (msg.arg.pagefault.address -
            (unsigned long)dst);
        move.dst = msg.arg.pagefault.address & ~(PAGE_SIZE - 1);
        move.len = PAGE_SIZE;
        move.mode = 0;

        ioctl(uffd, UFFDIO_MOVE, &move);
    }
    return NULL;
}

int main() {
    pthread_t fault_handler_thr, reader_thr;
    struct uffdio_api uffdio_api = { .api = UFFD_API, .features = 0 };
    struct uffdio_register uffdio_register;

    src = mmap(NULL, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE |
MAP_ANONYMOUS, -1, 0);
    dst = mmap(NULL, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE |
MAP_ANONYMOUS, -1, 0);
    memset(src, 0xAA, PAGE_SIZE);
    madvise(src, PAGE_SIZE, MADV_PAGEOUT);

    /* Consume all slots on swap device left only one entry (S1) */
    place_holder = mmap(NULL, SWAP_DEVICE_SIZE - 1, PROT_READ |
PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
    memset(place_holder, 0xAA, SWAP_DEVICE_SIZE - 1);
    madvise(place_holder, SWAP_DEVICE_SIZE - 1, MADV_PAGEOUT);

    /* Setup uffd handler and dst reader */
    uffd = syscall(SYS_userfaultfd, O_CLOEXEC | O_NONBLOCK);
    ioctl(uffd, UFFDIO_API, &uffdio_api);
    uffdio_register.range.start = (unsigned long)dst;
    uffdio_register.range.len = PAGE_SIZE;
    uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
    ioctl(uffd, UFFDIO_REGISTER, &uffdio_register);
    pthread_create(&fault_handler_thr, NULL, fault_handler_thread, NULL);
    pthread_create(&reader_thr, NULL, reader_thread, NULL);

    /* Wait for UFFDIO to start */
    sleep(1);

    /* Release src folio (A) from swap, freeing the entry S1 */
    read_in(src);

    /* Swapout another race folio (B) using S1 */
    race = mmap(NULL, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED |
MAP_ANONYMOUS, -1, 0);
    memset(race, 0xAA, PAGE_SIZE);
    madvise(race, PAGE_SIZE, MADV_PAGEOUT);

    /* Wait for UFFDIO swap lookup to see the race folio (B) */
    sleep(3);

    /* Free the race folio (B) from swap */
    read_in(race);
    /* And swap out src folio (A) again, using S1 */
    madvise(src, PAGE_SIZE, MADV_PAGEOUT);

    /* Kernel should have moved a wrong folio by now */

    pthread_join(reader_thr, NULL);
    pthread_cancel(fault_handler_thr);
    pthread_join(fault_handler_thr, NULL);
    munmap(race, PAGE_SIZE);
    munmap(src, PAGE_SIZE);
    munmap(dst, PAGE_SIZE);
    close(uffd);

    return 0;
}

3. Run the test with (ensure no other swap device is mounted and current dir is on a block device):
===
dd if=/dev/zero of=swap.img bs=1M count=1; mkswap swap.img; swapon swap.img; gcc test-uffd.c && ./a.out

Then we get the WARN:

[  348.200587] ------------[ cut here ]------------
[  348.200599] WARNING: CPU: 7 PID: 1856 at mm/userfaultfd.c:1104
move_pages_pte+0xdb8/0x11a0
[  348.207544] Modules linked in: loop
[  348.209401] CPU: 7 UID: 0 PID: 1856 Comm: a.out Kdump: loaded Not
tainted 6.15.0-rc6ptch-00381-g99f00d7c6c6f-dirty #304
PREEMPT(voluntary)
[  348.214579] Hardware name: QEMU QEMU Virtual Machine, BIOS
edk2-stable202408-prebuilt.qemu.org 08/13/2024
[  348.218656] pstate: 81400005 (Nzcv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
[  348.222013] pc : move_pages_pte+0xdb8/0x11a0
[  348.224062] lr : move_pages_pte+0x928/0x11a0
[  348.225881] sp : ffff800088b2b8f0
[  348.227360] x29: ffff800088b2b970 x28: 0000000000000000 x27: 0000ffffbc920000
[  348.230228] x26: fffffdffc335e4a8 x25: 0000000000000001 x24: fffffdffc3e4dd40
[  348.233159] x23: 080000010d792403 x22: ffff0000cd792900 x21: ffff0000c5a6d2c0
[  348.236339] x20: fffffdffc335e4a8 x19: 0000000000001004 x18: 0000000000000006
[  348.239269] x17: 0000ffffbc920000 x16: 0000ffffbc922fff x15: 0000000000000003
[  348.242703] x14: ffff8000812c3b68 x13: 0000000000000003 x12: 0000000000000003
[  348.245947] x11: 0000000000000000 x10: ffff800081e4feb8 x9 : 0000000000000001
[  348.249284] x8 : 0000000000000000 x7 : 6f696c6f6620746f x6 : 47203a4755424544
[  348.252071] x5 : ffff8000815789e3 x4 : ffff8000815789e5 x3 : 0000000000000000
[  348.255358] x2 : ffff0001fed2aef0 x1 : 0000000000000000 x0 : fffffdffc335e4a8
[  348.258134] Call trace:
[  348.259468]  move_pages_pte+0xdb8/0x11a0 (P)
[  348.261348]  move_pages+0x3c0/0x738
[  348.262987]  userfaultfd_ioctl+0x3d8/0x1f98
[  348.264916]  __arm64_sys_ioctl+0x88/0xd0
[  348.266779]  invoke_syscall+0x64/0xec
[  348.268347]  el0_svc_common+0xa8/0xd8
[  348.269967]  do_el0_svc+0x1c/0x28
[  348.271711]  el0_svc+0x40/0xe0
[  348.273345]  el0t_64_sync_handler+0x78/0x108
[  348.274821]  el0t_64_sync+0x19c/0x1a0
[  348.276117] ---[ end trace 0000000000000000 ]---
[  348.278638] Moving folio fffffdffc3e4dd40 (folio->swap = 0), orig_src_pte = 1

That's the new added WARN, but the test program also hung with D forever, and with errors with other tests like:

[  406.893936] BUG: Bad rss-counter state mm:ffff0000c5a9ddc0
type:MM_ANONPAGES val:-1
[  406.894071] BUG: Bad rss-counter state mm:ffff0000c5a9ddc0

type:MM_SHMEMPAGES val:1

Because the kernel just moved the wrong folio, so unmap takes forever looking for the missing folio, and counting went wrong too.

So this race is real. It's extremely unlikely to happen because it requires multiple collisions of multiple tiny race windows, however it's not impossible.

I'll post a fix very soon.

mm/swapfile.c static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int o · line 893

Quoted 9 lines

From: Kairui Song <[email protected]>

mm/swapfile.c +0 -2

@@ -889,10 +849,8 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int o
              /* Serialize HDD SWAP allocation for each device. */
              spin_lock(&si->global_cluster_lock);
              offset = si->global_cluster->next[order];
-             if (offset == SWAP_ENTRY_INVALID)
-                     goto new_cluster;

We are implicitly dropping this. Does it mean the current code is wrong? Do we need some clarification about this?

mm/swapfile.c static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int o · line 856

Quoted 7 lines

mm/swapfile.c +1 -1

-             ci = lock_cluster(si, offset);
+             ci = swap_lock_cluster(si, offset);
              /* Cluster could have been used by another order */
              if (cluster_is_usable(ci, order)) {
                      if (cluster_is_empty(ci))

Thanks
Barry

From: Kairui Song <[email protected]>

Move all mTHP swapin check into can_swapin_thp and use it for both pre IO check and post IO check. This way the code is more consolidated and make later commit easier to maintain.

Also clean up the comments while at it. The current comment of non_swapcache_batch is not correct: swap in bypassing swap cache won't reach the swap device as long as the entry is cached, because it still sets the SWAP_HAS_CACHE flag. If the folio is already in swap cache, raced swap in will either fail due to -EEXIST with swapcache_prepare, or see the cached folio.

The real reason this non_swapcache_batch is needed is that if a smaller folio is in the swap cache but not mapped, mTHP swapin will be blocked forever as it won't see the folio due to index offset, nor it can set the SWAP_HAS_CACHE bit, so it has to fallback to order 0 swap in.

Signed-off-by Kairui Song <[email protected]>

---
 mm/memory.c | 90 ++++++++++++++++++++++++-----------------------------
 1 file changed, 41 insertions(+), 49 deletions(-)

mm/memory.c +39 -41

index f2897d9059f2..1b6e192de6ec 100644
@@ -4319,12 +4319,6 @@ static inline int non_swapcache_batch(swp_entry_t entry, int max_nr)
 	pgoff_t offset = swp_offset(entry);
 	int i;
 
-	/*
-	 * While allocating a large folio and doing swap_read_folio, which is
-	 * the case the being faulted pte doesn't have swapcache. We need to
-	 * ensure all PTEs have no cache as well, otherwise, we might go to
-	 * swap devices while the content is in swapcache.
-	 */
 	for (i = 0; i < max_nr; i++) {
 		if ((si->swap_map[offset + i] & SWAP_HAS_CACHE))
 			return i;

@@ -4334,34 +4328,30 @@ static inline int non_swapcache_batch(swp_entry_t entry, int max_nr)
 }
 
 /*
- * Check if the PTEs within a range are contiguous swap entries
- * and have consistent swapcache, zeromap.
+ * Check if the page table is still suitable for large folio swap in.
+ * @vmf: The fault triggering the swap-in.
+ * @ptep: Pointer to the PTE that should be the head of the swap in folio.
+ * @addr: The address corresponding to the PTE.
+ * @nr_pages: Number of pages of the folio that suppose to be swapped in.
  */
-static bool can_swapin_thp(struct vm_fault *vmf, pte_t *ptep, int nr_pages)
+static bool can_swapin_thp(struct vm_fault *vmf, pte_t *ptep,
+			   unsigned long addr, unsigned int nr_pages)
 {
-	unsigned long addr;
-	swp_entry_t entry;
-	int idx;
-	pte_t pte;
-
-	addr = ALIGN_DOWN(vmf->address, nr_pages * PAGE_SIZE);
-	idx = (vmf->address - addr) / PAGE_SIZE;
-	pte = ptep_get(ptep);
+	pte_t pte = ptep_get(ptep);
+	unsigned long addr_end = addr + (PAGE_SIZE * nr_pages);
+	unsigned long pte_offset = (vmf->address - addr) / PAGE_SIZE;
 
-	if (!pte_same(pte, pte_move_swp_offset(vmf->orig_pte, -idx)))
+	VM_WARN_ON_ONCE(!IS_ALIGNED(addr, PAGE_SIZE) ||
+			addr > vmf->address || addr_end <= vmf->address);
+	if (unlikely(addr < max(addr & PMD_MASK, vmf->vma->vm_start) ||
+		     addr_end > pmd_addr_end(addr, vmf->vma->vm_end)))
 		return false;
-	entry = pte_to_swp_entry(pte);
-	if (swap_pte_batch(ptep, nr_pages, pte) != nr_pages)
-		return false;
-
 	/*
-	 * swap_read_folio() can't handle the case a large folio is hybridly
-	 * from different backends. And they are likely corner cases. Similar
-	 * things might be added once zswap support large folios.
+	 * All swap entries must from the same swap device, in same
+	 * cgroup, with same exclusiveness, only differs in offset.
 	 */
-	if (unlikely(swap_zeromap_batch(entry, nr_pages, NULL) != nr_pages))
-		return false;
-	if (unlikely(non_swapcache_batch(entry, nr_pages) != nr_pages))
+	if (!pte_same(pte, pte_move_swp_offset(vmf->orig_pte, -pte_offset)) ||
+	    swap_pte_batch(ptep, nr_pages, pte) != nr_pages)
 		return false;
 
 	return true;

@@ -4441,13 +4431,24 @@ static struct folio *alloc_swap_folio(struct vm_fault *vmf)
 	 * completely swap entries with contiguous swap offsets.
 	 */
 	order = highest_order(orders);
-	while (orders) {
-		addr = ALIGN_DOWN(vmf->address, PAGE_SIZE << order);
-		if (can_swapin_thp(vmf, pte + pte_index(addr), 1 << order))
-			break;
-		order = next_order(&orders, order);
+	for (; orders; order = next_order(&orders, order)) {
+		unsigned long nr_pages = 1 << order;
+		swp_entry_t swap_entry = { .val = ALIGN_DOWN(entry.val, nr_pages) };
+		addr = ALIGN_DOWN(vmf->address, nr_pages * PAGE_SIZE);
+		if (!can_swapin_thp(vmf, pte + pte_index(addr), addr, nr_pages))
+			continue;
+		/*
+		 * If there is already a smaller folio in cache, it will
+		 * conflict with the larger folio in the swap cache layer
+		 * and block the swap in.
+		 */
+		if (unlikely(non_swapcache_batch(swap_entry, nr_pages) != nr_pages))
+			continue;
+		/* Zero map doesn't work with large folio yet. */
+		if (unlikely(swap_zeromap_batch(swap_entry, nr_pages, NULL) != nr_pages))
+			continue;
+		break;
 	}
-
 	pte_unmap_unlock(pte, ptl);
 
 	/* Try allocating the highest of the remaining orders. */

@@ -4731,27 +4732,18 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 	page_idx = 0;
 	address = vmf->address;
 	ptep = vmf->pte;
+
 	if (folio_test_large(folio) && folio_test_swapcache(folio)) {
-		int nr = folio_nr_pages(folio);
+		unsigned long nr = folio_nr_pages(folio);
 		unsigned long idx = folio_page_idx(folio, page);
-		unsigned long folio_start = address - idx * PAGE_SIZE;
-		unsigned long folio_end = folio_start + nr * PAGE_SIZE;
-		pte_t *folio_ptep;
-		pte_t folio_pte;
+		unsigned long folio_address = address - idx * PAGE_SIZE;
+		pte_t *folio_ptep = vmf->pte - idx;
 
-		if (unlikely(folio_start < max(address & PMD_MASK, vma->vm_start)))
-			goto check_folio;