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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 20 Oct 2006 17:05:38 +0100
From:	Ralf Baechle <ralf@...ux-mips.org>
To:	Nick Piggin <nickpiggin@...oo.com.au>
Cc:	David Miller <davem@...emloft.net>, torvalds@...l.org,
	akpm@...l.org, linux-kernel@...r.kernel.org, anemo@....ocn.ne.jp
Subject: Re: [PATCH 1/3] Fix COW D-cache aliasing on fork

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@...ux-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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/