Re: Slab corruption in 2.6.16-rc5-mm2

[Linus Torvalds]

Ok, 
 I have a new favorite suspect.

It is this one: commit 4d268eba1187ef66844a6a33b9431e5d0dadd4ad:

    [PATCH] slab: extract slab order calculation to separate function
    
    This patch moves the ugly loop that determines the 'optimal' size (page order)
    of cache slabs from kmem_cache_create() to a separate function and cleans it
    up a bit.
    
    Thanks to Matthew Wilcox for the help with this patch.
    
    Signed-off-by: Matthew Dobson <colpatch@xxxxxxxxxx>
    Signed-off-by: Andrew Morton <akpm@xxxxxxxx>
    Signed-off-by: Linus Torvalds <torvalds@xxxxxxxx>

and I think it may be broken.

In particular, as far as I can tell, that

+               /* More than offslab_limit objects will cause problems */
+               if (flags & CFLGS_OFF_SLAB && cachep->num > offslab_limit)
+                       break;

has been incorrectly translated for several reasons:

 - we shouldn't check "cachep->num > offslab_limit". We should check just 
   "num > offslab_limit" (cachep->num is the _previous_ number we tested).

 - when we do "break", we've already incremented "gfporder", and we should 
   correct for that.

Now, maybe I'm just off my rocker again (I've certainly been batting 0.000 
so far, even if I think I've been finding real bugs). So who knows. But I 
get the feeling that that patch is broken.

Either revert it, or try this (TOTALLY UNTESTED!!!) patch..

And hey, maybe I'm just crazy.

		Linus

----
diff --git a/mm/slab.c b/mm/slab.c
index 2b0b151..1cca41d 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1628,25 +1628,22 @@ static inline size_t calculate_slab_orde
 			size_t size, size_t align, unsigned long flags)
 {
 	size_t left_over = 0;
+	int gfporder;
 
-	for (;; cachep->gfporder++) {
+	for (gfporder = 0 ; gfporder < MAX_GFP_ORDER; gfporder++) {
 		unsigned int num;
 		size_t remainder;
 
-		if (cachep->gfporder > MAX_GFP_ORDER) {
-			cachep->num = 0;
-			break;
-		}
-
-		cache_estimate(cachep->gfporder, size, align, flags,
-			       &remainder, &num);
+		cache_estimate(gfporder, size, align, flags, &remainder, &num);
 		if (!num)
 			continue;
+
 		/* More than offslab_limit objects will cause problems */
-		if (flags & CFLGS_OFF_SLAB && cachep->num > offslab_limit)
+		if ((flags & CFLGS_OFF_SLAB) && num > offslab_limit)
 			break;
 
 		cachep->num = num;
+		cachep->gfporder = gfporder;
 		left_over = remainder;
 
 		/*
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/