[PATCH] slab: ensure cache_alloc_refill terminates

[PATCH] slab: ensure cache_alloc_refill terminates

[Pekka J Enberg]

From: Pekka Enberg <penberg@cs.helsinki.fi>

If slab->inuse is corrupted, cache_alloc_refill can enter an infinite
loop as detailed by Michael Richardson in the following post:
<http://lkml.org/lkml/2007/2/16/292>. This adds a BUG_ON to catch
those cases.

Cc: Michael Richardson <mcr@sandelman.ca>
Cc: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 mm/slab.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: 2.6/mm/slab.c
===================================================================
--- 2.6.orig/mm/slab.c	2007-02-19 15:15:17.000000000 +0200
+++ 2.6/mm/slab.c	2007-02-20 08:39:26.000000000 +0200
@@ -2987,6 +2987,14 @@
 		slabp = list_entry(entry, struct slab, list);
 		check_slabp(cachep, slabp);
 		check_spinlock_acquired(cachep);
+
+		/*
+		 * The slab was either on partial or free list so
+		 * there must be at least one object available for
+		 * allocation.
+		 */
+		BUG_ON(slabp->inuse < 0 || slabp->inuse >= cachep->num);
+
 		while (slabp->inuse < cachep->num && batchcount--) {
 			STATS_INC_ALLOCED(cachep);
 			STATS_INC_ACTIVE(cachep);

Re: [PATCH] slab: ensure cache_alloc_refill terminates

[Christoph Lameter]

On Wed, 21 Feb 2007, Pekka J Enberg wrote:

> +		 */
> +		BUG_ON(slabp->inuse < 0 || slabp->inuse >= cachep->num);
> +
>  		while (slabp->inuse < cachep->num && batchcount--) {

I think you only need to check for <0. If slabp->inuse > cachep->num then 
the loop will not be taken.

Re: [PATCH] slab: ensure cache_alloc_refill terminates

[Pekka Enberg]

On Wed, 21 Feb 2007, Pekka J Enberg wrote:
> > +              */
> > +             BUG_ON(slabp->inuse < 0 || slabp->inuse >= cachep->num);
> > +
> >               while (slabp->inuse < cachep->num && batchcount--) {

On 2/21/07, Christoph Lameter <clameter@sgi.com> wrote:
> I think you only need to check for <0. If slabp->inuse > cachep->num then
> the loop will not be taken.

...and batchcount is not decremented and we're effectively in an
infinite loop. Or am I missing something here?

Re: [PATCH] slab: ensure cache_alloc_refill terminates

[Christoph Lameter]

On Wed, 21 Feb 2007, Pekka Enberg wrote:

> ...and batchcount is not decremented and we're effectively in an
> infinite loop. Or am I missing something here?

No you are right.

Acked-by: Christoph Lameter <clameter@sgi.com>

Re: [PATCH] slab: ensure cache_alloc_refill terminates

[Christoph Lameter]

On Mon, 19 Feb 2007, Pekka J Enberg wrote:

> If slab->inuse is corrupted, cache_alloc_refill can enter an infinite
> loop as detailed by Michael Richardson in the following post:
> <http://lkml.org/lkml/2007/2/16/292>.

We have seen that corruption too.

>  		check_spinlock_acquired(cachep);
> +
> +		/*
> +		 * The slab was either on partial or free list so
> +		 * there must be at least one object available for
> +		 * allocation.
> +		 */
> +		BUG_ON(slabp->inuse >= cachep->num);

slabp->inuse is an integer and we have seen it become -1. The proposed 
test will not catch those cases.

Re: [PATCH] slab: ensure cache_alloc_refill terminates

[Pekka J Enberg]

On Mon, 19 Feb 2007, KAMEZAWA Hiroyuki wrote:
> From my experience, this infinite loop in cache_alloc_refill() is caused by 
> double-free, always...(I'm sorry if my knowledge around the slab is too old.)

Well, I don't know what exactly caused slabp->inuse (could be cachep->num 
too, although sounds unlikely) to corrupt for Michael. But yeah, freeing 
an object twice obviously corrupts slabp->inuse too.

On Mon, 19 Feb 2007, KAMEZAWA Hiroyuki wrote:
> And it looks this additional check can caught the problem but maybe no help for
> fixing problem...How about adding printk like this ?
> ==
> if (unlikely(slabp->inuse >= cachep->num)) {
> 	printk("A problem is detected on slab %s\n", cachep->name);//this information is useful.
> 	printk("please use DEBUG_SLAB kernel for detecting what happens.");
> 	BUG(); 
> }

I don't think cachep->name is enough to debug the problem and as soon as 
you turn on CONFIG_SLAB_DEBUG double-frees should show up in 
verify_redzone_free with nice debugging info. So, I'd prefer we keep the 
simple BUG_ON and simply ask people to turn on debugging if they hit it.

				Pekka

Re: [PATCH] slab: ensure cache_alloc_refill terminates

[KAMEZAWA Hiroyuki]

On Mon, 19 Feb 2007 10:22:52 +0200 (EET)
Pekka J Enberg <penberg@cs.helsinki.fi> wrote:

> @@ -2987,6 +2987,14 @@
>  		slabp = list_entry(entry, struct slab, list);
>  		check_slabp(cachep, slabp);
>  		check_spinlock_acquired(cachep);
> +
> +		/*
> +		 * The slab was either on partial or free list so
> +		 * there must be at least one object available for
> +		 * allocation.
> +		 */
> +		BUG_ON(slabp->inuse >= cachep->num);
> +
I welcome this patch. 

>From my experience, this infinite loop in cache_alloc_refill() is caused by 
double-free, always...(I'm sorry if my knowledge around the slab is too old.)

And it looks this additional check can caught the problem but maybe no help for
fixing problem...How about adding printk like this ?
==
if (unlikely(slabp->inuse >= cachep->num)) {
	printk("A problem is detected on slab %s\n", cachep->name);//this information is useful.
	printk("please use DEBUG_SLAB kernel for detecting what happens.");
	BUG(); 
}


-Kame

[PATCH] slab: ensure cache_alloc_refill terminates

[Pekka J Enberg]

From: Pekka Enberg <penberg@cs.helsinki.fi>

If slab->inuse is corrupted, cache_alloc_refill can enter an infinite
loop as detailed by Michael Richardson in the following post:
<http://lkml.org/lkml/2007/2/16/292>.

This patch adds a BUG_ON for the case where a slab in partial or free
list has no available objects for allocation to ensure
cache_alloc_refill terminates.

Cc: Michael Richardson <mcr@sandelman.ca>
Cc: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 mm/slab.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: 2.6/mm/slab.c
===================================================================
--- 2.6.orig/mm/slab.c	2007-02-19 09:58:28.000000000 +0200
+++ 2.6/mm/slab.c	2007-02-19 10:19:20.000000000 +0200
@@ -2987,6 +2987,14 @@
 		slabp = list_entry(entry, struct slab, list);
 		check_slabp(cachep, slabp);
 		check_spinlock_acquired(cachep);
+
+		/*
+		 * The slab was either on partial or free list so
+		 * there must be at least one object available for
+		 * allocation.
+		 */
+		BUG_ON(slabp->inuse >= cachep->num);
+
 		while (slabp->inuse < cachep->num && batchcount--) {
 			STATS_INC_ALLOCED(cachep);
 			STATS_INC_ACTIVE(cachep);