* [PATCH v2] mm: userfaultfd: fix race of userfaultfd_move and swap cache
@ 2025-06-01 20:01 Kairui Song
2025-06-01 23:02 ` Barry Song
2025-06-02 1:43 ` Lokesh Gidra
0 siblings, 2 replies; 4+ messages in thread
From: Kairui Song @ 2025-06-01 20:01 UTC (permalink / raw)
To: linux-mm
Cc: Andrew Morton, Barry Song, Peter Xu, Suren Baghdasaryan,
Andrea Arcangeli, David Hildenbrand, Lokesh Gidra, stable,
linux-kernel, Kairui Song
From: Kairui Song <kasong@tencent.com>
On seeing a swap entry PTE, userfaultfd_move does a lockless swap cache
lookup, and try to move the found folio to the faulting vma when.
Currently, it relies on the PTE value check to ensure the moved folio
still belongs to the src swap entry, which turns out is not reliable.
While working and reviewing the swap table series with Barry, following
existing race is observed and reproduced [1]:
( move_pages_pte is moving src_pte to dst_pte, where src_pte is a
swap entry PTE holding swap entry S1, and S1 isn't in the swap cache.)
CPU1 CPU2
userfaultfd_move
move_pages_pte()
entry = pte_to_swp_entry(orig_src_pte);
// Here it got entry = S1
... < Somehow interrupted> ...
<swapin src_pte, alloc and use folio A>
// folio A is just a new allocated folio
// and get installed into src_pte
<frees swap entry S1>
// src_pte now points to folio A, S1
// has swap count == 0, it can be freed
// by folio_swap_swap or swap
// allocator's reclaim.
<try to swap out another folio B>
// folio B is a folio in another VMA.
<put folio B to swap cache using S1 >
// S1 is freed, folio B could use it
// for swap out with no problem.
...
folio = filemap_get_folio(S1)
// Got folio B here !!!
... < Somehow interrupted again> ...
<swapin folio B and free S1>
// Now S1 is free to be used again.
<swapout src_pte & folio A using S1>
// Now src_pte is a swap entry pte
// holding S1 again.
folio_trylock(folio)
move_swap_pte
double_pt_lock
is_pte_pages_stable
// Check passed because src_pte == S1
folio_move_anon_rmap(...)
// Moved invalid folio B here !!!
The race window is very short and requires multiple collisions of
multiple rare events, so it's very unlikely to happen, but with a
deliberately constructed reproducer and increased time window, it can be
reproduced [1].
It's also possible that folio (A) is swapped in, and swapped out again
after the filemap_get_folio lookup, in such case folio (A) may stay in
swap cache so it needs to be moved too. In this case we should also try
again so kernel won't miss a folio move.
Fix this by checking if the folio is the valid swap cache folio after
acquiring the folio lock, and checking the swap cache again after
acquiring the src_pte lock.
SWP_SYNCRHONIZE_IO path does make the problem more complex, but so far
we don't need to worry about that since folios only might get exposed to
swap cache in the swap out path, and it's covered in this patch too by
checking the swap cache again after acquiring src_pte lock.
Testing with a simple C program to allocate and move several GB of memory
did not show any observable performance change.
Cc: <stable@vger.kernel.org>
Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
Closes: https://lore.kernel.org/linux-mm/CAMgjq7B1K=6OOrK2OUZ0-tqCzi+EJt+2_K97TPGoSt=9+JwP7Q@mail.gmail.com/ [1]
Signed-off-by: Kairui Song <kasong@tencent.com>
---
V1: https://lore.kernel.org/linux-mm/20250530201710.81365-1-ryncsn@gmail.com/
Changes:
- Check swap_map instead of doing a filemap lookup after acquiring the
PTE lock to minimize critical section overhead [ Barry Song, Lokesh Gidra ]
mm/userfaultfd.c | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index bc473ad21202..a74ede04996c 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -1084,8 +1084,11 @@ static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
pte_t orig_dst_pte, pte_t orig_src_pte,
pmd_t *dst_pmd, pmd_t dst_pmdval,
spinlock_t *dst_ptl, spinlock_t *src_ptl,
- struct folio *src_folio)
+ struct folio *src_folio,
+ struct swap_info_struct *si)
{
+ swp_entry_t entry;
+
double_pt_lock(dst_ptl, src_ptl);
if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
@@ -1102,6 +1105,16 @@ 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 {
+ /*
+ * Check if the swap entry is cached after acquiring the src_pte
+ * lock. Or we might miss a new loaded swap cache folio.
+ */
+ entry = pte_to_swp_entry(orig_src_pte);
+ if (si->swap_map[swp_offset(entry)] & SWAP_HAS_CACHE) {
+ double_pt_unlock(dst_ptl, src_ptl);
+ return -EAGAIN;
+ }
}
orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);
@@ -1409,10 +1422,20 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
folio_lock(src_folio);
goto retry;
}
+ /*
+ * Check if the folio still belongs to the target swap entry after
+ * acquiring the lock. Folio can be freed in the swap cache while
+ * not locked.
+ */
+ if (unlikely(!folio_test_swapcache(folio) ||
+ entry.val != folio->swap.val)) {
+ err = -EAGAIN;
+ goto out;
+ }
}
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);
+ dst_ptl, src_ptl, src_folio, si);
}
out:
--
2.49.0
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] mm: userfaultfd: fix race of userfaultfd_move and swap cache
2025-06-01 20:01 [PATCH v2] mm: userfaultfd: fix race of userfaultfd_move and swap cache Kairui Song
@ 2025-06-01 23:02 ` Barry Song
2025-06-02 1:43 ` Lokesh Gidra
1 sibling, 0 replies; 4+ messages in thread
From: Barry Song @ 2025-06-01 23:02 UTC (permalink / raw)
To: Kairui Song
Cc: linux-mm, Andrew Morton, Peter Xu, Suren Baghdasaryan,
Andrea Arcangeli, David Hildenbrand, Lokesh Gidra, stable,
linux-kernel
On Mon, Jun 2, 2025 at 4:01 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> From: Kairui Song <kasong@tencent.com>
>
> On seeing a swap entry PTE, userfaultfd_move does a lockless swap cache
> lookup, and try to move the found folio to the faulting vma when.
> Currently, it relies on the PTE value check to ensure the moved folio
> still belongs to the src swap entry, which turns out is not reliable.
>
> While working and reviewing the swap table series with Barry, following
> existing race is observed and reproduced [1]:
>
> ( move_pages_pte is moving src_pte to dst_pte, where src_pte is a
> swap entry PTE holding swap entry S1, and S1 isn't in the swap cache.)
>
> CPU1 CPU2
> userfaultfd_move
> move_pages_pte()
> entry = pte_to_swp_entry(orig_src_pte);
> // Here it got entry = S1
> ... < Somehow interrupted> ...
> <swapin src_pte, alloc and use folio A>
> // folio A is just a new allocated folio
> // and get installed into src_pte
> <frees swap entry S1>
> // src_pte now points to folio A, S1
> // has swap count == 0, it can be freed
> // by folio_swap_swap or swap
> // allocator's reclaim.
> <try to swap out another folio B>
> // folio B is a folio in another VMA.
> <put folio B to swap cache using S1 >
> // S1 is freed, folio B could use it
> // for swap out with no problem.
> ...
> folio = filemap_get_folio(S1)
> // Got folio B here !!!
> ... < Somehow interrupted again> ...
> <swapin folio B and free S1>
> // Now S1 is free to be used again.
> <swapout src_pte & folio A using S1>
> // Now src_pte is a swap entry pte
> // holding S1 again.
> folio_trylock(folio)
> move_swap_pte
> double_pt_lock
> is_pte_pages_stable
> // Check passed because src_pte == S1
> folio_move_anon_rmap(...)
> // Moved invalid folio B here !!!
>
> The race window is very short and requires multiple collisions of
> multiple rare events, so it's very unlikely to happen, but with a
> deliberately constructed reproducer and increased time window, it can be
> reproduced [1].
>
> It's also possible that folio (A) is swapped in, and swapped out again
> after the filemap_get_folio lookup, in such case folio (A) may stay in
> swap cache so it needs to be moved too. In this case we should also try
> again so kernel won't miss a folio move.
>
> Fix this by checking if the folio is the valid swap cache folio after
> acquiring the folio lock, and checking the swap cache again after
> acquiring the src_pte lock.
>
> SWP_SYNCRHONIZE_IO path does make the problem more complex, but so far
> we don't need to worry about that since folios only might get exposed to
> swap cache in the swap out path, and it's covered in this patch too by
> checking the swap cache again after acquiring src_pte lock.
>
> Testing with a simple C program to allocate and move several GB of memory
> did not show any observable performance change.
>
> Cc: <stable@vger.kernel.org>
> Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> Closes: https://lore.kernel.org/linux-mm/CAMgjq7B1K=6OOrK2OUZ0-tqCzi+EJt+2_K97TPGoSt=9+JwP7Q@mail.gmail.com/ [1]
> Signed-off-by: Kairui Song <kasong@tencent.com>
Reviewed-by: Barry Song <baohua@kernel.org>
>
> ---
>
> V1: https://lore.kernel.org/linux-mm/20250530201710.81365-1-ryncsn@gmail.com/
> Changes:
> - Check swap_map instead of doing a filemap lookup after acquiring the
> PTE lock to minimize critical section overhead [ Barry Song, Lokesh Gidra ]
>
> mm/userfaultfd.c | 27 +++++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index bc473ad21202..a74ede04996c 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1084,8 +1084,11 @@ static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> pte_t orig_dst_pte, pte_t orig_src_pte,
> pmd_t *dst_pmd, pmd_t dst_pmdval,
> spinlock_t *dst_ptl, spinlock_t *src_ptl,
> - struct folio *src_folio)
> + struct folio *src_folio,
> + struct swap_info_struct *si)
> {
> + swp_entry_t entry;
> +
> double_pt_lock(dst_ptl, src_ptl);
>
> if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> @@ -1102,6 +1105,16 @@ 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 {
> + /*
> + * Check if the swap entry is cached after acquiring the src_pte
> + * lock. Or we might miss a new loaded swap cache folio.
> + */
> + entry = pte_to_swp_entry(orig_src_pte);
> + if (si->swap_map[swp_offset(entry)] & SWAP_HAS_CACHE) {
> + double_pt_unlock(dst_ptl, src_ptl);
> + return -EAGAIN;
> + }
> }
>
> orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);
> @@ -1409,10 +1422,20 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> folio_lock(src_folio);
> goto retry;
> }
> + /*
> + * Check if the folio still belongs to the target swap entry after
> + * acquiring the lock. Folio can be freed in the swap cache while
> + * not locked.
> + */
> + if (unlikely(!folio_test_swapcache(folio) ||
> + entry.val != folio->swap.val)) {
> + err = -EAGAIN;
> + goto out;
> + }
> }
> 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);
> + dst_ptl, src_ptl, src_folio, si);
> }
>
> out:
> --
> 2.49.0
>
Thanks
Barry
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] mm: userfaultfd: fix race of userfaultfd_move and swap cache
2025-06-01 20:01 [PATCH v2] mm: userfaultfd: fix race of userfaultfd_move and swap cache Kairui Song
2025-06-01 23:02 ` Barry Song
@ 2025-06-02 1:43 ` Lokesh Gidra
2025-06-02 5:47 ` Kairui Song
1 sibling, 1 reply; 4+ messages in thread
From: Lokesh Gidra @ 2025-06-02 1:43 UTC (permalink / raw)
To: Kairui Song
Cc: linux-mm, Andrew Morton, Barry Song, Peter Xu,
Suren Baghdasaryan, Andrea Arcangeli, David Hildenbrand, stable,
linux-kernel
On Sun, Jun 1, 2025 at 1:01 PM Kairui Song <ryncsn@gmail.com> wrote:
>
> From: Kairui Song <kasong@tencent.com>
>
> On seeing a swap entry PTE, userfaultfd_move does a lockless swap cache
> lookup, and try to move the found folio to the faulting vma when.
> Currently, it relies on the PTE value check to ensure the moved folio
> still belongs to the src swap entry, which turns out is not reliable.
>
> While working and reviewing the swap table series with Barry, following
> existing race is observed and reproduced [1]:
>
> ( move_pages_pte is moving src_pte to dst_pte, where src_pte is a
> swap entry PTE holding swap entry S1, and S1 isn't in the swap cache.)
>
> CPU1 CPU2
> userfaultfd_move
> move_pages_pte()
> entry = pte_to_swp_entry(orig_src_pte);
> // Here it got entry = S1
> ... < Somehow interrupted> ...
> <swapin src_pte, alloc and use folio A>
> // folio A is just a new allocated folio
> // and get installed into src_pte
> <frees swap entry S1>
> // src_pte now points to folio A, S1
> // has swap count == 0, it can be freed
> // by folio_swap_swap or swap
> // allocator's reclaim.
> <try to swap out another folio B>
> // folio B is a folio in another VMA.
> <put folio B to swap cache using S1 >
> // S1 is freed, folio B could use it
> // for swap out with no problem.
> ...
> folio = filemap_get_folio(S1)
> // Got folio B here !!!
> ... < Somehow interrupted again> ...
> <swapin folio B and free S1>
> // Now S1 is free to be used again.
> <swapout src_pte & folio A using S1>
> // Now src_pte is a swap entry pte
> // holding S1 again.
> folio_trylock(folio)
> move_swap_pte
> double_pt_lock
> is_pte_pages_stable
> // Check passed because src_pte == S1
> folio_move_anon_rmap(...)
> // Moved invalid folio B here !!!
>
> The race window is very short and requires multiple collisions of
> multiple rare events, so it's very unlikely to happen, but with a
> deliberately constructed reproducer and increased time window, it can be
> reproduced [1].
>
> It's also possible that folio (A) is swapped in, and swapped out again
> after the filemap_get_folio lookup, in such case folio (A) may stay in
> swap cache so it needs to be moved too. In this case we should also try
> again so kernel won't miss a folio move.
>
> Fix this by checking if the folio is the valid swap cache folio after
> acquiring the folio lock, and checking the swap cache again after
> acquiring the src_pte lock.
>
> SWP_SYNCRHONIZE_IO path does make the problem more complex, but so far
> we don't need to worry about that since folios only might get exposed to
> swap cache in the swap out path, and it's covered in this patch too by
> checking the swap cache again after acquiring src_pte lock.
>
> Testing with a simple C program to allocate and move several GB of memory
> did not show any observable performance change.
>
> Cc: <stable@vger.kernel.org>
> Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> Closes: https://lore.kernel.org/linux-mm/CAMgjq7B1K=6OOrK2OUZ0-tqCzi+EJt+2_K97TPGoSt=9+JwP7Q@mail.gmail.com/ [1]
> Signed-off-by: Kairui Song <kasong@tencent.com>
>
> ---
>
> V1: https://lore.kernel.org/linux-mm/20250530201710.81365-1-ryncsn@gmail.com/
> Changes:
> - Check swap_map instead of doing a filemap lookup after acquiring the
> PTE lock to minimize critical section overhead [ Barry Song, Lokesh Gidra ]
>
> mm/userfaultfd.c | 27 +++++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index bc473ad21202..a74ede04996c 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1084,8 +1084,11 @@ static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> pte_t orig_dst_pte, pte_t orig_src_pte,
> pmd_t *dst_pmd, pmd_t dst_pmdval,
> spinlock_t *dst_ptl, spinlock_t *src_ptl,
> - struct folio *src_folio)
> + struct folio *src_folio,
> + struct swap_info_struct *si)
> {
> + swp_entry_t entry;
> +
> double_pt_lock(dst_ptl, src_ptl);
>
> if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> @@ -1102,6 +1105,16 @@ 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 {
> + /*
> + * Check if the swap entry is cached after acquiring the src_pte
> + * lock. Or we might miss a new loaded swap cache folio.
> + */
> + entry = pte_to_swp_entry(orig_src_pte);
Can we pass this also from move_pages_pte()? It would be great to
minimize PTL critical section.
> + if (si->swap_map[swp_offset(entry)] & SWAP_HAS_CACHE) {
> + double_pt_unlock(dst_ptl, src_ptl);
> + return -EAGAIN;
> + }
> }
>
> orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);
> @@ -1409,10 +1422,20 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> folio_lock(src_folio);
> goto retry;
> }
> + /*
> + * Check if the folio still belongs to the target swap entry after
> + * acquiring the lock. Folio can be freed in the swap cache while
> + * not locked.
> + */
> + if (unlikely(!folio_test_swapcache(folio) ||
> + entry.val != folio->swap.val)) {
> + err = -EAGAIN;
> + goto out;
> + }
This check will get skipped if the folio was locked by folio_lock()
rather than folio_trylock(). It seems to me that the correct way to do
this is to check outside this if block (right before calling
move_swap_pte()) and with 'src_folio' instead of 'folio'. Even better,
if you pass 'entry' as well to move_swap_pte() as per my previous
comment, then you can move this check also in move_swap_pte().
> }
> 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);
> + dst_ptl, src_ptl, src_folio, si);
> }
>
> out:
> --
> 2.49.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] mm: userfaultfd: fix race of userfaultfd_move and swap cache
2025-06-02 1:43 ` Lokesh Gidra
@ 2025-06-02 5:47 ` Kairui Song
0 siblings, 0 replies; 4+ messages in thread
From: Kairui Song @ 2025-06-02 5:47 UTC (permalink / raw)
To: Lokesh Gidra
Cc: linux-mm, Andrew Morton, Barry Song, Peter Xu,
Suren Baghdasaryan, Andrea Arcangeli, David Hildenbrand, stable,
linux-kernel
On Mon, Jun 2, 2025 at 9:43 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
>
> On Sun, Jun 1, 2025 at 1:01 PM Kairui Song <ryncsn@gmail.com> wrote:
> >
> > From: Kairui Song <kasong@tencent.com>
> >
> > On seeing a swap entry PTE, userfaultfd_move does a lockless swap cache
> > lookup, and try to move the found folio to the faulting vma when.
> > Currently, it relies on the PTE value check to ensure the moved folio
> > still belongs to the src swap entry, which turns out is not reliable.
> >
> > While working and reviewing the swap table series with Barry, following
> > existing race is observed and reproduced [1]:
> >
> > ( move_pages_pte is moving src_pte to dst_pte, where src_pte is a
> > swap entry PTE holding swap entry S1, and S1 isn't in the swap cache.)
> >
> > CPU1 CPU2
> > userfaultfd_move
> > move_pages_pte()
> > entry = pte_to_swp_entry(orig_src_pte);
> > // Here it got entry = S1
> > ... < Somehow interrupted> ...
> > <swapin src_pte, alloc and use folio A>
> > // folio A is just a new allocated folio
> > // and get installed into src_pte
> > <frees swap entry S1>
> > // src_pte now points to folio A, S1
> > // has swap count == 0, it can be freed
> > // by folio_swap_swap or swap
> > // allocator's reclaim.
> > <try to swap out another folio B>
> > // folio B is a folio in another VMA.
> > <put folio B to swap cache using S1 >
> > // S1 is freed, folio B could use it
> > // for swap out with no problem.
> > ...
> > folio = filemap_get_folio(S1)
> > // Got folio B here !!!
> > ... < Somehow interrupted again> ...
> > <swapin folio B and free S1>
> > // Now S1 is free to be used again.
> > <swapout src_pte & folio A using S1>
> > // Now src_pte is a swap entry pte
> > // holding S1 again.
> > folio_trylock(folio)
> > move_swap_pte
> > double_pt_lock
> > is_pte_pages_stable
> > // Check passed because src_pte == S1
> > folio_move_anon_rmap(...)
> > // Moved invalid folio B here !!!
> >
> > The race window is very short and requires multiple collisions of
> > multiple rare events, so it's very unlikely to happen, but with a
> > deliberately constructed reproducer and increased time window, it can be
> > reproduced [1].
> >
> > It's also possible that folio (A) is swapped in, and swapped out again
> > after the filemap_get_folio lookup, in such case folio (A) may stay in
> > swap cache so it needs to be moved too. In this case we should also try
> > again so kernel won't miss a folio move.
> >
> > Fix this by checking if the folio is the valid swap cache folio after
> > acquiring the folio lock, and checking the swap cache again after
> > acquiring the src_pte lock.
> >
> > SWP_SYNCRHONIZE_IO path does make the problem more complex, but so far
> > we don't need to worry about that since folios only might get exposed to
> > swap cache in the swap out path, and it's covered in this patch too by
> > checking the swap cache again after acquiring src_pte lock.
> >
> > Testing with a simple C program to allocate and move several GB of memory
> > did not show any observable performance change.
> >
> > Cc: <stable@vger.kernel.org>
> > Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> > Closes: https://lore.kernel.org/linux-mm/CAMgjq7B1K=6OOrK2OUZ0-tqCzi+EJt+2_K97TPGoSt=9+JwP7Q@mail.gmail.com/ [1]
> > Signed-off-by: Kairui Song <kasong@tencent.com>
> >
> > ---
> >
> > V1: https://lore.kernel.org/linux-mm/20250530201710.81365-1-ryncsn@gmail.com/
> > Changes:
> > - Check swap_map instead of doing a filemap lookup after acquiring the
> > PTE lock to minimize critical section overhead [ Barry Song, Lokesh Gidra ]
> >
> > mm/userfaultfd.c | 27 +++++++++++++++++++++++++--
> > 1 file changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > index bc473ad21202..a74ede04996c 100644
> > --- a/mm/userfaultfd.c
> > +++ b/mm/userfaultfd.c
> > @@ -1084,8 +1084,11 @@ static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> > pte_t orig_dst_pte, pte_t orig_src_pte,
> > pmd_t *dst_pmd, pmd_t dst_pmdval,
> > spinlock_t *dst_ptl, spinlock_t *src_ptl,
> > - struct folio *src_folio)
> > + struct folio *src_folio,
> > + struct swap_info_struct *si)
> > {
> > + swp_entry_t entry;
> > +
> > double_pt_lock(dst_ptl, src_ptl);
> >
> > if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> > @@ -1102,6 +1105,16 @@ 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 {
> > + /*
> > + * Check if the swap entry is cached after acquiring the src_pte
> > + * lock. Or we might miss a new loaded swap cache folio.
> > + */
> > + entry = pte_to_swp_entry(orig_src_pte);
>
> Can we pass this also from move_pages_pte()? It would be great to
> minimize PTL critical section.
I checked the objdump output. It seems the compiler is doing a good
job on optimizing all the overhead off since it's an inlined macro,
but I can pass it in too, just in case.
>
> > + if (si->swap_map[swp_offset(entry)] & SWAP_HAS_CACHE) {
> > + double_pt_unlock(dst_ptl, src_ptl);
> > + return -EAGAIN;
> > + }
> > }
> >
> > orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);
> > @@ -1409,10 +1422,20 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > folio_lock(src_folio);
> > goto retry;
> > }
> > + /*
> > + * Check if the folio still belongs to the target swap entry after
> > + * acquiring the lock. Folio can be freed in the swap cache while
> > + * not locked.
> > + */
> > + if (unlikely(!folio_test_swapcache(folio) ||
> > + entry.val != folio->swap.val)) {
> > + err = -EAGAIN;
> > + goto out;
> > + }
>
> This check will get skipped if the folio was locked by folio_lock()
> rather than folio_trylock(). It seems to me that the correct way to do
> this is to check outside this if block (right before calling
> move_swap_pte()) and with 'src_folio' instead of 'folio'. Even better,
> if you pass 'entry' as well to move_swap_pte() as per my previous
> comment, then you can move this check also in move_swap_pte().
Good catch, let me move it into move_swap_pte then, it's much easier
to follow that way.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-06-02 5:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-06-01 20:01 [PATCH v2] mm: userfaultfd: fix race of userfaultfd_move and swap cache Kairui Song
2025-06-01 23:02 ` Barry Song
2025-06-02 1:43 ` Lokesh Gidra
2025-06-02 5:47 ` Kairui Song
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox