Message-ID: <6800270845306_1302d294a4@dwillia2-xfh.jf.intel.com.notmuch>
Date: Wed, 16 Apr 2025 14:54:16 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Rakie Kim <rakie.kim@...com>, <akpm@...ux-foundation.org>
CC: <gourry@...rry.net>, <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
	<linux-cxl@...r.kernel.org>, <joshua.hahnjy@...il.com>,
	<dan.j.williams@...el.com>, <ying.huang@...ux.alibaba.com>,
	<david@...hat.com>, <Jonathan.Cameron@...wei.com>, <osalvador@...e.de>,
	<kernel_team@...ynix.com>, <honggyu.kim@...com>, <yunjeong.mun@...com>,
	<rakie.kim@...com>
Subject: Re: [PATCH v8 1/3] mm/mempolicy: Fix memory leaks in weighted
 interleave sysfs

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 proper `kobject_del()` and `kobject_put()` sequences,
> ensuring correct teardown and preventing memory leaks.
> 
> By explicitly calling `kobject_del()` before `kobject_put()`, the
> release function is now invoked safely, and internal sysfs state is
> correctly cleaned up. This guarantees that the memory associated with
> the kobject is fully released and avoids resource leaks, thereby
> improving system stability.
> 
> Additionally, sysfs_remove_file() is no longer called from the release
> function to avoid accessing invalid sysfs state after kobject_del().
> All attribute removals are now done before kobject_del(), preventing
> WARN_ON() in kernfs and ensuring safe and consistent cleanup of sysfs
> entries.
> 
> Fixes: dce41f5ae253 ("mm/mempolicy: implement the sysfs-based weighted_interleave interface")
> Signed-off-by: Rakie Kim <rakie.kim@...com>
> Reviewed-by: Gregory Price <gourry@...rry.net>
> Reviewed-by: Joshua Hahn <joshua.hahnjy@...il.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> ---
>  mm/mempolicy.c | 111 +++++++++++++++++++++++++++----------------------
>  1 file changed, 61 insertions(+), 50 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index b28a1e6ae096..dcf03c389b51 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -3463,8 +3463,8 @@ static ssize_t node_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  static struct iw_node_attr **node_attrs;
>  
> -static void sysfs_wi_node_release(struct iw_node_attr *node_attr,
> -				  struct kobject *parent)
> +static void sysfs_wi_node_delete(struct iw_node_attr *node_attr,
> +				 struct kobject *parent)
>  {
>  	if (!node_attr)
>  		return;
> @@ -3473,18 +3473,41 @@ static void sysfs_wi_node_release(struct iw_node_attr *node_attr,
>  	kfree(node_attr);
>  }
>  
> -static void sysfs_wi_release(struct kobject *wi_kobj)
> +static void sysfs_wi_node_delete_all(struct kobject *wi_kobj)
>  {
> -	int i;
> +	int nid;
>  
> -	for (i = 0; i < nr_node_ids; i++)
> -		sysfs_wi_node_release(node_attrs[i], wi_kobj);
> -	kobject_put(wi_kobj);
> +	for (nid = 0; nid < nr_node_ids; nid++)
> +		sysfs_wi_node_delete(node_attrs[nid], wi_kobj);
> +}
> +
> +static void iw_table_free(void)
> +{
> +	u8 *old;
> +
> +	mutex_lock(&iw_table_lock);
> +	old = rcu_dereference_protected(iw_table,
> +					lockdep_is_held(&iw_table_lock));
> +	if (old) {
> +		rcu_assign_pointer(iw_table, NULL);
> +		mutex_unlock(&iw_table_lock);
> +
> +		synchronize_rcu();
> +		kfree(old);
> +	} else
> +		mutex_unlock(&iw_table_lock);

This looks correct. I personally would not have spent the effort to
avoid the synchronize_rcu() because this is an error path that rarely
gets triggered, and kfree(NULL) is already a nop, so no pressing need to be
careful there either:

	mutex_lock(&iw_table_lock);
	old = rcu_dereference_protected(iw_table,
					lockdep_is_held(&iw_table_lock));
	rcu_assign_pointer(iw_table, NULL);
	mutex_unlock(&iw_table_lock);
	synchronize_rcu();
	kfree(old);

> +}
> +
> +static void wi_kobj_release(struct kobject *wi_kobj)
> +{
> +	iw_table_free();

This memory can be freed as soon as node_attrs have been deleted. By
waiting until final wi_kobj release it confuses the lifetime rules.

> +	kfree(node_attrs);

This memory too can be freed as soon as the attributes are deleted.

...the rationale for considering these additional cleanups below:

> +	kfree(wi_kobj);
>  }
>  
>  static const struct kobj_type wi_ktype = {
>  	.sysfs_ops = &kobj_sysfs_ops,
> -	.release = sysfs_wi_release,
> +	.release = wi_kobj_release,
>  };
>  
>  static int add_weight_node(int nid, struct kobject *wi_kobj)
> @@ -3525,41 +3548,42 @@ static int add_weighted_interleave_group(struct kobject *root_kobj)
>  	struct kobject *wi_kobj;
>  	int nid, err;
>  
> +	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)
> +	if (!wi_kobj) {
> +		kfree(node_attrs);
>  		return -ENOMEM;
> +	}
>  
>  	err = kobject_init_and_add(wi_kobj, &wi_ktype, root_kobj,
>  				   "weighted_interleave");

If you fix wi_kobj_release() to stop being responsible to free memory
that should have been handled in the delete path (@node_attrs,
iw_table_free()), then you can also drop the wi_ktype and
wi_kobj_release() callback altogether.

I.e. once releasing @wi_kobj is just "kfree(wi_kobj)", then this
sequence:

	wi_kobj = kzalloc(...)
	kobject_init_and_add(wi_kob, &wi_ktype, ...)

Can simply become:

	wi_kobj = kobject_create_and_add("weighted_interleave", root_kobj);

> -	if (err) {
> -		kfree(wi_kobj);
> -		return err;
> -	}
> +	if (err)
> +		goto err_put_kobj;
>  
>  	for_each_node_state(nid, N_POSSIBLE) {
>  		err = add_weight_node(nid, wi_kobj);
>  		if (err) {
>  			pr_err("failed to add sysfs [node%d]\n", nid);
> -			break;
> +			goto err_cleanup_kobj;
>  		}
>  	}
> -	if (err)
> -		kobject_put(wi_kobj);
> +
>  	return 0;
> +
> +err_cleanup_kobj:
> +	sysfs_wi_node_delete_all(wi_kobj);
> +	kobject_del(wi_kobj);
> +err_put_kobj:
> +	kobject_put(wi_kobj);
> +	return err;
>  }
>  
>  static void mempolicy_kobj_release(struct kobject *kobj)
>  {
> -	u8 *old;
> -
> -	mutex_lock(&iw_table_lock);
> -	old = rcu_dereference_protected(iw_table,
> -					lockdep_is_held(&iw_table_lock));
> -	rcu_assign_pointer(iw_table, NULL);
> -	mutex_unlock(&iw_table_lock);
> -	synchronize_rcu();
> -	kfree(old);
> -	kfree(node_attrs);
>  	kfree(kobj);
>  }
>  
> @@ -3573,37 +3597,24 @@ 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");

Similar comment as above, now that mempolicy_kobj_release() is simply
kfree(@kobj), you can use kobject_create_and_add() and make this all
that much simpler.

So the patch looks technically correct as is, but if you make those
final cleanups I will add my review tag.

Powered by blists - more mailing lists