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

Rakie Kim posted 3 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH v6 1/3] mm/mempolicy: Fix memory leaks in weighted interleave sysfs
Posted by Rakie Kim 1 month, 1 week ago
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>
---
 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;
+	}
+
 	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);
+		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;
 
 	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);
 	return err;
 }
 
-- 
2.34.1
Re: [PATCH v6 1/3] mm/mempolicy: Fix memory leaks in weighted interleave sysfs
Posted by Jonathan Cameron 1 month, 1 week ago
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;
>  }
>
Re: [PATCH v6 1/3] mm/mempolicy: Fix memory leaks in weighted interleave sysfs
Posted by Rakie Kim 1 month, 1 week ago
On Fri, 4 Apr 2025 13:59:06 +0100 Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 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.
> 

Thank you for your response regarding this patch.
I appreciate your willingness to revisit this code and share your
thoughts. Please feel free to provide suggestions anytime.

> > ---
> >  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.

Regarding your suggestion:
Is the following change what you had in mind?
               
    wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL);
    if (!wi_kobj) {
        kfree(node_attrs);
        return -ENOMEM;
    }
I will apply this change accordingly.

> 
> 		
> > +	}
> > +
> >  	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.
> 

As for your second suggestion about restructuring the error handling,
you are right that having unified labels helps make the flow cleaner.
I will update the code to use:

err = kobject_init_and_add(wi_kobj, &wi_ktype, root_kobj,
                           "weighted_interleave");
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);
        goto err_del_kobj;
    }
}

err_del_kobj:
    kobject_del(wi_kobj);
err_put_kobj:
    kobject_put(wi_kobj);
    return err;

This structure keeps error handling more consistent and avoids redundancy.

> 
> > +		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.

Lastly, I agree with your point about `err_out`.
I will revise it to use `err_del_kobj` and `err_put_kobj` as needed.

Thanks again for the detailed review.

Rakie

> 
> >  	return err;
> >  }
> >  
>