[PATCH] staging: zcache: fix crash on cpu remove

[PATCH] staging: zcache: fix crash on cpu remove

[Seth Jennings]

In the case that a cpu is taken offline before zcache_do_preload() is
ever called on the cpu, the per-cpu zcache_preloads structure will
be uninitialized.  In the CPU_DEAD case for zcache_cpu_notifier(),
kp->obj is not checked before calling kmem_cache_free() on it.
If it is NULL, a crash results.

This patch ensures that both kp->obj and kp->page are not NULL before
calling the respective free functions. In practice, just checking
one or the other should be sufficient since they are assigned together
in zcache_do_preload(), but I check both for safety.

Based on v3.1-rc8

Signed-off-by: Seth Jennings <sjenning@linux.vnet.ibm.com>
---
 drivers/staging/zcache/zcache-main.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
index 462fbc2..41d1ba1 100644
--- a/drivers/staging/zcache/zcache-main.c
+++ b/drivers/staging/zcache/zcache-main.c
@@ -1357,8 +1357,14 @@ static int zcache_cpu_notifier(struct notifier_block *nb,
 			kp->objnodes[kp->nr - 1] = NULL;
 			kp->nr--;
 		}
-		kmem_cache_free(zcache_obj_cache, kp->obj);
-		free_page((unsigned long)kp->page);
+		if (kp->obj) {
+			kmem_cache_free(zcache_obj_cache, kp->obj);
+			kp->obj = NULL;
+		}
+		if (kp->page) {
+			free_page((unsigned long)kp->page);
+			kp->page = NULL;
+		}
 		break;
 	default:
 		break;
-- 
1.7.4.1

Re: [PATCH] staging: zcache: fix crash on cpu remove

[Dave Hansen]

On Thu, 2011-10-06 at 14:28 -0500, Seth Jennings wrote:
> In the case that a cpu is taken offline before zcache_do_preload() is
> ever called on the cpu, the per-cpu zcache_preloads structure will
> be uninitialized.  In the CPU_DEAD case for zcache_cpu_notifier(),
> kp->obj is not checked before calling kmem_cache_free() on it.
> If it is NULL, a crash results.
> 
> This patch ensures that both kp->obj and kp->page are not NULL before
> calling the respective free functions. In practice, just checking
> one or the other should be sufficient since they are assigned together
> in zcache_do_preload(), but I check both for safety.

zcache_objnode_alloc() and zcache_obj_alloc() seem to do the same thing:
dereference 'kp' without checking it for NULL.  That's fine, but it
requires that preemption be off between zcache_do_preload() and calling
those functions.  That seems to be the case today, but it worries me.

I can see another user getting added that does zcache_obj_alloc() but
forgot to disable preemption.  

We at least need a BUG_ON(!in_atomic()) near the dereferences.  The
enable/disable paths out of zcache_do_preload() look OK to me, but
they're not trivial.

Was there another purpose behind the preempt_disabling()?  We have a
number of spots on the kernel that do preloads but *don't* protect the
percpu buffers in the same way.  The radix-tree code, for instance, uses
preempt_disable() to provide the guarantee that an allocation does not
occur, but its data structures are safe to access without preempt.

IOW, with the radix code, a missed preempt_disable() costs you an
-ENOMEM.  With zcache it costs you an oops.

-- Dave

Re: [PATCH] staging: zcache: fix crash on cpu remove

[Dave Hansen]

On Thu, 2011-10-06 at 14:28 -0500, Seth Jennings wrote:
> 
> +               if (kp->obj) {
> +                       kmem_cache_free(zcache_obj_cache, kp->obj);
> +                       kp->obj = NULL;
> +               }
> +               if (kp->page) {
> +                       free_page((unsigned long)kp->page);
> +                       kp->page = NULL;
> +               } 

Getting back from my preempt tangent... :)

I'm not sure how free_page() can oops.  If kp->page is NULL free_pages()
will rescue you:
        
        void free_pages(unsigned long addr, unsigned int order)
        {
                if (addr != 0) {
                        VM_BUG_ON(!virt_addr_valid((void *)addr));
                        __free_pages(virt_to_page((void *)addr), order);
                }
        }

I *thought* we did some of the same for kmem_cache_free(), but maybe
it's just kmalloc/kfree().  Anyway, it's all good.  I don't like relying
on the allocators to be nice when freeing NULL pointers anyway.

Acked-by: Dave Hansen <dave@linux.vnet.ibm.com>

-- Dave