Re: [PATCH 1/3] mm: Move arch_do_swap_page() call to before swap_free()

On Tue, May 16, 2023 at 5:31 AM David Hildenbrand [off-list ref] wrote:

On 15.05.23 19:34, Catalin Marinas wrote:

quoted

On Sat, May 13, 2023 at 05:29:53AM +0200, David Hildenbrand wrote:

quoted

On 13.05.23 01:57, Peter Collingbourne wrote:

quoted

diff --git a/mm/memory.c b/mm/memory.c
index 01a23ad48a04..83268d287ff1 100644
--- a/mm/memory.c
+++ b/mm/memory.c

@@ -3914,19 +3914,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
            }
    }
-   /*
-    * Remove the swap entry and conditionally try to free up the swapcache.
-    * We're already holding a reference on the page but haven't mapped it
-    * yet.
-    */
-   swap_free(entry);
-   if (should_try_to_free_swap(folio, vma, vmf->flags))
-           folio_free_swap(folio);
-
-   inc_mm_counter(vma->vm_mm, MM_ANONPAGES);
-   dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
    pte = mk_pte(page, vma->vm_page_prot);
-
    /*
     * Same logic as in do_wp_page(); however, optimize for pages that are
     * certainly not shared either because we just allocated them without

@@ -3946,8 +3934,21 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
            pte = pte_mksoft_dirty(pte);
    if (pte_swp_uffd_wp(vmf->orig_pte))
            pte = pte_mkuffd_wp(pte);
+   arch_do_swap_page(vma->vm_mm, vma, vmf->address, pte, vmf->orig_pte);
    vmf->orig_pte = pte;
+   /*
+    * Remove the swap entry and conditionally try to free up the swapcache.
+    * We're already holding a reference on the page but haven't mapped it
+    * yet.
+    */
+   swap_free(entry);
+   if (should_try_to_free_swap(folio, vma, vmf->flags))
+           folio_free_swap(folio);
+
+   inc_mm_counter(vma->vm_mm, MM_ANONPAGES);
+   dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
+
    /* ksm created a completely new copy */
    if (unlikely(folio != swapcache && swapcache)) {
            page_add_new_anon_rmap(page, vma, vmf->address);

@@ -3959,7 +3960,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
    VM_BUG_ON(!folio_test_anon(folio) ||
                    (pte_write(pte) && !PageAnonExclusive(page)));
    set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte);
-   arch_do_swap_page(vma->vm_mm, vma, vmf->address, pte, vmf->orig_pte);
    folio_unlock(folio);
    if (folio != swapcache && swapcache) {

You are moving the folio_free_swap() call after the folio_ref_count(folio)
== 1 check, which means that such (previously) swapped pages that are
exclusive cannot be detected as exclusive.

There must be a better way to handle MTE here.

Where are the tags stored, how is the location identified, and when are they
effectively restored right now?

I haven't gone through Peter's patches yet but a pretty good description
of the problem is here:
https://lore.kernel.org/all/5050805753ac469e8d727c797c2218a9d780d434.camel@mediatek.com/ (local).
I couldn't reproduce it with my swap setup but both Qun-wei and Peter
triggered it.

When a tagged page is swapped out, the arm64 code stores the metadata
(tags) in a local xarray indexed by the swap pte. When restoring from
swap, the arm64 set_pte_at() checks this xarray using the old swap pte
and spills the tags onto the new page. Apparently something changed in
the kernel recently that causes swap_range_free() to be called before
set_pte_at(). The arm64 arch_swap_invalidate_page() frees the metadata
from the xarray and the subsequent set_pte_at() won't find it.

If we have the page, the metadata can be restored before set_pte_at()
and I guess that's what Peter is trying to do (again, I haven't looked
at the details yet; leaving it for tomorrow).

Thanks for the details! I was missing that we also have a hook in
swap_range_free().

quoted

Is there any other way of handling this? E.g. not release the metadata
in arch_swap_invalidate_page() but later in set_pte_at() once it was
restored. But then we may leak this metadata if there's no set_pte_at()
(the process mapping the swap entry died).

That was my immediate thought: do we really have to hook into
swap_range_free() at all?

As I alluded to in another reply, without the hook in
swap_range_free() I think we would either end up with a race or an
effective memory leak in the arch code that maintains the metadata for
swapped out pages, as there would be no way for the arch-specific code
to know when it is safe to free it after swapin.

And I also wondered why we have to do this
from set_pte_at() and not do this explicitly (maybe that's the other
arch_* callback on the swapin path).

I don't think it's necessary, as the set_pte_at() call sites for
swapped in pages are known. I'd much rather do this via an explicit
hook at those call sites, as the existing approach of implicit
restoring seems too subtle and easy to be overlooked when refactoring,
as we have seen with this bug. In the end we only have 3 call sites
for the hook and hopefully the comments that I'm adding are sufficient
to ensure that any new swapin code should end up with a call to the
hook in the right place.

Peter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel