LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] slab: ensure cache_alloc_refill terminates
@ 2007-02-19  8:22 Pekka J Enberg
  2007-02-19  8:48 ` KAMEZAWA Hiroyuki
  2007-02-20  5:05 ` Christoph Lameter
  0 siblings, 2 replies; 8+ messages in thread
From: Pekka J Enberg @ 2007-02-19  8:22 UTC (permalink / raw)
  To: akpm; +Cc: linux-kernel, mcr, clameter

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);

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] slab: ensure cache_alloc_refill terminates
  2007-02-19  8:22 [PATCH] slab: ensure cache_alloc_refill terminates Pekka J Enberg
@ 2007-02-19  8:48 ` KAMEZAWA Hiroyuki
  2007-02-19  8:58   ` Pekka J Enberg
  2007-02-20  5:05 ` Christoph Lameter
  1 sibling, 1 reply; 8+ messages in thread
From: KAMEZAWA Hiroyuki @ 2007-02-19  8:48 UTC (permalink / raw)
  To: Pekka J Enberg; +Cc: akpm, linux-kernel, mcr, clameter

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] slab: ensure cache_alloc_refill terminates
  2007-02-19  8:48 ` KAMEZAWA Hiroyuki
@ 2007-02-19  8:58   ` Pekka J Enberg
  0 siblings, 0 replies; 8+ messages in thread
From: Pekka J Enberg @ 2007-02-19  8:58 UTC (permalink / raw)
  To: KAMEZAWA Hiroyuki; +Cc: akpm, linux-kernel, mcr, clameter

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] slab: ensure cache_alloc_refill terminates
  2007-02-19  8:22 [PATCH] slab: ensure cache_alloc_refill terminates Pekka J Enberg
  2007-02-19  8:48 ` KAMEZAWA Hiroyuki
@ 2007-02-20  5:05 ` Christoph Lameter
  1 sibling, 0 replies; 8+ messages in thread
From: Christoph Lameter @ 2007-02-20  5:05 UTC (permalink / raw)
  To: Pekka J Enberg; +Cc: akpm, linux-kernel, mcr

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] slab: ensure cache_alloc_refill terminates
  2007-02-21 18:22   ` Pekka Enberg
@ 2007-02-21 18:28     ` Christoph Lameter
  0 siblings, 0 replies; 8+ messages in thread
From: Christoph Lameter @ 2007-02-21 18:28 UTC (permalink / raw)
  To: Pekka Enberg; +Cc: akpm, mcr, linux-kernel

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>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] slab: ensure cache_alloc_refill terminates
  2007-02-21 17:51 ` Christoph Lameter
@ 2007-02-21 18:22   ` Pekka Enberg
  2007-02-21 18:28     ` Christoph Lameter
  0 siblings, 1 reply; 8+ messages in thread
From: Pekka Enberg @ 2007-02-21 18:22 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: akpm, mcr, linux-kernel

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?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] slab: ensure cache_alloc_refill terminates
  2007-02-21  8:30 Pekka J Enberg
@ 2007-02-21 17:51 ` Christoph Lameter
  2007-02-21 18:22   ` Pekka Enberg
  0 siblings, 1 reply; 8+ messages in thread
From: Christoph Lameter @ 2007-02-21 17:51 UTC (permalink / raw)
  To: Pekka J Enberg; +Cc: akpm, mcr, linux-kernel

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] slab: ensure cache_alloc_refill terminates
@ 2007-02-21  8:30 Pekka J Enberg
  2007-02-21 17:51 ` Christoph Lameter
  0 siblings, 1 reply; 8+ messages in thread
From: Pekka J Enberg @ 2007-02-21  8:30 UTC (permalink / raw)
  To: akpm; +Cc: mcr, clameter, linux-kernel

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);

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2007-02-21 18:28 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-02-19  8:22 [PATCH] slab: ensure cache_alloc_refill terminates Pekka J Enberg
2007-02-19  8:48 ` KAMEZAWA Hiroyuki
2007-02-19  8:58   ` Pekka J Enberg
2007-02-20  5:05 ` Christoph Lameter
2007-02-21  8:30 Pekka J Enberg
2007-02-21 17:51 ` Christoph Lameter
2007-02-21 18:22   ` Pekka Enberg
2007-02-21 18:28     ` Christoph Lameter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).