* [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak
@ 2022-10-31 13:47 Liu Shixin
2022-10-31 13:47 ` [PATCH v2 1/3] mm/slab_common: Move cache_name to create_cache() Liu Shixin
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Liu Shixin @ 2022-10-31 13:47 UTC (permalink / raw)
To: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Vlastimil Babka, Roman Gushchin, Hyeonggon Yoo
Cc: linux-mm, linux-kernel, Liu Shixin
I found a memory leak of kobj->name in sysfs_slab_add() which is introduced
by 80da026a8e5d ("mm/slub: fix slab double-free in case of duplicate sysfs filename").
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.
We should use kobject_put() to free kobject.
But we can't simply add kobject_put() since it will free kmem_cache too.
If we use kobject_put(), we need to skip other release functions.
In this series, We refactor the code to separate sysfs_slab_add() and
debugfs_slab_add() from __kmem_cache_create(), and then use kobject_put()
to free kobject in sysfs_slab_add(). This can fix the memory leak of
kobject->name.
v1->v2: Fix build error reported by kernel test robot <lkp@intel.com>.
Liu Shixin (3):
mm/slab_common: Move cache_name to create_cache()
mm/slub: Refactor __kmem_cache_create()
mm/slub: Fix memory leak of kobj->name in sysfs_slab_add()
include/linux/slub_def.h | 11 +++++++++
mm/slab_common.c | 44 ++++++++++++++++++----------------
mm/slub.c | 52 ++++++++++------------------------------
3 files changed, 48 insertions(+), 59 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/3] mm/slab_common: Move cache_name to create_cache()
2022-10-31 13:47 [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak Liu Shixin
@ 2022-10-31 13:47 ` Liu Shixin
2022-10-31 13:47 ` [PATCH v2 2/3] mm/slub: Refactor __kmem_cache_create() Liu Shixin
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Liu Shixin @ 2022-10-31 13:47 UTC (permalink / raw)
To: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Vlastimil Babka, Roman Gushchin, Hyeonggon Yoo
Cc: linux-mm, linux-kernel, Liu Shixin
The string cache_name and its kmem_cache have same life cycle. The latter
is allocated in create_cache() so move cache_name to create_cache() too
for better error handing.
Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
mm/slab_common.c | 34 ++++++++++++++--------------------
1 file changed, 14 insertions(+), 20 deletions(-)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 33b1886b06eb..e5f430a17d95 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -209,17 +209,21 @@ static struct kmem_cache *create_cache(const char *name,
struct kmem_cache *root_cache)
{
struct kmem_cache *s;
- int err;
+ const char *cache_name;
+ int err = -ENOMEM;
if (WARN_ON(useroffset + usersize > object_size))
useroffset = usersize = 0;
- err = -ENOMEM;
s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);
if (!s)
- goto out;
+ return ERR_PTR(err);
- s->name = name;
+ cache_name = kstrdup_const(name, GFP_KERNEL);
+ if (!cache_name)
+ goto out_free_cache;
+
+ s->name = cache_name;
s->size = s->object_size = object_size;
s->align = align;
s->ctor = ctor;
@@ -228,18 +232,17 @@ static struct kmem_cache *create_cache(const char *name,
err = __kmem_cache_create(s, flags);
if (err)
- goto out_free_cache;
+ goto out_free_name;
s->refcount = 1;
list_add(&s->list, &slab_caches);
-out:
- if (err)
- return ERR_PTR(err);
return s;
+out_free_name:
+ kfree_const(s->name);
out_free_cache:
kmem_cache_free(kmem_cache, s);
- goto out;
+ return ERR_PTR(err);
}
/**
@@ -278,7 +281,6 @@ kmem_cache_create_usercopy(const char *name,
void (*ctor)(void *))
{
struct kmem_cache *s = NULL;
- const char *cache_name;
int err;
#ifdef CONFIG_SLUB_DEBUG
@@ -326,19 +328,11 @@ kmem_cache_create_usercopy(const char *name,
if (s)
goto out_unlock;
- cache_name = kstrdup_const(name, GFP_KERNEL);
- if (!cache_name) {
- err = -ENOMEM;
- goto out_unlock;
- }
-
- s = create_cache(cache_name, size,
+ s = create_cache(name, size,
calculate_alignment(flags, align, size),
flags, useroffset, usersize, ctor, NULL);
- if (IS_ERR(s)) {
+ if (IS_ERR(s))
err = PTR_ERR(s);
- kfree_const(cache_name);
- }
out_unlock:
mutex_unlock(&slab_mutex);
--
2.25.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] mm/slub: Refactor __kmem_cache_create()
2022-10-31 13:47 [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak Liu Shixin
2022-10-31 13:47 ` [PATCH v2 1/3] mm/slab_common: Move cache_name to create_cache() Liu Shixin
@ 2022-10-31 13:47 ` Liu Shixin
2022-10-31 13:47 ` [PATCH v2 3/3] mm/slub: Fix memory leak of kobj->name in sysfs_slab_add() Liu Shixin
2022-11-02 7:46 ` [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak Hyeonggon Yoo
3 siblings, 0 replies; 8+ messages in thread
From: Liu Shixin @ 2022-10-31 13:47 UTC (permalink / raw)
To: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Vlastimil Babka, Roman Gushchin, Hyeonggon Yoo
Cc: linux-mm, linux-kernel, Liu Shixin
Separate sysfs_slab_add() and debugfs_slab_add() from __kmem_cache_create()
can help to fix a memory leak about kobject. After this patch, we can fix
the memory leak naturally by calling kobject_put() to free kobject and
associated kmem_cache when sysfs_slab_add() failed.
Besides, after that, we can easy to provide sysfs and debugfs support for
other allocators too.
Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
include/linux/slub_def.h | 11 ++++++++++
mm/slab_common.c | 12 +++++++++++
mm/slub.c | 44 +++++++---------------------------------
3 files changed, 30 insertions(+), 37 deletions(-)
diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index f9c68a9dac04..26d56c4c74d1 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -144,9 +144,14 @@ struct kmem_cache {
#ifdef CONFIG_SYSFS
#define SLAB_SUPPORTS_SYSFS
+int sysfs_slab_add(struct kmem_cache *);
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)
+{
+ return 0;
+}
static inline void sysfs_slab_unlink(struct kmem_cache *s)
{
}
@@ -155,6 +160,12 @@ static inline void sysfs_slab_release(struct kmem_cache *s)
}
#endif
+#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB_DEBUG)
+void debugfs_slab_add(struct kmem_cache *);
+#else
+static inline void debugfs_slab_add(struct kmem_cache *s) { }
+#endif
+
void *fixup_red_left(struct kmem_cache *s, void *p);
static inline void *nearest_obj(struct kmem_cache *cache, const struct slab *slab,
diff --git a/mm/slab_common.c b/mm/slab_common.c
index e5f430a17d95..55e2cf064dfe 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -234,6 +234,18 @@ static struct kmem_cache *create_cache(const char *name,
if (err)
goto out_free_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);
+ return ERR_PTR(err);
+ }
+ debugfs_slab_add(s);
+ }
+#endif
+
s->refcount = 1;
list_add(&s->list, &slab_caches);
return s;
diff --git a/mm/slub.c b/mm/slub.c
index ba94eb6fda78..a1ad759753ce 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -299,20 +299,12 @@ struct track {
enum track_item { TRACK_ALLOC, TRACK_FREE };
#ifdef CONFIG_SYSFS
-static int sysfs_slab_add(struct kmem_cache *);
static int sysfs_slab_alias(struct kmem_cache *, const char *);
#else
-static inline int sysfs_slab_add(struct kmem_cache *s) { return 0; }
static inline int sysfs_slab_alias(struct kmem_cache *s, const char *p)
{ return 0; }
#endif
-#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB_DEBUG)
-static void debugfs_slab_add(struct kmem_cache *);
-#else
-static inline void debugfs_slab_add(struct kmem_cache *s) { }
-#endif
-
static inline void stat(const struct kmem_cache *s, enum stat_item si)
{
#ifdef CONFIG_SLUB_STATS
@@ -4297,7 +4289,7 @@ static int calculate_sizes(struct kmem_cache *s)
return !!oo_objects(s->oo);
}
-static int kmem_cache_open(struct kmem_cache *s, slab_flags_t flags)
+int __kmem_cache_create(struct kmem_cache *s, slab_flags_t flags)
{
s->flags = kmem_cache_flags(s->size, flags, s->name);
#ifdef CONFIG_SLAB_FREELIST_HARDENED
@@ -4900,30 +4892,6 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align,
return s;
}
-int __kmem_cache_create(struct kmem_cache *s, slab_flags_t flags)
-{
- int err;
-
- err = kmem_cache_open(s, flags);
- if (err)
- return err;
-
- /* Mutex is not taken during early boot */
- if (slab_state <= UP)
- return 0;
-
- err = sysfs_slab_add(s);
- if (err) {
- __kmem_cache_release(s);
- return err;
- }
-
- if (s->flags & SLAB_STORE_USER)
- debugfs_slab_add(s);
-
- return 0;
-}
-
#ifdef CONFIG_SYSFS
static int count_inuse(struct slab *slab)
{
@@ -5913,7 +5881,7 @@ static char *create_unique_id(struct kmem_cache *s)
return name;
}
-static int sysfs_slab_add(struct kmem_cache *s)
+int sysfs_slab_add(struct kmem_cache *s)
{
int err;
const char *name;
@@ -6236,10 +6204,13 @@ static const struct file_operations slab_debugfs_fops = {
.release = slab_debug_trace_release,
};
-static void debugfs_slab_add(struct kmem_cache *s)
+void debugfs_slab_add(struct kmem_cache *s)
{
struct dentry *slab_cache_dir;
+ if (!(s->flags & SLAB_STORE_USER))
+ return;
+
if (unlikely(!slab_debugfs_root))
return;
@@ -6264,8 +6235,7 @@ static int __init slab_debugfs_init(void)
slab_debugfs_root = debugfs_create_dir("slab", NULL);
list_for_each_entry(s, &slab_caches, list)
- if (s->flags & SLAB_STORE_USER)
- debugfs_slab_add(s);
+ debugfs_slab_add(s);
return 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] mm/slub: Fix memory leak of kobj->name in sysfs_slab_add()
2022-10-31 13:47 [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak Liu Shixin
2022-10-31 13:47 ` [PATCH v2 1/3] mm/slab_common: Move cache_name to create_cache() Liu Shixin
2022-10-31 13:47 ` [PATCH v2 2/3] mm/slub: Refactor __kmem_cache_create() Liu Shixin
@ 2022-10-31 13:47 ` Liu Shixin
2022-11-02 7:46 ` [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak Hyeonggon Yoo
3 siblings, 0 replies; 8+ messages in thread
From: Liu Shixin @ 2022-10-31 13:47 UTC (permalink / raw)
To: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Vlastimil Babka, Roman Gushchin, Hyeonggon Yoo
Cc: linux-mm, linux-kernel, Liu Shixin
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.
Fixes: 80da026a8e5d ("mm/slub: fix slab double-free in case of duplicate sysfs filename")
Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
mm/slab_common.c | 4 +---
mm/slub.c | 8 ++++++--
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 55e2cf064dfe..9337724b5c76 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -238,10 +238,8 @@ static struct kmem_cache *create_cache(const char *name,
/* Mutex is not taken during early boot */
if (slab_state >= FULL) {
err = sysfs_slab_add(s);
- if (err) {
- slab_kmem_cache_release(s);
+ if (err)
return ERR_PTR(err);
- }
debugfs_slab_add(s);
}
#endif
diff --git a/mm/slub.c b/mm/slub.c
index a1ad759753ce..f8883bc642b8 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5911,14 +5911,16 @@ int sysfs_slab_add(struct kmem_cache *s)
* for the symlinks.
*/
name = create_unique_id(s);
- if (IS_ERR(name))
+ if (IS_ERR(name)) {
+ 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 +5936,8 @@ int sysfs_slab_add(struct kmem_cache *s)
return err;
out_del_kobj:
kobject_del(&s->kobj);
+out_put_kobj:
+ kobject_put(&s->kobj);
goto out;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak
2022-10-31 13:47 [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak Liu Shixin
` (2 preceding siblings ...)
2022-10-31 13:47 ` [PATCH v2 3/3] mm/slub: Fix memory leak of kobj->name in sysfs_slab_add() Liu Shixin
@ 2022-11-02 7:46 ` Hyeonggon Yoo
2022-11-02 8:53 ` Liu Shixin
3 siblings, 1 reply; 8+ messages in thread
From: Hyeonggon Yoo @ 2022-11-02 7:46 UTC (permalink / raw)
To: Liu Shixin
Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Vlastimil Babka, Roman Gushchin, linux-mm,
linux-kernel
On Mon, Oct 31, 2022 at 09:47:44PM +0800, Liu Shixin wrote:
> I found a memory leak of kobj->name in sysfs_slab_add() which is introduced
> by 80da026a8e5d ("mm/slub: fix slab double-free in case of duplicate sysfs filename").
> Following the rules stated in the comment for kobject_init_and_add():
Thank you for reporting this! Indeed it seems tried to fix double free but
introduced a leak.
> If this function returns an error, kobject_put() must be called to
> properly clean up the memory associated with the object.
>
> We should use kobject_put() to free kobject.
But what to do if a cache is created early and later sysfs_slab_add() failed?
(Which is unlikely on normal condition)
With this series it introduces use-after-free if sysfs_slab_add() in
slab_sysfs_init() failed. Should we just call BUG() or something like that?
> But we can't simply add kobject_put() since it will free kmem_cache too.
> If we use kobject_put(), we need to skip other release functions.
>
> In this series, We refactor the code to separate sysfs_slab_add() and
> debugfs_slab_add() from __kmem_cache_create(), and then use kobject_put()
> to free kobject in sysfs_slab_add(). This can fix the memory leak of
> kobject->name.
>
> v1->v2: Fix build error reported by kernel test robot <lkp@intel.com>.
>
> Liu Shixin (3):
> mm/slab_common: Move cache_name to create_cache()
> mm/slub: Refactor __kmem_cache_create()
> mm/slub: Fix memory leak of kobj->name in sysfs_slab_add()
>
> include/linux/slub_def.h | 11 +++++++++
> mm/slab_common.c | 44 ++++++++++++++++++----------------
> mm/slub.c | 52 ++++++++++------------------------------
> 3 files changed, 48 insertions(+), 59 deletions(-)
>
> --
> 2.25.1
>
--
Thanks,
Hyeonggon
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak
2022-11-02 7:46 ` [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak Hyeonggon Yoo
@ 2022-11-02 8:53 ` Liu Shixin
2022-11-03 13:23 ` Hyeonggon Yoo
0 siblings, 1 reply; 8+ messages in thread
From: Liu Shixin @ 2022-11-02 8:53 UTC (permalink / raw)
To: Hyeonggon Yoo
Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Vlastimil Babka, Roman Gushchin, linux-mm,
linux-kernel
On 2022/11/2 15:46, Hyeonggon Yoo wrote:
> On Mon, Oct 31, 2022 at 09:47:44PM +0800, Liu Shixin wrote:
>> I found a memory leak of kobj->name in sysfs_slab_add() which is introduced
>> by 80da026a8e5d ("mm/slub: fix slab double-free in case of duplicate sysfs filename").
>> Following the rules stated in the comment for kobject_init_and_add():
> Thank you for reporting this! Indeed it seems tried to fix double free but
> introduced a leak.
>
>> If this function returns an error, kobject_put() must be called to
>> properly clean up the memory associated with the object.
>>
>> We should use kobject_put() to free kobject.
> But what to do if a cache is created early and later sysfs_slab_add() failed?
> (Which is unlikely on normal condition)
>
> With this series it introduces use-after-free if sysfs_slab_add() in
> slab_sysfs_init() failed. Should we just call BUG() or something like that?
Thanks for your discovery, what I missed. I prefer to panic directly, just as create_boot_cache() does.
Of couse, if you want the system to continue booting, I think it's possible to distinguish them
by slab_state.
Looking forward to your advice.
Thanks,
>
>> But we can't simply add kobject_put() since it will free kmem_cache too.
>> If we use kobject_put(), we need to skip other release functions.
>>
>> In this series, We refactor the code to separate sysfs_slab_add() and
>> debugfs_slab_add() from __kmem_cache_create(), and then use kobject_put()
>> to free kobject in sysfs_slab_add(). This can fix the memory leak of
>> kobject->name.
>>
>> v1->v2: Fix build error reported by kernel test robot <lkp@intel.com>.
>>
>> Liu Shixin (3):
>> mm/slab_common: Move cache_name to create_cache()
>> mm/slub: Refactor __kmem_cache_create()
>> mm/slub: Fix memory leak of kobj->name in sysfs_slab_add()
>>
>> include/linux/slub_def.h | 11 +++++++++
>> mm/slab_common.c | 44 ++++++++++++++++++----------------
>> mm/slub.c | 52 ++++++++++------------------------------
>> 3 files changed, 48 insertions(+), 59 deletions(-)
>>
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak
2022-11-02 8:53 ` Liu Shixin
@ 2022-11-03 13:23 ` Hyeonggon Yoo
2022-11-09 11:16 ` Vlastimil Babka
0 siblings, 1 reply; 8+ messages in thread
From: Hyeonggon Yoo @ 2022-11-03 13:23 UTC (permalink / raw)
To: Liu Shixin
Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Vlastimil Babka, Roman Gushchin, linux-mm,
linux-kernel
On Wed, Nov 02, 2022 at 04:53:08PM +0800, Liu Shixin wrote:
> On 2022/11/2 15:46, Hyeonggon Yoo wrote:
> > On Mon, Oct 31, 2022 at 09:47:44PM +0800, Liu Shixin wrote:
> >> I found a memory leak of kobj->name in sysfs_slab_add() which is introduced
> >> by 80da026a8e5d ("mm/slub: fix slab double-free in case of duplicate sysfs filename").
> >> Following the rules stated in the comment for kobject_init_and_add():
> > Thank you for reporting this! Indeed it seems tried to fix double free but
> > introduced a leak.
> >
> >> If this function returns an error, kobject_put() must be called to
> >> properly clean up the memory associated with the object.
> >>
> >> We should use kobject_put() to free kobject.
> > But what to do if a cache is created early and later sysfs_slab_add() failed?
> > (Which is unlikely on normal condition)
> >
> > With this series it introduces use-after-free if sysfs_slab_add() in
> > slab_sysfs_init() failed. Should we just call BUG() or something like that?
>
> Thanks for your discovery, what I missed.
You're welcome.
> I prefer to panic directly, just as create_boot_cache() does.
IMHO that should be nothing serious. but let's hear maintainers' opinion.
> Of couse, if you want the system to continue booting, I think it's possible to distinguish them
> by slab_state.
I'm afraid to make it more complex :(
> Looking forward to your advice.
> Thanks,
> >
> >> But we can't simply add kobject_put() since it will free kmem_cache too.
> >> If we use kobject_put(), we need to skip other release functions.
> >>
> >> In this series, We refactor the code to separate sysfs_slab_add() and
> >> debugfs_slab_add() from __kmem_cache_create(), and then use kobject_put()
> >> to free kobject in sysfs_slab_add(). This can fix the memory leak of
> >> kobject->name.
> >>
> >> v1->v2: Fix build error reported by kernel test robot <lkp@intel.com>.
> >>
> >> Liu Shixin (3):
> >> mm/slab_common: Move cache_name to create_cache()
> >> mm/slub: Refactor __kmem_cache_create()
> >> mm/slub: Fix memory leak of kobj->name in sysfs_slab_add()
> >>
> >> include/linux/slub_def.h | 11 +++++++++
> >> mm/slab_common.c | 44 ++++++++++++++++++----------------
> >> mm/slub.c | 52 ++++++++++------------------------------
> >> 3 files changed, 48 insertions(+), 59 deletions(-)
> >>
> >> --
> >> 2.25.1
> >>
>
--
Thanks,
Hyeonggon
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak
2022-11-03 13:23 ` Hyeonggon Yoo
@ 2022-11-09 11:16 ` Vlastimil Babka
0 siblings, 0 replies; 8+ messages in thread
From: Vlastimil Babka @ 2022-11-09 11:16 UTC (permalink / raw)
To: Hyeonggon Yoo, Liu Shixin
Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Roman Gushchin, linux-mm, linux-kernel,
Rasmus Villemoes
On 11/3/22 14:23, Hyeonggon Yoo wrote:
> On Wed, Nov 02, 2022 at 04:53:08PM +0800, Liu Shixin wrote:
>> On 2022/11/2 15:46, Hyeonggon Yoo wrote:
>> > On Mon, Oct 31, 2022 at 09:47:44PM +0800, Liu Shixin wrote:
>> >> I found a memory leak of kobj->name in sysfs_slab_add() which is introduced
>> >> by 80da026a8e5d ("mm/slub: fix slab double-free in case of duplicate sysfs filename").
>> >> Following the rules stated in the comment for kobject_init_and_add():
>> > Thank you for reporting this! Indeed it seems tried to fix double free but
>> > introduced a leak.
>> >
>> >> If this function returns an error, kobject_put() must be called to
>> >> properly clean up the memory associated with the object.
>> >>
>> >> We should use kobject_put() to free kobject.
>> > But what to do if a cache is created early and later sysfs_slab_add() failed?
>> > (Which is unlikely on normal condition)
>> >
>> > With this series it introduces use-after-free if sysfs_slab_add() in
>> > slab_sysfs_init() failed. Should we just call BUG() or something like that?
>>
>> Thanks for your discovery, what I missed.
>
> You're welcome.
>
>> I prefer to panic directly, just as create_boot_cache() does.
>
> IMHO that should be nothing serious. but let's hear maintainers' opinion.
>
>> Of couse, if you want the system to continue booting, I think it's possible to distinguish them
>> by slab_state.
>
> I'm afraid to make it more complex :(
+Cc Rasmus who did a recent patch in this area. Thread starts here:
https://lore.kernel.org/all/20221031134747.3049593-1-liushixin2@huawei.com/
As for me, I don't think we should be addung new BUG() or panic() in
general, and especially not for a failing sysfs add. AFAICS
create_boot_cache() might panic, but not because of sysfs, as that's delayed
until slab_sysfs_init() and we don't panic anymore in the latter.
So yeah, it could work to tell sysfs_slab_add() whether it should not do the
kobject_put() as it's a boot cache. The slab_state should work.
>
>> Looking forward to your advice.
>> Thanks,
>> >
>> >> But we can't simply add kobject_put() since it will free kmem_cache too.
>> >> If we use kobject_put(), we need to skip other release functions.
>> >>
>> >> In this series, We refactor the code to separate sysfs_slab_add() and
>> >> debugfs_slab_add() from __kmem_cache_create(), and then use kobject_put()
>> >> to free kobject in sysfs_slab_add(). This can fix the memory leak of
>> >> kobject->name.
>> >>
>> >> v1->v2: Fix build error reported by kernel test robot <lkp@intel.com>.
>> >>
>> >> Liu Shixin (3):
>> >> mm/slab_common: Move cache_name to create_cache()
>> >> mm/slub: Refactor __kmem_cache_create()
>> >> mm/slub: Fix memory leak of kobj->name in sysfs_slab_add()
>> >>
>> >> include/linux/slub_def.h | 11 +++++++++
>> >> mm/slab_common.c | 44 ++++++++++++++++++----------------
>> >> mm/slub.c | 52 ++++++++++------------------------------
>> >> 3 files changed, 48 insertions(+), 59 deletions(-)
>> >>
>> >> --
>> >> 2.25.1
>> >>
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-11-09 11:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-31 13:47 [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak Liu Shixin
2022-10-31 13:47 ` [PATCH v2 1/3] mm/slab_common: Move cache_name to create_cache() Liu Shixin
2022-10-31 13:47 ` [PATCH v2 2/3] mm/slub: Refactor __kmem_cache_create() Liu Shixin
2022-10-31 13:47 ` [PATCH v2 3/3] mm/slub: Fix memory leak of kobj->name in sysfs_slab_add() Liu Shixin
2022-11-02 7:46 ` [PATCH v2 0/3] Refactor __kmem_cache_create() and fix memory leak Hyeonggon Yoo
2022-11-02 8:53 ` Liu Shixin
2022-11-03 13:23 ` Hyeonggon Yoo
2022-11-09 11:16 ` Vlastimil Babka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).