linux-kernel - Re: [PATCH] slab: fix wrong retval on kmem_cache_create

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] [day] [month] [year] [list]

Date:	Sun, 26 Jan 2014 12:15:28 +0400
From:	Vladimir Davydov <vdavydov@...allels.com>
To:	David Rientjes <rientjes@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>
CC:	<linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>,
	Dave Jones <davej@...hat.com>,
	Pekka Enberg <penberg@...nel.org>,
	Christoph Lameter <cl@...ux.com>
Subject: Re: [PATCH] slab: fix wrong retval on kmem_cache_create_memcg error
 path

On 01/26/2014 08:39 AM, David Rientjes wrote:
> On Sat, 25 Jan 2014, Vladimir Davydov wrote:
>
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index 8e40321..499b53c 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
>> @@ -249,7 +249,6 @@ out_unlock:
>>  				name, err);
>>  			dump_stack();
>>  		}
>> -		return NULL;
>>  	}
>>  	return s;
>>  
>> @@ -257,6 +256,7 @@ out_free_cache:
>>  	memcg_free_cache_params(s);
>>  	kfree(s->name);
>>  	kmem_cache_free(kmem_cache, s);
>> +	s = NULL;
>>  	goto out_unlock;
>>  }
>>  
> I thought I left spaghetti code back in my BASIC 2.0 days.  It should be 
> much more readable to just do
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -233,14 +233,15 @@ out_unlock:
>  	mutex_unlock(&slab_mutex);
>  	put_online_cpus();
>  
> -	/*
> -	 * 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 (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.
> +		 */
> +		if (memcg)
> +			return NULL;
> +
>  		if (flags & SLAB_PANIC)
>  			panic("kmem_cache_create: Failed to create slab '%s'. Error %d\n",
>  				name, err);
>
> and stop trying to remember what err, memcg, and s are in all possible 
> contexts.  Sheesh.

Hi, David,

Although it's rather a matter of personal preference, I tend to agree
with you.

Andrew,

The fix by David Rientjes is attached. It's up to you to decide, which
one looks better.

Thank you and sorry about the noise.

View attachment "0001-slab-fix-wrong-retval-on-kmem_cache_create_memcg-err.patch" of type "text/x-patch" (2875 bytes)