* Re: [PATCH] mm/slab: Fix a kmemleak in kmem_cache_destroy()
2024-02-28 3:04 [PATCH] mm/slab: Fix a kmemleak in kmem_cache_destroy() Xiaolei Wang
@ 2024-02-28 7:34 ` Vlastimil Babka
2024-02-28 8:15 ` xiaolei wang
2024-02-28 9:31 ` Chengming Zhou
2024-03-01 15:46 ` Vlastimil Babka
2 siblings, 1 reply; 5+ messages in thread
From: Vlastimil Babka @ 2024-02-28 7:34 UTC (permalink / raw)
To: Xiaolei Wang, cl, penberg, rientjes, iamjoonsoo.kim, akpm,
roman.gushchin, 42.hyeyoo, linux-mm, linux-kernel
On 2/28/24 04:04, Xiaolei Wang wrote:
> For earlier kmem cache creation, slab_sysfs_init() has not been called.
> Consequently, kmem_cache_destroy() cannot utilize kobj_type::release to
> release the kmem_cache structure. Therefore, tweak kmem_cache_release()
> to use slab_kmem_cache_release() for releasing kmem_cache when slab_state
> isn't FULL. This will fixes the memory leaks like following:
So do I understand correctly the leak happens when a cache is destroyed
while slab_state < FULL? Do we ever destroy a cache that early?
> unreferenced object 0xffff0000c2d87080 (size 128):
> comm "swapper/0", pid 1, jiffies 4294893428
> hex dump (first 32 bytes):
> 00 00 00 00 ad 4e ad de ff ff ff ff 6b 6b 6b 6b .....N......kkkk
> ff ff ff ff ff ff ff ff b8 ab 48 89 00 80 ff ff.....H.....
> backtrace (crc 8819d0f6):
> [<ffff80008317a298>] kmemleak_alloc+0xb0/0xc4
> [<ffff8000807e553c>] kmem_cache_alloc_node+0x288/0x3a8
> [<ffff8000807e95f0>] __kmem_cache_create+0x1e4/0x64c
> [<ffff8000807216bc>] kmem_cache_create_usercopy+0x1c4/0x2cc
> [<ffff8000807217e0>] kmem_cache_create+0x1c/0x28
> [<ffff8000819f6278>] arm_v7s_alloc_pgtable+0x1c0/0x6d4
> [<ffff8000819f53a0>] alloc_io_pgtable_ops+0xe8/0x2d0
> [<ffff800084b2d2c4>] arm_v7s_do_selftests+0xe0/0x73c
> [<ffff800080016b68>] do_one_initcall+0x11c/0x7ac
> [<ffff800084a71ddc>] kernel_init_freeable+0x53c/0xbb8
> [<ffff8000831728d8>] kernel_init+0x24/0x144
> [<ffff800080018e98>] ret_from_fork+0x10/0x20
>
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> ---
> mm/slab_common.c | 8 ++++++--
> mm/slub.c | 6 ++----
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 238293b1dbe1..b6b35bcdd196 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -404,8 +404,12 @@ EXPORT_SYMBOL(kmem_cache_create);
> */
> static void kmem_cache_release(struct kmem_cache *s)
> {
> - sysfs_slab_unlink(s);
> - sysfs_slab_release(s);
> + if (slab_state >= FULL) {
> + sysfs_slab_unlink(s);
> + sysfs_slab_release(s);
> + } else {
> + slab_kmem_cache_release(s);
> + }
> }
> #else
> static void kmem_cache_release(struct kmem_cache *s)
> diff --git a/mm/slub.c b/mm/slub.c
> index 2ef88bbf56a3..9ba59d064b6d 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -6792,14 +6792,12 @@ static int sysfs_slab_add(struct kmem_cache *s)
>
> void sysfs_slab_unlink(struct kmem_cache *s)
> {
> - if (slab_state >= FULL)
> - kobject_del(&s->kobj);
> + kobject_del(&s->kobj);
> }
>
> void sysfs_slab_release(struct kmem_cache *s)
> {
> - if (slab_state >= FULL)
> - kobject_put(&s->kobj);
> + kobject_put(&s->kobj);
> }
>
> /*
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] mm/slab: Fix a kmemleak in kmem_cache_destroy()
2024-02-28 7:34 ` Vlastimil Babka
@ 2024-02-28 8:15 ` xiaolei wang
0 siblings, 0 replies; 5+ messages in thread
From: xiaolei wang @ 2024-02-28 8:15 UTC (permalink / raw)
To: Vlastimil Babka, cl, penberg, rientjes, iamjoonsoo.kim, akpm,
roman.gushchin, 42.hyeyoo, linux-mm, linux-kernel
On 2/28/24 15:34, Vlastimil Babka wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> On 2/28/24 04:04, Xiaolei Wang wrote:
>> For earlier kmem cache creation, slab_sysfs_init() has not been called.
>> Consequently, kmem_cache_destroy() cannot utilize kobj_type::release to
>> release the kmem_cache structure. Therefore, tweak kmem_cache_release()
>> to use slab_kmem_cache_release() for releasing kmem_cache when slab_state
>> isn't FULL. This will fixes the memory leaks like following:
> So do I understand correctly the leak happens when a cache is destroyed
> while slab_state < FULL? Do we ever destroy a cache that early?
Yes, like some test programs, test_meminit_init() or
arm_v7s_alloc_pgtable(),
destroy will cause leaks after use, and when slab_state < FULL,
there are many places to create cache, which itself also has the risk of
leaks
thanks
xiaolei
>
>> unreferenced object 0xffff0000c2d87080 (size 128):
>> comm "swapper/0", pid 1, jiffies 4294893428
>> hex dump (first 32 bytes):
>> 00 00 00 00 ad 4e ad de ff ff ff ff 6b 6b 6b 6b .....N......kkkk
>> ff ff ff ff ff ff ff ff b8 ab 48 89 00 80 ff ff.....H.....
>> backtrace (crc 8819d0f6):
>> [<ffff80008317a298>] kmemleak_alloc+0xb0/0xc4
>> [<ffff8000807e553c>] kmem_cache_alloc_node+0x288/0x3a8
>> [<ffff8000807e95f0>] __kmem_cache_create+0x1e4/0x64c
>> [<ffff8000807216bc>] kmem_cache_create_usercopy+0x1c4/0x2cc
>> [<ffff8000807217e0>] kmem_cache_create+0x1c/0x28
>> [<ffff8000819f6278>] arm_v7s_alloc_pgtable+0x1c0/0x6d4
>> [<ffff8000819f53a0>] alloc_io_pgtable_ops+0xe8/0x2d0
>> [<ffff800084b2d2c4>] arm_v7s_do_selftests+0xe0/0x73c
>> [<ffff800080016b68>] do_one_initcall+0x11c/0x7ac
>> [<ffff800084a71ddc>] kernel_init_freeable+0x53c/0xbb8
>> [<ffff8000831728d8>] kernel_init+0x24/0x144
>> [<ffff800080018e98>] ret_from_fork+0x10/0x20
>>
>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>> ---
>> mm/slab_common.c | 8 ++++++--
>> mm/slub.c | 6 ++----
>> 2 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index 238293b1dbe1..b6b35bcdd196 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
>> @@ -404,8 +404,12 @@ EXPORT_SYMBOL(kmem_cache_create);
>> */
>> static void kmem_cache_release(struct kmem_cache *s)
>> {
>> - sysfs_slab_unlink(s);
>> - sysfs_slab_release(s);
>> + if (slab_state >= FULL) {
>> + sysfs_slab_unlink(s);
>> + sysfs_slab_release(s);
>> + } else {
>> + slab_kmem_cache_release(s);
>> + }
>> }
>> #else
>> static void kmem_cache_release(struct kmem_cache *s)
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 2ef88bbf56a3..9ba59d064b6d 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -6792,14 +6792,12 @@ static int sysfs_slab_add(struct kmem_cache *s)
>>
>> void sysfs_slab_unlink(struct kmem_cache *s)
>> {
>> - if (slab_state >= FULL)
>> - kobject_del(&s->kobj);
>> + kobject_del(&s->kobj);
>> }
>>
>> void sysfs_slab_release(struct kmem_cache *s)
>> {
>> - if (slab_state >= FULL)
>> - kobject_put(&s->kobj);
>> + kobject_put(&s->kobj);
>> }
>>
>> /*
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/slab: Fix a kmemleak in kmem_cache_destroy()
2024-02-28 3:04 [PATCH] mm/slab: Fix a kmemleak in kmem_cache_destroy() Xiaolei Wang
2024-02-28 7:34 ` Vlastimil Babka
@ 2024-02-28 9:31 ` Chengming Zhou
2024-03-01 15:46 ` Vlastimil Babka
2 siblings, 0 replies; 5+ messages in thread
From: Chengming Zhou @ 2024-02-28 9:31 UTC (permalink / raw)
To: Xiaolei Wang, cl, penberg, rientjes, iamjoonsoo.kim, akpm,
vbabka, roman.gushchin, 42.hyeyoo, linux-mm, linux-kernel
On 2024/2/28 11:04, Xiaolei Wang wrote:
> For earlier kmem cache creation, slab_sysfs_init() has not been called.
> Consequently, kmem_cache_destroy() cannot utilize kobj_type::release to
> release the kmem_cache structure. Therefore, tweak kmem_cache_release()
> to use slab_kmem_cache_release() for releasing kmem_cache when slab_state
> isn't FULL. This will fixes the memory leaks like following:
>
> unreferenced object 0xffff0000c2d87080 (size 128):
> comm "swapper/0", pid 1, jiffies 4294893428
> hex dump (first 32 bytes):
> 00 00 00 00 ad 4e ad de ff ff ff ff 6b 6b 6b 6b .....N......kkkk
> ff ff ff ff ff ff ff ff b8 ab 48 89 00 80 ff ff.....H.....
> backtrace (crc 8819d0f6):
> [<ffff80008317a298>] kmemleak_alloc+0xb0/0xc4
> [<ffff8000807e553c>] kmem_cache_alloc_node+0x288/0x3a8
> [<ffff8000807e95f0>] __kmem_cache_create+0x1e4/0x64c
> [<ffff8000807216bc>] kmem_cache_create_usercopy+0x1c4/0x2cc
> [<ffff8000807217e0>] kmem_cache_create+0x1c/0x28
> [<ffff8000819f6278>] arm_v7s_alloc_pgtable+0x1c0/0x6d4
> [<ffff8000819f53a0>] alloc_io_pgtable_ops+0xe8/0x2d0
> [<ffff800084b2d2c4>] arm_v7s_do_selftests+0xe0/0x73c
> [<ffff800080016b68>] do_one_initcall+0x11c/0x7ac
> [<ffff800084a71ddc>] kernel_init_freeable+0x53c/0xbb8
> [<ffff8000831728d8>] kernel_init+0x24/0x144
> [<ffff800080018e98>] ret_from_fork+0x10/0x20
>
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
Looks good to me. And I notice the PARTIAL_NODE state of slab_state is
also useless since SLAB removed.
Reviewed-by: Chengming Zhou <chengming.zhou@linux.dev>
Thanks.
> ---
> mm/slab_common.c | 8 ++++++--
> mm/slub.c | 6 ++----
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 238293b1dbe1..b6b35bcdd196 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -404,8 +404,12 @@ EXPORT_SYMBOL(kmem_cache_create);
> */
> static void kmem_cache_release(struct kmem_cache *s)
> {
> - sysfs_slab_unlink(s);
> - sysfs_slab_release(s);
> + if (slab_state >= FULL) {
> + sysfs_slab_unlink(s);
> + sysfs_slab_release(s);
> + } else {
> + slab_kmem_cache_release(s);
> + }
> }
> #else
> static void kmem_cache_release(struct kmem_cache *s)
> diff --git a/mm/slub.c b/mm/slub.c
> index 2ef88bbf56a3..9ba59d064b6d 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -6792,14 +6792,12 @@ static int sysfs_slab_add(struct kmem_cache *s)
>
> void sysfs_slab_unlink(struct kmem_cache *s)
> {
> - if (slab_state >= FULL)
> - kobject_del(&s->kobj);
> + kobject_del(&s->kobj);
> }
>
> void sysfs_slab_release(struct kmem_cache *s)
> {
> - if (slab_state >= FULL)
> - kobject_put(&s->kobj);
> + kobject_put(&s->kobj);
> }
>
> /*
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] mm/slab: Fix a kmemleak in kmem_cache_destroy()
2024-02-28 3:04 [PATCH] mm/slab: Fix a kmemleak in kmem_cache_destroy() Xiaolei Wang
2024-02-28 7:34 ` Vlastimil Babka
2024-02-28 9:31 ` Chengming Zhou
@ 2024-03-01 15:46 ` Vlastimil Babka
2 siblings, 0 replies; 5+ messages in thread
From: Vlastimil Babka @ 2024-03-01 15:46 UTC (permalink / raw)
To: Xiaolei Wang, cl, penberg, rientjes, iamjoonsoo.kim, akpm,
roman.gushchin, 42.hyeyoo, linux-mm, linux-kernel
On 2/28/24 04:04, Xiaolei Wang wrote:
> For earlier kmem cache creation, slab_sysfs_init() has not been called.
> Consequently, kmem_cache_destroy() cannot utilize kobj_type::release to
> release the kmem_cache structure. Therefore, tweak kmem_cache_release()
> to use slab_kmem_cache_release() for releasing kmem_cache when slab_state
> isn't FULL. This will fixes the memory leaks like following:
>
> unreferenced object 0xffff0000c2d87080 (size 128):
> comm "swapper/0", pid 1, jiffies 4294893428
> hex dump (first 32 bytes):
> 00 00 00 00 ad 4e ad de ff ff ff ff 6b 6b 6b 6b .....N......kkkk
> ff ff ff ff ff ff ff ff b8 ab 48 89 00 80 ff ff.....H.....
> backtrace (crc 8819d0f6):
> [<ffff80008317a298>] kmemleak_alloc+0xb0/0xc4
> [<ffff8000807e553c>] kmem_cache_alloc_node+0x288/0x3a8
> [<ffff8000807e95f0>] __kmem_cache_create+0x1e4/0x64c
> [<ffff8000807216bc>] kmem_cache_create_usercopy+0x1c4/0x2cc
> [<ffff8000807217e0>] kmem_cache_create+0x1c/0x28
> [<ffff8000819f6278>] arm_v7s_alloc_pgtable+0x1c0/0x6d4
> [<ffff8000819f53a0>] alloc_io_pgtable_ops+0xe8/0x2d0
> [<ffff800084b2d2c4>] arm_v7s_do_selftests+0xe0/0x73c
> [<ffff800080016b68>] do_one_initcall+0x11c/0x7ac
> [<ffff800084a71ddc>] kernel_init_freeable+0x53c/0xbb8
> [<ffff8000831728d8>] kernel_init+0x24/0x144
> [<ffff800080018e98>] ret_from_fork+0x10/0x20
>
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
Thanks, added to slab/for-next
> ---
> mm/slab_common.c | 8 ++++++--
> mm/slub.c | 6 ++----
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 238293b1dbe1..b6b35bcdd196 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -404,8 +404,12 @@ EXPORT_SYMBOL(kmem_cache_create);
> */
> static void kmem_cache_release(struct kmem_cache *s)
> {
> - sysfs_slab_unlink(s);
> - sysfs_slab_release(s);
> + if (slab_state >= FULL) {
> + sysfs_slab_unlink(s);
> + sysfs_slab_release(s);
> + } else {
> + slab_kmem_cache_release(s);
> + }
> }
> #else
> static void kmem_cache_release(struct kmem_cache *s)
> diff --git a/mm/slub.c b/mm/slub.c
> index 2ef88bbf56a3..9ba59d064b6d 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -6792,14 +6792,12 @@ static int sysfs_slab_add(struct kmem_cache *s)
>
> void sysfs_slab_unlink(struct kmem_cache *s)
> {
> - if (slab_state >= FULL)
> - kobject_del(&s->kobj);
> + kobject_del(&s->kobj);
> }
>
> void sysfs_slab_release(struct kmem_cache *s)
> {
> - if (slab_state >= FULL)
> - kobject_put(&s->kobj);
> + kobject_put(&s->kobj);
> }
>
> /*
^ permalink raw reply [flat|nested] 5+ messages in thread