[PATCH] SLUB: Use WARN_ON instead of BUG_ON where appropriate

[PATCH] SLUB: Use WARN_ON instead of BUG_ON where appropriate

[Pekka J Enberg]

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

It's better to WARN_ON and let the kernel limp along to increase the likelihood
of an user detecting the error and reporting it to us.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 mm/slub.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 6392ae5..e4404a1 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1119,7 +1119,8 @@ static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
 	void *last;
 	void *p;
 
-	BUG_ON(flags & GFP_SLAB_BUG_MASK);
+	if (WARN_ON(flags & GFP_SLAB_BUG_MASK))
+		return NULL;
 
 	page = allocate_slab(s,
 		flags & (GFP_RECLAIM_MASK | GFP_CONSTRAINT_MASK), node);
@@ -2746,12 +2747,13 @@ void kfree(const void *x)
 		return;
 
 	page = virt_to_head_page(x);
-	if (unlikely(!PageSlab(page))) {
-		BUG_ON(!PageCompound(page));
-		put_page(page);
+	if (likely(PageSlab(page))) {
+		slab_free(page->slab, page, object, _RET_IP_);
 		return;
-	}
-	slab_free(page->slab, page, object, _RET_IP_);
+	} else if (WARN_ON(!PageCompound(page)))
+		return;
+
+	put_page(page);
 }
 EXPORT_SYMBOL(kfree);
 
@@ -4285,7 +4287,8 @@ static char *create_unique_id(struct kmem_cache *s)
 	char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
 	char *p = name;
 
-	BUG_ON(!name);
+	if (WARN_ON(!name))
+		return NULL;
 
 	*p++ = ':';
 	/*
@@ -4304,7 +4307,8 @@ static char *create_unique_id(struct kmem_cache *s)
 	if (p != name + 1)
 		*p++ = '-';
 	p += sprintf(p, "%07d", s->size);
-	BUG_ON(p > name + ID_STR_LENGTH - 1);
+	if (WARN_ON(p > name + ID_STR_LENGTH - 1))
+		return NULL;
 	return name;
 }
 
@@ -4333,6 +4337,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
 		 * for the symlinks.
 		 */
 		name = create_unique_id(s);
+		if (!name)
+			return -EINVAL;
 	}
 
 	s->kobj.kset = slab_kset;
-- 
1.5.4.3

Re: [PATCH] SLUB: Use WARN_ON instead of BUG_ON where appropriate

[Christoph Lameter]

On Wed, 14 Jan 2009, Pekka J Enberg wrote:

> From: Pekka Enberg <penberg@cs.helsinki.fi>
>
> It's better to WARN_ON and let the kernel limp along to increase the likelihood
> of an user detecting the error and reporting it to us.

That only works if a error condition can be signaled and be handled
appropriately.

> diff --git a/mm/slub.c b/mm/slub.c
> index 6392ae5..e4404a1 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1119,7 +1119,8 @@ static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
>  	void *last;
>  	void *p;
>
> -	BUG_ON(flags & GFP_SLAB_BUG_MASK);
> +	if (WARN_ON(flags & GFP_SLAB_BUG_MASK))
> +		return NULL;
>
>  	page = allocate_slab(s,
>  		flags & (GFP_RECLAIM_MASK | GFP_CONSTRAINT_MASK), node);

Ok. this relies on kmalloc returning NULL to the app.

> @@ -4285,7 +4287,8 @@ static char *create_unique_id(struct kmem_cache *s)
>  	char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
>  	char *p = name;
>
> -	BUG_ON(!name);
> +	if (WARN_ON(!name))
> +		return NULL;
>
>  	*p++ = ':';
>  	/*
> @@ -4304,7 +4307,8 @@ static char *create_unique_id(struct kmem_cache *s)
>  	if (p != name + 1)
>  		*p++ = '-';
>  	p += sprintf(p, "%07d", s->size);
> -	BUG_ON(p > name + ID_STR_LENGTH - 1);
> +	if (WARN_ON(p > name + ID_STR_LENGTH - 1))
> +		return NULL;
>  	return name;
>  }
>

NULL name? Guess the kfree will just be a nop but what does
kobject_init_and_add do with a NULL name?


> @@ -4333,6 +4337,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
>  		 * for the symlinks.
>  		 */
>  		name = create_unique_id(s);
> +		if (!name)
> +			return -EINVAL;
>  	}
>
>  	s->kobj.kset = slab_kset;
>

-EINVAL may cause a panic anyways if called from create_kmalloc_cache.

Re: [PATCH] SLUB: Use WARN_ON instead of BUG_ON where appropriate

[Pekka Enberg]

Hi Christoph,

On Wed, 2009-01-14 at 11:45 -0600, Christoph Lameter wrote:
> On Wed, 14 Jan 2009, Pekka J Enberg wrote:
> 
> > From: Pekka Enberg <penberg@cs.helsinki.fi>
> >
> > It's better to WARN_ON and let the kernel limp along to increase the likelihood
> > of an user detecting the error and reporting it to us.
> 
> That only works if a error condition can be signaled and be handled
> appropriately.

Yes and I think the cases I converted we can.

> > diff --git a/mm/slub.c b/mm/slub.c
> > index 6392ae5..e4404a1 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -1119,7 +1119,8 @@ static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
> >  	void *last;
> >  	void *p;
> >
> > -	BUG_ON(flags & GFP_SLAB_BUG_MASK);
> > +	if (WARN_ON(flags & GFP_SLAB_BUG_MASK))
> > +		return NULL;
> >
> >  	page = allocate_slab(s,
> >  		flags & (GFP_RECLAIM_MASK | GFP_CONSTRAINT_MASK), node);
> 
> Ok. this relies on kmalloc returning NULL to the app.

Which should be fine because allocate_slab() can return NULL for oom
conditions.

> > @@ -4285,7 +4287,8 @@ static char *create_unique_id(struct kmem_cache *s)
> >  	char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
> >  	char *p = name;
> >
> > -	BUG_ON(!name);
> > +	if (WARN_ON(!name))
> > +		return NULL;
> >
> >  	*p++ = ':';
> >  	/*
> > @@ -4304,7 +4307,8 @@ static char *create_unique_id(struct kmem_cache *s)
> >  	if (p != name + 1)
> >  		*p++ = '-';
> >  	p += sprintf(p, "%07d", s->size);
> > -	BUG_ON(p > name + ID_STR_LENGTH - 1);
> > +	if (WARN_ON(p > name + ID_STR_LENGTH - 1))
> > +		return NULL;
> >  	return name;
> >  }
> >
> 
> NULL name? Guess the kfree will just be a nop but what does
> kobject_init_and_add do with a NULL name?

We never get that far because we check if create_unique_id() returns
NULL and bail out early.

> 
> 
> > @@ -4333,6 +4337,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
> >  		 * for the symlinks.
> >  		 */
> >  		name = create_unique_id(s);
> > +		if (!name)
> > +			return -EINVAL;
> >  	}
> >
> >  	s->kobj.kset = slab_kset;
> >
> 
> -EINVAL may cause a panic anyways if called from create_kmalloc_cache.

Yes. I think I'll drop these bits and just change the BUG_ON() in
new_slab() to WARN_ON().