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 <[EMAIL PROTECTED]>

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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 <[EMAIL PROTECTED]> 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?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] slab: ensure cache_alloc_refill terminates

[Pekka J Enberg]

From: Pekka Enberg <[EMAIL PROTECTED]>

If slab->inuse is corrupted, cache_alloc_refill can enter an infinite
loop as detailed by Michael Richardson in the following post:
. This adds a BUG_ON to catch
those cases.

Cc: Michael Richardson <[EMAIL PROTECTED]>
Cc: Christoph Lameter <[EMAIL PROTECTED]>
Signed-off-by: Pekka Enberg <[EMAIL PROTECTED]>
---
 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.0 +0200
+++ 2.6/mm/slab.c   2007-02-20 08:39:26.0 +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);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] slab: ensure cache_alloc_refill terminates

[Pekka J Enberg]

From: Pekka Enberg [EMAIL PROTECTED]

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 [EMAIL PROTECTED]
Cc: Christoph Lameter [EMAIL PROTECTED]
Signed-off-by: Pekka Enberg [EMAIL PROTECTED]
---
 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.0 +0200
+++ 2.6/mm/slab.c   2007-02-20 08:39:26.0 +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);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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 [EMAIL PROTECTED] 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?
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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 [EMAIL PROTECTED]

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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:
> .

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] slab: ensure cache_alloc_refill terminates

[KAMEZAWA Hiroyuki]

On Mon, 19 Feb 2007 10:22:52 +0200 (EET)
Pekka J Enberg <[EMAIL PROTECTED]> 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] slab: ensure cache_alloc_refill terminates

[Pekka J Enberg]

From: Pekka Enberg <[EMAIL PROTECTED]>

If slab->inuse is corrupted, cache_alloc_refill can enter an infinite
loop as detailed by Michael Richardson in the following post:
.

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 <[EMAIL PROTECTED]>
Cc: Christoph Lameter <[EMAIL PROTECTED]>
Signed-off-by: Pekka Enberg <[EMAIL PROTECTED]>
---
 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.0 +0200
+++ 2.6/mm/slab.c   2007-02-19 10:19:20.0 +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);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] slab: ensure cache_alloc_refill terminates

[Pekka J Enberg]

From: Pekka Enberg [EMAIL PROTECTED]

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 [EMAIL PROTECTED]
Cc: Christoph Lameter [EMAIL PROTECTED]
Signed-off-by: Pekka Enberg [EMAIL PROTECTED]
---
 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.0 +0200
+++ 2.6/mm/slab.c   2007-02-19 10:19:20.0 +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);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] slab: ensure cache_alloc_refill terminates

[KAMEZAWA Hiroyuki]

On Mon, 19 Feb 2007 10:22:52 +0200 (EET)
Pekka J Enberg [EMAIL PROTECTED] 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

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/