Re: slab: clean up kmem_cache_create_memcg() error handling

Re: slab: clean up kmem_cache_create_memcg() error handling

[Dave Jones]

On Fri, Jan 24, 2014 at 03:33:41AM +0000, Linux Kernel wrote:
 > Gitweb:     http://git.kernel.org/linus/;a=commit;h=3965fc3652244651006ebb31c8c45318ce84818f
 > Commit:     3965fc3652244651006ebb31c8c45318ce84818f
 > Parent:     309381feaee564281c3d9e90fbca8963bb7428ad
 > Author:     Vladimir Davydov <vdavydov@parallels.com>
 > AuthorDate: Thu Jan 23 15:52:55 2014 -0800
 > Committer:  Linus Torvalds <torvalds@linux-foundation.org>
 > CommitDate: Thu Jan 23 16:36:50 2014 -0800
 > 
 >     slab: clean up kmem_cache_create_memcg() error handling
 >     
 >     Currently kmem_cache_create_memcg() backoffs on failure inside
 >     conditionals, without using gotos.  This results in the rollback code
 >     duplication, which makes the function look cumbersome even though on
 >     error we should only free the allocated cache.  Since in the next patch
 >     I am going to add yet another rollback function call on error path
 >     there, let's employ labels instead of conditionals for undoing any
 >     changes on failure to keep things clean.

...

 > +out_unlock:
 >  	mutex_unlock(&slab_mutex);
 >  	put_online_cpus();
 >  
 >  	if (err) {
 > -
 >  		if (flags & SLAB_PANIC)
 >  			panic("kmem_cache_create: Failed to create slab '%s'. Error %d\n",
 >  				name, err);
 > @@ -236,11 +230,14 @@ out_locked:
 >  				name, err);
 >  			dump_stack();
 >  		}
 > -
 >  		return NULL;
 >  	}
 > -
 >  	return s;
 > +
 > +out_free_cache:
 > +	kfree(s->name);
 > +	kmem_cache_free(kmem_cache, s);
 > +	goto out_unlock;
 >  }

This is now returning a freed pointer as 's' if an error occurs.
Perhaps the patch below ?

	Dave


diff --git a/mm/slab_common.c b/mm/slab_common.c
index 8e40321da091..2c62294cee23 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -257,6 +257,7 @@ out_free_cache:
 	memcg_free_cache_params(s);
 	kfree(s->name);
 	kmem_cache_free(kmem_cache, s);
+	s = NULL;
 	goto out_unlock;
 }
 

Re: slab: clean up kmem_cache_create_memcg() error handling

[Vladimir Davydov]

On 01/24/2014 10:20 PM, Dave Jones wrote:
> On Fri, Jan 24, 2014 at 03:33:41AM +0000, Linux Kernel wrote:
>  > Gitweb:     http://git.kernel.org/linus/;a=commit;h=3965fc3652244651006ebb31c8c45318ce84818f
>  > Commit:     3965fc3652244651006ebb31c8c45318ce84818f
>  > Parent:     309381feaee564281c3d9e90fbca8963bb7428ad
>  > Author:     Vladimir Davydov <vdavydov@parallels.com>
>  > AuthorDate: Thu Jan 23 15:52:55 2014 -0800
>  > Committer:  Linus Torvalds <torvalds@linux-foundation.org>
>  > CommitDate: Thu Jan 23 16:36:50 2014 -0800
>  > 
>  >     slab: clean up kmem_cache_create_memcg() error handling
>  >     
>  >     Currently kmem_cache_create_memcg() backoffs on failure inside
>  >     conditionals, without using gotos.  This results in the rollback code
>  >     duplication, which makes the function look cumbersome even though on
>  >     error we should only free the allocated cache.  Since in the next patch
>  >     I am going to add yet another rollback function call on error path
>  >     there, let's employ labels instead of conditionals for undoing any
>  >     changes on failure to keep things clean.
>
> ...
>
>  > +out_unlock:
>  >  	mutex_unlock(&slab_mutex);
>  >  	put_online_cpus();
>  >  
>  >  	if (err) {
>  > -
>  >  		if (flags & SLAB_PANIC)
>  >  			panic("kmem_cache_create: Failed to create slab '%s'. Error %d\n",
>  >  				name, err);
>  > @@ -236,11 +230,14 @@ out_locked:
>  >  				name, err);
>  >  			dump_stack();
>  >  		}
>  > -
>  >  		return NULL;
>  >  	}
>  > -
>  >  	return s;
>  > +
>  > +out_free_cache:
>  > +	kfree(s->name);
>  > +	kmem_cache_free(kmem_cache, s);
>  > +	goto out_unlock;
>  >  }
>
> This is now returning a freed pointer as 's' if an error occurs.

If we go to out_free_cache, we set err, and since under out_unlock we have:

> if (err) {
...
>     return NULL
>}

we will return NULL, which is right.

However this behavior was broken by another my 'fix' :-(

commit    f717eb3abb5ea38f60e671dbfdbf512c2c93d22e
slab: do not panic if we fail to create memcg cache
> -    if (err) {
> +    /*
> +     * There is no point in flooding logs with warnings or especially
> +     * crashing the system if we fail to create a cache for a memcg. In
> +     * this case we will be accounting the memcg allocation to the root
> +     * cgroup until we succeed to create its own cache, but it isn't that
> +     * critical.
> +     */
> +    if (err && !memcg) {
>          if (flags & SLAB_PANIC)
>              panic("kmem_cache_create: Failed to create slab '%s'.
> Error %d\n",
>                  name, err);

In case memcg != NULL we can return crap on error. So you are right in
the end. Thank you for catching this!

> Perhaps the patch below ?
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 8e40321da091..2c62294cee23 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -257,6 +257,7 @@ out_free_cache:
>  	memcg_free_cache_params(s);
>  	kfree(s->name);
>  	kmem_cache_free(kmem_cache, s);
> +	s = NULL;
>  	goto out_unlock;
>  }

This one looks correct to me. I'll send it on behalf of you.

Thanks!