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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Fri, 4 Apr 2025 16:46:19 +0900
Rakie Kim <rakie.kim@sk.com> 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.
> 
> 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>
I've pretty much forgotten earlier discussions so apologies if I revisit
old ground! 

The fix is fine I think. But the resulting code structure
could be improved, without (I think) complicating what is here by much.

> ---
>  mm/mempolicy.c | 64 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 34 insertions(+), 30 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index bbaadbeeb291..af3753925573 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;
As this is only place where we do kfree(node_attrs)
why not just do that here and return directly.

		
> +	}
> +
>  	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,18 @@ static int add_weighted_interleave_group(struct kobject *root_kobj)
>  			break;
>  		}
>  	}
> -	if (err)
> +	if (err) {
> +		kobject_del(wi_kobj);
>  		kobject_put(wi_kobj);

For this and the one above, a unified exit kind of makes sense as
can do two labels and have the put only once.

If not, why not move this up into the loop and return directly?
If you move to an error handling block

err_del_obj:
	kobject_del(wi_kobj);
err_put_obj:
	kobject_put(wi_kobj);

then you could also do the goto from within that loop.


> +		goto err_out;
> +	}
> +
>  	return 0;
> +
> +node_out:
> +	kfree(node_attrs);
> +err_out:
> +	return err;
>  }
>  
>  static void mempolicy_kobj_release(struct kobject *kobj)
> @@ -3528,7 +3546,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 +3559,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");
>  	if (err)
> -		goto node_out;
> +		goto err_out;
		goto err_put_kobj;
 or something like that.

>  
>  	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_del;
>  
> -	return err;
> -node_out:
> -	kfree(node_attrs);
> -mempol_out:
> -	kfree(mempolicy_kobj);
> +	return 0;
> +
> +err_del:
> +	kobject_del(mempolicy_kobj);
>  err_out:
> -	pr_err("failed to add mempolicy kobject to the system\n");
> +	kobject_put(mempolicy_kobj);
If we keep an err_out, usual expectation is all it does is return
+ maybe print a message. We'd not expect a put.

>  	return err;
>  }
>