Re: [PATCH 1/3] Fix COW D-cache aliasing on fork

[Ralf Baechle <ralf@linux-mips.org>]

On Sat, Oct 21, 2006 at 12:39:40AM +1000, Nick Piggin wrote:

> >>That would require changing the order of cache flush and tlb flush.
> >>To keep certain architectures that require a valid translation in
> >>the TLB the cacheflush has to be done first.  Not sure if those
> >>architectures need a writeable mapping for dirty cachelines - I
> >>think hypersparc was one of them.
> >
> >
> >There just has to be "a mapping" in the TLB so that the L2 cache can
> >translate the virtual address to a physical one for the writeback to
> >main memory.
> 
> So moving the flush_cache_mm below the copy_page_range, to just
> before the flush_tlb_mm, would work then? This would make the
> race much smaller than with this patchset.

90% of this changeset are MIPS-specific code.  Of that in turn much is
just infrastructure which is already being used anyway.

> But doesn't that still leave a race?

Both calls would have to be done  under the mmap_sem to close any races.

> What if another thread writes to cache after we have flushed it
> but before flushing the TLBs? Although we've marked the the ptes
> readonly, the CPU won't trap if the TLB is valid? There must be
> some special way for the arch to handle this, but I can't see it.

There isn't really.  Reordering with a patch like:

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

diff --git a/kernel/fork.c b/kernel/fork.c
index 29ebb30..28e51e0 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -202,7 +202,6 @@ static inline int dup_mmap(struct mm_str
 	struct mempolicy *pol;
 
 	down_write(&oldmm->mmap_sem);
-	flush_cache_mm(oldmm);
 	/*
 	 * Not linked in yet - no deadlock potential:
 	 */
@@ -287,8 +286,9 @@ static inline int dup_mmap(struct mm_str
 	}
 	retval = 0;
 out:
-	up_write(&mm->mmap_sem);
 	flush_tlb_mm(oldmm);
+	flush_cache_mm(oldmm);
+	up_write(&mm->mmap_sem);
 	up_write(&oldmm->mmap_sem);
 	return retval;
 fail_nomem_policy:

should close the hole for all effected architectures.  I say should
because this patch would need another round of linux-arch reviewing and I
haven't tested it this patch yet myself.

But even so that doesn't change that I would really like to make
copy_user_highpage() an arch interface replacing copy_to_user_page.

The current way of doing things enforces a cacheflush on MIPS which itself
is pricy - 1,000 cycles when it's cheap but could be several times as
expensive.  And as a side effect of the cacheflush the process breaking
a COW page will start with a cold page.

Or if an architecture wants to be clever about aliasing and uses the
vto argument of copy_user_page to create a non-conflicting mapping it
means the mapping setup by copy_user_highpage will be unused ...

  Ralf