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

[Nadav Amit]

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,
Nadav