Re: [PATCH v2 4/4] mm: fix KSM data corruption

[Minchan Kim]

On Tue, Aug 01, 2017 at 12:21:41PM -0700, Nadav Amit wrote:
> Minchan Kim <minchan@xxxxxxxxxx> 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@xxxxxxxxx
> > 
> > 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@xxxxxxxxx>
> > Cc: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
> > Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> > Cc: Andrea Arcangeli <aarcange@xxxxxxxxxx>
> > Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
> > ---
> > 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!