On Sat, Nov 12, 2022 at 07:46:02PM +0800, Liu Shixin wrote:
> There is a memory leak of kobj->name in sysfs_slab_add():
>
> unreferenced object 0xffff88817e446440 (size 32):
> comm "insmod", pid 4085, jiffies 4296564501 (age 126.272s)
> hex dump (first 32 bytes):
> 75 62 69 66 73 5f 69 6e 6f 64 65 5f 73 6c 61 62 ubifs_inode_slab
> 00 65 44 7e 81 88 ff ff 00 00 00 00 00 00 00 00 .eD~............
> backtrace:
> [<000000005b30fbbd>] __kmalloc_node_track_caller+0x4e/0x150
> [<000000002f70da0c>] kstrdup_const+0x4b/0x80
> [<00000000c6712c61>] kobject_set_name_vargs+0x2f/0xb0
> [<00000000b151218e>] kobject_init_and_add+0xb0/0x120
> [<00000000e56a4cf5>] sysfs_slab_add+0x17d/0x220
> [<000000009326fd57>] __kmem_cache_create+0x406/0x590
> [<00000000dde33cff>] kmem_cache_create_usercopy+0x1fc/0x300
> [<00000000fe90cedb>] kmem_cache_create+0x12/0x20
> [<000000007a6531c8>] 0xffffffffa02d802d
> [<000000000e3b13c7>] do_one_initcall+0x87/0x2a0
> [<00000000995ecdcf>] do_init_module+0xdf/0x320
> [<000000008821941f>] load_module+0x2f98/0x3330
> [<00000000ef51efa4>] __do_sys_finit_module+0x113/0x1b0
> [<000000009339fbce>] do_syscall_64+0x35/0x80
> [<000000006b7f2033>] entry_SYSCALL_64_after_hwframe+0x46/0xb0
>
> Following the rules stated in the comment for kobject_init_and_add():
> If this function returns an error, kobject_put() must be called to
> properly clean up the memory associated with the object.
>
> kobject_put() is more appropriate for error handling after kobject_init().
> And we can use this function to solve this problem.
>
> For the cache created early, the related sysfs_slab_add() is called in
> slab_sysfs_init(). Skip free these kmem_cache since they are important
> for system. Keep them working without sysfs.
>
> Fixes: 80da026a8e5d ("mm/slub: fix slab double-free in case of duplicate sysfs filename")
> Signed-off-by: Liu Shixin <liushixin2@xxxxxxxxxx>
> ---
> include/linux/slub_def.h | 4 ++--
> mm/slab_common.c | 6 ++----
> mm/slub.c | 21 +++++++++++++++++----
> 3 files changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
> index 26d56c4c74d1..90c3e06b77b1 100644
> --- a/include/linux/slub_def.h
> +++ b/include/linux/slub_def.h
> @@ -144,11 +144,11 @@ struct kmem_cache {
>
> #ifdef CONFIG_SYSFS
> #define SLAB_SUPPORTS_SYSFS
> -int sysfs_slab_add(struct kmem_cache *);
> +int sysfs_slab_add(struct kmem_cache *, bool);
> void sysfs_slab_unlink(struct kmem_cache *);
> void sysfs_slab_release(struct kmem_cache *);
> #else
> -static inline int sysfs_slab_add(struct kmem_cache *s)
> +static inline int sysfs_slab_add(struct kmem_cache *s, bool free_slab)
> {
> return 0;
> }
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 55e2cf064dfe..30808a1d1b32 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -237,11 +237,9 @@ static struct kmem_cache *create_cache(const char *name,
> #ifdef SLAB_SUPPORTS_SYSFS
> /* Mutex is not taken during early boot */
> if (slab_state >= FULL) {
> - err = sysfs_slab_add(s);
> - if (err) {
> - slab_kmem_cache_release(s);
> + err = sysfs_slab_add(s, true);
> + if (err)
> return ERR_PTR(err);
> - }
> debugfs_slab_add(s);
> }
> #endif
> diff --git a/mm/slub.c b/mm/slub.c
> index a1ad759753ce..25575bce0c3c 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5881,7 +5881,7 @@ static char *create_unique_id(struct kmem_cache *s)
> return name;
> }
>
> -int sysfs_slab_add(struct kmem_cache *s)
> +int sysfs_slab_add(struct kmem_cache *s, bool free_slab)
free_slab is confusing.
Maybe 'release' or 'shutdown'?
> {
> int err;
> const char *name;
> @@ -5911,14 +5911,17 @@ int sysfs_slab_add(struct kmem_cache *s)
> * for the symlinks.
> */
> name = create_unique_id(s);
> - if (IS_ERR(name))
> + if (IS_ERR(name)) {
> + if (free_slab)
> + slab_kmem_cache_release(s);
> return PTR_ERR(name);
> + }
> }
>
> s->kobj.kset = kset;
> err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, "%s", name);
> if (err)
> - goto out;
> + goto out_put_kobj;
>
> err = sysfs_create_group(&s->kobj, &slab_attr_group);
> if (err)
> @@ -5934,6 +5937,16 @@ int sysfs_slab_add(struct kmem_cache *s)
> return err;
> out_del_kobj:
> kobject_del(&s->kobj);
> +out_put_kobj:
> + /*
> + * Skip free kmem_cache that create early since they are important
> + * for system. Keep them working without sysfs. Only free the name
> + * for early allocated kmem_cache.
> + */
> + if (free_slab)
> + kobject_put(&s->kobj);
> + else
> + kfree_const(s->kobj.name);
This is bypassing kobject API - can we initialize it
with different ktype that has different .release function,
depending on the boolean parameter?
> goto out;
> }
>
> @@ -6002,7 +6015,7 @@ static int __init slab_sysfs_init(void)
> slab_state = FULL;
>
> list_for_each_entry(s, &slab_caches, list) {
> - err = sysfs_slab_add(s);
> + err = sysfs_slab_add(s, false);
> if (err)
> pr_err("SLUB: Unable to add boot slab %s to sysfs\n",
> s->name);
> --
> 2.25.1
>
>
--
Thanks,
Hyeonggon
