| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250407093800.417-1-rakie.kim@sk.com>
Date: Mon, 7 Apr 2025 18:37:53 +0900
From: Rakie Kim <rakie.kim@...com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: akpm@...ux-foundation.org,
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,
osalvador@...e.de,
kernel_team@...ynix.com,
honggyu.kim@...com,
yunjeong.mun@...com,
Rakie Kim <rakie.kim@...com>
Subject: Re: [PATCH v6 1/3] mm/mempolicy: Fix memory leaks in weighted interleave sysfs
On Fri, 4 Apr 2025 13:59:06 +0100 Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:
> On Fri, 4 Apr 2025 16:46:19 +0900
> Rakie Kim <rakie.kim@...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@...com>
> > Signed-off-by: Honggyu Kim <honggyu.kim@...com>
> > Signed-off-by: Yunjeong Mun <yunjeong.mun@...com>
> > Reviewed-by: Gregory Price <gourry@...rry.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;
> > }
> >
>
Powered by blists - more mailing lists