Re: [PATCH v4 1/3] mm/mempolicy: Fix memory leaks in weighted interleave sysfs

[Rakie Kim <rakie.kim@sk.com>]

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