We received a report that the copy-on-write issue repored by Jann Horn in
https://bugs.chromium.org/p/project-zero/issues/detail?id=2045 is still
reproducible on 4.14 and 4.19 kernels (the first issue with the reproducer
coded in vmsplice.c). I confirmed this and also that the issue was not
reproducible with 5.10 kernel. I tracked the fix to the following patch
introduced in 5.9 which changes the do_wp_page() logic:
09854ba94c6a 'mm: do_wp_page() simplification'
I backported this patch (#2 in the series) along with 2 prerequisite patches
(#1 and #4) that keep the backports clean and two followup fixes to the main
patch (#3 and #5). I had to skip the following fix:
feb889fb40fa 'mm: don't put pinned pages into the swap cache'
because it uses page_maybe_dma_pinned() which does not exists in earlier
kernels. Because pin_user_pages() does not exist there as well, I *think*
we can safely skip this fix on older kernels, but I would appreciate if
someone could confirm that claim.
The patchset cleanly applies over: stable linux-4.14.y, tag: v4.14.228
Note: 4.14 and 4.19 backports are very similar, so while I backported
only to these two versions I think backports for other versions can be
done easily.
Kirill Tkhai (1):
mm: reuse only-pte-mapped KSM page in do_wp_page()
Linus Torvalds (2):
mm: do_wp_page() simplification
mm: fix misplaced unlock_page in do_wp_page()
Nadav Amit (1):
mm/userfaultfd: fix memory corruption due to writeprotect
Shaohua Li (1):
userfaultfd: wp: add helper for writeprotect check
include/linux/ksm.h | 7 ++++
include/linux/userfaultfd_k.h | 10 ++++++
mm/ksm.c | 30 ++++++++++++++++--
mm/memory.c | 60 ++++++++++++++++-------------------
4 files changed, 73 insertions(+), 34 deletions(-)
--
2.31.0.291.g576ba9dcdaf-goog
From: Kirill Tkhai <[email protected]>
Add an optimization for KSM pages almost in the same way that we have
for ordinary anonymous pages. If there is a write fault in a page,
which is mapped to an only pte, and it is not related to swap cache; the
page may be reused without copying its content.
[ Note that we do not consider PageSwapCache() pages at least for now,
since we don't want to complicate __get_ksm_page(), which has nice
optimization based on this (for the migration case). Currenly it is
spinning on PageSwapCache() pages, waiting for when they have
unfreezed counters (i.e., for the migration finish). But we don't want
to make it also spinning on swap cache pages, which we try to reuse,
since there is not a very high probability to reuse them. So, for now
we do not consider PageSwapCache() pages at all. ]
So in reuse_ksm_page() we check for 1) PageSwapCache() and 2)
page_stable_node(), to skip a page, which KSM is currently trying to
link to stable tree. Then we do page_ref_freeze() to prohibit KSM to
merge one more page into the page, we are reusing. After that, nobody
can refer to the reusing page: KSM skips !PageSwapCache() pages with
zero refcount; and the protection against of all other participants is
the same as for reused ordinary anon pages pte lock, page lock and
mmap_sem.
[[email protected]: replace BUG_ON()s with WARN_ON()s]
Link: http://lkml.kernel.org/r/154471491016.31352.1168978849911555609.stgit@localhost.localdomain
Signed-off-by: Kirill Tkhai <[email protected]>
Reviewed-by: Yang Shi <[email protected]>
Cc: "Kirill A. Shutemov" <[email protected]>
Cc: Hugh Dickins <[email protected]>
Cc: Andrea Arcangeli <[email protected]>
Cc: Christian Koenig <[email protected]>
Cc: Claudio Imbrenda <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: Huang Ying <[email protected]>
Cc: Minchan Kim <[email protected]>
Cc: Kirill Tkhai <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
---
include/linux/ksm.h | 7 +++++++
mm/ksm.c | 30 ++++++++++++++++++++++++++++--
mm/memory.c | 16 ++++++++++++++--
3 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/include/linux/ksm.h b/include/linux/ksm.h
index 44368b19b27e..def48a2d87aa 100644
--- a/include/linux/ksm.h
+++ b/include/linux/ksm.h
@@ -64,6 +64,8 @@ struct page *ksm_might_need_to_copy(struct page *page,
void rmap_walk_ksm(struct page *page, struct rmap_walk_control *rwc);
void ksm_migrate_page(struct page *newpage, struct page *oldpage);
+bool reuse_ksm_page(struct page *page,
+ struct vm_area_struct *vma, unsigned long address);
#else /* !CONFIG_KSM */
@@ -103,6 +105,11 @@ static inline void rmap_walk_ksm(struct page *page,
static inline void ksm_migrate_page(struct page *newpage, struct page *oldpage)
{
}
+static inline bool reuse_ksm_page(struct page *page,
+ struct vm_area_struct *vma, unsigned long address)
+{
+ return false;
+}
#endif /* CONFIG_MMU */
#endif /* !CONFIG_KSM */
diff --git a/mm/ksm.c b/mm/ksm.c
index 65d4bf52f543..62419735ee9c 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -695,8 +695,9 @@ static struct page *get_ksm_page(struct stable_node *stable_node, bool lock_it)
* case this node is no longer referenced, and should be freed;
* however, it might mean that the page is under page_freeze_refs().
* The __remove_mapping() case is easy, again the node is now stale;
- * but if page is swapcache in migrate_page_move_mapping(), it might
- * still be our page, in which case it's essential to keep the node.
+ * the same is in reuse_ksm_page() case; but if page is swapcache
+ * in migrate_page_move_mapping(), it might still be our page,
+ * in which case it's essential to keep the node.
*/
while (!get_page_unless_zero(page)) {
/*
@@ -2609,6 +2610,31 @@ void rmap_walk_ksm(struct page *page, struct rmap_walk_control *rwc)
goto again;
}
+bool reuse_ksm_page(struct page *page,
+ struct vm_area_struct *vma,
+ unsigned long address)
+{
+#ifdef CONFIG_DEBUG_VM
+ if (WARN_ON(is_zero_pfn(page_to_pfn(page))) ||
+ WARN_ON(!page_mapped(page)) ||
+ WARN_ON(!PageLocked(page))) {
+ dump_page(page, "reuse_ksm_page");
+ return false;
+ }
+#endif
+
+ if (PageSwapCache(page) || !page_stable_node(page))
+ return false;
+ /* Prohibit parallel get_ksm_page() */
+ if (!page_ref_freeze(page, 1))
+ return false;
+
+ page_move_anon_rmap(page, vma);
+ page->index = linear_page_index(vma, address);
+ page_ref_unfreeze(page, 1);
+
+ return true;
+}
#ifdef CONFIG_MIGRATION
void ksm_migrate_page(struct page *newpage, struct page *oldpage)
{
diff --git a/mm/memory.c b/mm/memory.c
index 21a0bbb9c21f..6920bfb3f89c 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2831,8 +2831,11 @@ static int do_wp_page(struct vm_fault *vmf)
* Take out anonymous pages first, anonymous shared vmas are
* not dirty accountable.
*/
- if (PageAnon(vmf->page) && !PageKsm(vmf->page)) {
+ if (PageAnon(vmf->page)) {
int total_map_swapcount;
+ if (PageKsm(vmf->page) && (PageSwapCache(vmf->page) ||
+ page_count(vmf->page) != 1))
+ goto copy;
if (!trylock_page(vmf->page)) {
get_page(vmf->page);
pte_unmap_unlock(vmf->pte, vmf->ptl);
@@ -2847,6 +2850,15 @@ static int do_wp_page(struct vm_fault *vmf)
}
put_page(vmf->page);
}
+ if (PageKsm(vmf->page)) {
+ bool reused = reuse_ksm_page(vmf->page, vmf->vma,
+ vmf->address);
+ unlock_page(vmf->page);
+ if (!reused)
+ goto copy;
+ wp_page_reuse(vmf);
+ return VM_FAULT_WRITE;
+ }
if (reuse_swap_page(vmf->page, &total_map_swapcount)) {
if (total_map_swapcount == 1) {
/*
@@ -2867,7 +2879,7 @@ static int do_wp_page(struct vm_fault *vmf)
(VM_WRITE|VM_SHARED))) {
return wp_page_shared(vmf);
}
-
+copy:
/*
* Ok, we need to copy. Oh, well..
*/
--
2.31.0.291.g576ba9dcdaf-goog
From: Nadav Amit <[email protected]>
Userfaultfd self-test fails occasionally, indicating a memory corruption.
Analyzing this problem indicates that there is a real bug since mmap_lock
is only taken for read in mwriteprotect_range() and defers flushes, and
since there is insufficient consideration of concurrent deferred TLB
flushes in wp_page_copy(). Although the PTE is flushed from the TLBs in
wp_page_copy(), this flush takes place after the copy has already been
performed, and therefore changes of the page are possible between the time
of the copy and the time in which the PTE is flushed.
To make matters worse, memory-unprotection using userfaultfd also poses a
problem. Although memory unprotection is logically a promotion of PTE
permissions, and therefore should not require a TLB flush, the current
userrfaultfd code might actually cause a demotion of the architectural PTE
permission: when userfaultfd_writeprotect() unprotects memory region, it
unintentionally *clears* the RW-bit if it was already set. Note that this
unprotecting a PTE that is not write-protected is a valid use-case: the
userfaultfd monitor might ask to unprotect a region that holds both
write-protected and write-unprotected PTEs.
The scenario that happens in selftests/vm/userfaultfd is as follows:
cpu0 cpu1 cpu2
---- ---- ----
[ Writable PTE
cached in TLB ]
userfaultfd_writeprotect()
[ write-*unprotect* ]
mwriteprotect_range()
mmap_read_lock()
change_protection()
change_protection_range()
...
change_pte_range()
[ *clear* “write”-bit ]
[ defer TLB flushes ]
[ page-fault ]
...
wp_page_copy()
cow_user_page()
[ copy page ]
[ write to old
page ]
...
set_pte_at_notify()
A similar scenario can happen:
cpu0 cpu1 cpu2 cpu3
---- ---- ---- ----
[ Writable PTE
cached in TLB ]
userfaultfd_writeprotect()
[ write-protect ]
[ deferred TLB flush ]
userfaultfd_writeprotect()
[ write-unprotect ]
[ deferred TLB flush]
[ page-fault ]
wp_page_copy()
cow_user_page()
[ copy page ]
... [ write to page ]
set_pte_at_notify()
This race exists since commit 292924b26024 ("userfaultfd: wp: apply
_PAGE_UFFD_WP bit"). Yet, as Yu Zhao pointed, these races became apparent
since commit 09854ba94c6a ("mm: do_wp_page() simplification") which made
wp_page_copy() more likely to take place, specifically if page_count(page)
> 1.
To resolve the aforementioned races, check whether there are pending
flushes on uffd-write-protected VMAs, and if there are, perform a flush
before doing the COW.
Further optimizations will follow to avoid during uffd-write-unprotect
unnecassary PTE write-protection and TLB flushes.
Link: https://lkml.kernel.org/r/[email protected]
Fixes: 09854ba94c6a ("mm: do_wp_page() simplification")
Signed-off-by: Nadav Amit <[email protected]>
Suggested-by: Yu Zhao <[email protected]>
Reviewed-by: Peter Xu <[email protected]>
Tested-by: Peter Xu <[email protected]>
Cc: Andrea Arcangeli <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Pavel Emelyanov <[email protected]>
Cc: Mike Kravetz <[email protected]>
Cc: Mike Rapoport <[email protected]>
Cc: Minchan Kim <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: <[email protected]> [5.9+]
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
---
mm/memory.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/mm/memory.c b/mm/memory.c
index 14470ceaf3f2..3f33651a2a39 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2810,6 +2810,14 @@ static int do_wp_page(struct vm_fault *vmf)
{
struct vm_area_struct *vma = vmf->vma;
+ /*
+ * Userfaultfd write-protect can defer flushes. Ensure the TLB
+ * is flushed in this case before copying.
+ */
+ if (unlikely(userfaultfd_wp(vmf->vma) &&
+ mm_tlb_flush_pending(vmf->vma->vm_mm)))
+ flush_tlb_page(vmf->vma, vmf->address);
+
vmf->page = vm_normal_page(vma, vmf->address, vmf->orig_pte);
if (!vmf->page) {
/*
--
2.31.0.291.g576ba9dcdaf-goog
From: Linus Torvalds <[email protected]>
How about we just make sure we're the only possible valid user fo the
page before we bother to reuse it?
Simplify, simplify, simplify.
And get rid of the nasty serialization on the page lock at the same time.
[peterx: add subject prefix]
Signed-off-by: Linus Torvalds <[email protected]>
Signed-off-by: Peter Xu <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
---
mm/memory.c | 58 ++++++++++++++++-------------------------------------
1 file changed, 17 insertions(+), 41 deletions(-)
diff --git a/mm/memory.c b/mm/memory.c
index 6920bfb3f89c..e84648d81d6d 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2832,49 +2832,25 @@ static int do_wp_page(struct vm_fault *vmf)
* not dirty accountable.
*/
if (PageAnon(vmf->page)) {
- int total_map_swapcount;
- if (PageKsm(vmf->page) && (PageSwapCache(vmf->page) ||
- page_count(vmf->page) != 1))
+ struct page *page = vmf->page;
+
+ /* PageKsm() doesn't necessarily raise the page refcount */
+ if (PageKsm(page) || page_count(page) != 1)
+ goto copy;
+ if (!trylock_page(page))
+ goto copy;
+ if (PageKsm(page) || page_mapcount(page) != 1 || page_count(page) != 1) {
+ unlock_page(page);
goto copy;
- if (!trylock_page(vmf->page)) {
- get_page(vmf->page);
- pte_unmap_unlock(vmf->pte, vmf->ptl);
- lock_page(vmf->page);
- vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
- vmf->address, &vmf->ptl);
- if (!pte_same(*vmf->pte, vmf->orig_pte)) {
- unlock_page(vmf->page);
- pte_unmap_unlock(vmf->pte, vmf->ptl);
- put_page(vmf->page);
- return 0;
- }
- put_page(vmf->page);
- }
- if (PageKsm(vmf->page)) {
- bool reused = reuse_ksm_page(vmf->page, vmf->vma,
- vmf->address);
- unlock_page(vmf->page);
- if (!reused)
- goto copy;
- wp_page_reuse(vmf);
- return VM_FAULT_WRITE;
- }
- if (reuse_swap_page(vmf->page, &total_map_swapcount)) {
- if (total_map_swapcount == 1) {
- /*
- * The page is all ours. Move it to
- * our anon_vma so the rmap code will
- * not search our parent or siblings.
- * Protected against the rmap code by
- * the page lock.
- */
- page_move_anon_rmap(vmf->page, vma);
- }
- unlock_page(vmf->page);
- wp_page_reuse(vmf);
- return VM_FAULT_WRITE;
}
- unlock_page(vmf->page);
+ /*
+ * 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.
+ */
+ wp_page_reuse(vmf);
+ unlock_page(page);
+ return VM_FAULT_WRITE;
} else if (unlikely((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
(VM_WRITE|VM_SHARED))) {
return wp_page_shared(vmf);
--
2.31.0.291.g576ba9dcdaf-goog
From: Shaohua Li <[email protected]>
Patch series "userfaultfd: write protection support", v6.
Overview
========
The uffd-wp work was initialized by Shaohua Li [1], and later continued by
Andrea [2]. This series is based upon Andrea's latest userfaultfd tree,
and it is a continuous works from both Shaohua and Andrea. Many of the
follow up ideas come from Andrea too.
Besides the old MISSING register mode of userfaultfd, the new uffd-wp
support provides another alternative register mode called
UFFDIO_REGISTER_MODE_WP that can be used to listen to not only missing
page faults but also write protection page faults, or even they can be
registered together. At the same time, the new feature also provides a
new userfaultfd ioctl called UFFDIO_WRITEPROTECT which allows the
userspace to write protect a range or memory or fixup write permission of
faulted pages.
Please refer to the document patch "userfaultfd: wp:
UFFDIO_REGISTER_MODE_WP documentation update" for more information on the
new interface and what it can do.
The major workflow of an uffd-wp program should be:
1. Register a memory region with WP mode using UFFDIO_REGISTER_MODE_WP
2. Write protect part of the whole registered region using
UFFDIO_WRITEPROTECT, passing in UFFDIO_WRITEPROTECT_MODE_WP to
show that we want to write protect the range.
3. Start a working thread that modifies the protected pages,
meanwhile listening to UFFD messages.
4. When a write is detected upon the protected range, page fault
happens, a UFFD message will be generated and reported to the
page fault handling thread
5. The page fault handler thread resolves the page fault using the
new UFFDIO_WRITEPROTECT ioctl, but this time passing in
!UFFDIO_WRITEPROTECT_MODE_WP instead showing that we want to
recover the write permission. Before this operation, the fault
handler thread can do anything it wants, e.g., dumps the page to
a persistent storage.
6. The worker thread will continue running with the correctly
applied write permission from step 5.
Currently there are already two projects that are based on this new
userfaultfd feature.
QEMU Live Snapshot: The project provides a way to allow the QEMU
hypervisor to take snapshot of VMs without
stopping the VM [3].
LLNL umap library: The project provides a mmap-like interface and
"allow to have an application specific buffer of
pages cached from a large file, i.e. out-of-core
execution using memory map" [4][5].
Before posting the patchset, this series was smoke tested against QEMU
live snapshot and the LLNL umap library (by doing parallel quicksort using
128 sorting threads + 80 uffd servicing threads). My sincere thanks to
Marty Mcfadden and Denis Plotnikov for the help along the way.
TODO
====
- hugetlbfs/shmem support
- performance
- more architectures
- cooperate with mprotect()-allowed processes (???)
- ...
References
==========
[1] https://lwn.net/Articles/666187/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/andrea/aa.git/log/?h=userfault
[3] https://github.com/denis-plotnikov/qemu/commits/background-snapshot-kvm
[4] https://github.com/LLNL/umap
[5] https://llnl-umap.readthedocs.io/en/develop/
[6] https://git.kernel.org/pub/scm/linux/kernel/git/andrea/aa.git/commit/?h=userfault&id=b245ecf6cf59156966f3da6e6b674f6695a5ffa5
[7] https://lkml.org/lkml/2018/11/21/370
[8] https://lkml.org/lkml/2018/12/30/64
This patch (of 19):
Add helper for writeprotect check. Will use it later.
Signed-off-by: Shaohua Li <[email protected]>
Signed-off-by: Andrea Arcangeli <[email protected]>
Signed-off-by: Peter Xu <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Reviewed-by: Jerome Glisse <[email protected]>
Reviewed-by: Mike Rapoport <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: Kirill A. Shutemov <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: Hugh Dickins <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Bobby Powers <[email protected]>
Cc: Brian Geffon <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Denis Plotnikov <[email protected]>
Cc: "Dr . David Alan Gilbert" <[email protected]>
Cc: Martin Cracauer <[email protected]>
Cc: Marty McFadden <[email protected]>
Cc: Maya Gokhale <[email protected]>
Cc: Mike Kravetz <[email protected]>
Cc: Pavel Emelyanov <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Linus Torvalds <[email protected]>
---
include/linux/userfaultfd_k.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
index f2f3b68ba910..07878cd475f2 100644
--- a/include/linux/userfaultfd_k.h
+++ b/include/linux/userfaultfd_k.h
@@ -48,6 +48,11 @@ static inline bool userfaultfd_missing(struct vm_area_struct *vma)
return vma->vm_flags & VM_UFFD_MISSING;
}
+static inline bool userfaultfd_wp(struct vm_area_struct *vma)
+{
+ return vma->vm_flags & VM_UFFD_WP;
+}
+
static inline bool userfaultfd_armed(struct vm_area_struct *vma)
{
return vma->vm_flags & (VM_UFFD_MISSING | VM_UFFD_WP);
@@ -91,6 +96,11 @@ static inline bool userfaultfd_missing(struct vm_area_struct *vma)
return false;
}
+static inline bool userfaultfd_wp(struct vm_area_struct *vma)
+{
+ return false;
+}
+
static inline bool userfaultfd_armed(struct vm_area_struct *vma)
{
return false;
--
2.31.0.291.g576ba9dcdaf-goog
From: Linus Torvalds <[email protected]>
Commit 09854ba94c6a ("mm: do_wp_page() simplification") reorganized all
the code around the page re-use vs copy, but in the process also moved
the final unlock_page() around to after the wp_page_reuse() call.
That normally doesn't matter - but it means that the unlock_page() is
now done after releasing the page table lock. Again, not a big deal,
you'd think.
But it turns out that it's very wrong indeed, because once we've
released the page table lock, we've basically lost our only reference to
the page - the page tables - and it could now be free'd at any time. We
do hold the mmap_sem, so no actual unmap() can happen, but madvise can
come in and a MADV_DONTNEED will zap the page range - and free the page.
So now the page may be free'd just as we're unlocking it, which in turn
will usually trigger a "Bad page state" error in the freeing path. To
make matters more confusing, by the time the debug code prints out the
page state, the unlock has typically completed and everything looks fine
again.
This all doesn't happen in any normal situations, but it does trigger
with the dirtyc0w_child LTP test. And it seems to trigger much more
easily (but not expclusively) on s390 than elsewhere, probably because
s390 doesn't do the "batch pages up for freeing after the TLB flush"
that gives the unlock_page() more time to complete and makes the race
harder to hit.
Fixes: 09854ba94c6a ("mm: do_wp_page() simplification")
Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/linux-mm/[email protected]/
Reported-by: Qian Cai <[email protected]>
Reported-by: Alex Shi <[email protected]>
Bisected-and-analyzed-by: Gerald Schaefer <[email protected]>
Tested-by: Gerald Schaefer <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
---
mm/memory.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/memory.c b/mm/memory.c
index e84648d81d6d..14470ceaf3f2 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2848,8 +2848,8 @@ static int do_wp_page(struct vm_fault *vmf)
* page count reference, and the page is locked,
* it's dark out, and we're wearing sunglasses. Hit it.
*/
- wp_page_reuse(vmf);
unlock_page(page);
+ wp_page_reuse(vmf);
return VM_FAULT_WRITE;
} else if (unlikely((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
(VM_WRITE|VM_SHARED))) {
--
2.31.0.291.g576ba9dcdaf-goog
On Thu, Apr 1, 2021 at 11:17 AM Suren Baghdasaryan <[email protected]> wrote:
>
> We received a report that the copy-on-write issue repored by Jann Horn in
> https://bugs.chromium.org/p/project-zero/issues/detail?id=2045 is still
> reproducible on 4.14 and 4.19 kernels (the first issue with the reproducer
> coded in vmsplice.c).
Gaah.
> I confirmed this and also that the issue was not
> reproducible with 5.10 kernel. I tracked the fix to the following patch
> introduced in 5.9 which changes the do_wp_page() logic:
>
> 09854ba94c6a 'mm: do_wp_page() simplification'
The problem here is that there's a _lot_ more patches than the few you
found that fixed various other cases (THP etc).
> I backported this patch (#2 in the series) along with 2 prerequisite patches
> (#1 and #4) that keep the backports clean and two followup fixes to the main
> patch (#3 and #5). I had to skip the following fix:
>
> feb889fb40fa 'mm: don't put pinned pages into the swap cache'
>
> because it uses page_maybe_dma_pinned() which does not exists in earlier
> kernels. Because pin_user_pages() does not exist there as well, I *think*
> we can safely skip this fix on older kernels, but I would appreciate if
> someone could confirm that claim.
Hmm. I think this means that swap activity can now break the
connection to a GUP page (the whole pre-pinning model), but it
probably isn't a new problem for 4.9/4.19.
I suspect the test there should be something like
/* Single mapper, more references than us and the map? */
if (page_mapcount(page) == 1 && page_count(page) > 2)
goto keep_locked;
in the pre-pinning days.
But I really think that there are a number of other commits you're
missing too, because we had a whole series for THP fixes for the same
exact issue.
Added Peter Xu to the cc, because he probably tracked those issues
better than I did.
So NAK on this for now, I think this limited patch-set likely
introduces more problems than it fixes.
Linus
On Thu, Apr 01, 2021 at 11:17:37AM -0700, Suren Baghdasaryan wrote:
> From: Kirill Tkhai <[email protected]>
>
> Add an optimization for KSM pages almost in the same way that we have
> for ordinary anonymous pages. If there is a write fault in a page,
> which is mapped to an only pte, and it is not related to swap cache; the
> page may be reused without copying its content.
>
> [ Note that we do not consider PageSwapCache() pages at least for now,
> since we don't want to complicate __get_ksm_page(), which has nice
> optimization based on this (for the migration case). Currenly it is
> spinning on PageSwapCache() pages, waiting for when they have
> unfreezed counters (i.e., for the migration finish). But we don't want
> to make it also spinning on swap cache pages, which we try to reuse,
> since there is not a very high probability to reuse them. So, for now
> we do not consider PageSwapCache() pages at all. ]
>
> So in reuse_ksm_page() we check for 1) PageSwapCache() and 2)
> page_stable_node(), to skip a page, which KSM is currently trying to
> link to stable tree. Then we do page_ref_freeze() to prohibit KSM to
> merge one more page into the page, we are reusing. After that, nobody
> can refer to the reusing page: KSM skips !PageSwapCache() pages with
> zero refcount; and the protection against of all other participants is
> the same as for reused ordinary anon pages pte lock, page lock and
> mmap_sem.
>
> [[email protected]: replace BUG_ON()s with WARN_ON()s]
> Link: http://lkml.kernel.org/r/154471491016.31352.1168978849911555609.stgit@localhost.localdomain
> Signed-off-by: Kirill Tkhai <[email protected]>
> Reviewed-by: Yang Shi <[email protected]>
> Cc: "Kirill A. Shutemov" <[email protected]>
> Cc: Hugh Dickins <[email protected]>
> Cc: Andrea Arcangeli <[email protected]>
> Cc: Christian Koenig <[email protected]>
> Cc: Claudio Imbrenda <[email protected]>
> Cc: Rik van Riel <[email protected]>
> Cc: Huang Ying <[email protected]>
> Cc: Minchan Kim <[email protected]>
> Cc: Kirill Tkhai <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> Signed-off-by: Linus Torvalds <[email protected]>
> ---
> include/linux/ksm.h | 7 +++++++
> mm/ksm.c | 30 ++++++++++++++++++++++++++++--
> mm/memory.c | 16 ++++++++++++++--
> 3 files changed, 49 insertions(+), 4 deletions(-)
You forgot to put the git commit id of the upstream commit in here
somewhere so we can properly reference it and track it.
When/if you resend this, please add it to all of the commits.
thanks,
greg k-h
On Thu, Apr 1, 2021 at 11:59 AM Linus Torvalds
<[email protected]> wrote:
>
> On Thu, Apr 1, 2021 at 11:17 AM Suren Baghdasaryan <[email protected]> wrote:
> >
> > We received a report that the copy-on-write issue repored by Jann Horn in
> > https://bugs.chromium.org/p/project-zero/issues/detail?id=2045 is still
> > reproducible on 4.14 and 4.19 kernels (the first issue with the reproducer
> > coded in vmsplice.c).
>
> Gaah.
>
> > I confirmed this and also that the issue was not
> > reproducible with 5.10 kernel. I tracked the fix to the following patch
> > introduced in 5.9 which changes the do_wp_page() logic:
> >
> > 09854ba94c6a 'mm: do_wp_page() simplification'
>
> The problem here is that there's a _lot_ more patches than the few you
> found that fixed various other cases (THP etc).
>
> > I backported this patch (#2 in the series) along with 2 prerequisite patches
> > (#1 and #4) that keep the backports clean and two followup fixes to the main
> > patch (#3 and #5). I had to skip the following fix:
> >
> > feb889fb40fa 'mm: don't put pinned pages into the swap cache'
> >
> > because it uses page_maybe_dma_pinned() which does not exists in earlier
> > kernels. Because pin_user_pages() does not exist there as well, I *think*
> > we can safely skip this fix on older kernels, but I would appreciate if
> > someone could confirm that claim.
>
> Hmm. I think this means that swap activity can now break the
> connection to a GUP page (the whole pre-pinning model), but it
> probably isn't a new problem for 4.9/4.19.
>
> I suspect the test there should be something like
>
> /* Single mapper, more references than us and the map? */
> if (page_mapcount(page) == 1 && page_count(page) > 2)
> goto keep_locked;
>
> in the pre-pinning days.
>
> But I really think that there are a number of other commits you're
> missing too, because we had a whole series for THP fixes for the same
> exact issue.
>
> Added Peter Xu to the cc, because he probably tracked those issues
> better than I did.
>
> So NAK on this for now, I think this limited patch-set likely
> introduces more problems than it fixes.
Thanks for confirming my worries. I'll be happy to add additional
backports if Peter can point me to them.
Thanks,
Suren.
>
> Linus
On Thu, Apr 1, 2021 at 12:38 PM Greg KH <[email protected]> wrote:
>
> On Thu, Apr 01, 2021 at 11:17:37AM -0700, Suren Baghdasaryan wrote:
> > From: Kirill Tkhai <[email protected]>
> >
> > Add an optimization for KSM pages almost in the same way that we have
> > for ordinary anonymous pages. If there is a write fault in a page,
> > which is mapped to an only pte, and it is not related to swap cache; the
> > page may be reused without copying its content.
> >
> > [ Note that we do not consider PageSwapCache() pages at least for now,
> > since we don't want to complicate __get_ksm_page(), which has nice
> > optimization based on this (for the migration case). Currenly it is
> > spinning on PageSwapCache() pages, waiting for when they have
> > unfreezed counters (i.e., for the migration finish). But we don't want
> > to make it also spinning on swap cache pages, which we try to reuse,
> > since there is not a very high probability to reuse them. So, for now
> > we do not consider PageSwapCache() pages at all. ]
> >
> > So in reuse_ksm_page() we check for 1) PageSwapCache() and 2)
> > page_stable_node(), to skip a page, which KSM is currently trying to
> > link to stable tree. Then we do page_ref_freeze() to prohibit KSM to
> > merge one more page into the page, we are reusing. After that, nobody
> > can refer to the reusing page: KSM skips !PageSwapCache() pages with
> > zero refcount; and the protection against of all other participants is
> > the same as for reused ordinary anon pages pte lock, page lock and
> > mmap_sem.
> >
> > [[email protected]: replace BUG_ON()s with WARN_ON()s]
> > Link: http://lkml.kernel.org/r/154471491016.31352.1168978849911555609.stgit@localhost.localdomain
> > Signed-off-by: Kirill Tkhai <[email protected]>
> > Reviewed-by: Yang Shi <[email protected]>
> > Cc: "Kirill A. Shutemov" <[email protected]>
> > Cc: Hugh Dickins <[email protected]>
> > Cc: Andrea Arcangeli <[email protected]>
> > Cc: Christian Koenig <[email protected]>
> > Cc: Claudio Imbrenda <[email protected]>
> > Cc: Rik van Riel <[email protected]>
> > Cc: Huang Ying <[email protected]>
> > Cc: Minchan Kim <[email protected]>
> > Cc: Kirill Tkhai <[email protected]>
> > Signed-off-by: Andrew Morton <[email protected]>
> > Signed-off-by: Linus Torvalds <[email protected]>
> > ---
> > include/linux/ksm.h | 7 +++++++
> > mm/ksm.c | 30 ++++++++++++++++++++++++++++--
> > mm/memory.c | 16 ++++++++++++++--
> > 3 files changed, 49 insertions(+), 4 deletions(-)
>
> You forgot to put the git commit id of the upstream commit in here
> somewhere so we can properly reference it and track it.
>
> When/if you resend this, please add it to all of the commits.
Will do. Thanks!
>
> thanks,
>
> greg k-h
Hi, Suren,
On Thu, Apr 01, 2021 at 12:43:51PM -0700, Suren Baghdasaryan wrote:
> On Thu, Apr 1, 2021 at 11:59 AM Linus Torvalds
> <[email protected]> wrote:
> >
> > On Thu, Apr 1, 2021 at 11:17 AM Suren Baghdasaryan <[email protected]> wrote:
> > >
> > > We received a report that the copy-on-write issue repored by Jann Horn in
> > > https://bugs.chromium.org/p/project-zero/issues/detail?id=2045 is still
> > > reproducible on 4.14 and 4.19 kernels (the first issue with the reproducer
> > > coded in vmsplice.c).
> >
> > Gaah.
> >
> > > I confirmed this and also that the issue was not
> > > reproducible with 5.10 kernel. I tracked the fix to the following patch
> > > introduced in 5.9 which changes the do_wp_page() logic:
> > >
> > > 09854ba94c6a 'mm: do_wp_page() simplification'
> >
> > The problem here is that there's a _lot_ more patches than the few you
> > found that fixed various other cases (THP etc).
> >
> > > I backported this patch (#2 in the series) along with 2 prerequisite patches
> > > (#1 and #4) that keep the backports clean and two followup fixes to the main
> > > patch (#3 and #5). I had to skip the following fix:
> > >
> > > feb889fb40fa 'mm: don't put pinned pages into the swap cache'
> > >
> > > because it uses page_maybe_dma_pinned() which does not exists in earlier
> > > kernels. Because pin_user_pages() does not exist there as well, I *think*
> > > we can safely skip this fix on older kernels, but I would appreciate if
> > > someone could confirm that claim.
> >
> > Hmm. I think this means that swap activity can now break the
> > connection to a GUP page (the whole pre-pinning model), but it
> > probably isn't a new problem for 4.9/4.19.
> >
> > I suspect the test there should be something like
> >
> > /* Single mapper, more references than us and the map? */
> > if (page_mapcount(page) == 1 && page_count(page) > 2)
> > goto keep_locked;
> >
> > in the pre-pinning days.
> >
> > But I really think that there are a number of other commits you're
> > missing too, because we had a whole series for THP fixes for the same
> > exact issue.
> >
> > Added Peter Xu to the cc, because he probably tracked those issues
> > better than I did.
> >
> > So NAK on this for now, I think this limited patch-set likely
> > introduces more problems than it fixes.
>
> Thanks for confirming my worries. I'll be happy to add additional
> backports if Peter can point me to them.
If for a full-alignment with current upstream, I can at least think of below
series:
Early cow for general pages:
https://lore.kernel.org/lkml/[email protected]/
A race fix for copy_page and gup-fast:
https://lore.kernel.org/linux-mm/[email protected]/
Early cow for hugetlbfs (which is very recently):
https://lore.kernel.org/lkml/[email protected]/
But I believe they'll bring a number of dependencies too like the page pinned
work; so seems not easy.
Btw, AFAICT you don't need patch 4/5 in this series for 4.14/4.19, since
those're only for uffd-wp and it doesn't exist until 5.7.
Thanks,
--
Peter Xu
On Thu, Apr 1, 2021 at 4:47 PM Peter Xu <[email protected]> wrote:
>
> Hi, Suren,
>
> On Thu, Apr 01, 2021 at 12:43:51PM -0700, Suren Baghdasaryan wrote:
> > On Thu, Apr 1, 2021 at 11:59 AM Linus Torvalds
> > <[email protected]> wrote:
> > >
> > > On Thu, Apr 1, 2021 at 11:17 AM Suren Baghdasaryan <[email protected]> wrote:
> > > >
> > > > We received a report that the copy-on-write issue repored by Jann Horn in
> > > > https://bugs.chromium.org/p/project-zero/issues/detail?id=2045 is still
> > > > reproducible on 4.14 and 4.19 kernels (the first issue with the reproducer
> > > > coded in vmsplice.c).
> > >
> > > Gaah.
> > >
> > > > I confirmed this and also that the issue was not
> > > > reproducible with 5.10 kernel. I tracked the fix to the following patch
> > > > introduced in 5.9 which changes the do_wp_page() logic:
> > > >
> > > > 09854ba94c6a 'mm: do_wp_page() simplification'
> > >
> > > The problem here is that there's a _lot_ more patches than the few you
> > > found that fixed various other cases (THP etc).
> > >
> > > > I backported this patch (#2 in the series) along with 2 prerequisite patches
> > > > (#1 and #4) that keep the backports clean and two followup fixes to the main
> > > > patch (#3 and #5). I had to skip the following fix:
> > > >
> > > > feb889fb40fa 'mm: don't put pinned pages into the swap cache'
> > > >
> > > > because it uses page_maybe_dma_pinned() which does not exists in earlier
> > > > kernels. Because pin_user_pages() does not exist there as well, I *think*
> > > > we can safely skip this fix on older kernels, but I would appreciate if
> > > > someone could confirm that claim.
> > >
> > > Hmm. I think this means that swap activity can now break the
> > > connection to a GUP page (the whole pre-pinning model), but it
> > > probably isn't a new problem for 4.9/4.19.
> > >
> > > I suspect the test there should be something like
> > >
> > > /* Single mapper, more references than us and the map? */
> > > if (page_mapcount(page) == 1 && page_count(page) > 2)
> > > goto keep_locked;
> > >
> > > in the pre-pinning days.
> > >
> > > But I really think that there are a number of other commits you're
> > > missing too, because we had a whole series for THP fixes for the same
> > > exact issue.
> > >
> > > Added Peter Xu to the cc, because he probably tracked those issues
> > > better than I did.
> > >
> > > So NAK on this for now, I think this limited patch-set likely
> > > introduces more problems than it fixes.
> >
> > Thanks for confirming my worries. I'll be happy to add additional
> > backports if Peter can point me to them.
>
> If for a full-alignment with current upstream, I can at least think of below
> series:
>
> Early cow for general pages:
> https://lore.kernel.org/lkml/[email protected]/
>
> A race fix for copy_page and gup-fast:
> https://lore.kernel.org/linux-mm/[email protected]/
>
> Early cow for hugetlbfs (which is very recently):
> https://lore.kernel.org/lkml/[email protected]/
>
> But I believe they'll bring a number of dependencies too like the page pinned
> work; so seems not easy.
Thanks Peter. Let me try backporting these and I'll see if it's doable.
>
> Btw, AFAICT you don't need patch 4/5 in this series for 4.14/4.19, since
> those're only for uffd-wp and it doesn't exist until 5.7.
Got it. Will drop it from the next series.
Thanks,
Suren.
>
> Thanks,
>
> --
> Peter Xu
>
On 4/1/21 8:59 PM, Linus Torvalds wrote:
> On Thu, Apr 1, 2021 at 11:17 AM Suren Baghdasaryan <[email protected]> wrote:
Thanks Suren for bringing this up!
>> We received a report that the copy-on-write issue repored by Jann Horn in
>> https://bugs.chromium.org/p/project-zero/issues/detail?id=2045 is still
>> reproducible on 4.14 and 4.19 kernels (the first issue with the reproducer
>> coded in vmsplice.c).
Note that even upstream AFAIK still has the issue unfixed when Jann's reproducer
is converted to use THPs, as Andrea has shown in
https://lore.kernel.org/linux-mm/X%[email protected]/
> Gaah.
>
>> I confirmed this and also that the issue was not
>> reproducible with 5.10 kernel. I tracked the fix to the following patch
>> introduced in 5.9 which changes the do_wp_page() logic:
>>
>> 09854ba94c6a 'mm: do_wp_page() simplification'
>
> The problem here is that there's a _lot_ more patches than the few you
> found that fixed various other cases (THP etc).
>
>> I backported this patch (#2 in the series) along with 2 prerequisite patches
>> (#1 and #4) that keep the backports clean and two followup fixes to the main
>> patch (#3 and #5). I had to skip the following fix:
>>
>> feb889fb40fa 'mm: don't put pinned pages into the swap cache'
>>
>> because it uses page_maybe_dma_pinned() which does not exists in earlier
>> kernels. Because pin_user_pages() does not exist there as well, I *think*
>> we can safely skip this fix on older kernels, but I would appreciate if
>> someone could confirm that claim.
>
> Hmm. I think this means that swap activity can now break the
> connection to a GUP page (the whole pre-pinning model), but it
> probably isn't a new problem for 4.9/4.19.
>
> I suspect the test there should be something like
>
> /* Single mapper, more references than us and the map? */
> if (page_mapcount(page) == 1 && page_count(page) > 2)
> goto keep_locked;
>
> in the pre-pinning days.
>
> But I really think that there are a number of other commits you're
> missing too, because we had a whole series for THP fixes for the same
> exact issue.
>
> Added Peter Xu to the cc, because he probably tracked those issues
> better than I did.
Let me shamelessly plug these links for illustrating what kind of minefield we
would be going into backporting this. Also for references what not to miss, and
what may still become broken afterwards:
https://lwn.net/Articles/849638/
https://lwn.net/Articles/849876/
> So NAK on this for now, I think this limited patch-set likely
> introduces more problems than it fixes.
I personally think there are only two options safe enough for stable backports
(so that not more harm is caused than actually prevented).
1) Ignore the issue (outside of Android at least). The security model of zygote
is unusual. Where else a parent of fork() doesn't trust the child, which is the
same binary?
BTW, I think the CVE description is very misleading:
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-29374
"does not properly consider the semantics of read operations and therefore can
grant unintended write access" - the bug was never about an unintended write
access, but about an info leak from parent to child, no?
2) For backports go with the original approach of 17839856fd58 ("gup: document
and work around "COW can break either way" issue"), thus break COW during the
GUP. But only for vmplice() so that nothing else gets broken. I think 5.4 stable
(another LTS) actually backported only 17839856fd58 out of everything else, so
it should have even the THP case covered, but its userfaultfd() is now probably
broken...
> Linus
>
On Wed, Apr 07, 2021 at 03:21:55PM +0200, Vlastimil Babka wrote:
> 2) For backports go with the original approach of 17839856fd58 ("gup: document
> and work around "COW can break either way" issue"), thus break COW during the
> GUP. But only for vmplice() so that nothing else gets broken. I think 5.4 stable
> (another LTS) actually backported only 17839856fd58 out of everything else, so
> it should have even the THP case covered, but its userfaultfd() is now probably
> broken...
Since you mentioned this approach - AFAIU userfaultfd was only broken because
with that approach the kernel pretends some read accesses as writes, while
userfaultfd needs that accurate resolution. Adding something like
FOLL_BREAK_COW [1] upon 17839856fd58 should keep both the vmsplice issue fixed
but also uffd working since that'll keep the read/write operation separate.
Meanwhile, I know Andrea was actively working on a complete solution [2] that's
a few steps further. E.g., FOLL_BREAK_COW is done with FOLL_UNSHARE [3], speed
up in COW path [4] with similar idea of what we do right now with latest
upstream in 09854ba94c6aad7, allow write-protect with pinned pages (which is
right now forbidden), and something more. However that's definitely a huge
branch, even discussing upstream (or maybe stopped discussing for quite some
days already?).
Neither of above are within upstream, so I don't really know whether these
information could be anything useful, just raise it up. If Android could drop
userfaultfd, then I think solution 2) above is indeed the most efficient. Note
that I think only uffd-wp was affected by 17839856fd58 but not the "missing
mode", so if Android is only using missing mode it still looks fine to only
have 17839856fd58. It's just that I remembered there's another report besides
uffd-wp on 17839856fd58, but I can't remember the details of the other report.
Thanks,
[1] https://lkml.org/lkml/2020/8/10/439
[2] https://git.kernel.org/pub/scm/linux/kernel/git/andrea/aa.git/log/?h=mapcount_deshare
[3] https://git.kernel.org/pub/scm/linux/kernel/git/andrea/aa.git/commit/?h=mapcount_deshare&id=7c3a31caa34ac6ac4a4ec0559b1307b5edfc0821
[4] https://git.kernel.org/pub/scm/linux/kernel/git/andrea/aa.git/commit/?h=mapcount_deshare&id=599aa62474f51a470408b28fd4365320a5357aca
--
Peter Xu
On Wed, Apr 7, 2021 at 6:22 AM Vlastimil Babka <[email protected]> wrote:
>
> 1) Ignore the issue (outside of Android at least). The security model of zygote
> is unusual. Where else a parent of fork() doesn't trust the child, which is the
> same binary?
Agreed. I think this is basically an android-only issue (with
_possibly_ some impact on crazy "pin-and-fork" loads), and doesn't
necessarily merit a backport at all.
If Android people insist on using very old kernels, knowing that they
do things that are questionable with those old kernels, at some point
it's just _their_ problem.
Linus
On Wed, Apr 7, 2021 at 9:33 AM Suren Baghdasaryan <[email protected]> wrote:
>
> Trying my hand at backporting the patchsets Peter mentioned proved
> this to be far from easy with many dependencies. Let me look into
> Vlastimil's suggestion to backport only 17839856fd58 and it sounds
> like 5.4 already followed that path.
Well, in many ways 17839856fd58 was the "simple and obvious" fix, and
I do think it's easily backportable.
But it *did* cause problems too. Those problems may not be issues on
those old kernels, though.
In particular, commit 17839856fd58 caused uffd-wp to stop working
right, and it caused some issues with debugging (I forget the exact
details, but I think it was strace accessing PROT_NONE or write-only
pages or something like that, and COW failed).
But yes, in many ways that commit is a much simpler and more
straightforward one (which is why I tried it once - we ended up with
the much more subtle and far-reaching fixes after the UFFD issues
crept up).
The issues that 17839856fd58 caused may be entire non-events in old
kernels. In fact, the uffd writeprotect API was added fairly recently
(see commit 63b2d4174c4a that made it into v5.7), so the uffd-wp issue
that was triggered probably cannot happen in the old kernels.
The strace issue might not be relevant either, but I forget what the
details were. Mikilas should know.
See
https://lore.kernel.org/lkml/alpine.LRH.2.02.2009031328040.6929@file01.intranet.prod.int.rdu2.redhat.com/
for Mikulas report. I never looked into it in detail, because by then
the uffd-wp issue had already come up, so it was juat another nail in
the coffin for that simpler approach.
Mikulas, do you remember?
Linus
On Wed, 7 Apr 2021, Linus Torvalds wrote:
> On Wed, Apr 7, 2021 at 9:33 AM Suren Baghdasaryan <[email protected]> wrote:
> >
> > Trying my hand at backporting the patchsets Peter mentioned proved
> > this to be far from easy with many dependencies. Let me look into
> > Vlastimil's suggestion to backport only 17839856fd58 and it sounds
> > like 5.4 already followed that path.
>
> Well, in many ways 17839856fd58 was the "simple and obvious" fix, and
> I do think it's easily backportable.
>
> But it *did* cause problems too. Those problems may not be issues on
> those old kernels, though.
>
> In particular, commit 17839856fd58 caused uffd-wp to stop working
> right, and it caused some issues with debugging (I forget the exact
> details, but I think it was strace accessing PROT_NONE or write-only
> pages or something like that, and COW failed).
>
> But yes, in many ways that commit is a much simpler and more
> straightforward one (which is why I tried it once - we ended up with
> the much more subtle and far-reaching fixes after the UFFD issues
> crept up).
>
> The issues that 17839856fd58 caused may be entire non-events in old
> kernels. In fact, the uffd writeprotect API was added fairly recently
> (see commit 63b2d4174c4a that made it into v5.7), so the uffd-wp issue
> that was triggered probably cannot happen in the old kernels.
>
> The strace issue might not be relevant either, but I forget what the
> details were. Mikilas should know.
>
> See
>
> https://lore.kernel.org/lkml/alpine.LRH.2.02.2009031328040.6929@file01.intranet.prod.int.rdu2.redhat.com/
>
> for Mikulas report. I never looked into it in detail, because by then
> the uffd-wp issue had already come up, so it was juat another nail in
> the coffin for that simpler approach.
>
> Mikulas, do you remember?
>
> Linus
Hi
I think that we never found a root cause for this bug. I was testing if
the whole system can run from persistent memory and found out that strace
didn't work. I bisected it, reported it and when I received Peter Xu's
patches (which fixed it), I stopped bothering about it.
So, we fixed it, but we don't know why.
Peter Xu's patchset that fixed it is here:
https://lore.kernel.org/lkml/[email protected]/
Mikulas
On Wed, Apr 7, 2021 at 11:47 AM Mikulas Patocka <[email protected]> wrote:
>
> So, we fixed it, but we don't know why.
>
> Peter Xu's patchset that fixed it is here:
> https://lore.kernel.org/lkml/[email protected]/
Yeah, that's the part that ends up being really painful to backport
(with all the subsequent fixes too), so the 4.14 people would prefer
to avoid it.
But I think that if it's a "requires dax pmem and ptrace on top", it
may simply be a non-issue for those users. Although who knows - maybe
that ends up being a real issue on Android..
Linus
On Wed, Apr 7, 2021 at 12:23 PM Linus Torvalds
<[email protected]> wrote:
>
> On Wed, Apr 7, 2021 at 11:47 AM Mikulas Patocka <[email protected]> wrote:
> >
> > So, we fixed it, but we don't know why.
> >
> > Peter Xu's patchset that fixed it is here:
> > https://lore.kernel.org/lkml/[email protected]/
>
> Yeah, that's the part that ends up being really painful to backport
> (with all the subsequent fixes too), so the 4.14 people would prefer
> to avoid it.
>
> But I think that if it's a "requires dax pmem and ptrace on top", it
> may simply be a non-issue for those users. Although who knows - maybe
> that ends up being a real issue on Android..
A lot to digest, so I need to do some reading now. Thanks everyone!
>
> Linus
On Wed, Apr 7, 2021 at 9:07 AM Linus Torvalds
<[email protected]> wrote:
>
> On Wed, Apr 7, 2021 at 6:22 AM Vlastimil Babka <[email protected]> wrote:
> >
> > 1) Ignore the issue (outside of Android at least). The security model of zygote
> > is unusual. Where else a parent of fork() doesn't trust the child, which is the
> > same binary?
>
> Agreed. I think this is basically an android-only issue (with
> _possibly_ some impact on crazy "pin-and-fork" loads), and doesn't
> necessarily merit a backport at all.
>
> If Android people insist on using very old kernels, knowing that they
> do things that are questionable with those old kernels, at some point
> it's just _their_ problem.
We don't really insist on using old kernels but rather we are stuck
with them for some time.
Trying my hand at backporting the patchsets Peter mentioned proved
this to be far from easy with many dependencies. Let me look into
Vlastimil's suggestion to backport only 17839856fd58 and it sounds
like 5.4 already followed that path. Thanks for all the information!
Suren.
>
> Linus
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
>
On Wed, Apr 7, 2021 at 2:53 PM Suren Baghdasaryan <[email protected]> wrote:
>
> On Wed, Apr 7, 2021 at 12:23 PM Linus Torvalds
> <[email protected]> wrote:
> >
> > On Wed, Apr 7, 2021 at 11:47 AM Mikulas Patocka <[email protected]> wrote:
> > >
> > > So, we fixed it, but we don't know why.
> > >
> > > Peter Xu's patchset that fixed it is here:
> > > https://lore.kernel.org/lkml/[email protected]/
> >
> > Yeah, that's the part that ends up being really painful to backport
> > (with all the subsequent fixes too), so the 4.14 people would prefer
> > to avoid it.
> >
> > But I think that if it's a "requires dax pmem and ptrace on top", it
> > may simply be a non-issue for those users. Although who knows - maybe
> > that ends up being a real issue on Android..
>
> A lot to digest, so I need to do some reading now. Thanks everyone!
After a delay due to vacation I prepared backports of 17839856fd58
("gup: document and work around "COW can break either way" issue") for
4.14 and 4.19 kernels. As Linus pointed out, uffd-wp was introduced
later in 5.7, so is not an issue for 4.x kernels. The issue with THPs
is still unresolved, so with or without this patch it's still there
(Android is not affected by this since we do not use THPs with older
kernels).
Andrea pointed out that there are other issues and to properly fix
them his COR approach is needed. However it has not been accepted yet,
so I can't really backport it. I'll be happy to do that though if it
is accepted in the future.
Peter, you mentioned https://lkml.org/lkml/2020/8/10/439 patch to
distinguish real writes vs enforced COW read requests, however I also
see that you had a later version of this patch here:
https://lore.kernel.org/patchwork/patch/1286506/. Which one should I
backport? Or is it not needed in the absence of uffd-wp support in the
earlier kernels?
Thanks,
Suren.
>
> >
> > Linus
Hi, Suren,
On Wed, Apr 21, 2021 at 01:01:34PM -0700, Suren Baghdasaryan wrote:
> Peter, you mentioned https://lkml.org/lkml/2020/8/10/439 patch to
> distinguish real writes vs enforced COW read requests, however I also
> see that you had a later version of this patch here:
> https://lore.kernel.org/patchwork/patch/1286506/. Which one should I
> backport? Or is it not needed in the absence of uffd-wp support in the
> earlier kernels?
Sorry I have no ability to evaluate the rest... but according to Linus's
previous reply, my understanding is that it is not needed, not to mention it's
not upstreamed too.
Thanks,
--
Peter Xu
On Wed, Apr 21, 2021 at 2:05 PM Peter Xu <[email protected]> wrote:
>
> Hi, Suren,
>
> On Wed, Apr 21, 2021 at 01:01:34PM -0700, Suren Baghdasaryan wrote:
> > Peter, you mentioned https://lkml.org/lkml/2020/8/10/439 patch to
> > distinguish real writes vs enforced COW read requests, however I also
> > see that you had a later version of this patch here:
> > https://lore.kernel.org/patchwork/patch/1286506/. Which one should I
> > backport? Or is it not needed in the absence of uffd-wp support in the
> > earlier kernels?
>
> Sorry I have no ability to evaluate the rest... but according to Linus's
> previous reply, my understanding is that it is not needed, not to mention it's
> not upstreamed too.
Thanks! Then I'll send the backports for 17839856fd58 alone and if
more backports are needed I'll post followup patches.
Cheers,
Suren.
>
> Thanks,
>
> --
> Peter Xu
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
>
On 4/21/21 10:01 PM, Suren Baghdasaryan wrote:
> On Wed, Apr 7, 2021 at 2:53 PM Suren Baghdasaryan <[email protected]> wrote:
>>
>> On Wed, Apr 7, 2021 at 12:23 PM Linus Torvalds
>> <[email protected]> wrote:
>> >
>> > On Wed, Apr 7, 2021 at 11:47 AM Mikulas Patocka <[email protected]> wrote:
>> > >
>> > > So, we fixed it, but we don't know why.
>> > >
>> > > Peter Xu's patchset that fixed it is here:
>> > > https://lore.kernel.org/lkml/[email protected]/
>> >
>> > Yeah, that's the part that ends up being really painful to backport
>> > (with all the subsequent fixes too), so the 4.14 people would prefer
>> > to avoid it.
>> >
>> > But I think that if it's a "requires dax pmem and ptrace on top", it
>> > may simply be a non-issue for those users. Although who knows - maybe
>> > that ends up being a real issue on Android..
>>
>> A lot to digest, so I need to do some reading now. Thanks everyone!
>
> After a delay due to vacation I prepared backports of 17839856fd58
> ("gup: document and work around "COW can break either way" issue") for
> 4.14 and 4.19 kernels. As Linus pointed out, uffd-wp was introduced
> later in 5.7, so is not an issue for 4.x kernels. The issue with THPs
> is still unresolved, so with or without this patch it's still there
> (Android is not affected by this since we do not use THPs with older
> kernels).
Which THP issue do you mean here? The race that was part of the same Project
zero report and was solved by a different patch adding some locking? Or the
vmsplice info leak but applied to THP's? Because if it's the latter then I
believe 17839856fd58 did solve that too. It was the later switch of approach to
rely just on page_count() that left THP side unfixed.
> Andrea pointed out that there are other issues and to properly fix
> them his COR approach is needed. However it has not been accepted yet,
> so I can't really backport it. I'll be happy to do that though if it
> is accepted in the future.
>
> Peter, you mentioned https://lkml.org/lkml/2020/8/10/439 patch to
> distinguish real writes vs enforced COW read requests, however I also
> see that you had a later version of this patch here:
> https://lore.kernel.org/patchwork/patch/1286506/. Which one should I
> backport? Or is it not needed in the absence of uffd-wp support in the
> earlier kernels?
> Thanks,
> Suren.
>
>>
>> >
>> > Linus
>
On Wed, Apr 21, 2021 at 3:59 PM Vlastimil Babka <[email protected]> wrote:
>
> On 4/21/21 10:01 PM, Suren Baghdasaryan wrote:
> > On Wed, Apr 7, 2021 at 2:53 PM Suren Baghdasaryan <[email protected]> wrote:
> >>
> >> On Wed, Apr 7, 2021 at 12:23 PM Linus Torvalds
> >> <[email protected]> wrote:
> >> >
> >> > On Wed, Apr 7, 2021 at 11:47 AM Mikulas Patocka <[email protected]> wrote:
> >> > >
> >> > > So, we fixed it, but we don't know why.
> >> > >
> >> > > Peter Xu's patchset that fixed it is here:
> >> > > https://lore.kernel.org/lkml/[email protected]/
> >> >
> >> > Yeah, that's the part that ends up being really painful to backport
> >> > (with all the subsequent fixes too), so the 4.14 people would prefer
> >> > to avoid it.
> >> >
> >> > But I think that if it's a "requires dax pmem and ptrace on top", it
> >> > may simply be a non-issue for those users. Although who knows - maybe
> >> > that ends up being a real issue on Android..
> >>
> >> A lot to digest, so I need to do some reading now. Thanks everyone!
> >
> > After a delay due to vacation I prepared backports of 17839856fd58
> > ("gup: document and work around "COW can break either way" issue") for
> > 4.14 and 4.19 kernels. As Linus pointed out, uffd-wp was introduced
> > later in 5.7, so is not an issue for 4.x kernels. The issue with THPs
> > is still unresolved, so with or without this patch it's still there
> > (Android is not affected by this since we do not use THPs with older
> > kernels).
>
> Which THP issue do you mean here? The race that was part of the same Project
> zero report and was solved by a different patch adding some locking? Or the
> vmsplice info leak but applied to THP's? Because if it's the latter then I
> believe 17839856fd58 did solve that too. It was the later switch of approach to
> rely just on page_count() that left THP side unfixed.
I meant the "vmsplice info leak applied to THP's" but now I realize
that 17839856fd58 does not use elevated reference count, so indeed
that should not be a problem. Thanks for the note!
>
> > Andrea pointed out that there are other issues and to properly fix
> > them his COR approach is needed. However it has not been accepted yet,
> > so I can't really backport it. I'll be happy to do that though if it
> > is accepted in the future.
> >
> > Peter, you mentioned https://lkml.org/lkml/2020/8/10/439 patch to
> > distinguish real writes vs enforced COW read requests, however I also
> > see that you had a later version of this patch here:
> > https://lore.kernel.org/patchwork/patch/1286506/. Which one should I
> > backport? Or is it not needed in the absence of uffd-wp support in the
> > earlier kernels?
> > Thanks,
> > Suren.
> >
> >>
> >> >
> >> > Linus
> >
>
On Wed, Apr 21, 2021 at 2:17 PM Suren Baghdasaryan <[email protected]> wrote:
>
> On Wed, Apr 21, 2021 at 2:05 PM Peter Xu <[email protected]> wrote:
> >
> > Hi, Suren,
> >
> > On Wed, Apr 21, 2021 at 01:01:34PM -0700, Suren Baghdasaryan wrote:
> > > Peter, you mentioned https://lkml.org/lkml/2020/8/10/439 patch to
> > > distinguish real writes vs enforced COW read requests, however I also
> > > see that you had a later version of this patch here:
> > > https://lore.kernel.org/patchwork/patch/1286506/. Which one should I
> > > backport? Or is it not needed in the absence of uffd-wp support in the
> > > earlier kernels?
> >
> > Sorry I have no ability to evaluate the rest... but according to Linus's
> > previous reply, my understanding is that it is not needed, not to mention it's
> > not upstreamed too.
>
> Thanks! Then I'll send the backports for 17839856fd58 alone and if
> more backports are needed I'll post followup patches.
Posted 4.19 backport: https://lore.kernel.org/patchwork/patch/1416747
and 4.14 backport: https://lore.kernel.org/patchwork/patch/1416748.
They are identical but Greg asked me to submit separate patches due to
an additional minor merge conflict in 4.14.
> Cheers,
> Suren.
>
> >
> > Thanks,
> >
> > --
> > Peter Xu
> >
> > --
> > To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
> >