linux-kernel - Re: [PATCH] slab: Error out on duplicate cache names when DEBUG

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: <cdc52bd0-dac8-4f3e-bd7b-aca7513a0464@suse.cz>
Date: Mon, 5 Aug 2024 12:38:29 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Pedro Falcato <pedro.falcato@...il.com>, Christoph Lameter
 <cl@...ux.com>, Pekka Enberg <penberg@...nel.org>,
 David Rientjes <rientjes@...gle.com>, Joonsoo Kim <iamjoonsoo.kim@....com>,
 Andrew Morton <akpm@...ux-foundation.org>
Cc: Roman Gushchin <roman.gushchin@...ux.dev>,
 Hyeonggon Yoo <42.hyeyoo@...il.com>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] slab: Error out on duplicate cache names when DEBUG_VM=y

On 8/5/24 12:24, Pedro Falcato wrote:
> On Sun, Aug 4, 2024 at 10:28 PM Pedro Falcato <pedro.falcato@...il.com> wrote:
>>
>> Duplicate slab cache names can create havoc for userspace tooling that
>> expects slab cache names to be unique. This is a reasonable expectation.
> 
> For completeness, I just had a look at duplicate cache names around
> the kernel using
> git grep -Eoh "kmem_cache_create.*\"," | grep -Eo \".*\" | uniq -d
> (which seems to be correct)
> 
> which results in the following patch (on top of torvalds/linux.git
> master, so file_lock_cache hasn't been fixed yet)
> 
> This patch being so small is what leads me to believe that erroring
> out here is safe. Of course, no one knows what the out-of-tree modules
> do.

What about module unload/reload with a SLAB_TYPESAFE_BY_RCU cache that will
delay its freeing. Soon also if there are kfree_rcu()'s in flight. And the
zombie cache can stay also permamently around if it fails to be destroy
because some objects were not freed.

In all these cases the cache's refcount should be 0 at that point, so
minimally the check should ignore those. But I would also rather make it be
a warning anyway, at least for a few releases.

> diff --git a/drivers/scsi/snic/snic_main.c b/drivers/scsi/snic/snic_main.c
> index cc824dcfe7d..abc78320c66 100644
> --- a/drivers/scsi/snic/snic_main.c
> +++ b/drivers/scsi/snic/snic_main.c
> @@ -873,7 +873,7 @@ snic_global_data_init(void)
>        snic_glob->req_cache[SNIC_REQ_CACHE_MAX_SGL] = cachep;
> 
>        len = sizeof(struct snic_host_req);
> -       cachep = kmem_cache_create("snic_req_maxsgl", len, SNIC_SG_DESC_ALIGN,
> +       cachep = kmem_cache_create("snic_req_tm", len, SNIC_SG_DESC_ALIGN,
>                                   SLAB_HWCACHE_ALIGN, NULL);
>        if (!cachep) {
>                SNIC_ERR("Failed to create snic tm req slab\n");
> diff --git a/fs/locks.c b/fs/locks.c
> index 9afb16e0683..e45cad40f8b 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2984,7 +2984,7 @@ static int __init filelock_init(void)
>        filelock_cache = kmem_cache_create("file_lock_cache",
>                        sizeof(struct file_lock), 0, SLAB_PANIC, NULL);
> 
> -       filelease_cache = kmem_cache_create("file_lock_cache",
> +       filelease_cache = kmem_cache_create("file_lease_cache",
>                        sizeof(struct file_lease), 0, SLAB_PANIC, NULL);
> 
>        for_each_possible_cpu(i) {