[PATCH V4] ARM: handle user space mapped pages in flush_kernel_dcache_page

[Jason Cooper]

It would be helpful if I actually did as I stated ;-)

Adding Greg for real this time.

On Mon, Jun 03, 2013 at 02:03:39PM -0400, Jason Cooper wrote:
> All,
> 
> Adding gregkh for insight into -stable and applying fixes to previous
> kernels when the bug was not against them (but the offending commit
> exists in them).
> 
> Greg,
> 
> Some history:  This bug stems from attempting to write O_DIRECT to a
> dm-crypt device on ARM architectures.  In my case, it was trying (years
> ago!) to put an LVM into a dm-crypt container.  This data corruption can
> be more readily seen by using dd to write to a dm-crypt device with the
> direct flag.
> 
> On Mon, Jun 03, 2013 at 07:38:18PM +0200, Simon Baatz wrote:
> > Hi Catalin,
> > 
> > On Fri, May 31, 2013 at 03:15:08PM +0100, Catalin Marinas wrote:
> > > On Fri, May 31, 2013 at 01:05:19PM +0100, Jason Cooper wrote:
> > > > On Thu, May 30, 2013 at 05:43:35PM +0100, Catalin Marinas wrote:
> > > > > On Wed, May 29, 2013 at 08:16:57PM +0100, Simon Baatz wrote:
> > > > > > On Wed, May 29, 2013 at 12:05:09PM +0100, Catalin Marinas wrote:
> > > > > > > On Sun, May 12, 2013 at 06:35:56AM +0100, Simon Baatz wrote:
> > > > > > > > Commit f8b63c1 made flush_kernel_dcache_page a no-op assuming that the pages
> > > > > > > > it needs to handle are kernel mapped only.  However, for example when doing
> > > > > > > > direct I/O, pages with user space mappings may occur.
> > > > > > > 
> > > > > > > After Nico's clarification, I think the original commit introducing this
> > > > > > > function was also incomplete (commit 73be1591 - [ARM] 5545/2: add
> > > > > > > flush_kernel_dcache_page() for ARM) since it ignores highmem pages and
> > > > > > > their flushing could be deferred for a long time.
> > > > > > 
> > > > > > Yes, I agree.
> > > > > >  
> > > > > > > For my understanding (if I re-read this tread) - basically code like
> > > > > > > this should not leave the user mapping inconsistent:
> > > > > > > 
> > > > > > > kmap()
> > > > > > > ...
> > > > > > > flush_kernel_dcache_page()
> > > > > > > kunmap()
> > > ...
> > > > 
> > > > wrt to 73be1591, this appears to go all the way back to v2.6.31.x... Can
> > > > Simon go ahead and put this in rmk's patch tracker and mention that it
> > > > should go to all -stable trees?
> > > 
> > > I'm not sure how easy it is to apply this patch on past stable versions.
> > > Maybe something simpler for stable like always flushing the cache in
> > > flush_kernel_dcache_page() with optimisation only in recent stable
> > > versions.
> > 
> > Looking at how to adapt the change to previous kernel versions, it
> > occurred to me that we could probably simplify the current patch
> > since we can make stronger assumptions than __flush_dcache_page()
> > can do. As discussed, __flush_dcache_page() does:
> > 
> >                         addr = kmap_high_get(page);
> >                         if (addr) {
> >                                 __cpuc_flush_dcache_area(addr, PAGE_SIZE);
> >                                 kunmap_high(page);
> >                         }
> > 
> > The kmap_high_get() is used to ensure that the page is/remains pinned
> > during the flush.  However, the API of flush_kernel_dcache_page()
> > ensures that the page is already kmapped when it is called (as in the
> > quoted example above).
> > 
> > Thus, it is already pinned and we can simply look at page_address(). 
> > If it is NULL, the page must have been obtained via kmap_atomic() and
> > we don't need to flush anyway since kunmap_atomic() will do this. 
> > 
> > The actual flush covering both the lowmem and highmem cases actually
> > is as simple as this (inspired by __flush_dcache_page() in 2.6.32):
> > 
> > 	addr = page_address(page);
> > #ifdef CONFIG_HIGHMEM
> > 	/*
> > 	 * kmap_atomic() doesn't set the page virtual address, and
> > 	 * kunmap_atomic() takes care of cache flushing already.
> > 	 */
> > 	if (addr)
> > #endif
> > 		__cpuc_flush_dcache_area(addr, PAGE_SIZE);
> > 
> > If you agree that this makes sense, I will send a new version.
> > 
> > If we change the patch this way, there is no need to split off
> > __flush_kernel_dcache_page() from __flush_dcache_page(), which also
> > makes it simpler to apply to past stable kernels.
> > 
> > The only thing we need to be aware of is the change of
> > flush_kernel_dcache_page() in 2.6.37 (commit f8b63c1).  In earlier
> > versions, we would only need to fix the highmem case.  However, I
> > wonder whether it makes sense to apply a fix to 2.6.32 and 2.6.34
> > without this ever being reported as a problem.
> 
> imho, if you discover a problem while running kernel v3.9.x, and the
> commit causing the problem goes all the way back to 2.6.32, then the fix
> should go back that far as well (assuming those kernels are still
> actively maintained).  I don't think we would need a bug report for each
> kernel version.
> 
> However, some more nuanced problems are not the result of a single
> commit, but potentially the interaction of several commits.  In this
> case, a more conservative application may be in order.
> 
> Greg, any thoughts?
> 
> thx,
> 
> Jason.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel