| Message ID | 20221031134747.3049593-1-liushixin2@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Refactor __kmem_cache_create() and fix memory leak | expand |
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 >
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 >>
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 > >> >
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 >> >> >> >
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(-)