| Message ID | 20250401090901.1050-2-rakie.kim@sk.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enhance sysfs handling for memory hotplug in weighted interleave | expand |
Rakie Kim wrote: > Memory leaks occurred when removing sysfs attributes for weighted > interleave. Improper kobject deallocation led to unreleased memory > when initialization failed or when nodes were removed. > > This patch resolves the issue by replacing unnecessary `kfree()` > calls with `kobject_put()`, ensuring proper cleanup and preventing > memory leaks. > > By correctly using `kobject_put()`, the release function now > properly deallocates memory without causing resource leaks, > thereby improving system stability. > > Fixes: dce41f5ae253 ("mm/mempolicy: implement the sysfs-based weighted_interleave interface") > Signed-off-by: Rakie Kim <rakie.kim@sk.com> > Signed-off-by: Honggyu Kim <honggyu.kim@sk.com> > Signed-off-by: Yunjeong Mun <yunjeong.mun@sk.com> > Reviewed-by: Gregory Price <gourry@gourry.net> > --- > mm/mempolicy.c | 61 +++++++++++++++++++++++++------------------------- > 1 file changed, 31 insertions(+), 30 deletions(-) > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index bbaadbeeb291..5950d5d5b85e 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -3448,7 +3448,9 @@ static void sysfs_wi_release(struct kobject *wi_kobj) > > for (i = 0; i < nr_node_ids; i++) > sysfs_wi_node_release(node_attrs[i], wi_kobj); > - kobject_put(wi_kobj); > + > + kfree(node_attrs); > + kfree(wi_kobj); > } > > static const struct kobj_type wi_ktype = { > @@ -3494,15 +3496,22 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) > struct kobject *wi_kobj; > int nid, err; > > - wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL); > - if (!wi_kobj) > + node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), > + GFP_KERNEL); > + if (!node_attrs) > return -ENOMEM; > > + wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL); > + if (!wi_kobj) { > + err = -ENOMEM; > + goto node_out; > + } > + > err = kobject_init_and_add(wi_kobj, &wi_ktype, root_kobj, > "weighted_interleave"); It would be nice if this could take advantage of scope-based cleanup to avoid the new gotos. It would need a new: DEFINE_FREE(kobject_put, struct kobject *, if (!IS_ERR_OR_NULL(_T)) kobject_put(_T)) ...and a wrapper around kobject_init_and_add() to support auto cleanup: struct kobject *kobject_init_and_add_or_errptr(struct kobject *kobj) { int err = kobject_init_and_add(kobj...); if (err) return ERR_PTR(err); return kobj; } With those then you could do: struct kobject *wi_kobj __free(kfree) = kzalloc(sizeof(struct kobject), GFP_KERNEL); struct kobject *kobj __free(kobject_put) = kobject_init_and_add_or_errptr(no_free_ptr(wi_kobj), ...) Otherwise, the patch does look good to me as is, but it seems like an opportunity for further cleanups that might also help other kobject_init_and_add() code paths.
On Tue, 1 Apr 2025 14:21:12 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > Rakie Kim wrote: > > Memory leaks occurred when removing sysfs attributes for weighted > > interleave. Improper kobject deallocation led to unreleased memory > > when initialization failed or when nodes were removed. > > > > This patch resolves the issue by replacing unnecessary `kfree()` > > calls with `kobject_put()`, ensuring proper cleanup and preventing > > memory leaks. > > > > By correctly using `kobject_put()`, the release function now > > properly deallocates memory without causing resource leaks, > > thereby improving system stability. > > > > Fixes: dce41f5ae253 ("mm/mempolicy: implement the sysfs-based weighted_interleave interface") > > Signed-off-by: Rakie Kim <rakie.kim@sk.com> > > Signed-off-by: Honggyu Kim <honggyu.kim@sk.com> > > Signed-off-by: Yunjeong Mun <yunjeong.mun@sk.com> > > Reviewed-by: Gregory Price <gourry@gourry.net> > > --- > > mm/mempolicy.c | 61 +++++++++++++++++++++++++------------------------- > > 1 file changed, 31 insertions(+), 30 deletions(-) > > > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > > index bbaadbeeb291..5950d5d5b85e 100644 > > --- a/mm/mempolicy.c > > +++ b/mm/mempolicy.c > > @@ -3448,7 +3448,9 @@ static void sysfs_wi_release(struct kobject *wi_kobj) > > > > for (i = 0; i < nr_node_ids; i++) > > sysfs_wi_node_release(node_attrs[i], wi_kobj); > > - kobject_put(wi_kobj); > > + > > + kfree(node_attrs); > > + kfree(wi_kobj); > > } > > > > static const struct kobj_type wi_ktype = { > > @@ -3494,15 +3496,22 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) > > struct kobject *wi_kobj; > > int nid, err; > > > > - wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL); > > - if (!wi_kobj) > > + node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), > > + GFP_KERNEL); > > + if (!node_attrs) > > return -ENOMEM; > > > > + wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL); > > + if (!wi_kobj) { > > + err = -ENOMEM; > > + goto node_out; > > + } > > + > > err = kobject_init_and_add(wi_kobj, &wi_ktype, root_kobj, > > "weighted_interleave"); > > It would be nice if this could take advantage of scope-based cleanup to > avoid the new gotos. It would need a new: > > DEFINE_FREE(kobject_put, struct kobject *, if (!IS_ERR_OR_NULL(_T)) kobject_put(_T)) > > ...and a wrapper around kobject_init_and_add() to support auto cleanup: > > struct kobject *kobject_init_and_add_or_errptr(struct kobject *kobj) > { > int err = kobject_init_and_add(kobj...); > > if (err) > return ERR_PTR(err); > return kobj; > } > > With those then you could do: > > struct kobject *wi_kobj __free(kfree) = kzalloc(sizeof(struct kobject), GFP_KERNEL); > struct kobject *kobj __free(kobject_put) = kobject_init_and_add_or_errptr(no_free_ptr(wi_kobj), ...) > > Otherwise, the patch does look good to me as is, but it seems like an > opportunity for further cleanups that might also help other > kobject_init_and_add() code paths. Thank you for your response regarding this patch. I believe that the method you suggested using `DEFINE_FREE` is an optimal solution for addressing the memory release issue related to `kobject`. I also think that similar problems may arise in many other kernel modules that use `kobject`, and therefore, it would be beneficial to address this issue more generally in those modules as well. However, I believe that including such functionality as part of this patch series may not be ideal. Instead, I think it's better to introduce this approach as a separate patch for broader application and review. I will work on creating a follow-up patch based on your suggestion. Once again, thank you for proposing a clean and helpful solution. Rakie
diff --git a/mm/mempolicy.c b/mm/mempolicy.c index bbaadbeeb291..5950d5d5b85e 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -3448,7 +3448,9 @@ static void sysfs_wi_release(struct kobject *wi_kobj) for (i = 0; i < nr_node_ids; i++) sysfs_wi_node_release(node_attrs[i], wi_kobj); - kobject_put(wi_kobj); + + kfree(node_attrs); + kfree(wi_kobj); } static const struct kobj_type wi_ktype = { @@ -3494,15 +3496,22 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) struct kobject *wi_kobj; int nid, err; - wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL); - if (!wi_kobj) + node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), + GFP_KERNEL); + if (!node_attrs) return -ENOMEM; + wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL); + if (!wi_kobj) { + err = -ENOMEM; + goto node_out; + } + err = kobject_init_and_add(wi_kobj, &wi_ktype, root_kobj, "weighted_interleave"); if (err) { - kfree(wi_kobj); - return err; + kobject_put(wi_kobj); + goto err_out; } for_each_node_state(nid, N_POSSIBLE) { @@ -3512,9 +3521,17 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) break; } } - if (err) + if (err) { kobject_put(wi_kobj); + goto err_out; + } + return 0; + +node_out: + kfree(node_attrs); +err_out: + return err; } static void mempolicy_kobj_release(struct kobject *kobj) @@ -3528,7 +3545,6 @@ static void mempolicy_kobj_release(struct kobject *kobj) mutex_unlock(&iw_table_lock); synchronize_rcu(); kfree(old); - kfree(node_attrs); kfree(kobj); } @@ -3542,37 +3558,22 @@ static int __init mempolicy_sysfs_init(void) static struct kobject *mempolicy_kobj; mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL); - if (!mempolicy_kobj) { - err = -ENOMEM; - goto err_out; - } - - node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), - GFP_KERNEL); - if (!node_attrs) { - err = -ENOMEM; - goto mempol_out; - } + if (!mempolicy_kobj) + return -ENOMEM; err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj, "mempolicy"); if (err) - goto node_out; + goto err_out; err = add_weighted_interleave_group(mempolicy_kobj); - if (err) { - pr_err("mempolicy sysfs structure failed to initialize\n"); - kobject_put(mempolicy_kobj); - return err; - } + if (err) + goto err_out; + + return 0; - return err; -node_out: - kfree(node_attrs); -mempol_out: - kfree(mempolicy_kobj); err_out: - pr_err("failed to add mempolicy kobject to the system\n"); + kobject_put(mempolicy_kobj); return err; }
Memory leaks occurred when removing sysfs attributes for weighted interleave. Improper kobject deallocation led to unreleased memory when initialization failed or when nodes were removed. This patch resolves the issue by replacing unnecessary `kfree()` calls with `kobject_put()`, ensuring proper cleanup and preventing memory leaks. By correctly using `kobject_put()`, the release function now properly deallocates memory without causing resource leaks, thereby improving system stability. Fixes: dce41f5ae253 ("mm/mempolicy: implement the sysfs-based weighted_interleave interface") Signed-off-by: Rakie Kim <rakie.kim@sk.com> Signed-off-by: Honggyu Kim <honggyu.kim@sk.com> Signed-off-by: Yunjeong Mun <yunjeong.mun@sk.com> Reviewed-by: Gregory Price <gourry@gourry.net> --- mm/mempolicy.c | 61 +++++++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 30 deletions(-)