[patch 05/16] mm: flush dcache before writing into page to avoid alias

[patch 05/16] mm: flush dcache before writing into page to avoid alias

[akpm]

From: anfei zhou <anfei.zhou@gmail.com>

The cache alias problem will happen if the changes of user shared mapping
is not flushed before copying, then user and kernel mapping may be mapped
into two different cache line, it is impossible to guarantee the coherence
after iov_iter_copy_from_user_atomic.  So the right steps should be:

	flush_dcache_page(page);
	kmap_atomic(page);
	write to page;
	kunmap_atomic(page);
	flush_dcache_page(page);

More precisely, we might create two new APIs flush_dcache_user_page and
flush_dcache_kern_page to replace the two flush_dcache_page accordingly.

Here is a snippet tested on omap2430 with VIPT cache, and I think it is
not ARM-specific:

	int val = 0x11111111;
	fd = open("abc", O_RDWR);
	addr = mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
	*(addr+0) = 0x44444444;
	tmp = *(addr+0);
	*(addr+1) = 0x77777777;
	write(fd, &val, sizeof(int));
	close(fd);

The results are not always 0x11111111 0x77777777 at the beginning as expected.  Sometimes we see 0x44444444 0x77777777.

Signed-off-by: Anfei <anfei.zhou@gmail.com>
Cc: Russell King <rmk@arm.linux.org.uk>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Nick Piggin <nickpiggin@yahoo.com.au>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: <linux-arch@vger.kernel.org>
Cc: <stable@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/fuse/file.c |    3 +++
 mm/filemap.c   |    3 +++
 2 files changed, 6 insertions(+)

diff -puN fs/fuse/file.c~mm-flush-dcache-before-writing-into-page-to-avoid-alias fs/fuse/file.c
--- a/fs/fuse/file.c~mm-flush-dcache-before-writing-into-page-to-avoid-alias
+++ a/fs/fuse/file.c
@@ -828,6 +828,9 @@ static ssize_t fuse_fill_write_pages(str
 		if (!page)
 			break;
 
+		if (mapping_writably_mapped(mapping))
+			flush_dcache_page(page);
+
 		pagefault_disable();
 		tmp = iov_iter_copy_from_user_atomic(page, ii, offset, bytes);
 		pagefault_enable();
diff -puN mm/filemap.c~mm-flush-dcache-before-writing-into-page-to-avoid-alias mm/filemap.c
--- a/mm/filemap.c~mm-flush-dcache-before-writing-into-page-to-avoid-alias
+++ a/mm/filemap.c
@@ -2232,6 +2232,9 @@ again:
 		if (unlikely(status))
 			break;
 
+		if (mapping_writably_mapped(mapping))
+			flush_dcache_page(page);
+
 		pagefault_disable();
 		copied = iov_iter_copy_from_user_atomic(page, i, offset, bytes);
 		pagefault_enable();
_

Re: [patch 05/16] mm: flush dcache before writing into page to avoid alias

[James Bottomley]

On Tue, 2010-02-02 at 13:44 -0800, akpm@linux-foundation.org wrote:
> From: anfei zhou <anfei.zhou@gmail.com>
> 
> The cache alias problem will happen if the changes of user shared mapping
> is not flushed before copying, then user and kernel mapping may be mapped
> into two different cache line, it is impossible to guarantee the coherence
> after iov_iter_copy_from_user_atomic.  So the right steps should be:
> 
> 	flush_dcache_page(page);

This is likely unnecessary if the page has come down a standard path ...
for fuse, it is possible it didn't go through __get_user_pages().

> 	kmap_atomic(page);
> 	write to page;
> 	kunmap_atomic(page);
> 	flush_dcache_page(page);
> 
> More precisely, we might create two new APIs flush_dcache_user_page and
> flush_dcache_kern_page to replace the two flush_dcache_page accordingly.

We already have one of those: flush_kernel_dcache_page().

James

Re: [patch 05/16] mm: flush dcache before writing into page to avoid alias

[anfei]

On Wed, Feb 03, 2010 at 12:25:07PM -0600, James Bottomley wrote:
> On Tue, 2010-02-02 at 13:44 -0800, akpm@linux-foundation.org wrote:
> > From: anfei zhou <anfei.zhou@gmail.com>
> > 
> > The cache alias problem will happen if the changes of user shared mapping
> > is not flushed before copying, then user and kernel mapping may be mapped
> > into two different cache line, it is impossible to guarantee the coherence
> > after iov_iter_copy_from_user_atomic.  So the right steps should be:
> > 
> > 	flush_dcache_page(page);
> 
> This is likely unnecessary if the page has come down a standard path ...
> for fuse, it is possible it didn't go through __get_user_pages().
> 
The problem here is iov_iter_copy_from_user_atomic will write the page,
and that page has been written by (*addr = val) before, but not flushed
yet.  If these two writes are at the same cache line (but aliased), then
we can not guarantee the result by any order.

> > 	kmap_atomic(page);
> > 	write to page;
> > 	kunmap_atomic(page);
> > 	flush_dcache_page(page);
> > 
> > More precisely, we might create two new APIs flush_dcache_user_page and
> > flush_dcache_kern_page to replace the two flush_dcache_page accordingly.
> 
> We already have one of those: flush_kernel_dcache_page().
> 
Yes, but it's only called in a few places, and others usually use
flush_dcache_page, why?

And not every arch with aliasing problem has implement it, such as MIPS.

Regards,
Anfei.

> James
> 
>