[f2fs-dev] [PATCH] f2fs: fix possible memory leak in f2fs_init_sysfs()
Inject fault while probing module, kset_register() may fail, if it fails, but the refcount of kobject is not decreased to 0, the name allocated in kobject_set_name() is leaked. Fix this by calling kset_put(), so that name can be freed in callback function kobject_cleanup(). unreferenced object 0x888101b7cc80 (size 8): comm "modprobe", pid 252, jiffies 4294691378 (age 31.760s) hex dump (first 8 bytes): 66 32 66 73 00 88 ff ff f2fs backtrace: [<1db5b408>] __kmalloc_node_track_caller+0x44/0x1b0 [<2783a073>] kstrdup+0x3a/0x70 [] kstrdup_const+0x63/0x80 [<3e5cf8f7>] kvasprintf_const+0x149/0x180 [ ] kobject_set_name_vargs+0x56/0x150 [<44611660>] kobject_set_name+0xab/0xe0 Fixes: bf9e697ecd42 ("f2fs: expose features to sysfs entry") Signed-off-by: Yang Yingliang --- fs/f2fs/sysfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c index df27afd71ef4..2ef7a48967be 100644 --- a/fs/f2fs/sysfs.c +++ b/fs/f2fs/sysfs.c @@ -1250,8 +1250,10 @@ int __init f2fs_init_sysfs(void) kobject_set_name(&f2fs_kset.kobj, "f2fs"); f2fs_kset.kobj.parent = fs_kobj; ret = kset_register(&f2fs_kset); - if (ret) + if (ret) { + kset_put(&f2fs_kset); return ret; + } ret = kobject_init_and_add(&f2fs_feat, &f2fs_feat_ktype, NULL, "features"); -- 2.25.1 ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 10/11] drm/amdgpu/discovery: fix possible memory leak
If kset_register() fails, the refcount of kobject is not 0, the name allocated in kobject_set_name(&kset.kobj, ...) is leaked. Fix this by calling kset_put(), so that it will be freed in callback function kobject_cleanup(). Cc: [email protected] Fixes: a6c40b178092 ("drm/amdgpu: Show IP discovery in sysfs") Signed-off-by: Yang Yingliang Reviewed-by: Luben Tuikov --- drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c index 3993e6134914..638edcf70227 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c @@ -863,7 +863,7 @@ static int amdgpu_discovery_sysfs_ips(struct amdgpu_device *adev, res = kset_register(&ip_hw_id->hw_id_kset); if (res) { DRM_ERROR("Couldn't register ip_hw_id kset"); - kfree(ip_hw_id); + kset_put(&ip_hw_id->hw_id_kset); return res; } if (hw_id_names[ii]) { @@ -954,7 +954,7 @@ static int amdgpu_discovery_sysfs_recurse(struct amdgpu_device *adev) res = kset_register(&ip_die_entry->ip_kset); if (res) { DRM_ERROR("Couldn't register ip_die_entry kset"); - kfree(ip_die_entry); + kset_put(&ip_die_entry->ip_kset); return res; } @@ -989,6 +989,7 @@ static int amdgpu_discovery_sysfs_init(struct amdgpu_device *adev) res = kset_register(&adev->ip_top->die_kset); if (res) { DRM_ERROR("Couldn't register die_kset"); + kset_put(&adev->ip_top->die_kset); goto Err; } -- 2.25.1 ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 04/11] kobject: fix possible memory leak in kset_create_and_add()
Inject fault while loading module (e.g. qemu_fw_cfg.ko), kset_register() may fail in kset_create_and_add(), if it fails, but the refcount of kobject is not decreased to 0, the name allocated in kset_create() is leaked. To fix this by calling kset_put(), so that name can be freed in callback function kobject_cleanup() and kset can be freed in kset_release(). unreferenced object 0x888103cc8c08 (size 8): comm "modprobe", pid 508, jiffies 4294915182 (age 120.020s) hex dump (first 8 bytes): 62 79 5f 6e 61 6d 65 00 by_name. backtrace: [<572f97f9>] __kmalloc_track_caller+0x1ae/0x320 [] kstrdup+0x3a/0x70 [<1cd0d05e>] kstrdup_const+0x68/0x80 [ ] kvasprintf_const+0x10b/0x190 [<88f2b8df>] kobject_set_name_vargs+0x56/0x150 [<3f8aca68>] kobject_set_name+0xab/0xe0 [<249f7816>] kset_create_and_add+0x72/0x200 Fixes: b727c702896f ("kset: add kset_create_and_add function") Signed-off-by: Yang Yingliang --- lib/kobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/kobject.c b/lib/kobject.c index 6da04353d974..e77f37200876 100644 --- a/lib/kobject.c +++ b/lib/kobject.c @@ -985,7 +985,7 @@ struct kset *kset_create_and_add(const char *name, return NULL; error = kset_register(kset); if (error) { - kfree(kset); + kset_put(kset); return NULL; } return kset; -- 2.25.1 ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 06/11] firmware: qemu_fw_cfg: fix possible memory leak in fw_cfg_build_symlink()
Inject fault while loading module, kset_register() may fail, if it fails, but the refcount of kobject is not decreased to 0, the name allocated in kobject_set_name() is leaked. To fix this by calling kset_put(), so that name can be freed in callback function kobject_cleanup() and 'subdir' is freed in kset_release(). unreferenced object 0x88810ad69050 (size 8): comm "swapper/0", pid 1, jiffies 4294677178 (age 38.812s) hex dump (first 8 bytes): 65 74 63 00 81 88 ff ff etc. backtrace: [] __kmalloc_node_track_caller+0x44/0x1b0 [<3f0167c7>] kstrdup+0x3a/0x70 [<49336709>] kstrdup_const+0x41/0x60 [<175616e4>] kvasprintf_const+0xf5/0x180 [<4bcc30f7>] kobject_set_name_vargs+0x56/0x150 [<4b0251bd>] kobject_set_name+0xab/0xe0 [<700151fb>] fw_cfg_sysfs_probe+0xa5b/0x1320 Fixes: 246c46ebaeae ("firmware: create directory hierarchy for sysfs fw_cfg entries") Signed-off-by: Yang Yingliang --- drivers/firmware/qemu_fw_cfg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c index a69399a6b7c0..d036e69cabbb 100644 --- a/drivers/firmware/qemu_fw_cfg.c +++ b/drivers/firmware/qemu_fw_cfg.c @@ -544,7 +544,7 @@ static int fw_cfg_build_symlink(struct kset *dir, } ret = kset_register(subdir); if (ret) { - kfree(subdir); + kset_put(subdir); break; } -- 2.25.1 ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 11/11] ubifs: Fix memory leak in ubifs_sysfs_init()
From: Liu Shixin
When insmod ubifs.ko, a kmemleak reported as below:
unreferenced object 0x88817fb1a780 (size 8):
comm "insmod", pid 25265, jiffies 4295239702 (age 100.130s)
hex dump (first 8 bytes):
75 62 69 66 73 00 ff ff ubifs...
backtrace:
[] slab_post_alloc_hook+0x9c/0x3c0
[] __kmalloc_track_caller+0x183/0x410
[] kstrdup+0x3a/0x80
[] kstrdup_const+0x66/0x80
[] kvasprintf_const+0x155/0x190
[] kobject_set_name_vargs+0x5b/0x150
[] kobject_set_name+0xbb/0xf0
[] do_one_initcall+0x14c/0x5a0
[] do_init_module+0x1f0/0x660
[] load_module+0x6d7e/0x7590
[] __do_sys_finit_module+0x19f/0x230
[] __x64_sys_finit_module+0x73/0xb0
[] do_syscall_64+0x35/0x80
[] entry_SYSCALL_64_after_hwframe+0x63/0xcd
When kset_register() failed, we should call kset_put to cleanup it.
Fixes: 2e3cbf425804 ("ubifs: Export filesystem error counters")
Signed-off-by: Liu Shixin
Signed-off-by: Yang Yingliang
---
fs/ubifs/sysfs.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/ubifs/sysfs.c b/fs/ubifs/sysfs.c
index 06ad8fa1fcfb..54270ad36321 100644
--- a/fs/ubifs/sysfs.c
+++ b/fs/ubifs/sysfs.c
@@ -144,6 +144,8 @@ int __init ubifs_sysfs_init(void)
kobject_set_name(&ubifs_kset.kobj, "ubifs");
ubifs_kset.kobj.parent = fs_kobj;
ret = kset_register(&ubifs_kset);
+ if (ret)
+ kset_put(&ubifs_kset);
return ret;
}
--
2.25.1
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 07/11] f2fs: fix possible memory leak in f2fs_init_sysfs()
Inject fault while loading module, kset_register() may fail, if it fails, but the refcount of kobject is not decreased to 0, the name allocated in kobject_set_name() is leaked. Fix this by calling kset_put(), so that name can be freed in callback function kobject_cleanup(). unreferenced object 0x888101b7cc80 (size 8): comm "modprobe", pid 252, jiffies 4294691378 (age 31.760s) hex dump (first 8 bytes): 66 32 66 73 00 88 ff ff f2fs backtrace: [<1db5b408>] __kmalloc_node_track_caller+0x44/0x1b0 [<2783a073>] kstrdup+0x3a/0x70 [] kstrdup_const+0x63/0x80 [<3e5cf8f7>] kvasprintf_const+0x149/0x180 [ ] kobject_set_name_vargs+0x56/0x150 [<44611660>] kobject_set_name+0xab/0xe0 Fixes: bf9e697ecd42 ("f2fs: expose features to sysfs entry") Signed-off-by: Yang Yingliang Reviewed-by: Chao Yu --- fs/f2fs/sysfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c index df27afd71ef4..2ef7a48967be 100644 --- a/fs/f2fs/sysfs.c +++ b/fs/f2fs/sysfs.c @@ -1250,8 +1250,10 @@ int __init f2fs_init_sysfs(void) kobject_set_name(&f2fs_kset.kobj, "f2fs"); f2fs_kset.kobj.parent = fs_kobj; ret = kset_register(&f2fs_kset); - if (ret) + if (ret) { + kset_put(&f2fs_kset); return ret; + } ret = kobject_init_and_add(&f2fs_feat, &f2fs_feat_ktype, NULL, "features"); -- 2.25.1 ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 00/11] fix memory leak while kset_register() fails
The previous discussion link: https://lore.kernel.org/lkml/[email protected]/T/ kset_register() is currently used in some places without calling kset_put() in error path, because the callers think it should be kset internal thing to do, but the driver core can not know what caller doing with that memory at times. The memory could be freed both in kset_put() and error path of caller, if it is called in kset_register(). So make the function documentation more explicit about calling kset_put() in the error path of caller first, so that people have a chance to know what to do here, then fixes this leaks by calling kset_put() from callers. Liu Shixin (1): ubifs: Fix memory leak in ubifs_sysfs_init() Yang Yingliang (10): kset: fix documentation for kset_register() kset: add null pointer check in kset_put() bus: fix possible memory leak in bus_register() kobject: fix possible memory leak in kset_create_and_add() class: fix possible memory leak in __class_register() firmware: qemu_fw_cfg: fix possible memory leak in fw_cfg_build_symlink() f2fs: fix possible memory leak in f2fs_init_sysfs() erofs: fix possible memory leak in erofs_init_sysfs() ocfs2: possible memory leak in mlog_sys_init() drm/amdgpu/discovery: fix possible memory leak drivers/base/bus.c| 4 +++- drivers/base/class.c | 6 ++ drivers/firmware/qemu_fw_cfg.c| 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 5 +++-- fs/erofs/sysfs.c | 4 +++- fs/f2fs/sysfs.c | 4 +++- fs/ocfs2/cluster/masklog.c| 7 ++- fs/ubifs/sysfs.c | 2 ++ include/linux/kobject.h | 3 ++- lib/kobject.c | 5 - 10 files changed, 33 insertions(+), 9 deletions(-) -- 2.25.1 ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 03/11] bus: fix possible memory leak in bus_register()
Inject fault while loading module (e.g. edac_core.ko), kset_register() may fail in bus_register(), if it fails, but the refcount of kobject is not decreased to 0, the name allocated in kobject_set_name() is leaked. To fix this by calling kset_put(), so that name can be freed in callback function kobject_cleanup(). unreferenced object 0x888103bddb68 (size 8): comm "systemd-udevd", pid 341, jiffies 4294903262 (age 42.212s) hex dump (first 8 bytes): 65 64 61 63 00 00 00 00 edac backtrace: [<9e31d566>] __kmalloc_track_caller+0x1ae/0x320 [] kstrdup+0x3a/0x70 [<3d0ec369>] kstrdup_const+0x68/0x80 [<8e5c3b20>] kvasprintf_const+0x10b/0x190 [ ] kobject_set_name_vargs+0x56/0x150 [<0df9278c>] kobject_set_name+0xab/0xe0 [ ] bus_register+0x132/0x350 [<7d91c2e5>] subsys_register+0x23/0x220 Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Yang Yingliang --- drivers/base/bus.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/base/bus.c b/drivers/base/bus.c index 7ca47e5b3c1f..301b5330f9d8 100644 --- a/drivers/base/bus.c +++ b/drivers/base/bus.c @@ -804,8 +804,10 @@ int bus_register(struct bus_type *bus) priv->drivers_autoprobe = 1; retval = kset_register(&priv->subsys); - if (retval) + if (retval) { + kset_put(&priv->subsys); goto out; + } retval = bus_create_file(bus, &bus_attr_uevent); if (retval) -- 2.25.1 ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 08/11] erofs: fix possible memory leak in erofs_init_sysfs()
Inject fault while loading module, kset_register() may fail, if it fails, but the refcount of kobject is not decreased to 0, the name allocated in kobject_set_name() is leaked. Fix this by calling kset_put(), so that name can be freed in callback function kobject_cleanup(). unreferenced object 0x888101d228c0 (size 8): comm "modprobe", pid 276, jiffies 4294722700 (age 13.151s) hex dump (first 8 bytes): 65 72 6f 66 73 00 ff ff erofs... backtrace: [] __kmalloc_node_track_caller+0x44/0x1b0 [ ] kstrdup+0x3a/0x70 [<4a0e01d2>] kstrdup_const+0x63/0x80 [<051b6cda>] kvasprintf_const+0x149/0x180 [<4dc51dad>] kobject_set_name_vargs+0x56/0x150 [ ] kobject_set_name+0xab/0xe0 Fixes: 168e9a76200c ("erofs: add sysfs interface") Signed-off-by: Yang Yingliang --- fs/erofs/sysfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c index 783bb7b21b51..653b35001bc5 100644 --- a/fs/erofs/sysfs.c +++ b/fs/erofs/sysfs.c @@ -254,8 +254,10 @@ int __init erofs_init_sysfs(void) kobject_set_name(&erofs_root.kobj, "erofs"); erofs_root.kobj.parent = fs_kobj; ret = kset_register(&erofs_root); - if (ret) + if (ret) { + kset_put(&erofs_root); goto root_err; + } ret = kobject_init_and_add(&erofs_feat, &erofs_feat_ktype, NULL, "features"); -- 2.25.1 ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 01/11] kset: fix documentation for kset_register()
kset_register() is currently used in some places without calling
kset_put() in error path, because the callers think it should be
kset internal thing to do, but the driver core can not know what
caller doing with that memory at times. The memory could be freed
both in kset_put() and error path of caller, if it is called in
kset_register().
So make the function documentation more explicit about calling
kset_put() in the error path of caller.
Signed-off-by: Yang Yingliang
---
lib/kobject.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/lib/kobject.c b/lib/kobject.c
index a0b2dbfcfa23..6da04353d974 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops);
/**
* kset_register() - Initialize and add a kset.
* @k: kset.
+ *
+ * If this function returns an error, kset_put() must be called to
+ * properly clean up the memory associated with the object.
*/
int kset_register(struct kset *k)
{
--
2.25.1
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 02/11] kset: add null pointer check in kset_put()
kset_put() can be called from drivers, add null pointer
check to make it more robust.
Signed-off-by: Yang Yingliang
---
include/linux/kobject.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index 57fb972fea05..e81de8ba41a2 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -195,7 +195,8 @@ static inline struct kset *kset_get(struct kset *k)
static inline void kset_put(struct kset *k)
{
- kobject_put(&k->kobj);
+ if (k)
+ kobject_put(&k->kobj);
}
static inline const struct kobj_type *get_ktype(struct kobject *kobj)
--
2.25.1
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 09/11] ocfs2: possible memory leak in mlog_sys_init()
Inject fault while loading module, kset_register() may fail,
if it fails, but the refcount of kobject is not decreased to
0, the name allocated in kobject_set_name() is leaked. Fix
this by calling kset_put(), so that name can be freed in
callback function kobject_cleanup().
unreferenced object 0x888100da9348 (size 8):
comm "modprobe", pid 257, jiffies 4294701096 (age 33.334s)
hex dump (first 8 bytes):
6c 6f 67 6d 61 73 6b 00 logmask.
backtrace:
[<306e441c>] __kmalloc_node_track_caller+0x44/0x1b0
[<7c491a9e>] kstrdup+0x3a/0x70
[<15719a3b>] kstrdup_const+0x63/0x80
[<84e458ea>] kvasprintf_const+0x149/0x180
[<91302b42>] kobject_set_name_vargs+0x56/0x150
[<5f48eeac>] kobject_set_name+0xab/0xe0
Fixes: 34980ca8faeb ("Drivers: clean up direct setting of the name of a kset")
Signed-off-by: Yang Yingliang
---
fs/ocfs2/cluster/masklog.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c
index 563881ddbf00..7f9ba816d955 100644
--- a/fs/ocfs2/cluster/masklog.c
+++ b/fs/ocfs2/cluster/masklog.c
@@ -156,6 +156,7 @@ static struct kset mlog_kset = {
int mlog_sys_init(struct kset *o2cb_kset)
{
int i = 0;
+ int ret;
while (mlog_attrs[i].attr.mode) {
mlog_default_attrs[i] = &mlog_attrs[i].attr;
@@ -165,7 +166,11 @@ int mlog_sys_init(struct kset *o2cb_kset)
kobject_set_name(&mlog_kset.kobj, "logmask");
mlog_kset.kobj.kset = o2cb_kset;
- return kset_register(&mlog_kset);
+ ret = kset_register(&mlog_kset);
+ if (ret)
+ kset_put(&mlog_kset);
+
+ return ret;
}
void mlog_sys_shutdown(void)
--
2.25.1
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH 05/11] class: fix possible memory leak in __class_register()
Inject fault while loading module (e.g. pktcdvd.ko), kset_register() may fail in __class_register(), if it fails, but the refcount of kobject is not decreased to 0, the name allocated in kobject_set_name() is leaked. To fix this by calling kfree_const(). unreferenced object 0x888102fa8190 (size 8): comm "modprobe", pid 502, jiffies 4294906074 (age 49.296s) hex dump (first 8 bytes): 70 6b 74 63 64 76 64 00 pktcdvd. backtrace: [] __kmalloc_track_caller+0x1ae/0x320 [<5e4d70bc>] kstrdup+0x3a/0x70 [ ] kstrdup_const+0x68/0x80 [<0049a8c7>] kvasprintf_const+0x10b/0x190 [<29123163>] kobject_set_name_vargs+0x56/0x150 [<747219c9>] kobject_set_name+0xab/0xe0 [<05f1ea4e>] __class_register+0x15c/0x49a If class_add_groups() fails, it need delete kobject and free its name, besides, subsys_private also need be freed. unreferenced object 0x888037274000 (size 1024): comm "modprobe", pid 502, jiffies 4294906074 (age 49.296s) hex dump (first 32 bytes): 00 40 27 37 80 88 ff ff 00 40 27 37 80 88 ff ff .@'7.@'7 00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00 .N.. backtrace: [<151f9600>] kmem_cache_alloc_trace+0x17c/0x2f0 [ ] __class_register+0x86/0x49a It can not call kset_put() or kset_unregister() in error path, because the 'cls' will be freed in callback function class_release() and it also freed in error path, it will cause double free. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Yang Yingliang --- drivers/base/class.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/base/class.c b/drivers/base/class.c index 64f7b9a0970f..87de0a04ee9b 100644 --- a/drivers/base/class.c +++ b/drivers/base/class.c @@ -187,11 +187,17 @@ int __class_register(struct class *cls, struct lock_class_key *key) error = kset_register(&cp->subsys); if (error) { + kfree_const(cp->subsys.kobj.name); kfree(cp); return error; } error = class_add_groups(class_get(cls), cls->class_groups); class_put(cls); + if (error) { + kobject_del(&cp->subsys.kobj); + kfree_const(cp->subsys.kobj.name); + kfree(cp); + } return error; } EXPORT_SYMBOL_GPL(__class_register); -- 2.25.1 ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 00/11] fix memory leak while kset_register() fails
Hi, On 2022/10/21 13:29, Luben Tuikov wrote: On 2022-10-20 22:20, Yang Yingliang wrote: The previous discussion link: https://lore.kernel.org/lkml/[email protected]/T/ The very first discussion on this was here: https://www.spinics.net/lists/dri-devel/msg368077.html Please use this link, and not the that one up there you which quoted above, and whose commit description is taken verbatim from the this link. I found this leaks in bus_register()/class_register()/kset_create_and_add() at first, and describe the reason in these patches which is using kobject_set_name() description, here is the patches: https://lore.kernel.org/lkml/[email protected]/T/ https://lore.kernel.org/lkml/[email protected]/ https://lore.kernel.org/lkml/[email protected]/T/ And then I found other subsystem also have this problem, so posted the fix patches for them (including qemu_fw_cfg/f2fs/erofs/ocfs2/amdgpu_discovery): https://www.mail-archive.com/[email protected]/msg915553.html https://lore.kernel.org/linux-f2fs-devel/[email protected]/T/#t https://lore.kernel.org/linux-erofs/[email protected]/ https://lore.kernel.org/lkml/[email protected]/T/ https://www.spinics.net/lists/dri-devel/msg368092.html In the amdgpu_discovery patch, I sent a old one which using wrong description and you pointer out, and then I send a v2. And then the maintainer of ocfs2 has different thought about this, so we had a discussion in the link that I gave out, and Greg suggested me to update kset_register() documentation and then put the fix patches together in one series, so I sent this patchset and use the link. Thanks, Yang kset_register() is currently used in some places without calling kset_put() in error path, because the callers think it should be kset internal thing to do, but the driver core can not know what caller doing with that memory at times. The memory could be freed both in kset_put() and error path of caller, if it is called in kset_register(). As I explained in the link above, the reason there's a memory leak is that one cannot call kset_register() without the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, in this case, i.e. kset_register() fails with -EINVAL. Thus, the most common usage is something like this: kobj_set_name(&kset->kobj, format, ...); kset->kobj.kset = parent_kset; kset->kobj.ktype = ktype; res = kset_register(kset); So, what is being leaked, is the memory allocated in kobj_set_name(), by the common idiom shown above. This needs to be mentioned in the documentation, at least, in case, in the future this is absolved in kset_register() redesign, etc. Regards, Luben So make the function documentation more explicit about calling kset_put() in the error path of caller first, so that people have a chance to know what to do here, then fixes this leaks by calling kset_put() from callers. Liu Shixin (1): ubifs: Fix memory leak in ubifs_sysfs_init() Yang Yingliang (10): kset: fix documentation for kset_register() kset: add null pointer check in kset_put() bus: fix possible memory leak in bus_register() kobject: fix possible memory leak in kset_create_and_add() class: fix possible memory leak in __class_register() firmware: qemu_fw_cfg: fix possible memory leak in fw_cfg_build_symlink() f2fs: fix possible memory leak in f2fs_init_sysfs() erofs: fix possible memory leak in erofs_init_sysfs() ocfs2: possible memory leak in mlog_sys_init() drm/amdgpu/discovery: fix possible memory leak drivers/base/bus.c| 4 +++- drivers/base/class.c | 6 ++ drivers/firmware/qemu_fw_cfg.c| 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 5 +++-- fs/erofs/sysfs.c | 4 +++- fs/f2fs/sysfs.c | 4 +++- fs/ocfs2/cluster/masklog.c| 7 ++- fs/ubifs/sysfs.c | 2 ++ include/linux/kobject.h | 3 ++- lib/kobject.c | 5 - 10 files changed, 33 insertions(+), 9 deletions(-) . ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 01/11] kset: fix documentation for kset_register()
On 2022/10/21 13:34, Luben Tuikov wrote:
On 2022-10-20 22:20, Yang Yingliang wrote:
kset_register() is currently used in some places without calling
kset_put() in error path, because the callers think it should be
kset internal thing to do, but the driver core can not know what
caller doing with that memory at times. The memory could be freed
both in kset_put() and error path of caller, if it is called in
kset_register().
So make the function documentation more explicit about calling
kset_put() in the error path of caller.
Signed-off-by: Yang Yingliang
---
lib/kobject.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/lib/kobject.c b/lib/kobject.c
index a0b2dbfcfa23..6da04353d974 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops);
/**
* kset_register() - Initialize and add a kset.
* @k: kset.
+ *
+ * If this function returns an error, kset_put() must be called to
+ * properly clean up the memory associated with the object.
*/
And I'd continue the sentence, with " ... with the object,
for instance the memory for the kset.kobj.name when kobj_set_name(&kset.kobj,
format, ...)
was called before calling kset_register()."
kobject_cleanup() not only frees name, but aslo calls ->release() to
free another resources.
This makes it clear what we want to make sure is freed, in case of an early
error
from kset_register().
How about like this:
If this function returns an error, kset_put() must be called to clean up the
name of
kset object and other memory associated with the object.
Regards,
Luben
int kset_register(struct kset *k)
{
.
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 00/11] fix memory leak while kset_register() fails
On 2022/10/21 13:37, Greg KH wrote: On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: On 2022-10-20 22:20, Yang Yingliang wrote: The previous discussion link: https://lore.kernel.org/lkml/[email protected]/T/ The very first discussion on this was here: https://www.spinics.net/lists/dri-devel/msg368077.html Please use this link, and not the that one up there you which quoted above, and whose commit description is taken verbatim from the this link. kset_register() is currently used in some places without calling kset_put() in error path, because the callers think it should be kset internal thing to do, but the driver core can not know what caller doing with that memory at times. The memory could be freed both in kset_put() and error path of caller, if it is called in kset_register(). As I explained in the link above, the reason there's a memory leak is that one cannot call kset_register() without the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, in this case, i.e. kset_register() fails with -EINVAL. Thus, the most common usage is something like this: kobj_set_name(&kset->kobj, format, ...); kset->kobj.kset = parent_kset; kset->kobj.ktype = ktype; res = kset_register(kset); So, what is being leaked, is the memory allocated in kobj_set_name(), by the common idiom shown above. This needs to be mentioned in the documentation, at least, in case, in the future this is absolved in kset_register() redesign, etc. Based on this, can kset_register() just clean up from itself when an error happens? Ideally that would be the case, as the odds of a kset being embedded in a larger structure is probably slim, but we would have to search the tree to make sure. I have search the whole tree, the kset used in bus_register() - patch #3, kset_create_and_add() - patch #4 __class_register() - patch #5, fw_cfg_build_symlink() - patch #6 and amdgpu_discovery.c - patch #10 is embedded in a larger structure. In these cases, we can not call kset_put() in error path in kset_register() itself. Thanks, Yang thanks, greg k-h . ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 00/11] fix memory leak while kset_register() fails
On 2022/10/21 16:36, Greg KH wrote: On Fri, Oct 21, 2022 at 04:24:23PM +0800, Yang Yingliang wrote: On 2022/10/21 13:37, Greg KH wrote: On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: On 2022-10-20 22:20, Yang Yingliang wrote: The previous discussion link: https://lore.kernel.org/lkml/[email protected]/T/ The very first discussion on this was here: https://www.spinics.net/lists/dri-devel/msg368077.html Please use this link, and not the that one up there you which quoted above, and whose commit description is taken verbatim from the this link. kset_register() is currently used in some places without calling kset_put() in error path, because the callers think it should be kset internal thing to do, but the driver core can not know what caller doing with that memory at times. The memory could be freed both in kset_put() and error path of caller, if it is called in kset_register(). As I explained in the link above, the reason there's a memory leak is that one cannot call kset_register() without the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, in this case, i.e. kset_register() fails with -EINVAL. Thus, the most common usage is something like this: kobj_set_name(&kset->kobj, format, ...); kset->kobj.kset = parent_kset; kset->kobj.ktype = ktype; res = kset_register(kset); So, what is being leaked, is the memory allocated in kobj_set_name(), by the common idiom shown above. This needs to be mentioned in the documentation, at least, in case, in the future this is absolved in kset_register() redesign, etc. Based on this, can kset_register() just clean up from itself when an error happens? Ideally that would be the case, as the odds of a kset being embedded in a larger structure is probably slim, but we would have to search the tree to make sure. I have search the whole tree, the kset used in bus_register() - patch #3, kset_create_and_add() - patch #4 __class_register() - patch #5, fw_cfg_build_symlink() - patch #6 and amdgpu_discovery.c - patch #10 is embedded in a larger structure. In these cases, we can not call kset_put() in error path in kset_register() Yes you can as the kobject in the kset should NOT be controling the lifespan of those larger objects. If it is, please point out the call chain here as I don't think that should be possible. Note all of this is a mess because the kobject name stuff was added much later, after the driver model had been created and running for a while. We missed this error path when adding the dynamic kobject name logic, thank for looking into this. If you could test the patch posted with your error injection systems, that could make this all much simpler to solve. The patch posted by Luben will cause double free in some cases. From 71e0a22801c0699f67ea40ed96e0a7d7d9e0f318 Mon Sep 17 00:00:00 2001 From: Luben Tuikov Date: Fri, 21 Oct 2022 03:34:21 -0400 Subject: [PATCH] kobject: Add kset_put() if kset_register() fails X-check-string-leak: v1.0 If kset_register() fails, we call kset_put() before returning the error. This makes sure that we free memory allocated by kobj_set_name() for the kset, since kset_register() cannot be called unless the kset has a name, usually gotten via kobj_set_name(&kset->kobj, format, ...); Cc: Greg Kroah-Hartman Cc: Rafael J. Wysocki Cc: Yang Yingliang Cc: Linux Kernel Mailing List Signed-off-by: Luben Tuikov --- lib/kobject.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/kobject.c b/lib/kobject.c index a0b2dbfcfa2334..c122b979f2b75e 100644 --- a/lib/kobject.c +++ b/lib/kobject.c @@ -844,8 +844,10 @@ int kset_register(struct kset *k) kset_init(k); err = kobject_add_internal(&k->kobj); - if (err) + if (err) { + kset_put(k); return err; + } kobject_uevent(&k->kobj, KOBJ_ADD); return 0; } -- 2.38.0-rc2 thanks, greg k-h . ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 00/11] fix memory leak while kset_register() fails
On 2022/10/21 16:36, Greg KH wrote: On Fri, Oct 21, 2022 at 04:24:23PM +0800, Yang Yingliang wrote: On 2022/10/21 13:37, Greg KH wrote: On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: On 2022-10-20 22:20, Yang Yingliang wrote: The previous discussion link: https://lore.kernel.org/lkml/[email protected]/T/ The very first discussion on this was here: https://www.spinics.net/lists/dri-devel/msg368077.html Please use this link, and not the that one up there you which quoted above, and whose commit description is taken verbatim from the this link. kset_register() is currently used in some places without calling kset_put() in error path, because the callers think it should be kset internal thing to do, but the driver core can not know what caller doing with that memory at times. The memory could be freed both in kset_put() and error path of caller, if it is called in kset_register(). As I explained in the link above, the reason there's a memory leak is that one cannot call kset_register() without the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, in this case, i.e. kset_register() fails with -EINVAL. Thus, the most common usage is something like this: kobj_set_name(&kset->kobj, format, ...); kset->kobj.kset = parent_kset; kset->kobj.ktype = ktype; res = kset_register(kset); So, what is being leaked, is the memory allocated in kobj_set_name(), by the common idiom shown above. This needs to be mentioned in the documentation, at least, in case, in the future this is absolved in kset_register() redesign, etc. Based on this, can kset_register() just clean up from itself when an error happens? Ideally that would be the case, as the odds of a kset being embedded in a larger structure is probably slim, but we would have to search the tree to make sure. I have search the whole tree, the kset used in bus_register() - patch #3, kset_create_and_add() - patch #4 __class_register() - patch #5, fw_cfg_build_symlink() - patch #6 and amdgpu_discovery.c - patch #10 is embedded in a larger structure. In these cases, we can not call kset_put() in error path in kset_register() Yes you can as the kobject in the kset should NOT be controling the lifespan of those larger objects. Read through the code the only leak in this case is the name, so can we free it directly in kset_register(): --- a/lib/kobject.c +++ b/lib/kobject.c @@ -844,8 +844,11 @@ int kset_register(struct kset *k) kset_init(k); err = kobject_add_internal(&k->kobj); - if (err) + if (err) { + kfree_const(k->kobj.name); + k->kobj.name = NULL; return err; + } kobject_uevent(&k->kobj, KOBJ_ADD); return 0; } or unset ktype of kobject, then call kset_put(): --- a/lib/kobject.c +++ b/lib/kobject.c @@ -844,8 +844,11 @@ int kset_register(struct kset *k) kset_init(k); err = kobject_add_internal(&k->kobj); - if (err) + if (err) { + k->kobj.ktype = NULL; + kset_put(k); return err; + } kobject_uevent(&k->kobj, KOBJ_ADD); return 0; } If it is, please point out the call chain here as I don't think that should be possible. Note all of this is a mess because the kobject name stuff was added much later, after the driver model had been created and running for a while. We missed this error path when adding the dynamic kobject name logic, thank for looking into this. If you could test the patch posted with your error injection systems, that could make this all much simpler to solve. thanks, greg k-h . ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 00/11] fix memory leak while kset_register() fails
On 2022/10/21 16:41, Luben Tuikov wrote: On 2022-10-21 04:24, Luben Tuikov wrote: On 2022-10-21 04:18, Greg KH wrote: On Fri, Oct 21, 2022 at 03:55:18AM -0400, Luben Tuikov wrote: On 2022-10-21 01:37, Greg KH wrote: On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: On 2022-10-20 22:20, Yang Yingliang wrote: The previous discussion link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F0db486eb-6927-927e-3629-958f8f211194%40huawei.com%2FT%2F&data=05%7C01%7Cluben.tuikov%40amd.com%7Cd41da3fd6449492d01f808dab33cdb75%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019371236833115%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=C%2Bj1THkHpzVGks5eqB%2Fm%2FPAkMRohR7CYvRnOCqUqdcM%3D&reserved=0 The very first discussion on this was here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdri-devel%2Fmsg368077.html&data=05%7C01%7Cluben.tuikov%40amd.com%7Cd41da3fd6449492d01f808dab33cdb75%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019371236833115%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=pSR10abmK8nAMvKSezqWC0SPUBL4qEwtCCizyIKW7Dc%3D&reserved=0 Please use this link, and not the that one up there you which quoted above, and whose commit description is taken verbatim from the this link. kset_register() is currently used in some places without calling kset_put() in error path, because the callers think it should be kset internal thing to do, but the driver core can not know what caller doing with that memory at times. The memory could be freed both in kset_put() and error path of caller, if it is called in kset_register(). As I explained in the link above, the reason there's a memory leak is that one cannot call kset_register() without the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, in this case, i.e. kset_register() fails with -EINVAL. Thus, the most common usage is something like this: kobj_set_name(&kset->kobj, format, ...); kset->kobj.kset = parent_kset; kset->kobj.ktype = ktype; res = kset_register(kset); So, what is being leaked, is the memory allocated in kobj_set_name(), by the common idiom shown above. This needs to be mentioned in the documentation, at least, in case, in the future this is absolved in kset_register() redesign, etc. Based on this, can kset_register() just clean up from itself when an error happens? Ideally that would be the case, as the odds of a kset being embedded in a larger structure is probably slim, but we would have to search the tree to make sure. Looking at kset_register(), we can add kset_put() in the error path, when kobject_add_internal(&kset->kobj) fails. See the attached patch. It needs to be tested with the same error injection as Yang has been doing. Now, struct kset is being embedded in larger structs--see amdgpu_discovery.c starting at line 575. If you're on an AMD system, it gets you the tree structure you'll see when you run "tree /sys/class/drm/card0/device/ip_discovery/". That shouldn't be a problem though. Yes, that shouldn't be an issue as the kobject embedded in a kset is ONLY for that kset itself, the kset structure should not be controling the lifespan of the object it is embedded in, right? Yes, and it doesn't. It only does a kobject_get(parent) and kobject_put(parent). So that's fine and natural. Yang, do you want to try the patch in my previous email in this thread, since you've got the error injection set up already? I spoke too soon. I believe you're onto something, because looking at the idiom: kobj_set_name(&kset->kobj, format, ...); kset->kobj.kset = parent_kset; kset->kobj.ktype = ktype; res = kset_register(kset); The ktype defines a release method, which frees the larger struct the kset is embedded in. And this would result in double free, for instance in the amdgpu_discovery.c code, if kset_put() is called after kset_register() fails, since we kzalloc the larger object just before and kfree it on error just after. Ideally, we'd only "revert" the actions done by kobj_set_name(), as there's some error recovery on create_dir() in kobject_add_internal(). So, we cannot do this business with the kset_put() on error from kset_register(), after all. Not sure how this wasn't caught in Yang's testing--the kernel should've complained. I have already tried the change that in your posted patch when I was debugging this issue before sent these fixes, because it may lead double free in some cases, and I had mentioned it in this mail: https://lore.kernel.org/lkml/[email protected]/T/#m68eade1993859dfc6c3d14d35f988d35a32ef837 Thanks, Yang Regards, Luben . ___ Linux-f2fs-devel mailing list [email protected] https://list
Re: [f2fs-dev] [PATCH 00/11] fix memory leak while kset_register() fails
On 2022/10/21 17:08, Luben Tuikov wrote: On 2022-10-21 04:59, Yang Yingliang wrote: On 2022/10/21 16:36, Greg KH wrote: On Fri, Oct 21, 2022 at 04:24:23PM +0800, Yang Yingliang wrote: On 2022/10/21 13:37, Greg KH wrote: On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: On 2022-10-20 22:20, Yang Yingliang wrote: The previous discussion link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F0db486eb-6927-927e-3629-958f8f211194%40huawei.com%2FT%2F&data=05%7C01%7Cluben.tuikov%40amd.com%7C74aa9b57192b406ef27408dab3429db4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019395979868103%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=RcK05cXm1J5%2BtYcLO2SMG7k6sjeymQzdBzMCDJSzfdE%3D&reserved=0 The very first discussion on this was here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdri-devel%2Fmsg368077.html&data=05%7C01%7Cluben.tuikov%40amd.com%7C74aa9b57192b406ef27408dab3429db4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019395979868103%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=sHZ6kfLF8HxrNXV6%2FVjgdH%2BmQM4T3Zv0U%2FAwddT97cE%3D&reserved=0 Please use this link, and not the that one up there you which quoted above, and whose commit description is taken verbatim from the this link. kset_register() is currently used in some places without calling kset_put() in error path, because the callers think it should be kset internal thing to do, but the driver core can not know what caller doing with that memory at times. The memory could be freed both in kset_put() and error path of caller, if it is called in kset_register(). As I explained in the link above, the reason there's a memory leak is that one cannot call kset_register() without the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, in this case, i.e. kset_register() fails with -EINVAL. Thus, the most common usage is something like this: kobj_set_name(&kset->kobj, format, ...); kset->kobj.kset = parent_kset; kset->kobj.ktype = ktype; res = kset_register(kset); So, what is being leaked, is the memory allocated in kobj_set_name(), by the common idiom shown above. This needs to be mentioned in the documentation, at least, in case, in the future this is absolved in kset_register() redesign, etc. Based on this, can kset_register() just clean up from itself when an error happens? Ideally that would be the case, as the odds of a kset being embedded in a larger structure is probably slim, but we would have to search the tree to make sure. I have search the whole tree, the kset used in bus_register() - patch #3, kset_create_and_add() - patch #4 __class_register() - patch #5, fw_cfg_build_symlink() - patch #6 and amdgpu_discovery.c - patch #10 is embedded in a larger structure. In these cases, we can not call kset_put() in error path in kset_register() Yes you can as the kobject in the kset should NOT be controling the lifespan of those larger objects. If it is, please point out the call chain here as I don't think that should be possible. Note all of this is a mess because the kobject name stuff was added much later, after the driver model had been created and running for a while. We missed this error path when adding the dynamic kobject name logic, thank for looking into this. If you could test the patch posted with your error injection systems, that could make this all much simpler to solve. The patch posted by Luben will cause double free in some cases. Yes, I figured this out in the other email and posted the scenario Greg was asking about. But I believe the question still stands if we can do kset_put() after a *failed* kset_register(), namely if more is being done than necessary, which is just to free the memory allocated by kobject_set_name(). The name memory is allocated in kobject_set_name() in caller, and I think caller free the memory that it allocated is reasonable, it's weird that some callers allocate some memory and use function (kset_register) failed, then it free the memory allocated in callers, I think use kset_put()/kfree_const(name) in caller seems more reasonable. Thanks, Yang Regards, Luben . ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH v2] kset: fix memory leak when kset_register() returns error
Inject fault while loading module, kset_register() may fail.
If it fails, the name allocated by kobject_set_name() which
is called before kset_register() is leaked, because refcount
of kobject is hold in kset_init().
As a kset may be embedded in a larger structure which needs
be freed in release() function or error path in callers, we
can not call kset_put() in kset_register(), or it will cause
double free, so just call kfree_const() to free the name and
set it to NULL.
With this fix, the callers don't need to care about the name
freeing and call an extra kset_put() if kset_register() fails.
Suggested-by: Luben Tuikov
Signed-off-by: Yang Yingliang
---
v1 -> v2:
Free name inside of kset_register() instead of calling kset_put()
in drivers.
---
lib/kobject.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/lib/kobject.c b/lib/kobject.c
index a0b2dbfcfa23..3409a89c81e5 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops);
/**
* kset_register() - Initialize and add a kset.
* @k: kset.
+ *
+ * NOTE: On error, the kset.kobj.name allocated by() kobj_set_name()
+ * which is called before kset_register() in caller need be freed.
*/
int kset_register(struct kset *k)
{
@@ -844,8 +847,11 @@ int kset_register(struct kset *k)
kset_init(k);
err = kobject_add_internal(&k->kobj);
- if (err)
+ if (err) {
+ kfree_const(k->kobj.name);
+ k->kobj.name = NULL;
return err;
+ }
kobject_uevent(&k->kobj, KOBJ_ADD);
return 0;
}
--
2.25.1
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2] kset: fix memory leak when kset_register() returns error
On 2022/10/24 21:52, Greg KH wrote:
On Mon, Oct 24, 2022 at 08:19:10PM +0800, Yang Yingliang wrote:
Inject fault while loading module, kset_register() may fail.
If it fails, the name allocated by kobject_set_name() which
is called before kset_register() is leaked, because refcount
of kobject is hold in kset_init().
As a kset may be embedded in a larger structure which needs
be freed in release() function or error path in callers, we
can not call kset_put() in kset_register(), or it will cause
double free, so just call kfree_const() to free the name and
set it to NULL.
With this fix, the callers don't need to care about the name
freeing and call an extra kset_put() if kset_register() fails.
Suggested-by: Luben Tuikov
Signed-off-by: Yang Yingliang
---
v1 -> v2:
Free name inside of kset_register() instead of calling kset_put()
in drivers.
---
lib/kobject.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/lib/kobject.c b/lib/kobject.c
index a0b2dbfcfa23..3409a89c81e5 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops);
/**
* kset_register() - Initialize and add a kset.
* @k: kset.
+ *
+ * NOTE: On error, the kset.kobj.name allocated by() kobj_set_name()
+ * which is called before kset_register() in caller need be freed.
This comment doesn't make any sense anymore. No caller needs to worry
about this, right?
With this fix, the name is freed inside of kset_register(), it can not
be accessed,
if it allocated dynamically, but callers don't know this if no comment here,
they may use it in error path (something like to print error message
with it),
so how about comment like this to tell callers not to use the name:
NOTE: On error, the kset.kobj.name allocated by() kobj_set_name()
is freed, it can not be used any more.
*/
int kset_register(struct kset *k)
{
@@ -844,8 +847,11 @@ int kset_register(struct kset *k)
kset_init(k);
err = kobject_add_internal(&k->kobj);
- if (err)
+ if (err) {
+ kfree_const(k->kobj.name);
+ k->kobj.name = NULL;
Why are you setting the name here to NULL?
I set it to NULL to avoid accessing bad pointer in callers,
if callers use it in error path, current callers won't use this
name pointer in error path, so we can remove this assignment?
Thanks,
Yang
thanks,
greg k-h
.
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2] kset: fix memory leak when kset_register() returns error
On 2022/10/24 22:53, Greg KH wrote:
On Mon, Oct 24, 2022 at 10:39:44PM +0800, Yang Yingliang wrote:
On 2022/10/24 21:52, Greg KH wrote:
On Mon, Oct 24, 2022 at 08:19:10PM +0800, Yang Yingliang wrote:
Inject fault while loading module, kset_register() may fail.
If it fails, the name allocated by kobject_set_name() which
is called before kset_register() is leaked, because refcount
of kobject is hold in kset_init().
As a kset may be embedded in a larger structure which needs
be freed in release() function or error path in callers, we
can not call kset_put() in kset_register(), or it will cause
double free, so just call kfree_const() to free the name and
set it to NULL.
With this fix, the callers don't need to care about the name
freeing and call an extra kset_put() if kset_register() fails.
Suggested-by: Luben Tuikov
Signed-off-by: Yang Yingliang
---
v1 -> v2:
Free name inside of kset_register() instead of calling kset_put()
in drivers.
---
lib/kobject.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/lib/kobject.c b/lib/kobject.c
index a0b2dbfcfa23..3409a89c81e5 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops);
/**
* kset_register() - Initialize and add a kset.
* @k: kset.
+ *
+ * NOTE: On error, the kset.kobj.name allocated by() kobj_set_name()
+ * which is called before kset_register() in caller need be freed.
This comment doesn't make any sense anymore. No caller needs to worry
about this, right?
With this fix, the name is freed inside of kset_register(), it can not be
accessed,
Agreed.
if it allocated dynamically, but callers don't know this if no comment here,
they may use it in error path (something like to print error message with
it),
so how about comment like this to tell callers not to use the name:
NOTE: On error, the kset.kobj.name allocated by() kobj_set_name()
is freed, it can not be used any more.
Sure, that's a better way to word it.
*/
int kset_register(struct kset *k)
{
@@ -844,8 +847,11 @@ int kset_register(struct kset *k)
kset_init(k);
err = kobject_add_internal(&k->kobj);
- if (err)
+ if (err) {
+ kfree_const(k->kobj.name);
+ k->kobj.name = NULL;
Why are you setting the name here to NULL?
I set it to NULL to avoid accessing bad pointer in callers,
if callers use it in error path, current callers won't use this
name pointer in error path, so we can remove this assignment?
Ah, I didn't think about using it on error paths. Ideally that would
never happen, but that's good to set just to make it obvious. How about
adding a small comment here saying why you are setting it so we all
remember it in 5 years when we look at the code again.
OK, I can add it in v3.
Thanks,
Yang
thanks,
greg k-h
.
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2] kset: fix memory leak when kset_register() returns error
Hi, On 2022/10/25 5:25, Luben Tuikov wrote: On 2022-10-24 17:06, Luben Tuikov wrote: On 2022-10-24 08:19, Yang Yingliang wrote: Inject fault while loading module, kset_register() may fail. If it fails, the name allocated by kobject_set_name() which is called before kset_register() is leaked, because refcount of kobject is hold in kset_init(). "is hold" --> "was set". Also, I'd say "which must be called" instead of "is", since we cannot register kobj/kset without a name--the kobj code crashes, and we want to make this clear. IOW, a novice user may wonder where "is" it called, as opposed to learning that they "must" call it to allocate/set a name, before calling kset_register(). So, I'd say this: "If it fails, the name allocated by kobject_set_name() which must be called before a call to kset_regsiter() is leaked, since refcount of kobj was set in kset_init()." Actually, to be a bit more clear: "If kset_register() fails, the name allocated by kobject_set_name(), namely kset.kobj.name, which must be called before a call to kset_register(), may be leaked, if the caller doesn't explicitly free it, say by calling kset_put(). To mitigate this, we free the name in kset_register() when an error is encountered, i.e. when kset_register() returns an error." Thanks for you suggestion. As a kset may be embedded in a larger structure which needs be freed in release() function or error path in callers, we Drop "As", start with "A kset". "which needs _to_ be". Also please specify that the release is part of the ktype, like this: "A kset may be embedded in a larger structure which needs to be freed in ktype.release() or error path in callers, we ..." can not call kset_put() in kset_register(), or it will cause double free, so just call kfree_const() to free the name and set it to NULL. With this fix, the callers don't need to care about the name freeing and call an extra kset_put() if kset_register() fails. This is unclear because you're *missing* a verb: "and call an extra kset_put()". Please add the proper verb _between_ "and call", something like, "With this fix, the callers don't need to care about freeing the name of the kset, and _can_ call kset_put() if kset_register() fails." I was mean the callers don't need to care about freeing the name of the kset and the callers don't need to care about calling kset_put() Thanks, Yang Choose a proper verb here: can, should, cannot, should not, etc. We can do this because you set "kset.kobj.name to NULL, and this is checked for in kobject_cleanup(). We just need to stipulate whether they should/shouldn't have to call kset_put(), or can free the kset and/or the embedding object themselves. This really depends on how we want kset_register() to behave in the future, and on user's own ktype.release implementation... Forgot "may", "may not". So, do we want to say "may call kset_put()", like: "With this fix, the callers need not care about freeing the name of the kset, and _may_ call kset_put() if kset_register() fails." Or do we want to say "should" or even "must"--it really depends on what else is (would be) going on in kobj registration. Although, the user may have additional work to be done in the ktype.release() callback for the embedding object. It would be good to give them the freedom, i.e. "may", to call kset_put(). If that's not the case, this must be explicitly stipulated with the proper verb. Regards, Luben . ___ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH v3] kset: fix memory leak when kset_register() returns error
Inject fault while loading module, kset_register() may fail.
If it fails, the kset.kobj.name allocated by kobject_set_name()
which must be called before a call to kset_register() may be
leaked, since refcount of kobj was set in kset_init().
To mitigate this, we free the name in kset_register() when an
error is encountered, i.e. when kset_register() returns an error.
A kset may be embedded in a larger structure which may be dynamically
allocated in callers, it needs to be freed in ktype.release() or error
path in callers, in this case, we can not call kset_put() in kset_register(),
or it will cause double free, so just call kfree_const() to free the
name and set it to NULL to avoid accessing bad pointer in callers.
With this fix, the callers don't need care about freeing the name
and may call kset_put() if kset_register() fails.
Suggested-by: Luben Tuikov
Signed-off-by: Yang Yingliang
---
v2 -> v3:
Update commit message and comment of kset_register().
v1 -> v2:
Free name inside of kset_register() instead of calling kset_put()
in drivers.
---
lib/kobject.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/lib/kobject.c b/lib/kobject.c
index a0b2dbfcfa23..3cd19b9ca5ab 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops);
/**
* kset_register() - Initialize and add a kset.
* @k: kset.
+ *
+ * NOTE: On error, the kset.kobj.name allocated by() kobj_set_name()
+ * is freed, it can not be used any more.
*/
int kset_register(struct kset *k)
{
@@ -844,8 +847,12 @@ int kset_register(struct kset *k)
kset_init(k);
err = kobject_add_internal(&k->kobj);
- if (err)
+ if (err) {
+ kfree_const(k->kobj.name);
+ /* Set it to NULL to avoid accessing bad pointer in callers. */
+ k->kobj.name = NULL;
return err;
+ }
kobject_uevent(&k->kobj, KOBJ_ADD);
return 0;
}
--
2.25.1
___
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
