Re: [PATCH] mm: D-cache aliasing issue in cow_user_page

Re: [PATCH] mm: D-cache aliasing issue in cow_user_page

[Matt Reimer]

In light of James Bottomsley's commit[1] declaring that kmap() and
friends now have to take care of coherency issues, is the patch "mm:
D-cache aliasing issue in cow_user_page"[2] correct, or could it
potentially cause a slowdown by calling flush_dcache_page() a second
time (i.e. once in an architecture-specific kmap() implementation, and
once in cow_user_page())?

Matt

[1] http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=a6ca1b99ed434f3fb41bbed647ed36c0420501e5
[2] http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=c4ec7b0de4bc18ccb4380de638550984d9a65c25

Re: [PATCH] mm: D-cache aliasing issue in cow_user_page

[David Miller]

From: "Matt Reimer" <mattjreimer@gmail.com>
Date: Mon, 4 Dec 2006 23:38:13 -0800

> In light of James Bottomsley's commit[1] declaring that kmap() and
> friends now have to take care of coherency issues, is the patch "mm:
> D-cache aliasing issue in cow_user_page"[2] correct, or could it
> potentially cause a slowdown by calling flush_dcache_page() a second
> time (i.e. once in an architecture-specific kmap() implementation, and
> once in cow_user_page())?

kmap() is a NOP unless HIGHMEM is configured.

Therefore, it cannot possibly take care of D-cache aliasing issues
across the board.

Re: [PATCH] mm: D-cache aliasing issue in cow_user_page

[Matt Reimer]

On 12/5/06, David Miller <davem@davemloft.net> wrote:
> From: "Matt Reimer" <mattjreimer@gmail.com>
> Date: Mon, 4 Dec 2006 23:38:13 -0800
>
> > In light of James Bottomsley's commit[1] declaring that kmap() and
> > friends now have to take care of coherency issues, is the patch "mm:
> > D-cache aliasing issue in cow_user_page"[2] correct, or could it
> > potentially cause a slowdown by calling flush_dcache_page() a second
> > time (i.e. once in an architecture-specific kmap() implementation, and
> > once in cow_user_page())?
>
> kmap() is a NOP unless HIGHMEM is configured.
>
> Therefore, it cannot possibly take care of D-cache aliasing issues
> across the board.

Right, but isn't he declaring that each architecture needs to take
care of this? So, say, on ARM we'd need to make kunmap() not a NOP and
call flush_dcache_page() ?

Matt

Re: [PATCH] mm: D-cache aliasing issue in cow_user_page

[David Miller]

From: "Matt Reimer" <mattjreimer@gmail.com>
Date: Tue, 5 Dec 2006 16:57:12 -0800

> Right, but isn't he declaring that each architecture needs to take
> care of this? So, say, on ARM we'd need to make kunmap() not a NOP and
> call flush_dcache_page() ?

No.  He is only solving a problem that occurs on HIGHMEM
configurations on systems which can have D-cache aliasing
issues.

Re: [PATCH] mm: D-cache aliasing issue in cow_user_page

[Matt Reimer]

On 12/5/06, David Miller <davem@davemloft.net> wrote:
> From: "Matt Reimer" <mattjreimer@gmail.com>
> Date: Tue, 5 Dec 2006 16:57:12 -0800
>
> > Right, but isn't he declaring that each architecture needs to take
> > care of this? So, say, on ARM we'd need to make kunmap() not a NOP and
> > call flush_dcache_page() ?
>
> No.  He is only solving a problem that occurs on HIGHMEM
> configurations on systems which can have D-cache aliasing
> issues.

Ok. Thanks for the clarification.

Matt

Re: [PATCH] mm: D-cache aliasing issue in cow_user_page

[Matt Reimer]

On 12/5/06, David Miller <davem@davemloft.net> wrote:
> From: "Matt Reimer" <mattjreimer@gmail.com>
> Date: Tue, 5 Dec 2006 16:57:12 -0800
>
> > Right, but isn't he declaring that each architecture needs to take
> > care of this? So, say, on ARM we'd need to make kunmap() not a NOP and
> > call flush_dcache_page() ?
>
> No.  He is only solving a problem that occurs on HIGHMEM
> configurations on systems which can have D-cache aliasing
> issues.

Are you sure? James specifically mentions "non-highmem architectures,"
and "all architectures with coherence issues," which would seem to
include ARM (which is my concern).

For your convenience I quote the whole commit message below.

Matt

[PATCH] update to the kernel kmap/kunmap API

Give non-highmem architectures access to the kmap API for the purposes of
overriding (this is what the attached patch does).

The proposal is that we should now require all architectures with coherence
issues to manage data coherence via the kmap/kunmap API.  Thus driver
writers never have to write code like

    kmap(page)
    modify data in page
    flush_kernel_dcache_page(page)
    kunmap(page)

instead, kmap/kunmap will manage the coherence and driver (and filesystem)
writers don't need to worry about how to flush between kmap and kunmap.

For most architectures, the page only needs to be flushed if it was
actually written to *and* there are user mappings of it, so the best
implementation looks to be: clear the page dirty pte bit in the kernel page
tables on kmap and on kunmap, check page->mappings for user maps, and then
the dirty bit, and only flush if it both has user mappings and is dirty.

Re: [PATCH] mm: D-cache aliasing issue in cow_user_page

[David Miller]

From: "Matt Reimer" <mattjreimer@gmail.com>
Date: Wed, 6 Dec 2006 13:19:41 -0800

> On 12/5/06, David Miller <davem@davemloft.net> wrote:
> > From: "Matt Reimer" <mattjreimer@gmail.com>
> > Date: Tue, 5 Dec 2006 16:57:12 -0800
> >
> > > Right, but isn't he declaring that each architecture needs to take
> > > care of this? So, say, on ARM we'd need to make kunmap() not a NOP and
> > > call flush_dcache_page() ?
> >
> > No.  He is only solving a problem that occurs on HIGHMEM
> > configurations on systems which can have D-cache aliasing
> > issues.
> 
> Are you sure? James specifically mentions "non-highmem architectures,"
> and "all architectures with coherence issues," which would seem to
> include ARM (which is my concern).
> 
> For your convenience I quote the whole commit message below.

Ok, I see.

He's providing it an alternative way to solve the coherency
issues.

You can still solve it the traditional way via cache flushing
in flush_dcache_page() and {copy,clear}_user_page().

Re: [PATCH] mm: D-cache aliasing issue in cow_user_page

[Matt Reimer]

On 12/6/06, David Miller <davem@davemloft.net> wrote:
> From: "Matt Reimer" <mattjreimer@gmail.com>
> Date: Wed, 6 Dec 2006 13:19:41 -0800
>
> > On 12/5/06, David Miller <davem@davemloft.net> wrote:
> > > From: "Matt Reimer" <mattjreimer@gmail.com>
> > > Date: Tue, 5 Dec 2006 16:57:12 -0800
> > >
> > > > Right, but isn't he declaring that each architecture needs to take
> > > > care of this? So, say, on ARM we'd need to make kunmap() not a NOP and
> > > > call flush_dcache_page() ?
> > >
> > > No.  He is only solving a problem that occurs on HIGHMEM
> > > configurations on systems which can have D-cache aliasing
> > > issues.
> >
> > Are you sure? James specifically mentions "non-highmem architectures,"
> > and "all architectures with coherence issues," which would seem to
> > include ARM (which is my concern).
> >
> > For your convenience I quote the whole commit message below.
>
> Ok, I see.
>
> He's providing it an alternative way to solve the coherency
> issues.
>
> You can still solve it the traditional way via cache flushing
> in flush_dcache_page() and {copy,clear}_user_page().

Ok, good to know, since that's what we're doing with ARM drivers
presently. What's the preferred method going forward?

If architectures with coherency problems have to take care of this in
their kmap() implementations, wouldn't commits like [1] below result
in a pessmization for these architectures, since effectively the flush
would happen twice (once by architecture-specific kunmap, and once by
the flush_dcache_page() being added in this commit)?

Matt

[1] http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=c4ec7b0de4bc18ccb4380de638550984d9a65c25

Re: [PATCH] mm: D-cache aliasing issue in cow_user_page

[David Miller]

From: "Matt Reimer" <mattjreimer@gmail.com>
Date: Wed, 6 Dec 2006 22:20:22 -0800

> Ok, good to know, since that's what we're doing with ARM drivers
> presently. What's the preferred method going forward?

There are multiple ways provided to solve the problem so that
platforms can use whichever variant works best.

Re: [PATCH] mm:D-cache aliasing issue in cow_user_page

[Andrew Morton]

