[patch] mm: vmalloc fix lazy unmapping cache aliasing

[patch] mm: vmalloc fix lazy unmapping cache aliasing

[Nick Piggin]

Jim Radford has reported that the vmap subsystem rewrite was sometimes causing
his VIVT ARM system to behave strangely (seemed like going into infinite loops
trying to fault in pages to userspace).

We determined that the problem was most likely due to a cache aliasing issue.
flush_cache_vunmap was only being called at the moment the page tables were
to be taken down, however with lazy unmapping, this can happen after the page
has subsequently been freed and allocated for something else. The dangling
alias may still have dirty data attached to it.

The fix for this problem is to do the cache flushing when the caller has
called vunmap -- it would be a bug for them to write anything else to the
mapping at that point.

That appeared to solve Jim's problems.

Reported-by: Jim Radford <radford@blackbean.org>
Signed-off-by: Nick Piggin <npiggin@suse.de>
---
Index: linux-2.6/mm/vmalloc.c
===================================================================
--- linux-2.6.orig/mm/vmalloc.c
+++ linux-2.6/mm/vmalloc.c
@@ -77,7 +77,6 @@ static void vunmap_page_range(unsigned l
 
 	BUG_ON(addr >= end);
 	pgd = pgd_offset_k(addr);
-	flush_cache_vunmap(addr, end);
 	do {
 		next = pgd_addr_end(addr, end);
 		if (pgd_none_or_clear_bad(pgd))
@@ -543,9 +542,10 @@ static void purge_vmap_area_lazy(void)
 }
 
 /*
- * Free and unmap a vmap area
+ * Free and unmap a vmap area, caller ensuring flush_cache_vunmap had been
+ * called for the correct range previously.
  */
-static void free_unmap_vmap_area(struct vmap_area *va)
+static void free_unmap_vmap_area_noflush(struct vmap_area *va)
 {
 	va->flags |= VM_LAZY_FREE;
 	atomic_add((va->va_end - va->va_start) >> PAGE_SHIFT, &vmap_lazy_nr);
@@ -553,6 +553,15 @@ static void free_unmap_vmap_area(struct 
 		try_purge_vmap_area_lazy();
 }
 
+/*
+ * Free and unmap a vmap area
+ */
+static void free_unmap_vmap_area(struct vmap_area *va)
+{
+	flush_cache_vunmap(va->va_start, va->va_end);
+	free_unmap_vmap_area_noflush(va);
+}
+
 static struct vmap_area *find_vmap_area(unsigned long addr)
 {
 	struct vmap_area *va;
@@ -734,7 +743,7 @@ static void free_vmap_block(struct vmap_
 	spin_unlock(&vmap_block_tree_lock);
 	BUG_ON(tmp != vb);
 
-	free_unmap_vmap_area(vb->va);
+	free_unmap_vmap_area_noflush(vb->va);
 	call_rcu(&vb->rcu_head, rcu_free_vb);
 }
 
@@ -820,6 +829,8 @@ static void vb_free(const void *addr, un
 		free_vmap_block(vb);
 	} else
 		spin_unlock(&vb->lock);
+
+	flush_cache_vunmap((unsigned long)addr, (unsigned long)addr + size);
 }
 
 /**

Re: [patch] mm: vmalloc fix lazy unmapping cache aliasing

[Nick Piggin]

Added Jim to CC...

On Wed, Nov 19, 2008 at 05:53:38AM +0100, Nick Piggin wrote:
> 
> Jim Radford has reported that the vmap subsystem rewrite was sometimes causing
> his VIVT ARM system to behave strangely (seemed like going into infinite loops
> trying to fault in pages to userspace).
> 
> We determined that the problem was most likely due to a cache aliasing issue.
> flush_cache_vunmap was only being called at the moment the page tables were
> to be taken down, however with lazy unmapping, this can happen after the page
> has subsequently been freed and allocated for something else. The dangling
> alias may still have dirty data attached to it.
> 
> The fix for this problem is to do the cache flushing when the caller has
> called vunmap -- it would be a bug for them to write anything else to the
> mapping at that point.
> 
> That appeared to solve Jim's problems.
> 
> Reported-by: Jim Radford <radford@blackbean.org>
> Signed-off-by: Nick Piggin <npiggin@suse.de>
> ---
> Index: linux-2.6/mm/vmalloc.c
> ===================================================================
> --- linux-2.6.orig/mm/vmalloc.c
> +++ linux-2.6/mm/vmalloc.c
> @@ -77,7 +77,6 @@ static void vunmap_page_range(unsigned l
>  
>  	BUG_ON(addr >= end);
>  	pgd = pgd_offset_k(addr);
> -	flush_cache_vunmap(addr, end);
>  	do {
>  		next = pgd_addr_end(addr, end);
>  		if (pgd_none_or_clear_bad(pgd))
> @@ -543,9 +542,10 @@ static void purge_vmap_area_lazy(void)
>  }
>  
>  /*
> - * Free and unmap a vmap area
> + * Free and unmap a vmap area, caller ensuring flush_cache_vunmap had been
> + * called for the correct range previously.
>   */
> -static void free_unmap_vmap_area(struct vmap_area *va)
> +static void free_unmap_vmap_area_noflush(struct vmap_area *va)
>  {
>  	va->flags |= VM_LAZY_FREE;
>  	atomic_add((va->va_end - va->va_start) >> PAGE_SHIFT, &vmap_lazy_nr);
> @@ -553,6 +553,15 @@ static void free_unmap_vmap_area(struct 
>  		try_purge_vmap_area_lazy();
>  }
>  
> +/*
> + * Free and unmap a vmap area
> + */
> +static void free_unmap_vmap_area(struct vmap_area *va)
> +{
> +	flush_cache_vunmap(va->va_start, va->va_end);
> +	free_unmap_vmap_area_noflush(va);
> +}
> +
>  static struct vmap_area *find_vmap_area(unsigned long addr)
>  {
>  	struct vmap_area *va;
> @@ -734,7 +743,7 @@ static void free_vmap_block(struct vmap_
>  	spin_unlock(&vmap_block_tree_lock);
>  	BUG_ON(tmp != vb);
>  
> -	free_unmap_vmap_area(vb->va);
> +	free_unmap_vmap_area_noflush(vb->va);
>  	call_rcu(&vb->rcu_head, rcu_free_vb);
>  }
>  
> @@ -820,6 +829,8 @@ static void vb_free(const void *addr, un
>  		free_vmap_block(vb);
>  	} else
>  		spin_unlock(&vb->lock);
> +
> +	flush_cache_vunmap((unsigned long)addr, (unsigned long)addr + size);
>  }
>  
>  /**

Re: [patch] mm: vmalloc fix lazy unmapping cache aliasing

[Jim Radford]

On Wed, Nov 19, 2008 at 05:54:25AM +0100, Nick Piggin wrote:
> On Wed, Nov 19, 2008 at 05:53:38AM +0100, Nick Piggin wrote:
> > Jim Radford has reported that the vmap subsystem rewrite was sometimes causing
> > his VIVT ARM system to behave strangely (seemed like going into infinite loops
> > trying to fault in pages to userspace).

> > We determined that the problem was most likely due to a cache aliasing issue.
> > flush_cache_vunmap was only being called at the moment the page tables were
> > to be taken down, however with lazy unmapping, this can happen after the page
> > has subsequently been freed and allocated for something else. The dangling
> > alias may still have dirty data attached to it.

> > The fix for this problem is to do the cache flushing when the caller has
> > called vunmap -- it would be a bug for them to write anything else to the
> > mapping at that point.

> Added Jim to CC...

Thanks Nick.  I just wanted to confirm that this latest version of the
patch works for me as well.

-Jim

Re: [patch] mm: vmalloc fix lazy unmapping cache aliasing

[Russell King]

On Wed, Nov 19, 2008 at 05:53:38AM +0100, Nick Piggin wrote:
> We determined that the problem was most likely due to a cache aliasing
> issue.  flush_cache_vunmap was only being called at the moment the page
> tables were to be taken down, however with lazy unmapping, this can
> happen after the page has subsequently been freed and allocated for
> something else. The dangling alias may still have dirty data attached
> to it.

This would certainly cause problems - writes which hit the cache for the
vmapped pages will remain in the cache, and would be evicted some time
later.

With VIVT ARMs, that time is bounded by the context switch, which does a
full cache flush, but between that a page may well have been reallocated
for some other purpose.

So, ensuring that we have no dirty lines corresponding to stale mappings
on pages before they're freed is an absolute must for VIVT caches.

> @@ -734,7 +743,7 @@ static void free_vmap_block(struct vmap_
>  	spin_unlock(&vmap_block_tree_lock);
>  	BUG_ON(tmp != vb);
>  
> -	free_unmap_vmap_area(vb->va);
> +	free_unmap_vmap_area_noflush(vb->va);
>  	call_rcu(&vb->rcu_head, rcu_free_vb);
>  }
>  
> @@ -820,6 +829,8 @@ static void vb_free(const void *addr, un
>  		free_vmap_block(vb);
>  	} else
>  		spin_unlock(&vb->lock);
> +
> +	flush_cache_vunmap((unsigned long)addr, (unsigned long)addr + size);

One minor nit - this looks like we're flushing the cache after the
range has been marked as needing the mapping torn down.  Can the
mapping be torn down before the cache is flushed?

The reason I ask is that with VIPT caches, the mapping has to be
present so the hardware knows the physical tag to be flushed, so if
the mapping has been torn down, we'll get an exception.  _If_ and
only if we convert flush_cache_vunmap() to be less heavy weight
than its current flush_cache_all().

IOW, shouldn't flush_cache_vunmap() be towards the start of vb_free() ?

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

Re: [patch] mm: vmalloc fix lazy unmapping cache aliasing

[Nick Piggin]

On Wed, Nov 19, 2008 at 08:17:15PM +0000, Russell King wrote:
> On Wed, Nov 19, 2008 at 05:53:38AM +0100, Nick Piggin wrote:
> > We determined that the problem was most likely due to a cache aliasing
> > issue.  flush_cache_vunmap was only being called at the moment the page
> > tables were to be taken down, however with lazy unmapping, this can
> > happen after the page has subsequently been freed and allocated for
> > something else. The dangling alias may still have dirty data attached
> > to it.
> 
> This would certainly cause problems - writes which hit the cache for the
> vmapped pages will remain in the cache, and would be evicted some time
> later.
> 
> With VIVT ARMs, that time is bounded by the context switch, which does a
> full cache flush, but between that a page may well have been reallocated
> for some other purpose.
> 
> So, ensuring that we have no dirty lines corresponding to stale mappings
> on pages before they're freed is an absolute must for VIVT caches.
> 
> > @@ -734,7 +743,7 @@ static void free_vmap_block(struct vmap_
> >  	spin_unlock(&vmap_block_tree_lock);
> >  	BUG_ON(tmp != vb);
> >  
> > -	free_unmap_vmap_area(vb->va);
> > +	free_unmap_vmap_area_noflush(vb->va);
> >  	call_rcu(&vb->rcu_head, rcu_free_vb);
> >  }
> >  
> > @@ -820,6 +829,8 @@ static void vb_free(const void *addr, un
> >  		free_vmap_block(vb);
> >  	} else
> >  		spin_unlock(&vb->lock);
> > +
> > +	flush_cache_vunmap((unsigned long)addr, (unsigned long)addr + size);
> 
> One minor nit - this looks like we're flushing the cache after the
> range has been marked as needing the mapping torn down.  Can the
> mapping be torn down before the cache is flushed?
> 
> The reason I ask is that with VIPT caches, the mapping has to be
> present so the hardware knows the physical tag to be flushed, so if
> the mapping has been torn down, we'll get an exception.  _If_ and
> only if we convert flush_cache_vunmap() to be less heavy weight
> than its current flush_cache_all().
> 
> IOW, shouldn't flush_cache_vunmap() be towards the start of vb_free() ?

You are correct. Thanks.

Andrew, can you queue this incremental fix please?

--
Russell King wrote:
> One minor nit - this looks like we're flushing the cache after the
> range has been marked as needing the mapping torn down.  Can the
> mapping be torn down before the cache is flushed?
>
> The reason I ask is that with VIPT caches, the mapping has to be
> present so the hardware knows the physical tag to be flushed, so if
> the mapping has been torn down, we'll get an exception.  _If_ and
> only if we convert flush_cache_vunmap() to be less heavy weight
> than its current flush_cache_all().
>
> IOW, shouldn't flush_cache_vunmap() be towards the start of vb_free() ?

Signed-off-by: Nick Piggin <npiggin@suse.de>
---
Index: linux-2.6/mm/vmalloc.c
===================================================================
--- linux-2.6.orig/mm/vmalloc.c
+++ linux-2.6/mm/vmalloc.c
@@ -805,6 +805,9 @@ static void vb_free(const void *addr, un
 
 	BUG_ON(size & ~PAGE_MASK);
 	BUG_ON(size > PAGE_SIZE*VMAP_MAX_ALLOC);
+
+	flush_cache_vunmap((unsigned long)addr, (unsigned long)addr + size);
+
 	order = get_order(size);
 
 	offset = (unsigned long)addr & (VMAP_BLOCK_SIZE - 1);
@@ -829,8 +832,6 @@ static void vb_free(const void *addr, un
 		free_vmap_block(vb);
 	} else
 		spin_unlock(&vb->lock);
-
-	flush_cache_vunmap((unsigned long)addr, (unsigned long)addr + size);
 }
 
 /**

Re: [patch] mm: vmalloc fix lazy unmapping cache aliasing

[Catalin Marinas]

On Wed, 2008-11-19 at 20:17 +0000, Russell King wrote:
> The reason I ask is that with VIPT caches, the mapping has to be
> present so the hardware knows the physical tag to be flushed, so if
> the mapping has been torn down, we'll get an exception.  _If_ and
> only if we convert flush_cache_vunmap() to be less heavy weight
> than its current flush_cache_all().

I posted a patch for simplifying flush_cache_v(un)map some time ago but
only for the VIPT non-aliasing case. I think we still need it to do
cache flushing for the VIPT aliasing systems.

-- 
Catalin


-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: [patch] mm: vmalloc fix lazy unmapping cache aliasing

[Catalin Marinas]

On Wed, 2008-11-19 at 20:17 +0000, Russell King wrote:
> The reason I ask is that with VIPT caches, the mapping has to be
> present so the hardware knows the physical tag to be flushed, so if
> the mapping has been torn down, we'll get an exception.  _If_ and
> only if we convert flush_cache_vunmap() to be less heavy weight
> than its current flush_cache_all().

I posted a patch for simplifying flush_cache_v(un)map some time ago but
only for the VIPT non-aliasing case. I think we still need it to do
cache flushing for the VIPT aliasing systems.

-- 
Catalin