Re: [PATCH V3 1/2] slub: fix kmem cgroup bug in kmem_cache_alloc_bulk

[Vladimir Davydov <vdavydov@virtuozzo.com>]

On Mon, Nov 09, 2015 at 09:25:22PM +0100, Jesper Dangaard Brouer wrote:
> On Mon, 9 Nov 2015 22:13:35 +0300
> Vladimir Davydov <vdavydov@virtuozzo.com> wrote:
> 
> > On Mon, Nov 09, 2015 at 07:17:31PM +0100, Jesper Dangaard Brouer wrote:
> > ...
> > > @@ -2556,7 +2563,7 @@ redo:
> > >  	if (unlikely(gfpflags & __GFP_ZERO) && object)
> > >  		memset(object, 0, s->object_size);
> > >  
> > > -	slab_post_alloc_hook(s, gfpflags, object);
> > > +	slab_post_alloc_hook(s, gfpflags, 1, object);
> > 
> > I think it must be &object
> 
> The object is already a void ** type.

Let's forget about types for a second. object contains an address to the
newly allocated object, while slab_post_alloc_hook expects an array of
addresses to objects. Simple test. Suppose an allocation failed. Then
object equals 0. Passing 0 to slab_post_alloc_hook as @p and 1 as @size
will result in NULL ptr dereference.

> 
> > BTW why is object defined as void **? I suspect we can safely drop one
> > star.
> 
> Maybe Christoph can explain this?
> 
> 
> > >  
> > >  	return object;
> > >  }
> > ...
> > > @@ -2953,11 +2958,15 @@ bool kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > >  			memset(p[j], 0, s->object_size);
> > >  	}
> > >  
> > > +	/* memcg and kmem_cache debug support */
> > > +	slab_post_alloc_hook(s, flags, size, p);
> > > +
> > >  	return true;
> > >  
> > >  error:
> > >  	__kmem_cache_free_bulk(s, i, p);
> > >  	local_irq_enable();
> > > +	memcg_kmem_put_cache(s);
> > 
> > I wouldn't tear memcg_kmem_put_cache from slab_post_alloc_hook. If we
> > add something else to slab_post_alloc_hook (e.g. we might want to call
> > tracing functions from there), we'll have to modify this error path
> > either, which is easy to miss.
> > 
> > What about calling
> > 
> > 	slab_post_alloc_hook(s, flags, 0, NULL);
> > 
> > here?
> 
> Maybe the correct behavior here, to adhere to all the debugging options,
> is to call:
> 
> error:
>  local_irq_enable();
>  slab_post_alloc_hook(s, flags, i, p);
>  __kmem_cache_free_bulk(s, i, p);
>  return false;

Yeah, I think you're right, because __kmem_cache_free_bulk calls
slab_free_hook, which is supposed to undo slab_post_alloc_hook, so we
must call the latter for allocated objects.

Thanks,
Vladimir

> 
>  
> > >  	return false;
> > >  }
> > >  EXPORT_SYMBOL(kmem_cache_alloc_bulk);
> > > 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>