On Mon, May 15, 2023 at 06:34:30PM +0100, Catalin Marinas wrote:
On Sat, May 13, 2023 at 05:29:53AM +0200, David Hildenbrand wrote:
On 13.05.23 01:57, Peter Collingbourne wrote:
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@m.... I couldn't reproduce it with my swap setup but both Qun-wei and Peter triggered it.
In order to reproduce this bug it is necessary for the swap slot cache to be disabled, which is unlikely to occur during normal operation. I was only able to reproduce the bug by disabling it forcefully with the following patch:
diff --git a/mm/swap_slots.c b/mm/swap_slots.c index 0bec1f705f8e0..25afba16980c7 100644 --- a/mm/swap_slots.c +++ b/mm/swap_slots.c @@ -79,7 +79,7 @@ void disable_swap_slots_cache_lock(void)
static void __reenable_swap_slots_cache(void) { - swap_slot_cache_enabled = has_usable_swap(); + swap_slot_cache_enabled = false; }
void reenable_swap_slots_cache_unlock(void)
With that I can trigger the bug on an MTE-utilizing process by running a program that enumerates the process's private anonymous mappings and calls process_madvise(MADV_PAGEOUT) on all of them.
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).
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).
Another problem that I can see with this approach is that it does not respect reference counts for swap entries, and it's unclear whether that can be done in a non-racy fashion.
Another approach that I considered was to move the hook to swap_readpage() as in the patch below (sorry, it only applies to an older version of Android's android14-6.1 branch and not mainline, but you get the idea). But during a stress test (running the aforementioned program that calls process_madvise(MADV_PAGEOUT) in a loop during an Android "monkey" test) I discovered the following racy use-after-free that can occur when two tasks T1 and T2 concurrently restore the same page:
T1: | T2: arch_swap_readpage() | | arch_swap_readpage() -> mte_restore_tags() -> xe_load() swap_free() | | arch_swap_readpage() -> mte_restore_tags() -> mte_restore_page_tags()
We can avoid it by taking the swap_info_struct::lock spinlock in mte_restore_tags(), but it seems like it would lead to lock contention.
Peter
diff --git a/arch/arm64/include/asm/mte.h b/arch/arm64/include/asm/mte.h index 3f8199ba265a1..99c8be073f107 100644 --- a/arch/arm64/include/asm/mte.h +++ b/arch/arm64/include/asm/mte.h @@ -25,7 +25,7 @@ unsigned long mte_copy_tags_to_user(void __user *to, void *from, unsigned long n); int mte_save_tags(struct page *page); void mte_save_page_tags(const void *page_addr, void *tag_storage); -bool mte_restore_tags(swp_entry_t entry, struct page *page); +void mte_restore_tags(struct page *page); void mte_restore_page_tags(void *page_addr, const void *tag_storage); void mte_invalidate_tags(int type, pgoff_t offset); void mte_invalidate_tags_area(int type); diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h index 812373cff4eec..32d3c661a0eee 100644 --- a/arch/arm64/include/asm/pgtable.h +++ b/arch/arm64/include/asm/pgtable.h @@ -1054,11 +1054,11 @@ static inline void arch_swap_invalidate_area(int type) mte_invalidate_tags_area(type); }
-#define __HAVE_ARCH_SWAP_RESTORE -static inline void arch_swap_restore(swp_entry_t entry, struct folio *folio) +#define __HAVE_ARCH_SWAP_READPAGE +static inline void arch_swap_readpage(struct page *page) { - if (system_supports_mte() && mte_restore_tags(entry, &folio->page)) - set_page_mte_tagged(&folio->page); + if (system_supports_mte()) + mte_restore_tags(page); }
#endif /* CONFIG_ARM64_MTE */ diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c index 84a085d536f84..176f094ecaa1e 100644 --- a/arch/arm64/kernel/mte.c +++ b/arch/arm64/kernel/mte.c @@ -38,15 +38,6 @@ EXPORT_SYMBOL_GPL(mte_async_or_asymm_mode); static void mte_sync_page_tags(struct page *page, pte_t old_pte, bool check_swap, bool pte_is_tagged) { - if (check_swap && is_swap_pte(old_pte)) { - swp_entry_t entry = pte_to_swp_entry(old_pte); - - if (!non_swap_entry(entry) && mte_restore_tags(entry, page)) { - set_page_mte_tagged(page); - return; - } - } - if (!pte_is_tagged) return;
diff --git a/arch/arm64/mm/mteswap.c b/arch/arm64/mm/mteswap.c index 70f913205db99..3fe7774f32b3c 100644 --- a/arch/arm64/mm/mteswap.c +++ b/arch/arm64/mm/mteswap.c @@ -46,21 +46,23 @@ int mte_save_tags(struct page *page) return 0; }
-bool mte_restore_tags(swp_entry_t entry, struct page *page) +void mte_restore_tags(struct page *page) { + swp_entry_t entry = folio_swap_entry(page_folio(page)); void *tags = xa_load(&mte_pages, entry.val);
if (!tags) - return false; + return;
/* * Test PG_mte_tagged again in case it was racing with another * set_pte_at(). */ - if (!test_and_set_bit(PG_mte_tagged, &page->flags)) + if (!test_and_set_bit(PG_mte_tagged, &page->flags)) { mte_restore_page_tags(page_address(page), tags); - - return true; + if (kasan_hw_tags_enabled()) + page_kasan_tag_reset(page); + } }
void mte_invalidate_tags(int type, pgoff_t offset) diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h index 5f0d7d0b9471b..eea1e545595ca 100644 --- a/include/linux/pgtable.h +++ b/include/linux/pgtable.h @@ -793,8 +793,8 @@ static inline void arch_swap_invalidate_area(int type) } #endif
-#ifndef __HAVE_ARCH_SWAP_RESTORE -static inline void arch_swap_restore(swp_entry_t entry, struct folio *folio) +#ifndef __HAVE_ARCH_SWAP_READPAGE +static inline void arch_swap_readpage(struct page *page) { } #endif diff --git a/mm/page_io.c b/mm/page_io.c index 3a5f921b932e8..a2f53dbeca7b3 100644 --- a/mm/page_io.c +++ b/mm/page_io.c @@ -470,6 +470,12 @@ int swap_readpage(struct page *page, bool synchronous, } delayacct_swapin_start();
+ /* + * Some architectures may have to restore extra metadata to the + * page when reading from swap. + */ + arch_swap_readpage(page); + if (frontswap_load(page) == 0) { SetPageUptodate(page); unlock_page(page); diff --git a/mm/shmem.c b/mm/shmem.c index 0b335607bf2ad..82ccf1e6efe5d 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -1784,12 +1784,6 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index, } folio_wait_writeback(folio);
- /* - * Some architectures may have to restore extra metadata to the - * folio after reading from swap. - */ - arch_swap_restore(swap, folio); - if (shmem_should_replace_folio(folio, gfp)) { error = shmem_replace_folio(&folio, gfp, info, index); if (error)