[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

flush_anon_page on ARM - ABI change



I'd like to apply the two attached patches unless there are any
objections.  It implements flush_anon_page on ARM and for this changes
the parameter line passed to flush_anon_page() in the arch-independent
code.  I grepped debian/patches and it seems that nothing else touches
flush_anon_page() so it should be harmless for other architectures if
I apply this patch.

Having this patch fixes FUSE on ARM (see #402876) and possibly also
cryptsetup/LUKS (see #403426).

Any objections?
-- 
Martin Michlmayr
http://www.cyrius.com/
Given that no one has any outstanding issues with the following patch, I'm
going to ask akpm to put this into -mm, and shortly after (a week or so)
I'll submit it and the ARM flush_anon_page() patch to Linus for -rc to fix
ARM data corruption issues.

If anyone _does_ have a problem, holler ASAP.

On Thu, Dec 21, 2006 at 05:17:39PM +0000, Russell King wrote:
> On Thu, Dec 21, 2006 at 04:53:56PM +0100, Miklos Szeredi wrote:
> > Yes, note the flush_dcache_page() call in fuse_copy_finish().  That
> > could be replaced by the flush_kernel_dcache_page() (added by James
> > Bottomley together with flush_anon_page()) when all relevant
> > architectures have defined it.
> 
> I should say that flush_anon_page() in its current form is going to be
> problematic for ARM.  It is passed:
> 
> 1. the struct page
> 2. the virtual address in process memory for the page
> 
> It is not passed the mm or vma.  This means that we have no idea whether
> the virtual address is in the currently mapped VM space or not.  The
> common use of get_area_pages() is to get pages from other address
> spaces.
> 
> If we use the supplied virtual address to perform cache maintainence of
> the userspace mapping, we might end up hitting a completely different
> processes address space, which may contain some page sensitive to such
> operations, or may not contain any page and thereby could cause a page
> fault on some ARM CPUs.
> 
> Hence, I first need something like this patch so I can tell if the
> page to be flushed is in the current VMs address space or not:

diff --git a/Documentation/cachetlb.txt b/Documentation/cachetlb.txt
index 73e794f..920d0f6 100644
--- a/Documentation/cachetlb.txt
+++ b/Documentation/cachetlb.txt
@@ -373,7 +373,8 @@ maps this page at its virtual address.
 	likely that you will need to flush the instruction cache
 	for copy_to_user_page().
 
-  void flush_anon_page(struct page *page, unsigned long vmaddr)
+  void flush_anon_page(struct vm_area_struct *vma, struct page *page,
+                       unsigned long vmaddr)
   	When the kernel needs to access the contents of an anonymous
 	page, it calls this function (currently only
 	get_user_pages()).  Note: flush_dcache_page() deliberately
diff --git a/include/asm-parisc/cacheflush.h b/include/asm-parisc/cacheflush.h
index aedb051..a799dd8 100644
--- a/include/asm-parisc/cacheflush.h
+++ b/include/asm-parisc/cacheflush.h
@@ -186,7 +186,7 @@ flush_cache_page(struct vm_area_struct *
 }
 
 static inline void
-flush_anon_page(struct page *page, unsigned long vmaddr)
+flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned long vmaddr)
 {
 	if (PageAnon(page))
 		flush_user_dcache_page(vmaddr);
diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index ca9a602..645d440 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -8,7 +8,7 @@ #include <linux/uaccess.h>
 #include <asm/cacheflush.h>
 
 #ifndef ARCH_HAS_FLUSH_ANON_PAGE
-static inline void flush_anon_page(struct page *page, unsigned long vmaddr)
+static inline void flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned long vmaddr)
 {
 }
 #endif
diff --git a/mm/memory.c b/mm/memory.c
index c00bac6..13970fe 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1091,7 +1091,7 @@ int get_user_pages(struct task_struct *t
 			if (pages) {
 				pages[i] = page;
 
-				flush_anon_page(page, start);
+				flush_anon_page(vma, page, start);
 				flush_dcache_page(page);
 			}
 			if (vmas)


> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:
> -
> To unsubscribe from this list: send the line "unsubscribe linux-arch" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

On Sat, Dec 30, 2006 at 04:39:55PM +0000, Russell King wrote:
> Given that no one has any outstanding issues with the following patch, I'm
> going to ask akpm to put this into -mm, and shortly after (a week or so)
> I'll submit it and the ARM flush_anon_page() patch to Linus for -rc to fix
> ARM data corruption issues.

And here's the flush_anon_page() part.

Add flush_anon_page() for ARM, to avoid data corruption issues when using
fuse or other subsystems using get_user_pages().

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
index 628348c..86dd204 100644
--- a/arch/arm/mm/flush.c
+++ b/arch/arm/mm/flush.c
@@ -202,3 +202,39 @@ #endif
 	}
 }
 EXPORT_SYMBOL(flush_dcache_page);
+
+/*
+ * Flush an anonymous page so that users of get_user_pages()
+ * can safely access the data.  The expected sequence is:
+ *
+ *  get_user_pages()
+ *    -> flush_anon_page
+ *  memcpy() to/from page
+ *  if written to page, flush_dcache_page()
+ */
+void __flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned long vmaddr)
+{
+	/* VIPT non-aliasing caches need do nothing */
+	if (cache_is_vipt_nonaliasing())
+		return;
+
+	/*
+	 * Write back and invalidate userspace mapping.
+	 */
+	if (cache_is_vivt()) {
+		flush_cache_page(vma, vmaddr, page_to_pfn(page));
+	} else {
+		/*
+		 * For aliasing VIPT, we can flush an alias of the
+		 * userspace address only.
+		 */
+		flush_pfn_alias(page_to_pfn(page), vmaddr);
+	}
+
+	/*
+	 * Invalidate kernel mapping.  No data should be contained
+	 * in this mapping of the page.  FIXME: this is overkill
+	 * since we actually ask for a write-back and invalidate.
+	 */
+	__cpuc_flush_dcache_page(page_address(page));
+}
diff --git a/include/asm-arm/cacheflush.h b/include/asm-arm/cacheflush.h
index 378a3a2..506ef15 100644
--- a/include/asm-arm/cacheflush.h
+++ b/include/asm-arm/cacheflush.h
@@ -355,6 +355,16 @@ #define clean_dcache_area(start,size)	cp
  */
 extern void flush_dcache_page(struct page *);
 
+#define ARCH_HAS_FLUSH_ANON_PAGE
+static inline void flush_anon_page(struct vm_area_struct *vma,
+			 struct page *page, unsigned long vmaddr)
+{
+	extern void __flush_anon_page(struct vm_area_struct *vma,
+				struct page *, unsigned long);
+	if (PageAnon(page))
+		__flush_anon_page(vma, page, vmaddr);
+}
+
 #define flush_dcache_mmap_lock(mapping) \
 	write_lock_irq(&(mapping)->tree_lock)
 #define flush_dcache_mmap_unlock(mapping) \


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:


Reply to: