lkml.org 
[lkml]   [2017]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 4/4] mm: fix KSM data corruption
From
Date
Minchan Kim <minchan@kernel.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@gmail.com>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: Minchan Kim <minchan@kernel.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,
Nadav
\
 
 \ /
  Last update: 2017-08-01 21:23    [W:0.083 / U:5.476 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site