linux-kernel - Re: [PATCH v3] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

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: <000001391be89602-3ef322de-204d-4d93-9f13-38065ce8f757-000000@email.amazonses.com>
Date:	Sun, 12 Aug 2012 17:36:31 +0000
From:	Christoph <cl@...ux.com>
To:	"shuah.khan@...com" <shuah.khan@...com>
Cc:	"penberg@...nel.org" <penberg@...nel.org>,
	"glommer@...allels.com" <glommer@...allels.com>,
	"js1304@...il.com" <js1304@...il.com>,
	DavidRientjes <rientjes@...gle.com>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	"shuahkhan@...il.com" <shuahkhan@...il.com>
Subject: Re: [PATCH v3] mm: Restructure kmem_cache_create() to move debug cache integrity checks into a new function

Acked-by: Christoph Lameter <cl@...ux.com>



On Aug 12, 2012, at 11:40, Shuah Khan <shuah.khan@...com> wrote:

> kmem_cache_create() does cache integrity checks when CONFIG_DEBUG_VM
> is defined. These checks interspersed with the regular code path has
> lead to compile time warnings when compiled without CONFIG_DEBUG_VM
> defined. Restructuring the code to move the integrity checks in to a new
> function would eliminate the current compile warning problem and also
> will allow for future changes to the debug only code to evolve without
> introducing new warnings in the regular path. This restructuring work
> is based on the discussion in the following thread:
> 
> https://lkml.org/lkml/2012/7/13/424
> 
> Signed-off-by: Shuah Khan <shuah.khan@...com>
> ---
> mm/slab_common.c |   90 +++++++++++++++++++++++++++++-------------------------
> 1 file changed, 48 insertions(+), 42 deletions(-)
> 
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 12637ce..44facdf 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -23,6 +23,52 @@ enum slab_state slab_state;
> LIST_HEAD(slab_caches);
> DEFINE_MUTEX(slab_mutex);
> 
> +#ifdef CONFIG_DEBUG_VM
> +static int kmem_cache_sanity_check(const char *name, size_t size)
> +{
> +    struct kmem_cache *s = NULL;
> +
> +    if (!name || in_interrupt() || size < sizeof(void *) ||
> +        size > KMALLOC_MAX_SIZE) {
> +        pr_err("kmem_cache_create(%s) integrity check failed\n", name);
> +        return -EINVAL;
> +    }
> +
> +    list_for_each_entry(s, &slab_caches, list) {
> +        char tmp;
> +        int res;
> +
> +        /*
> +         * This happens when the module gets unloaded and doesn't
> +         * destroy its slab cache and no-one else reuses the vmalloc
> +         * area of the module.  Print a warning.
> +         */
> +        res = probe_kernel_address(s->name, tmp);
> +        if (res) {
> +            pr_err("Slab cache with size %d has lost its name\n",
> +                   s->object_size);
> +            continue;
> +        }
> +
> +        if (!strcmp(s->name, name)) {
> +            pr_err("%s (%s): Cache name already exists.\n",
> +                   __func__, name);
> +            dump_stack();
> +            s = NULL;
> +            return -EINVAL;
> +        }
> +    }
> +
> +    WARN_ON(strchr(name, ' '));    /* It confuses parsers */
> +    return 0;
> +}
> +#else
> +static inline int kmem_cache_sanity_check(const char *name, size_t size)
> +{
> +    return 0;
> +}
> +#endif
> +
> /*
>  * kmem_cache_create - Create a cache.
>  * @name: A string which is used in /proc/slabinfo to identify this cache.
> @@ -53,48 +99,11 @@ struct kmem_cache *kmem_cache_create(const char *name, size_t size, size_t align
> {
>    struct kmem_cache *s = NULL;
> 
> -#ifdef CONFIG_DEBUG_VM
> -    if (!name || in_interrupt() || size < sizeof(void *) ||
> -        size > KMALLOC_MAX_SIZE) {
> -        printk(KERN_ERR "kmem_cache_create(%s) integrity check"
> -            " failed\n", name);
> -        goto out;
> -    }
> -#endif
> -
>    get_online_cpus();
>    mutex_lock(&slab_mutex);
> 
> -#ifdef CONFIG_DEBUG_VM
> -    list_for_each_entry(s, &slab_caches, list) {
> -        char tmp;
> -        int res;
> -
> -        /*
> -         * This happens when the module gets unloaded and doesn't
> -         * destroy its slab cache and no-one else reuses the vmalloc
> -         * area of the module.  Print a warning.
> -         */
> -        res = probe_kernel_address(s->name, tmp);
> -        if (res) {
> -            printk(KERN_ERR
> -                   "Slab cache with size %d has lost its name\n",
> -                   s->object_size);
> -            continue;
> -        }
> -
> -        if (!strcmp(s->name, name)) {
> -            printk(KERN_ERR "kmem_cache_create(%s): Cache name"
> -                " already exists.\n",
> -                name);
> -            dump_stack();
> -            s = NULL;
> -            goto oops;
> -        }
> -    }
> -
> -    WARN_ON(strchr(name, ' '));    /* It confuses parsers */
> -#endif
> +    if (kmem_cache_sanity_check(name, size))
> +        goto oops;
> 
>    s = __kmem_cache_create(name, size, align, flags, ctor);
> 
> @@ -102,9 +111,6 @@ oops:
>    mutex_unlock(&slab_mutex);
>    put_online_cpus();
> 
> -#ifdef CONFIG_DEBUG_VM
> -out:
> -#endif
>    if (!s && (flags & SLAB_PANIC))
>        panic("kmem_cache_create: Failed to create slab '%s'\n", name);
> 
> -- 
> 1.7.9.5
> 
> 
--
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/