On Thu, 19 Oct 2006 00:22:37 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> > > Architectures typically take care of this in copy_user_page() and
> > > clear_user_page().  The absolutely depend upon those two routines
> > > being used for anonymous pages, and handle the D-cache issues there.
> > 
> > Only anonymous pages?  There are zillions of places where we modify
> > pagecache without a flush, especially against the blockdev mapping (fs
> > metadata).
> 
> It's cpu stores that matter, not device DMA and the like, and we have
> flush_dcache_page() calls in the correct spots.  You can see that
> we take care of this even in places such as the loop driver :-)

grep b_data fs/*/*.c

All of them ;)

It's possible to mmap /dev/hda, but it's a rather odd thing to do (INND
can do it, but not a live filesystem).

Re: [PATCH] mm:D-cache aliasing issue in cow_user_page

[David Miller]

From: Andrew Morton <akpm@osdl.org>
Date: Thu, 19 Oct 2006 00:17:47 -0700

> On Thu, 19 Oct 2006 00:00:27 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > Unfortunately, the kernel has just touched the page and thus there are
> > active cache lines for the kernel side mapping.  When we map this into
> > user space, userspace might see stale cachelines instead of the
> > memset() stores.
> 
> hm.  Has it always been that way or did something change?

Always.

> > Architectures typically take care of this in copy_user_page() and
> > clear_user_page().  The absolutely depend upon those two routines
> > being used for anonymous pages, and handle the D-cache issues there.
> 
> Only anonymous pages?  There are zillions of places where we modify
> pagecache without a flush, especially against the blockdev mapping (fs
> metadata).

It's cpu stores that matter, not device DMA and the like, and we have
flush_dcache_page() calls in the correct spots.  You can see that
we take care of this even in places such as the loop driver :-)

Re: [PATCH] mm:D-cache aliasing issue in cow_user_page

[Andrew Morton]

On Thu, 19 Oct 2006 00:00:27 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> > >   1452                  if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE))
> > >   1453                          memset(kaddr, 0, PAGE_SIZE);
> > >   1454                  kunmap_atomic(kaddr, KM_USER0);
> > >   #### D-cache have to be flushed here.
> > >   #### It seems it is just forgotten.
> > > 
> > >   1455                  return;
> > >   1456                  
> > >   1457          }
> > >   1458          copy_user_highpage(dst, src, va);
> > >   #### Ok here. flush_dcache_page() called from this func if arch need it 
> > >   1459  }
> > > 
> > 
> > This page has just been allocated and is private to the caller - there can
> > be no userspace mappings of it.
> 
> Unfortunately, the kernel has just touched the page and thus there are
> active cache lines for the kernel side mapping.  When we map this into
> user space, userspace might see stale cachelines instead of the
> memset() stores.

hm.  Has it always been that way or did something change?

> Architectures typically take care of this in copy_user_page() and
> clear_user_page().  The absolutely depend upon those two routines
> being used for anonymous pages, and handle the D-cache issues there.

Only anonymous pages?  There are zillions of places where we modify
pagecache without a flush, especially against the blockdev mapping (fs
metadata).

> But this code is going outside of that scope, and therefore needs
> an explicit D-cache flush.

OK, I'll add the patch.

Re: [PATCH] mm:D-cache aliasing issue in cow_user_page

[David Miller]

From: Andrew Morton <akpm@osdl.org>
Date: Wed, 18 Oct 2006 23:33:02 -0700

> On Tue, 17 Oct 2006 13:15:37 +0400
> Dmitriy Monakhov <dmonakhov@openvz.org> wrote:
> 
> >  from mm/memory.c:
> >   1434  static inline void cow_user_page(struct page *dst, struct page *src, unsigned long va)
> >   1435  {
> >   1436          /*
> >   1437           * If the source page was a PFN mapping, we don't have
> >   1438           * a "struct page" for it. We do a best-effort copy by
> >   1439           * just copying from the original user address. If that
> >   1440           * fails, we just zero-fill it. Live with it.
> >   1441           */
> >   1442          if (unlikely(!src)) {
> >   1443                  void *kaddr = kmap_atomic(dst, KM_USER0);
> >   1444                  void __user *uaddr = (void __user *)(va & PAGE_MASK);
> >   1445  
> >   1446                  /*
> >   1447                   * This really shouldn't fail, because the page is there
> >   1448                   * in the page tables. But it might just be unreadable,
> >   1449                   * in which case we just give up and fill the result with
> >   1450                   * zeroes.
> >   1451                   */
> >   1452                  if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE))
> >   1453                          memset(kaddr, 0, PAGE_SIZE);
> >   1454                  kunmap_atomic(kaddr, KM_USER0);
> >   #### D-cache have to be flushed here.
> >   #### It seems it is just forgotten.
> > 
> >   1455                  return;
> >   1456                  
> >   1457          }
> >   1458          copy_user_highpage(dst, src, va);
> >   #### Ok here. flush_dcache_page() called from this func if arch need it 
> >   1459  }
> > 
> 
> This page has just been allocated and is private to the caller - there can
> be no userspace mappings of it.

