linux-kernel - Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.02.1401301336530.15271@chino.kir.corp.google.com>
Date:	Thu, 30 Jan 2014 13:38:56 -0800 (PST)
From:	David Rientjes <rientjes@...gle.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
cc:	Vladimir Davydov <vdavydov@...allels.com>, mhocko@...e.cz,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache
 fail path

On Thu, 30 Jan 2014, Andrew Morton wrote:

> > What's funnier is that tmp_name isn't required at all since 
> > kmem_cache_create_memcg() is just going to do a kstrdup() on it anyway, so 
> > you could easily just pass in the pointer to memory that has been 
> > allocated for s->name rather than allocating memory twice.
> 
> We need a buffer to sprintf() into.
> 

Yeah, it shouldn't be temporary it should be the one and only allocation.  
We should construct the name in memcg_create_kmem_cache() and be done with 
it.

> diff -puN mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks mm/memcontrol.c
> --- a/mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks
> +++ a/mm/memcontrol.c
> @@ -3400,24 +3400,18 @@ void mem_cgroup_destroy_cache(struct kme
>  static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
>  						  struct kmem_cache *s)
>  {
> -	struct kmem_cache *new = NULL;
> -	static char *tmp_name = NULL;
> -	static DEFINE_MUTEX(mutex);	/* protects tmp_name */
> +	struct kmem_cache *new;
> +	char *tmp_name;
>  
>  	BUG_ON(!memcg_can_account_kmem(memcg));
>  
> -	mutex_lock(&mutex);
>  	/*
> -	 * kmem_cache_create_memcg duplicates the given name and
> -	 * cgroup_name for this name requires RCU context.
> -	 * This static temporary buffer is used to prevent from
> -	 * pointless shortliving allocation.
> +	 * kmem_cache_create_memcg duplicates the given name and cgroup_name()
> +	 * for this name requires rcu_read_lock().
>  	 */
> -	if (!tmp_name) {
> -		tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
> -		if (!tmp_name)
> -			goto out;
> -	}
> +	tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
> +	if (!tmp_name)
> +		return NULL;
>  
>  	rcu_read_lock();
>  	snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name,
> @@ -3430,8 +3424,7 @@ static struct kmem_cache *memcg_create_k
>  		new->allocflags |= __GFP_KMEMCG;
>  	else
>  		new = s;
> -out:
> -	mutex_unlock(&mutex);
> +	kfree(tmp_name);
>  	return new;
>  }
>  

This is fine, but kmem_cache_create_memcg() is still just going to do a 
pointless kstrdup() on it which isn't necessary.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/