Messages in this thread | | | Date | Sat, 25 Jan 2014 01:14:35 +0400 | From | Vladimir Davydov <> | Subject | Re: slab: clean up kmem_cache_create_memcg() error handling |
| |
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!
| |