Unfortunately, the kernel has just touched the page and thus there are
active cache lines for the kernel side mapping.  When we map this into
user space, userspace might see stale cachelines instead of the
memset() stores.

Architectures typically take care of this in copy_user_page() and
clear_user_page().  The absolutely depend upon those two routines
being used for anonymous pages, and handle the D-cache issues there.
But this code is going outside of that scope, and therefore needs
an explicit D-cache flush.

Re: [PATCH] mm:D-cache aliasing issue in cow_user_page

[Andrew Morton]

On Tue, 17 Oct 2006 13:15:37 +0400
Dmitriy Monakhov <dmonakhov@openvz.org> wrote:

>  from mm/memory.c:
>   1434  static inline void cow_user_page(struct page *dst, struct page *src, unsigned long va)
>   1435  {
>   1436          /*
>   1437           * If the source page was a PFN mapping, we don't have
>   1438           * a "struct page" for it. We do a best-effort copy by
>   1439           * just copying from the original user address. If that
>   1440           * fails, we just zero-fill it. Live with it.
>   1441           */
>   1442          if (unlikely(!src)) {
>   1443                  void *kaddr = kmap_atomic(dst, KM_USER0);
>   1444                  void __user *uaddr = (void __user *)(va & PAGE_MASK);
>   1445  
>   1446                  /*
>   1447                   * This really shouldn't fail, because the page is there
>   1448                   * in the page tables. But it might just be unreadable,
>   1449                   * in which case we just give up and fill the result with
>   1450                   * zeroes.
>   1451                   */
>   1452                  if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE))
>   1453                          memset(kaddr, 0, PAGE_SIZE);
>   1454                  kunmap_atomic(kaddr, KM_USER0);
>   #### D-cache have to be flushed here.
>   #### It seems it is just forgotten.
> 
>   1455                  return;
>   1456                  
>   1457          }
>   1458          copy_user_highpage(dst, src, va);
>   #### Ok here. flush_dcache_page() called from this func if arch need it 
>   1459  }
> 

This page has just been allocated and is private to the caller - there can
be no userspace mappings of it.

[PATCH] mm:D-cache aliasing issue in cow_user_page

[Dmitriy Monakhov]

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

 from mm/memory.c:
  1434  static inline void cow_user_page(struct page *dst, struct page *src, unsigned long va)
  1435  {
  1436          /*
  1437           * If the source page was a PFN mapping, we don't have
  1438           * a "struct page" for it. We do a best-effort copy by
  1439           * just copying from the original user address. If that
  1440           * fails, we just zero-fill it. Live with it.
  1441           */
  1442          if (unlikely(!src)) {
  1443                  void *kaddr = kmap_atomic(dst, KM_USER0);
  1444                  void __user *uaddr = (void __user *)(va & PAGE_MASK);
  1445  
  1446                  /*
  1447                   * This really shouldn't fail, because the page is there
  1448                   * in the page tables. But it might just be unreadable,
  1449                   * in which case we just give up and fill the result with
  1450                   * zeroes.
  1451                   */
  1452                  if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE))
  1453                          memset(kaddr, 0, PAGE_SIZE);
  1454                  kunmap_atomic(kaddr, KM_USER0);
  #### D-cache have to be flushed here.
  #### It seems it is just forgotten.

  1455                  return;
  1456                  
  1457          }
  1458          copy_user_highpage(dst, src, va);
  #### Ok here. flush_dcache_page() called from this func if arch need it 
  1459  }

Following is the patch  fix this issue:
Signed-off-by: Dmitriy Monakhov <dmonakhov@openvz.org>
---

[-- Attachment #2: d-cache-aliasing-issue-in-cow-user-page.patch --]
[-- Type: text/plain, Size: 341 bytes --]

diff --git a/mm/memory.c b/mm/memory.c
index b5a4aad..156861f 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1452,6 +1452,7 @@ static inline void cow_user_page(struct 
 		if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE))
 			memset(kaddr, 0, PAGE_SIZE);
 		kunmap_atomic(kaddr, KM_USER0);
+		flush_dcache_page(dst);
 		return;
 		
 	}

[-- Attachment #3: Type: text/plain, Size: 4 bytes --]

---