[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170802005813.GC6388@bbox>
Date: Wed, 2 Aug 2017 09:58:13 +0900
From: Minchan Kim <minchan@...nel.org>
To: Nadav Amit <nadav.amit@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
LKML <linux-kernel@...r.kernel.org>,
"open list:MEMORY MANAGEMENT" <linux-mm@...ck.org>,
kernel-team <kernel-team@....com>,
Mel Gorman <mgorman@...hsingularity.net>,
Hugh Dickins <hughd@...gle.com>,
Andrea Arcangeli <aarcange@...hat.com>
Subject: Re: [PATCH v2 4/4] mm: fix KSM data corruption
On Tue, Aug 01, 2017 at 12:21:41PM -0700, Nadav Amit wrote:
> Minchan Kim <minchan@...nel.org> wrote:
>
> > Nadav reported KSM can corrupt the user data by the TLB batching race[1].
> > That means data user written can be lost.
> >
> > Quote from Nadav Amit
> > "
> > For this race we need 4 CPUs:
> >
> > CPU0: Caches a writable and dirty PTE entry, and uses the stale value for
> > write later.
> >
> > CPU1: Runs madvise_free on the range that includes the PTE. It would clear
> > the dirty-bit. It batches TLB flushes.
> >
> > CPU2: Writes 4 to /proc/PID/clear_refs , clearing the PTEs soft-dirty. We
> > care about the fact that it clears the PTE write-bit, and of course, batches
> > TLB flushes.
> >
> > CPU3: Runs KSM. Our purpose is to pass the following test in
> > write_protect_page():
> >
> > if (pte_write(*pvmw.pte) || pte_dirty(*pvmw.pte) ||
> > (pte_protnone(*pvmw.pte) && pte_savedwrite(*pvmw.pte)))
> >
> > Since it will avoid TLB flush. And we want to do it while the PTE is stale.
> > Later, and before replacing the page, we would be able to change the page.
> >
> > Note that all the operations the CPU1-3 perform canhappen in parallel since
> > they only acquire mmap_sem for read.
> >
> > We start with two identical pages. Everything below regards the same
> > page/PTE.
> >
> > CPU0 CPU1 CPU2 CPU3
> > ---- ---- ---- ----
> > Write the same
> > value on page
> >
> > [cache PTE as
> > dirty in TLB]
> >
> > MADV_FREE
> > pte_mkclean()
> >
> > 4 > clear_refs
> > pte_wrprotect()
> >
> > write_protect_page()
> > [ success, no flush ]
> >
> > pages_indentical()
> > [ ok ]
> >
> > Write to page
> > different value
> >
> > [Ok, using stale
> > PTE]
> >
> > replace_page()
> >
> > Later, CPU1, CPU2 and CPU3 would flush the TLB, but that is too late. CPU0
> > already wrote on the page, but KSM ignored this write, and it got lost.
> > "
> >
> > In above scenario, MADV_FREE is fixed by changing TLB batching API
> > including [set|clear]_tlb_flush_pending. Remained thing is soft-dirty part.
> >
> > This patch changes soft-dirty uses TLB batching API instead of flush_tlb_mm
> > and KSM checks pending TLB flush by using mm_tlb_flush_pending so that
> > it will flush TLB to avoid data lost if there are other parallel threads
> > pending TLB flush.
> >
> > [1] http://lkml.kernel.org/r/BD3A0EBE-ECF4-41D4-87FA-C755EA9AB6BD@gmail.com
> >
> > Note:
> > I failed to reproduce this problem through Nadav's test program which
> > need to tune timing in my system speed so didn't confirm it work.
> > Nadav, Could you test this patch on your test machine?
> >
> > Thanks!
> >
> > Cc: Nadav Amit <nadav.amit@...il.com>
> > Cc: Mel Gorman <mgorman@...hsingularity.net>
> > Cc: Hugh Dickins <hughd@...gle.com>
> > Cc: Andrea Arcangeli <aarcange@...hat.com>
> > Signed-off-by: Minchan Kim <minchan@...nel.org>
> > ---
> > fs/proc/task_mmu.c | 4 +++-
> > mm/ksm.c | 3 ++-
> > 2 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index 9782dedeead7..58ef3a6abbc0 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -1018,6 +1018,7 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
> > enum clear_refs_types type;
> > int itype;
> > int rv;
> > + struct mmu_gather tlb;
> >
> > memset(buffer, 0, sizeof(buffer));
> > if (count > sizeof(buffer) - 1)
> > @@ -1062,6 +1063,7 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
> > }
> >
> > down_read(&mm->mmap_sem);
> > + tlb_gather_mmu(&tlb, mm, 0, -1);
> > if (type == CLEAR_REFS_SOFT_DIRTY) {
> > for (vma = mm->mmap; vma; vma = vma->vm_next) {
> > if (!(vma->vm_flags & VM_SOFTDIRTY))
> > @@ -1083,7 +1085,7 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
> > walk_page_range(0, mm->highest_vm_end, &clear_refs_walk);
> > if (type == CLEAR_REFS_SOFT_DIRTY)
> > mmu_notifier_invalidate_range_end(mm, 0, -1);
> > - flush_tlb_mm(mm);
> > + tlb_finish_mmu(&tlb, 0, -1);
> > up_read(&mm->mmap_sem);
> > out_mm:
> > mmput(mm);
> > diff --git a/mm/ksm.c b/mm/ksm.c
> > index 0c927e36a639..15dd7415f7b3 100644
> > --- a/mm/ksm.c
> > +++ b/mm/ksm.c
> > @@ -1038,7 +1038,8 @@ static int write_protect_page(struct vm_area_struct *vma, struct page *page,
> > goto out_unlock;
> >
> > if (pte_write(*pvmw.pte) || pte_dirty(*pvmw.pte) ||
> > - (pte_protnone(*pvmw.pte) && pte_savedwrite(*pvmw.pte))) {
> > + (pte_protnone(*pvmw.pte) && pte_savedwrite(*pvmw.pte)) ||
> > + mm_tlb_flush_pending(mm)) {
> > pte_t entry;
> >
> > swapped = PageSwapCache(page);
> > --
> > 2.7.4
>
> I tested the patch-set, and my PoC does not fail anymore.
Thanks for the testing with great reproduing application, Nadav!
Powered by blists - more mailing lists