mm: COW fixes part 1: fix the COW security issue for THP and swap
Subsystems (4)
v2 cover mm: COW fixes part 1: fix the COW security issue for THP and swap
This series is the result of the discussion on the previous approach [1]. More information on the general COW issues can be found there.
This series attempts to optimize and streamline the COW logic for ordinary anon pages and THP anon pages, fixing two remaining instances of CVE-2020-29374 in do_swap_page() and do_huge_pmd_wp_page(): information can leak from a parent process to a child process via anonymous pages shared during fork().
This issue, including other related COW issues, has been summarized in [2]:
"
1. Observing Memory Modifications of Private Pages From A Child Process
Long story short: process-private memory might not be as private as you think once you fork(): successive modifications of private memory regions in the parent process can still be observed by the child process, for example, by smart use of vmsplice()+munmap().
The core problem is that pinning pages readable in a child process, such as done via the vmsplice system call, can result in a child process observing memory modifications done in the parent process the child is not supposed to observe. [1] contains an excellent summary and [2] contains further details. This issue was assigned CVE-2020-29374 [9].
For this to trigger, it's required to use a fork() without subsequent exec(), for example, as used under Android zygote. Without further details about an application that forks less-privileged child processes, one cannot really say what's actually affected and what's not -- see the details section the end of this mail for a short sshd/openssh analysis.
While commit 17839856fd58 ("gup: document and work around "COW can break either way" issue") fixed this issue and resulted in other problems (e.g., ptrace on pmem), commit 09854ba94c6a ("mm: do_wp_page() simplification") re-introduced part of the problem unfortunately.
The original reproducer can be modified quite easily to use THP [3] and make the issue appear again on upstream kernels. I modified it to use hugetlb [4] and it triggers as well. The problem is certainly less severe with hugetlb than with THP; it merely highlights that we still have plenty of open holes we should be closing/fixing.
Regarding vmsplice(), the only known workaround is to disallow the vmsplice() system call ... or disable THP and hugetlb. But who knows what else is affected (RDMA? O_DIRECT?) to achieve the same goal -- in the end, it's a more generic issue.
"
This security issue was first reported by Jann Horn on 27 May 2020 and it currently affects anonymous pages during swapin, anonymous THP and hugetlb. This series tackles anonymous pages during swapin and anonymous THP:
* do_swap_page() for handling COW on PTEs *during* swapin directly
* do_huge_pmd_wp_page() for handling COW on PMD-mapped THP during write faults
With this series, we'll apply the same COW logic we have in do_wp_page() to all swappable anon pages: don't reuse (map writable) the page in case there are additional references (page_count() != 1). All users of reuse_swap_page() are remove, and consequently reuse_swap_page() is removed.
In general, we're struggling with the following COW-related issues:
(1) "missed COW": we miss to copy on write and reuse the page (map it writable) although we must copy because there are pending references from another process to this page. The result is a security issue.
(2) "wrong COW": we copy on write although we wouldn't have to and shouldn't: if there are valid GUP references, they will become out of sync with the pages mapped into the page table. We fail to detect that such a page can be reused safely, especially if never more than a single process mapped the page. The result is an intra process memory corruption.
(3) "unnecessary COW": we copy on write although we wouldn't have to:
performance degradation and temporary increases swap+memory consumption can be the result.
While this series fixes (1) for swappable anon pages, it tries to reduce reported cases of (3) first as good and easy as possible to limit the impact when streamlining. The individual patches try to describe in which cases we will run into (3).
This series certainly makes (2) worse for THP, because a THP will now get PTE-mapped on write faults if there are additional references, even if there was only ever a single process involved: once PTE-mapped, we'll copy each and every subpage and won't reuse any subpage as long as the underlying compound page wasn't split.
I'm working on an approach to fix (2) and improve (3): PageAnonExclusive to mark anon pages that are exclusive to a single process, allow GUP pins only on such exclusive pages, and allow turning exclusive pages shared (clearing PageAnonExclusive) only if there are no GUP pins. Anon pages with PageAnonExclusive set never have to be copied during write faults, but eventually during fork() if they cannot be turned shared. The improved reuse logic in this series will essentially also be the logic to reset PageAnonExclusive. This work will certainly take a while, but I'm planning on sharing details before having code fully ready.
#1-#5 can be applied independently of the rest. #6-#9 are mostly only cleanups related to reuse_swap_page().
Notes:
* For now, I'll leave hugetlb code untouched: "unnecessary COW" might easily break existing setups because hugetlb pages are a scarce resource and we could just end up having to crash the application when we run out of hugetlb pages. We have to be very careful and the security aspect with hugetlb is most certainly less relevant than for unprivileged anon pages.
* Instead of lru_add_drain() we might actually just drain the lru_add list or even just remove the single page of interest from the lru_add list. This would require a new helper function, and could be added if the conditional lru_add_drain() turn out to be a problem.
* I am pretty much confused about the swapcache handling in khugepaged code. This certainly needs an additional pair of eyes.
* I extended the test case already included in [1] to also test for the newly found do_swap_page() case. I'll send that out separately once/if this part was reviewed.
[1] https://lkml.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
David Hildenbrand (9):
mm: optimize do_wp_page() for exclusive pages in the swapcache
mm: optimize do_wp_page() for fresh pages in local LRU pagevecs
mm: slightly clarify KSM logic in do_swap_page()
mm: streamline COW logic in do_swap_page() mm/huge_memory: streamline COW logic in do_huge_pmd_wp_page() mm/khugepaged: remove reuse_swap_page() usage mm/swapfile: remove reuse_swap_page()
mm/huge_memory: remove stale page_trans_huge_mapcount() mm/huge_memory: remove stale locking logic from __split_huge_pmd()
include/linux/mm.h | 5 -- include/linux/swap.h | 4 -- mm/huge_memory.c | 99 ++++++---------------------------- mm/khugepaged.c | 16 ++++-- mm/memory.c | 123 +++++++++++++++++++++++++++++++------------ mm/swapfile.c | 104 ------------------------------------ 6 files changed, 119 insertions(+), 232 deletions(-)
base-commit: e783362eb54cd99b2cac8b3a9aeac942e6f6ac07
Signature · 1 line
2.34.1
RFC PATCH v2 1/9 mm: optimize do_wp_page() for exclusive pages in the swapcache
Liang Zhang reported [1] that the current COW logic in do_wp_page() is sub-optimal when it comes to swap+read fault+write fault of anonymous pages that have a single user, visible via a performance degradation in the redis benchmark. Something similar was previously reported [2] by Nadav with a simple reproducer.
Let's optimize for pages that have been added to the swapcache but only have an exclusive owner. Try removing the swapcache reference if there is hope that we're the exclusive user.
We will fail removing the swapcache reference in two scenarios:
(1) There are additional swap entries referencing the page: copying instead of reusing is the right thing to do.
(2) The page is under writeback: theoretically we might be able to reuse in some cases, however, we cannot remove the additional reference and will have to copy.
Further, we might have additional references from the LRU pagevecs, which will force us to copy instead of being able to reuse. We'll try handling such references for some scenarios next. Concurrent writeback cannot be handled easily and we'll always have to copy.
While at it, remove the superfluous page_mapcount() check: it's implicitly covered by the page_count() for ordinary anon pages.
[1] https://lkml.kernel.org/r/[email protected]
[2] https://lkml.kernel.org/r/[email protected]
--- mm/memory.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
mm/memory.c +14 -6
@@ -3291,19 +3291,27 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)if (PageAnon (vmf->page)) {struct page *page = vmf->page; -/* PageKsm() doesn't necessarily raise the page refcount */ -if (PageKsm (page) ||page_count (page) !=1 ) +/* +* We have to verify under page lock: these early checks are +* just an optimization to avoid locking the page and freeing +* the swapcache if there is little hope that we can reuse. +* +* PageKsm() doesn't necessarily raise the page refcount. +*/ +if (PageKsm (page) ||page_count (page) >1 +PageSwapCache (page)) goto copy;if (!trylock_page (page)) goto copy; -if (PageKsm (page) ||page_mapcount (page) !=1 ||page_count (page) !=1 ) { +if (PageSwapCache (page)) +try_to_free_swap (page); +if (PageKsm (page) ||page_count (page) !=1 ) {unlock_page (page); goto copy; }/* -* Ok, we've got the only map reference, and the only -* page count reference, and the page is locked, -* it's dark out, and we're wearing sunglasses. Hit it. +* Ok, we've got the only page reference from our mapping +* and the page is locked, it's dark out, and we're wearing +* sunglasses. Hit it. */ unlock_page (page);wp_page_reuse (vmf);
Signature · 1 line
2.34.1
RFC PATCH v2 2/9 mm: optimize do_wp_page() for fresh pages in local LRU pagevecs
In reply to David HildenbrandFor example, if a page just got swapped in via a read fault, the LRU pagevecs might still hold a reference to the page. If we trigger a write fault on such a page, the additional reference from the LRU pagevecs will prohibit reusing the page.
Let's conditionally drain the local LRU pagevecs when we stumble over a !PageLRU() page. We cannot easily drain remote LRU pagevecs and it might not be desirable performance-wise. Consequently, this will only avoid copying in some cases.
We cannot easily handle the following cases and we will always have to copy:
(1) The page is referenced in the LRU pagevecs of other CPUs. We really would have to drain the LRU pagevecs of all CPUs -- most probably copying is much cheaper.
(2) The page is already PageLRU() but is getting moved between LRU lists, for example, for activation (e.g., mark_page_accessed()), deactivation (MADV_COLD), or lazyfree (MADV_FREE). We'd have to drain mostly unconditionally, which might be bad performance-wise. Most probably this won't happen too often in practice.
Note that there are other reasons why an anon page might temporarily not be PageLRU(): for example, compaction and migration have to isolate LRU pages from the LRU lists first (isolate_lru_page()), moving them to temporary local lists and clearing PageLRU() and holding an additional reference on the page. In that case, we'll always copy.
This change seems to be fairly effective with the reproducer [1] shared by Nadav, as long as writeback is done synchronously, for example, using zram. However, with asynchronous writeback, we'll usually fail to free the swapcache because the page is still under writeback: something we cannot easily optimize for, and maybe it's not really relevant in practice.
[1] https://lkml.kernel.org/r/[email protected]
--- mm/memory.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
mm/memory.c +11 -1
@@ -3298,7 +3298,17 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)* * PageKsm() doesn't necessarily raise the page refcount. */ -if (PageKsm (page) ||page_count (page) >1 +PageSwapCache (page)) +if (PageKsm (page)) + goto copy; +if (page_count (page) >1 +PageSwapCache (page) + !PageLRU (page)) + goto copy; +if (!PageLRU (page)) +/* +* Note: We cannot easily detect+handle references from +* remote LRU pagevecs or references to PageLRU() pages. +*/ +lru_add_drain (); +if (page_count (page) >1 +PageSwapCache (page)) goto copy;if (!trylock_page (page)) goto copy;
Signature · 1 line
2.34.1
RFC PATCH v2 3/9 mm: slightly clarify KSM logic in do_swap_page()
In reply to David HildenbrandLet's make it clearer that KSM might only have to copy a page in case we have a page in the swapcache, not if we allocated a fresh page and bypassed the swapcache. While at it, add a comment why this is usually necessary and merge the two swapcache conditions.
--- mm/memory.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)
mm/memory.c +23 -15
@@ -3617,21 +3617,29 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) goto out_release; } -/* -* Make sure try_to_free_swap or reuse_swap_page or swapoff did not -* release the swapcache from under us. The page pin, and pte_same -* test below, are not enough to exclude that. Even if it is still -* swapcache, we need to check that the page's swap has not changed. -*/ -if (unlikely ((!PageSwapCache (page) || -page_private (page) != entry.val)) && swapcache) - goto out_page; - - page =ksm_might_need_to_copy (page, vma, vmf->address); -if (unlikely (!page)) { - ret =VM_FAULT_OOM ; - page = swapcache; - goto out_page; +if (swapcache) { +/* +* Make sure try_to_free_swap or reuse_swap_page or swapoff did +* not release the swapcache from under us. The page pin, and +* pte_same test below, are not enough to exclude that. Even if +* it is still swapcache, we need to check that the page's swap +* has not changed. +*/ +if (unlikely (!PageSwapCache (page) || +page_private (page) != entry.val)) + goto out_page; + +/* +* KSM sometimes has to copy on read faults, for example, if +* page->index of !PageKSM() pages would be nonlinear inside the +* anon VMA -- PageKSM() is lost on actual swapout. +*/ + page =ksm_might_need_to_copy (page, vma, vmf->address); +if (unlikely (!page)) { + ret =VM_FAULT_OOM ; + page = swapcache; + goto out_page; + } }cgroup_throttle_swaprate (page,GFP_KERNEL );
Signature · 1 line
2.34.1
RFC PATCH v2 4/9 mm: streamline COW logic in do_swap_page()
In reply to David HildenbrandCurrently we have a different COW logic when:
* triggering a read-fault to swapin first and then trigger a write-fault -> do_swap_page() + do_wp_page()
* triggering a write-fault to swapin
-> do_swap_page() + do_wp_page() only if we fail reuse in do_swap_page()
The COW logic in do_swap_page() is different than our reuse logic in do_wp_page(). The COW logic in do_wp_page() -- page_count() == 1 -- makes currently sure that we certainly don't have a remaining reference, e.g., via GUP, on the target page we want to reuse: if there is any unexpected reference, we have to copy to avoid information leaks.
As do_swap_page() behaves differently, in environments with swap enabled we can currently have an unintended information leak from the parent to the child, similar as known from CVE-2020-29374:
1. Parent writes to anonymous page -> Page is mapped writable and modified 2. Page is swapped out -> Page is unmapped and replaced by swap entry 3. fork() -> Swap entries are copied to child 4. Child pins page R/O -> Page is mapped R/O into child 5. Child unmaps page -> Child still holds GUP reference 6. Parent writes to page -> Page is reused in do_swap_page() -> Child can observe changes
Exchanging 2. and 3. should have the same effect.
Let's apply the same COW logic as in do_wp_page(), conditionally trying to remove the page from the swapcache after freeing the swap entry, however, before actually mapping our page. We can change the order now that we use try_to_free_swap(), which doesn't care about the mapcount, instead of reuse_swap_page().
To handle references from the LRU pagevecs, conditionally drain the local LRU pagevecs when required, however, don't consider the page_count() when deciding whether to drain to keep it simple for now.
--- mm/memory.c | 55 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 12 deletions(-)
mm/memory.c +43 -12
@@ -3499,6 +3499,25 @@ static vm_fault_t remove_device_exclusive_entry(struct vm_fault *vmf)return 0 ; } +static inline bool should_try_to_free_swap (struct page *page, +struct vm_area_struct *vma, +unsigned int fault_flags) +{ +if (!PageSwapCache (page)) +return false; +if (mem_cgroup_swap_full (page) || (vma->vm_flags &VM_LOCKED ) || +PageMlocked (page)) +return true; +/* +* 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 +* owner. Try freeing the swapcache to get rid of the swapcache +* reference in case it's likely that we will succeed. +*/ +return (fault_flags &FAULT_FLAG_WRITE ) && !PageKsm (page) && +page_count (page) ==2 ; +} +/* * We enter with non-exclusive mmap_lock (to exclude vma changes, * but allow concurrent faults), and pte mapped but not yet locked.
@@ -3640,6 +3659,16 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) page = swapcache; goto out_page; } + +/* +* 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 +* owner. Try removing the extra reference from the local LRU +* pagevecs if required. +*/ +if ((vmf->flags &FAULT_FLAG_WRITE ) && page == swapcache && + !PageKsm (page) && !PageLRU (page)) +lru_add_drain (); }cgroup_throttle_swaprate (page,GFP_KERNEL );
@@ -3658,19 +3687,25 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) }/* -* The page isn't present yet, go ahead with the fault. -* -* Be careful about the sequence of operations here. -* To get its accounting right, reuse_swap_page() must be called -* while the page is counted on swap but not yet in mapcount i.e. -* before page_add_anon_rmap() and swap_free(); try_to_free_swap() -* must be called after the swap_free(), or it will never succeed. +* 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 (page, vma, vmf->flags)) +try_to_free_swap (page);inc_mm_counter_fast (vma->vm_mm,MM_ANONPAGES );dec_mm_counter_fast (vma->vm_mm,MM_SWAPENTS ); pte =mk_pte (page, vma->vm_page_prot); -if ((vmf->flags &FAULT_FLAG_WRITE ) &&reuse_swap_page (page)) { + +/* +* Same logic as in do_wp_page(); however, optimize for fresh pages +* that are certainly not shared because we just allocated them without +* exposing them to the swapcache. +*/ +if ((vmf->flags &FAULT_FLAG_WRITE ) && !PageKsm (page) && + (page != swapcache ||page_count (page) ==1 )) { pte =maybe_mkwrite (pte_mkdirty (pte), vma); vmf->flags &= ~FAULT_FLAG_WRITE ; ret |=VM_FAULT_WRITE ;
@@ -3696,10 +3731,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)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); -swap_free (entry); -if (mem_cgroup_swap_full (page) || - (vma->vm_flags &VM_LOCKED ) ||PageMlocked (page)) -try_to_free_swap (page);unlock_page (page);if (page != swapcache && swapcache) {/*
Signature · 1 line
2.34.1
RFC PATCH v2 5/9 mm/huge_memory: streamline COW logic in do_huge_pmd_wp_page()
In reply to David HildenbrandWe currently have a different COW logic for anon THP than we have for ordinary anon pages in do_wp_page(): the effect is that the issue reported in CVE-2020-29374 is currently still possible for anon THP: an unintended information leak from the parent to the child.
Let's apply the same logic (page_count() == 1), with similar optimizations to remove additional references first as we really want to avoid PTE-mapping the THP and copying individual pages best we can.
If we end up with a page that has page_count() != 1, we'll have to PTE-map the THP and fallback to do_wp_page(), which will always copy the page.
Note that KSM does not apply to THP.
I. Interaction with the swapcache and writeback
While a THP is in the swapcache, the swapcache holds one reference on each subpage of the THP. So with PageSwapCache() set, we expect as many additional references as we have subpages. If we manage to remove the THP from the swapcache, all these references will be gone.
Usually, a THP is not split when entered into the swapcache and stays a compound page. However, try_to_unmap() will PTE-map the THP and use PTE swap entries. There are no PMD swap entries for that purpose, consequently, we always only swapin subpages into PTEs.
Removing a page from the swapcache can fail either when there are remaining swap entries (in which case COW is the right thing to do) or if the page is currently under writeback.
Having a locked, R/O PMD-mapped THP that is in the swapcache seems to be possible only in corner cases, for example, if try_to_unmap() failed after adding the page to the swapcache. However, it's comparatively easy to handle.
As we have to fully unmap a THP before starting writeback, and swapin is always done on the PTE level, we shouldn't find a R/O PMD-mapped THP in the swapcache that is under writeback. This should at least leave writeback out of the picture.
II. Interaction with GUP references
Having a R/O PMD-mapped THP with GUP references (i.e., R/O references) will result in PTE-mapping the THP on a write fault. Similar to ordinary anon pages, do_wp_page() will have to copy sub-pages and result in a disconnect between the GUP references and the pages actually mapped into the page tables. To improve the situation in the future, we'll need additional handling to mark anonymous pages as definitely exclusive to a single process, only allow GUP pins on exclusive anon pages, and disallow sharing of exclusive anon pages with GUP pins e.g., during fork().
III. Interaction with references from LRU pagevecs
Similar to ordinary anon pages, we can have LRU pagevecs referencing our THP. Reliably removing such references requires draining LRU pagevecs on all CPUs -- lru_add_drain_all() -- a possibly expensive operation that can sleep. For now, similar do do_wp_page(), let's conditionally drain the local LRU pagevecs only if we detect !PageLRU().
IV. Interaction with speculative/temporary references
Similar to ordinary anon pages, other speculative/temporary references on the THP, for example, from the pagecache or page migration code, will disallow exclusive reuse of the page. We'll have to PTE-map the THP.
--- mm/huge_memory.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
mm/huge_memory.c +15 -4
@@ -1286,6 +1286,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)struct page *page;unsigned long haddr = vmf->address &HPAGE_PMD_MASK ;pmd_t orig_pmd = vmf->orig_pmd; +int swapcache_refs =0 ; vmf->ptl =pmd_lockptr (vma->vm_mm, vmf->pmd);VM_BUG_ON_VMA (!vma->anon_vma, vma);
@@ -1303,7 +1304,6 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf) page =pmd_page (orig_pmd);VM_BUG_ON_PAGE (!PageHead (page), page); -/* Lock page for reuse_swap_page() */ if (!trylock_page (page)) {get_page (page);spin_unlock (vmf->ptl);
@@ -1319,10 +1319,20 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf) }/* -* We can only reuse the page if nobody else maps the huge page or it's -* part. +* See do_wp_page(): we can only map the page writable if there are +* no additional references. */ -if (reuse_swap_page (page)) { +if (PageSwapCache (page)) + swapcache_refs =thp_nr_pages (page); +if (page_count (page) >1 + swapcache_refs + !PageLRU (page)) + goto unlock_fallback; +if (!PageLRU (page)) +lru_add_drain (); +if (page_count (page) >1 + swapcache_refs) + goto unlock_fallback; +if (swapcache_refs) +try_to_free_swap (page); +if (page_count (page) ==1 ) {pmd_t entry; entry =pmd_mkyoung (orig_pmd); entry =maybe_pmd_mkwrite (pmd_mkdirty (entry), vma);
@@ -1333,6 +1343,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)return VM_FAULT_WRITE ; } +unlock_fallback:unlock_page (page);spin_unlock (vmf->ptl); fallback:
Signature · 1 line
2.34.1
RFC PATCH v2 6/9 mm/khugepaged: remove reuse_swap_page() usage
In reply to David Hildenbrandreuse_swap_page() currently indicates if we can write to an anon page without COW. A COW is required if the page is shared by multiple processes (either already mapped or via swap entries) or if there is concurrent writeback that cannot tolerate concurrent page modifications.
reuse_swap_page() doesn't check for pending references from other processes that already unmapped the page, however, is_refcount_suitable() essentially does the same thing in the context of khugepaged. khugepaged is the last remaining user of reuse_swap_page() and we want to remove that function.
In the context of khugepaged, we are not actually going to write to the page and we don't really care about other processes mapping the page:
for example, without swap, we don't care about shared pages at all.
The current logic seems to be:
* Writable: -> Not shared, but might be in the swapcache. Nobody can fault it in from the swapcache as there are no other swap entries.
* Readable and not in the swapcache: Might be shared (but nobody can fault it in from the swapcache).
* Readable and in the swapcache: Might be shared and someone might be able to fault it in from the swapcache. Make sure we're the exclusive owner via reuse_swap_page().
Having to guess due to lack of comments and documentation, the current logic really only wants to make sure that a page that might be shared cannot be faulted in from the swapcache while khugepaged is active. It's hard to guess why that is that case and if it's really still required, but let's try keeping that logic unmodified.
Instead of relying on reuse_swap_page(), let's unconditionally try_to_free_swap(), special casing PageKsm(). try_to_free_swap() will fail if there are still swap entries targeting the page or if the page is under writeback.
After a successful try_to_free_swap() that page cannot be readded to the swapcache because we're keeping the page locked and removed from the LRU until we actually perform the copy. So once we succeeded removing a page from the swapcache, it cannot be re-added until we're done copying. Add a comment stating that.
--- mm/khugepaged.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
mm/khugepaged.c +13 -3
@@ -683,10 +683,10 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma, goto out; }if (!pte_write (pteval) &&PageSwapCache (page) && - !reuse_swap_page (page)) { + (PageKsm (page) || !try_to_free_swap (page))) {/* -* Page is in the swap cache and cannot be re-used. -* It cannot be collapsed into a THP. +* Possibly shared page cannot be removed from the +* swapache. It cannot be collapsed into a THP. */ unlock_page (page); result =SCAN_SWAP_CACHE_PAGE ;
@@ -702,6 +702,16 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma, result =SCAN_DEL_PAGE_LRU ; goto out; } + +/* +* We're holding the page lock and removed the page from the +* LRU. Once done copying, we'll unlock and readd to the +* LRU via release_pte_page(). If the page is still in the +* swapcache, we're the exclusive owner. Due to the page lock +* the page cannot be added to the swapcache until we're done +* and consequently it cannot be faulted in from the swapcache +* into another process. +*/ mod_node_page_state (page_pgdat (page),NR_ISOLATED_ANON +page_is_file_lru (page),compound_nr (page));
Signature · 1 line
2.34.1
RFC PATCH v2 7/9 mm/swapfile: remove reuse_swap_page()
In reply to David HildenbrandAll users are gone, let's remove it. We'll let SWP_STABLE_WRITES stick around for now, as it might come in handy in the near future.
--- include/linux/swap.h | 4 -- mm/swapfile.c | 104 ------------------------------------------- 2 files changed, 108 deletions(-)
include/linux/swap.h +0 -4
@@ -514,7 +514,6 @@ extern int __swp_swapcount(swp_entry_t entry);extern int swp_swapcount (swp_entry_t entry);extern struct swap_info_struct *page_swap_info (struct page *);extern struct swap_info_struct *swp_swap_info (swp_entry_t entry); -extern bool reuse_swap_page (struct page *);extern int try_to_free_swap (struct page *);struct backing_dev_info ;extern int init_swap_address_space (unsigned int type,unsigned long nr_pages);
@@ -680,9 +679,6 @@ static inline int swp_swapcount(swp_entry_t entry)return 0 ; } -#define reuse_swap_page (page) \ - (page_trans_huge_mapcount(page) == 1) -static inline int try_to_free_swap (struct page *page) {return 0 ;
mm/swapfile.c +0 -104
116 lines not on this page · raw message
Signature · 1 line
2.34.1
RFC PATCH v2 8/9 mm/huge_memory: remove stale page_trans_huge_mapcount()
In reply to David HildenbrandAll users are gone, let's remove it.
--- include/linux/mm.h | 5 ----- mm/huge_memory.c | 48 ---------------------------------------------- 2 files changed, 53 deletions(-)
include/linux/mm.h +0 -5
@@ -820,16 +820,11 @@ static inline int page_mapcount(struct page *page)#ifdef CONFIG_TRANSPARENT_HUGEPAGE int total_mapcount (struct page *page); -int page_trans_huge_mapcount (struct page *page);#else static inline int total_mapcount (struct page *page) {return page_mapcount (page); } -static inline int page_trans_huge_mapcount (struct page *page) -{ -return page_mapcount (page); -}#endif static inline struct page *virt_to_head_page (const void *x)
mm/huge_memory.c +0 -48
54 lines not on this page · raw message
Signature · 1 line
2.34.1
RFC PATCH v2 9/9 mm/huge_memory: remove stale locking logic from __split_huge_pmd()
In reply to David HildenbrandLet's remove the stale logic that was required for reuse_swap_page().
--- mm/huge_memory.c | 32 +------------------------------- 1 file changed, 1 insertion(+), 31 deletions(-)
mm/huge_memory.c +1 -31
@@ -2158,8 +2158,6 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, {spinlock_t *ptl;struct mmu_notifier_range range; -bool do_unlock_page = false; -pmd_t _pmd; mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm, address & HPAGE_PMD_MASK,
@@ -2178,35 +2176,9 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, goto out; } -repeat:if (pmd_trans_huge (*pmd)) { -if (!page) { +if (!page) page =pmd_page (*pmd); -/* -* An anonymous page must be locked, to ensure that a -* concurrent reuse_swap_page() sees stable mapcount; -* but reuse_swap_page() is not used on shmem or file, -* and page lock must not be taken when zap_pmd_range() -* calls __split_huge_pmd() while i_mmap_lock is held. -*/ -if (PageAnon (page)) { -if (unlikely (!trylock_page (page))) { -get_page (page); - _pmd = *pmd; -spin_unlock (ptl); -lock_page (page); -spin_lock (ptl); -if (unlikely (!pmd_same (*pmd, _pmd))) { -unlock_page (page); -put_page (page); - page =NULL ; - goto repeat; - } -put_page (page); - } - do_unlock_page = true; - } - }if (PageMlocked (page))clear_page_mlock (page); }else if (!(pmd_devmap (*pmd) ||is_pmd_migration_entry (*pmd)))
@@ -2214,8 +2186,6 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,__split_huge_pmd_locked (vma, pmd, range.start, freeze); out:spin_unlock (ptl); -if (do_unlock_page) -unlock_page (page);/* * No need to double call mmu_notifier->invalidate_range() callback. * They are 3 cases to consider inside __split_huge_pmd_locked():
Signature · 1 line
2.34.1
Re: [PATCH RFC v2 1/9] mm: optimize do_wp_page() for exclusive pages in the swapcache
In reply to David HildenbrandQuoted 25 lines · David Hildenbrand
Liang Zhang reported [1] that the current COW logic in
do_wp_page()is sub-optimal when it comes to swap+read fault+write fault of anonymous pages that have a single user, visible via a performance degradation in the redis benchmark. Something similar was previously reported [2] by Nadav with a simple reproducer.Let's optimize for pages that have been added to the swapcache but only have an exclusive owner. Try removing the swapcache reference if there is hope that we're the exclusive user.
We will fail removing the swapcache reference in two scenarios:
(1) There are additional swap entries referencing the page: copying instead of reusing is the right thing to do.
(2) The page is under writeback: theoretically we might be able to reuse in some cases, however, we cannot remove the additional reference and will have to copy.Further, we might have additional references from the LRU pagevecs, which will force us to copy instead of being able to reuse. We'll try handling such references for some scenarios next. Concurrent writeback cannot be handled easily and we'll always have to copy.
While at it, remove the superfluous
page_mapcount()check: it's implicitly covered by thepage_count()for ordinary anon pages.
Quoted 6 lines
[1] https://lkml.kernel.org/r/[email protected]
[2] https://lkml.kernel.org/r/[email protected]Reported-by Liang Zhang <[email protected]>Reported-by Nadav Amit <[email protected]>Signed-off-by David Hildenbrand <[email protected]>
Re: [PATCH RFC v2 2/9] mm: optimize do_wp_page() for fresh pages in local LRU pagevecs
In reply to David HildenbrandQuoted 11 lines · David Hildenbrand
mm/memory.c +2
-1@@ -3298,7 +3298,17 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)* * PageKsm() doesn't necessarily raise the page refcount. */ -if (PageKsm (page) ||page_count (page) >1 +PageSwapCache (page)) +if (PageKsm (page)) + goto copy;
mm/memory.c static vm_fault_t do_wp_page(struct vm_fault *vmf) · line 3312
Quoted 10 lines
mm/memory.c +9
-0+if (page_count (page) >1 +PageSwapCache (page) + !PageLRU (page)) + goto copy; +if (!PageLRU (page)) +/* +* Note: We cannot easily detect+handle references from +* remote LRU pagevecs or references to PageLRU() pages. +*/ +lru_add_drain (); +if (page_count (page) >1 +PageSwapCache (page)) goto copy;
I worry we're starting to get too accurate here. How about:
if (PageKsm(page) || page_count(page) > 3) goto copy; if (!PageLRU(page)) lru_add_drain(); if (!trylock_page(page)) goto copy;
...
Quoted 5 lines
mm/memory.c +0
-0if (!trylock_page (page)) goto copy;Signature · 1 line
2.34.1
Re: [PATCH RFC v2 2/9] mm: optimize do_wp_page() for fresh pages in local LRU pagevecs
Quoted 16 lines · Matthew Wilcox
Quoted 15 lines · David Hildenbrand
mm/memory.c +6
-1@@ -3298,7 +3298,17 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)* * PageKsm() doesn't necessarily raise the page refcount. */ -if (PageKsm (page) ||page_count (page) >1 +PageSwapCache (page)) +if (PageKsm (page)) + goto copy; +if (page_count (page) >1 +PageSwapCache (page) + !PageLRU (page)) + goto copy; +if (!PageLRU (page)) +/*
Quoted 9 lines
mm/memory.c static vm_fault_t do_wp_page(struct vm_fault *vmf) · line 3312
Quoted 6 lines
mm/memory.c +5
-0+* Note: We cannot easily detect+handle references from +* remote LRU pagevecs or references to PageLRU() pages. +*/ +lru_add_drain (); +if (page_count (page) >1 +PageSwapCache (page)) goto copy;I worry we're starting to get too accurate here. How about:
In that simplified case we'll essentially trylock_page() and for most pages that are ordinarily shared by e.g., 2 processes.
Quoted 8 lines
if (PageKsm(page) || page_count(page) > 3) goto copy; if (!PageLRU(page)) lru_add_drain(); if (!trylock_page(page)) goto copy;...
I think we might at least want the
if (page_count(page) > 1 + PageSwapCache(page))
goto copy;
check here.
Quoted 6 lines
Quoted 5 lines
mm/memory.c +0
-0if (!trylock_page (page)) goto copy;Signature · 1 line
2.34.1
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH RFC v2 5/9] mm/huge_memory: streamline COW logic in do_huge_pmd_wp_page()
In reply to David HildenbrandQuoted 98 lines · David Hildenbrand
We currently have a different COW logic for anon THP than we have for ordinary anon pages in
do_wp_page(): the effect is that the issue reported in CVE-2020-29374 is currently still possible for anon THP: an unintended information leak from the parent to the child.Let's apply the same logic (
page_count()== 1), with similar optimizations to remove additional references first as we really want to avoid PTE-mapping the THP and copying individual pages best we can.If we end up with a page that has
page_count()!= 1, we'll have to PTE-map the THP and fallback todo_wp_page(), which will always copy the page.Note that KSM does not apply to THP.
I. Interaction with the swapcache and writeback
While a THP is in the swapcache, the swapcache holds one reference on each subpage of the THP. So with
PageSwapCache()set, we expect as many additional references as we have subpages. If we manage to remove the THP from the swapcache, all these references will be gone.Usually, a THP is not split when entered into the swapcache and stays a compound page. However,
try_to_unmap()will PTE-map the THP and use PTE swap entries. There are no PMD swap entries for that purpose, consequently, we always only swapin subpages into PTEs.Removing a page from the swapcache can fail either when there are remaining swap entries (in which case COW is the right thing to do) or if the page is currently under writeback.
Having a locked, R/O PMD-mapped THP that is in the swapcache seems to be possible only in corner cases, for example, if
try_to_unmap()failed after adding the page to the swapcache. However, it's comparatively easy to handle.As we have to fully unmap a THP before starting writeback, and swapin is always done on the PTE level, we shouldn't find a R/O PMD-mapped THP in the swapcache that is under writeback. This should at least leave writeback out of the picture.
II. Interaction with GUP references
Having a R/O PMD-mapped THP with GUP references (i.e., R/O references) will result in PTE-mapping the THP on a write fault. Similar to ordinary anon pages,
do_wp_page()will have to copy sub-pages and result in a disconnect between the GUP references and the pages actually mapped into the page tables. To improve the situation in the future, we'll need additional handling to mark anonymous pages as definitely exclusive to a single process, only allow GUP pins on exclusive anon pages, and disallow sharing of exclusive anon pages with GUP pins e.g., duringfork().III. Interaction with references from LRU pagevecs
Similar to ordinary anon pages, we can have LRU pagevecs referencing our THP. Reliably removing such references requires draining LRU pagevecs on all CPUs --
lru_add_drain_all()-- a possibly expensive operation that can sleep. For now, similar dodo_wp_page(), let's conditionally drain the local LRU pagevecs only if we detect !PageLRU().IV. Interaction with speculative/temporary references
Similar to ordinary anon pages, other speculative/temporary references on the THP, for example, from the pagecache or page migration code, will disallow exclusive reuse of the page. We'll have to PTE-map the THP.
Signed-off-by David Hildenbrand <[email protected]>--- mm/huge_memory.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)mm/huge_memory.c +1
-3@@ -1286,6 +1286,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)struct page *page;unsigned long haddr = vmf->address &HPAGE_PMD_MASK ;pmd_t orig_pmd = vmf->orig_pmd; +int swapcache_refs =0 ; vmf->ptl =pmd_lockptr (vma->vm_mm, vmf->pmd);VM_BUG_ON_VMA (!vma->anon_vma, vma);@@ -1303,7 +1304,6 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf) page =pmd_page (orig_pmd);VM_BUG_ON_PAGE (!PageHead (page), page); -/* Lock page for reuse_swap_page() */ if (!trylock_page (page)) {get_page (page);spin_unlock (vmf->ptl);@@ -1319,10 +1319,20 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf) }/* -* We can only reuse the page if nobody else maps the huge page or it's -* part.
mm/huge_memory.c vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf) · line 1330
Quoted 10 lines
mm/huge_memory.c +8
-1+* See do_wp_page(): we can only map the page writable if there are +* no additional references. */ -if (reuse_swap_page (page)) { +if (PageSwapCache (page)) + swapcache_refs =thp_nr_pages (page); +if (page_count (page) >1 + swapcache_refs + !PageLRU (page)) + goto unlock_fallback; +if (!PageLRU (page)) +lru_add_drain ();
IMHO, draining lru doesn't help out too much for THP since THP will be drained to LRU immediately once it is added into pagevec.
Quoted 19 lines
mm/huge_memory.c +6
-1+if (page_count (page) >1 + swapcache_refs) + goto unlock_fallback; +if (swapcache_refs) +try_to_free_swap (page); +if (page_count (page) ==1 ) {pmd_t entry; entry =pmd_mkyoung (orig_pmd); entry =maybe_pmd_mkwrite (pmd_mkdirty (entry), vma);@@ -1333,6 +1343,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)return VM_FAULT_WRITE ; } +unlock_fallback:unlock_page (page);spin_unlock (vmf->ptl); fallback: --2.34.1
Re: [PATCH RFC v2 5/9] mm/huge_memory: streamline COW logic in do_huge_pmd_wp_page()
Quoted 96 lines · Yang Shi
Quoted 95 lines · David Hildenbrand
We currently have a different COW logic for anon THP than we have for ordinary anon pages in
do_wp_page(): the effect is that the issue reported in CVE-2020-29374 is currently still possible for anon THP: an unintended information leak from the parent to the child.Let's apply the same logic (
page_count()== 1), with similar optimizations to remove additional references first as we really want to avoid PTE-mapping the THP and copying individual pages best we can.If we end up with a page that has
page_count()!= 1, we'll have to PTE-map the THP and fallback todo_wp_page(), which will always copy the page.Note that KSM does not apply to THP.
I. Interaction with the swapcache and writeback
While a THP is in the swapcache, the swapcache holds one reference on each subpage of the THP. So with
PageSwapCache()set, we expect as many additional references as we have subpages. If we manage to remove the THP from the swapcache, all these references will be gone.Usually, a THP is not split when entered into the swapcache and stays a compound page. However,
try_to_unmap()will PTE-map the THP and use PTE swap entries. There are no PMD swap entries for that purpose, consequently, we always only swapin subpages into PTEs.Removing a page from the swapcache can fail either when there are remaining swap entries (in which case COW is the right thing to do) or if the page is currently under writeback.
Having a locked, R/O PMD-mapped THP that is in the swapcache seems to be possible only in corner cases, for example, if
try_to_unmap()failed after adding the page to the swapcache. However, it's comparatively easy to handle.As we have to fully unmap a THP before starting writeback, and swapin is always done on the PTE level, we shouldn't find a R/O PMD-mapped THP in the swapcache that is under writeback. This should at least leave writeback out of the picture.
II. Interaction with GUP references
Having a R/O PMD-mapped THP with GUP references (i.e., R/O references) will result in PTE-mapping the THP on a write fault. Similar to ordinary anon pages,
do_wp_page()will have to copy sub-pages and result in a disconnect between the GUP references and the pages actually mapped into the page tables. To improve the situation in the future, we'll need additional handling to mark anonymous pages as definitely exclusive to a single process, only allow GUP pins on exclusive anon pages, and disallow sharing of exclusive anon pages with GUP pins e.g., duringfork().III. Interaction with references from LRU pagevecs
Similar to ordinary anon pages, we can have LRU pagevecs referencing our THP. Reliably removing such references requires draining LRU pagevecs on all CPUs --
lru_add_drain_all()-- a possibly expensive operation that can sleep. For now, similar dodo_wp_page(), let's conditionally drain the local LRU pagevecs only if we detect !PageLRU().IV. Interaction with speculative/temporary references
Similar to ordinary anon pages, other speculative/temporary references on the THP, for example, from the pagecache or page migration code, will disallow exclusive reuse of the page. We'll have to PTE-map the THP.
Signed-off-by David Hildenbrand <[email protected]>--- mm/huge_memory.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)mm/huge_memory.c +1
-1@@ -1286,6 +1286,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)struct page *page;unsigned long haddr = vmf->address &HPAGE_PMD_MASK ;pmd_t orig_pmd = vmf->orig_pmd; +int swapcache_refs =0 ; vmf->ptl =pmd_lockptr (vma->vm_mm, vmf->pmd);VM_BUG_ON_VMA (!vma->anon_vma, vma);@@ -1303,7 +1304,6 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf) page =pmd_page (orig_pmd);VM_BUG_ON_PAGE (!PageHead (page), page); -/* Lock page for reuse_swap_page() */ if (!trylock_page (page)) {get_page (page);spin_unlock (vmf->ptl);@@ -1319,10 +1319,20 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf) }
Quoted 16 lines
mm/huge_memory.c vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf) · line 1330
Quoted 13 lines
mm/huge_memory.c +8
-3/* -* We can only reuse the page if nobody else maps the huge page or it's -* part. +* See do_wp_page(): we can only map the page writable if there are +* no additional references. */ -if (reuse_swap_page (page)) { +if (PageSwapCache (page)) + swapcache_refs =thp_nr_pages (page); +if (page_count (page) >1 + swapcache_refs + !PageLRU (page)) + goto unlock_fallback; +if (!PageLRU (page)) +lru_add_drain ();IMHO, draining lru doesn't help out too much for THP since THP will be drained to LRU immediately once it is added into pagevec.
Oh, thanks, I think you're right. The interesting bit is
static bool pagevec_add_and_need_flush(struct pagevec *pvec, struct page *page)
{
bool ret = false;
if (!pagevec_add(pvec, page) || PageCompound(page) ||
lru_cache_disabled())
ret = true;
return ret;
}Which indeed drains after adding it to the pagevec.
Will adjust the patch and update the description/comment accordingly, thanks!
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH RFC v2 6/9] mm/khugepaged: remove reuse_swap_page() usage
In reply to David HildenbrandQuoted 25 lines · David Hildenbrand
reuse_swap_page()currently indicates if we can write to an anon page without COW. A COW is required if the page is shared by multiple processes (either already mapped or via swap entries) or if there is concurrent writeback that cannot tolerate concurrent page modifications.
reuse_swap_page()doesn't check for pending references from other processes that already unmapped the page, however,is_refcount_suitable()essentially does the same thing in the context of khugepaged. khugepaged is the last remaining user ofreuse_swap_page()and we want to remove that function.In the context of khugepaged, we are not actually going to write to the page and we don't really care about other processes mapping the page:
for example, without swap, we don't care about shared pages at all.The current logic seems to be:
* Writable: -> Not shared, but might be in the swapcache. Nobody can fault it in from the swapcache as there are no other swap entries.
* Readable and not in the swapcache: Might be shared (but nobody can fault it in from the swapcache).
* Readable and in the swapcache: Might be shared and someone might be able to fault it in from the swapcache. Make sure we're the exclusive owner viareuse_swap_page().
Quoted 5 lines
Having to guess due to lack of comments and documentation, the current logic really only wants to make sure that a page that might be shared cannot be faulted in from the swapcache while khugepaged is active. It's hard to guess why that is that case and if it's really still required, but let's try keeping that logic unmodified.
I don't think it could be faulted in while khugepaged is active since khugepaged does hold mmap_lock in write mode IIUC. So page fault is serialized against khugepaged.
My wild guess is that collapsing shared pages was not supported before v5.8, so we need reuse_swap_page() to tell us if the page in swap cache is shared or not. But it is not true anymore. And khugepaged just allocates a THP then copy the data from base pages to huge page then replace PTEs to PMD, it doesn't change the content of the page, so I failed to see a problem by collapsing a shared page in swap cache. But I'm really not entirely sure, I may miss something...
Quoted 55 lines
Instead of relying on
reuse_swap_page(), let's unconditionallytry_to_free_swap(), special casingPageKsm().try_to_free_swap()will fail if there are still swap entries targeting the page or if the page is under writeback.After a successful
try_to_free_swap()that page cannot be readded to the swapcache because we're keeping the page locked and removed from the LRU until we actually perform the copy. So once we succeeded removing a page from the swapcache, it cannot be re-added until we're done copying. Add a comment stating that.Signed-off-by David Hildenbrand <[email protected]>--- mm/khugepaged.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)mm/khugepaged.c +13
-4@@ -683,10 +683,10 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma, goto out; }if (!pte_write (pteval) &&PageSwapCache (page) && - !reuse_swap_page (page)) { + (PageKsm (page) || !try_to_free_swap (page))) {/* -* Page is in the swap cache and cannot be re-used. -* It cannot be collapsed into a THP. +* Possibly shared page cannot be removed from the +* swapache. It cannot be collapsed into a THP. */ unlock_page (page); result =SCAN_SWAP_CACHE_PAGE ;@@ -702,6 +702,16 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma, result =SCAN_DEL_PAGE_LRU ; goto out; } + +/* +* We're holding the page lock and removed the page from the +* LRU. Once done copying, we'll unlock and readd to the +* LRU via release_pte_page(). If the page is still in the +* swapcache, we're the exclusive owner. Due to the page lock +* the page cannot be added to the swapcache until we're done +* and consequently it cannot be faulted in from the swapcache +* into another process. +*/ mod_node_page_state (page_pgdat (page),NR_ISOLATED_ANON +page_is_file_lru (page),compound_nr (page)); --2.34.1
Re: [PATCH RFC v2 6/9] mm/khugepaged: remove reuse_swap_page() usage
Quoted 32 lines · Yang Shi
Quoted 25 lines · David Hildenbrand
reuse_swap_page()currently indicates if we can write to an anon page without COW. A COW is required if the page is shared by multiple processes (either already mapped or via swap entries) or if there is concurrent writeback that cannot tolerate concurrent page modifications.
reuse_swap_page()doesn't check for pending references from other processes that already unmapped the page, however,is_refcount_suitable()essentially does the same thing in the context of khugepaged. khugepaged is the last remaining user ofreuse_swap_page()and we want to remove that function.In the context of khugepaged, we are not actually going to write to the page and we don't really care about other processes mapping the page:
for example, without swap, we don't care about shared pages at all.The current logic seems to be:
* Writable: -> Not shared, but might be in the swapcache. Nobody can fault it in from the swapcache as there are no other swap entries.
* Readable and not in the swapcache: Might be shared (but nobody can fault it in from the swapcache).
* Readable and in the swapcache: Might be shared and someone might be able to fault it in from the swapcache. Make sure we're the exclusive owner viareuse_swap_page().Quoted 5 lines
Having to guess due to lack of comments and documentation, the current logic really only wants to make sure that a page that might be shared cannot be faulted in from the swapcache while khugepaged is active. It's hard to guess why that is that case and if it's really still required, but let's try keeping that logic unmodified.
Quoted 3 lines
I don't think it could be faulted in while khugepaged is active since khugepaged does hold
mmap_lockin write mode IIUC. So page fault is serialized against khugepaged.
It could get faulted in by another process sharing the page, because we only synchronize against the current process.
Quoted 8 lines
My wild guess is that collapsing shared pages was not supported before v5.8, so we need
reuse_swap_page()to tell us if the page in swap cache is shared or not. But it is not true anymore. And khugepaged just allocates a THP then copy the data from base pages to huge page then replace PTEs to PMD, it doesn't change the content of the page, so I failed to see a problem by collapsing a shared page in swap cache. But I'm really not entirely sure, I may miss something...
Looking more closely where this logic originates from, it was introduced in:
commit 10359213d05acf804558bda7cc9b8422a828d1cd
Author: Ebru Akagunduz <[email protected]> Date: Wed Feb 11 15:28:28 2015 -0800
mm: incorporate read-only pages into transparent huge pages
This patch aims to improve THP collapse rates, by allowing THP collapse in the presence of read-only ptes, like those left in place by do_swap_page after a read fault.
Currently THP can collapse 4kB pages into a THP when there are up to khugepaged_max_ptes_none pte_none ptes in a 2MB range. This patch applies the same limit for read-only ptes.
The change essentially results in a read-only mapped PTE page getting copied and mapped writable via a new PMD-mapped THP.
It mentions do_swap_page(), so I assume it just tried to do what do_swap_page() would do when trying to map a page swapped in from the page cache writable immediately.
But we differ from do_swap_page() that we're not actually going to map the page writable, we're going to copy the page (__collapse_huge_page_copy()) and map the copy writable.
I assume we can remove that logic.
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH RFC v2 1/9] mm: optimize do_wp_page() for exclusive pages in the swapcache
In reply to David HildenbrandQuoted 5 lines · David Hildenbrand
Liang Zhang reported [1] that the current COW logic in
do_wp_page()is sub-optimal when it comes to swap+read fault+write fault of anonymous pages that have a single user, visible via a performance degradation in the redis benchmark. Something similar was previously reported [2] by Nadav with a simple reproducer.
Can we make the description more self-contained? I.e. describe that sub-optimal COW means we copy when it's not necessary, and this can happen if swap-out is followed by a swap-in for read and a then a write fault (IIUC), because the swap cache reference increases page_count()...
Quoted 3 lines
Let's optimize for pages that have been added to the swapcache but only have an exclusive owner. Try removing the swapcache reference if there is hope that we're the exclusive user.
Can we expect any downside for reclaim efficiency due to the more aggressive removal from swapcache? Probably not, as we are doing the removal when the page is about to get dirty, so we wouldn't be able to reuse any previously swapped out content anyway. Maybe it's even beneficial?
Quoted 15 lines
We will fail removing the swapcache reference in two scenarios:
(1) There are additional swap entries referencing the page: copying instead of reusing is the right thing to do.
(2) The page is under writeback: theoretically we might be able to reuse in some cases, however, we cannot remove the additional reference and will have to copy.Further, we might have additional references from the LRU pagevecs, which will force us to copy instead of being able to reuse. We'll try handling such references for some scenarios next. Concurrent writeback cannot be handled easily and we'll always have to copy.
While at it, remove the superfluous
page_mapcount()check: it's implicitly covered by thepage_count()for ordinary anon pages.
Quoted 6 lines
[1] https://lkml.kernel.org/r/[email protected]
[2] https://lkml.kernel.org/r/[email protected]Reported-by Liang Zhang <[email protected]>Reported-by Nadav Amit <[email protected]>Signed-off-by David Hildenbrand <[email protected]>
Quoted 42 lines
--- mm/memory.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)mm/memory.c +14
-6@@ -3291,19 +3291,27 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)if (PageAnon (vmf->page)) {struct page *page = vmf->page; -/* PageKsm() doesn't necessarily raise the page refcount */ -if (PageKsm (page) ||page_count (page) !=1 ) +/* +* We have to verify under page lock: these early checks are +* just an optimization to avoid locking the page and freeing +* the swapcache if there is little hope that we can reuse. +* +* PageKsm() doesn't necessarily raise the page refcount. +*/ +if (PageKsm (page) ||page_count (page) >1 +PageSwapCache (page)) goto copy;if (!trylock_page (page)) goto copy; -if (PageKsm (page) ||page_mapcount (page) !=1 ||page_count (page) !=1 ) { +if (PageSwapCache (page)) +try_to_free_swap (page); +if (PageKsm (page) ||page_count (page) !=1 ) {unlock_page (page); goto copy; }/* -* Ok, we've got the only map reference, and the only -* page count reference, and the page is locked, -* it's dark out, and we're wearing sunglasses. Hit it. +* Ok, we've got the only page reference from our mapping +* and the page is locked, it's dark out, and we're wearing +* sunglasses. Hit it. */ unlock_page (page);wp_page_reuse (vmf);
Re: [PATCH RFC v2 1/9] mm: optimize do_wp_page() for exclusive pages in the swapcache
Quoted 11 lines · Vlastimil Babka
Quoted 5 lines · David Hildenbrand
Liang Zhang reported [1] that the current COW logic in
do_wp_page()is sub-optimal when it comes to swap+read fault+write fault of anonymous pages that have a single user, visible via a performance degradation in the redis benchmark. Something similar was previously reported [2] by Nadav with a simple reproducer.Can we make the description more self-contained? I.e. describe that sub-optimal COW means we copy when it's not necessary, and this can happen if swap-out is followed by a swap-in for read and a then a write fault (IIUC), because the swap cache reference increases
page_count()..
Sure, I can add some more details.
Quoted 9 lines
Quoted 4 lines
Let's optimize for pages that have been added to the swapcache but only have an exclusive owner. Try removing the swapcache reference if there is hope that we're the exclusive user.
Can we expect any downside for reclaim efficiency due to the more aggressive removal from swapcache? Probably not, as we are doing the removal when the page is about to get dirty, so we wouldn't be able to reuse any previously swapped out content anyway. Maybe it's even beneficial?
We're only try removing the swapcache reference if it's likely that we succeed in reusing the page after we succeeded in removing the swapcache reference (IOW, no other swap entries, no writeback).
So the early "page_count(page) > 1 + PageSwapCache(page)" check is of big value I think. It should be rare that we remove the reference and still end up with "page_count(page) != 1".
And if we're reusing the page (re-dirtying it), removing it from the swapache is the right thing to do: reuse_swap_cache() would have done the same.
[...]
Quoted 6 lines
Quoted 4 lines
Reported-by Liang Zhang <[email protected]>Reported-by Nadav Amit <[email protected]>Signed-off-by David Hildenbrand <[email protected]>Acked-by Vlastimil Babka <[email protected]>
Thanks!
Signature · 3 lines
Thanks, David / dhildenb
Re: [PATCH RFC v2 6/9] mm/khugepaged: remove reuse_swap_page() usage
In reply to David HildenbrandQuoted 34 lines · David Hildenbrand
Quoted 32 lines · Yang Shi
Quoted 30 lines · David Hildenbrand
reuse_swap_page()currently indicates if we can write to an anon page without COW. A COW is required if the page is shared by multiple processes (either already mapped or via swap entries) or if there is concurrent writeback that cannot tolerate concurrent page modifications.
reuse_swap_page()doesn't check for pending references from other processes that already unmapped the page, however,is_refcount_suitable()essentially does the same thing in the context of khugepaged. khugepaged is the last remaining user ofreuse_swap_page()and we want to remove that function.In the context of khugepaged, we are not actually going to write to the page and we don't really care about other processes mapping the page:
for example, without swap, we don't care about shared pages at all.The current logic seems to be:
* Writable: -> Not shared, but might be in the swapcache. Nobody can fault it in from the swapcache as there are no other swap entries.
* Readable and not in the swapcache: Might be shared (but nobody can fault it in from the swapcache).
* Readable and in the swapcache: Might be shared and someone might be able to fault it in from the swapcache. Make sure we're the exclusive owner viareuse_swap_page().Having to guess due to lack of comments and documentation, the current logic really only wants to make sure that a page that might be shared cannot be faulted in from the swapcache while khugepaged is active. It's hard to guess why that is that case and if it's really still required, but let's try keeping that logic unmodified.
Quoted 6 lines
Quoted 3 lines
I don't think it could be faulted in while khugepaged is active since khugepaged does hold
mmap_lockin write mode IIUC. So page fault is serialized against khugepaged.It could get faulted in by another process sharing the page, because we only synchronize against the current process.
Yes, sure. I'm supposed you meant do_swap_page() called by another process. But it is serialized by page lock. So khugepaged won't see something in limbo state IIUC.
Quoted 31 lines
Quoted 9 lines
My wild guess is that collapsing shared pages was not supported before v5.8, so we need
reuse_swap_page()to tell us if the page in swap cache is shared or not. But it is not true anymore. And khugepaged just allocates a THP then copy the data from base pages to huge page then replace PTEs to PMD, it doesn't change the content of the page, so I failed to see a problem by collapsing a shared page in swap cache. But I'm really not entirely sure, I may miss something...Looking more closely where this logic originates from, it was introduced in:
commit
10359213d05acf804558bda7cc9b8422a828d1cdAuthor: Ebru Akagunduz <[email protected]> Date: Wed Feb 11 15:28:28 2015 -0800mm: incorporate read-only pages into transparent huge pages
This patch aims to improve THP collapse rates, by allowing THP collapse in the presence of read-only ptes, like those left in place by
do_swap_pageafter a read fault.Currently THP can collapse 4kB pages into a THP when there are up to
khugepaged_max_ptes_nonepte_noneptes in a 2MB range. This patch applies the same limit for read-only ptes.The change essentially results in a read-only mapped PTE page getting copied and mapped writable via a new PMD-mapped THP.
Quoted 7 lines
It mentions
do_swap_page(), so I assume it just tried to do whatdo_swap_page()would do when trying to map a page swapped in from the page cache writable immediately.But we differ from
do_swap_page()that we're not actually going to map the page writable, we're going to copy the page (__collapse_huge_page_copy()) and map the copy writable.
Yeah, this is the point. Khugepaged or the process being collapsed won't write to the original page. Just unshare it.
Quoted 8 lines
I assume we can remove that logic.
--
Thanks,David / dhildenb