ACK/Cmnt: [SRU Focal/Groovy] UBUNTU: SAUCE: Revert "mm: memcg/slab: fix memory leak at non-root kmem_cache destroy"

[Thadeu Lima de Souza Cascardo]

On Thu, Sep 17, 2020 at 09:34:20AM +0200, Stefan Bader wrote:
> On 16.09.20 12:46, Thadeu Lima de Souza Cascardo wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1894780
> > 
> > This reverts commit 79ffe7107b13042c69c4a06394175362121b06b5. This is
> > commit d38a2b7a9c939e6d7329ab92b96559ccebf7b135 upstream.
> > 
> > Said commit causes same-sized kmemcaches to become unmergeable, and when a
> > new kmemcache is created, it will fail creating the sysfs entry, making the
> > kmemcache creation to fail.
> > 
> > Considering the original commit fix a leak but causes a different leak and
> > failures to create kmemcaches, the revert is preferable until a proper fix
> > is developed.
> > 
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo at canonical.com>
> Acked-by: Stefan Bader <stefan.bader at canonical.com>
> > ---
> 
> I agree with this revert but you have not really answered (at least it was not
> obvious to me) what should happen to the revert we already have done in Focal.
> Should that be brought back or also reverted in Groovy in addition to this new
> revert?
> 
> -Stefan
> 

I wrote:

> We should keep the revert and fix it some other way upstream. But I'd rather
> have a leak than the crash caused by the double free.

To be clear, we should also revert that on Groovy, until we have a better fix.
In case kobject_init_and_add fails, the reverted commit would cause a
double-free, leading to the crash that was reported on LP: #1894780. This
second commit we are reverting was causing kobject_init_and_add to return
EEXIST, but there are other reasons kboject_init_and_add would fail. And I
would rather have the memleak that the first commit claims to fix than the
double free it causes.

I will prepare a fix for Groovy (or just point Seth to the one that was already
sent), and work on an alternative fix.

Thanks.
Cascardo.


> >  mm/slab_common.c | 35 +++++++----------------------------
> >  1 file changed, 7 insertions(+), 28 deletions(-)
> > 
> > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > index e36dd36c7076..8c1ffbf7de45 100644
> > --- a/mm/slab_common.c
> > +++ b/mm/slab_common.c
> > @@ -326,14 +326,6 @@ int slab_unmergeable(struct kmem_cache *s)
> >  	if (s->refcount < 0)
> >  		return 1;
> >  
> > -#ifdef CONFIG_MEMCG_KMEM
> > -	/*
> > -	 * Skip the dying kmem_cache.
> > -	 */
> > -	if (s->memcg_params.dying)
> > -		return 1;
> > -#endif
> > -
> >  	return 0;
> >  }
> >  
> > @@ -894,15 +886,12 @@ static int shutdown_memcg_caches(struct kmem_cache *s)
> >  	return 0;
> >  }
> >  
> > -static void memcg_set_kmem_cache_dying(struct kmem_cache *s)
> > +static void flush_memcg_workqueue(struct kmem_cache *s)
> >  {
> >  	spin_lock_irq(&memcg_kmem_wq_lock);
> >  	s->memcg_params.dying = true;
> >  	spin_unlock_irq(&memcg_kmem_wq_lock);
> > -}
> >  
> > -static void flush_memcg_workqueue(struct kmem_cache *s)
> > -{
> >  	/*
> >  	 * SLAB and SLUB deactivate the kmem_caches through call_rcu. Make
> >  	 * sure all registered rcu callbacks have been invoked.
> > @@ -934,6 +923,10 @@ static inline int shutdown_memcg_caches(struct kmem_cache *s)
> >  {
> >  	return 0;
> >  }
> > +
> > +static inline void flush_memcg_workqueue(struct kmem_cache *s)
> > +{
> > +}
> >  #endif /* CONFIG_MEMCG_KMEM */
> >  
> >  void slab_kmem_cache_release(struct kmem_cache *s)
> > @@ -951,6 +944,8 @@ void kmem_cache_destroy(struct kmem_cache *s)
> >  	if (unlikely(!s))
> >  		return;
> >  
> > +	flush_memcg_workqueue(s);
> > +
> >  	get_online_cpus();
> >  	get_online_mems();
> >  
> > @@ -960,22 +955,6 @@ void kmem_cache_destroy(struct kmem_cache *s)
> >  	if (s->refcount)
> >  		goto out_unlock;
> >  
> > -#ifdef CONFIG_MEMCG_KMEM
> > -	memcg_set_kmem_cache_dying(s);
> > -
> > -	mutex_unlock(&slab_mutex);
> > -
> > -	put_online_mems();
> > -	put_online_cpus();
> > -
> > -	flush_memcg_workqueue(s);
> > -
> > -	get_online_cpus();
> > -	get_online_mems();
> > -
> > -	mutex_lock(&slab_mutex);
> > -#endif
> > -
> >  	err = shutdown_memcg_caches(s);
> >  	if (!err)
> >  		err = shutdown_cache(s);
> > 
> 
>