| 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: <20250417015009.650-1-rakie.kim@sk.com>
Date: Thu, 17 Apr 2025 10:49:38 +0900
From: Rakie Kim <rakie.kim@...com>
To: Dan Williams <dan.j.williams@...el.com>
Cc: gourry@...rry.net,
linux-mm@...ck.org,
linux-kernel@...r.kernel.org,
linux-cxl@...r.kernel.org,
joshua.hahnjy@...il.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,
akpm@...ux-foundation.org
Subject: Re: [PATCH v8 1/3] mm/mempolicy: Fix memory leaks in weighted interleave sysfs
On Wed, 16 Apr 2025 14:54:16 -0700 Dan Williams <dan.j.williams@...el.com> wrote:
> Rakie Kim wrote:
> > +
> > +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);
I will modify the structure as you suggested.
>
> > +}
> > +
> > +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:
>
I created a new function named wi_cleanup(), as you proposed.
static void wi_cleanup(struct kobject *wi_kobj) {
sysfs_wi_node_delete_all(wi_kobj);
iw_table_free();
kfree(node_attrs);
}
And I changed the error handling code to call this function.
static int add_weighted_interleave_group(struct kobject *root_kobj)
{
...
err_cleanup_kobj:
wi_cleanup(wi_kobj);
kobject_del(wi_kobj);
err_put_kobj:
kobject_put(wi_kobj);
return err;
}
However, I have one question.
With this change, iw_table and node_attrs will not be freed at system
shutdown. Is it acceptable to leave this memory unfreed on shutdown?
(As you previously noted, the sysfs files in this patch are also
not removed during system shutdown.)
> > + 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 understand your suggestion about simplifying the kobject
handling.
If we only consider Patch1, then replacing kobject_init_and_add
with kobject_create_and_add would be the right choice.
However, in Patch2, the code changes as follows:
struct sysfs_wi_group {
struct kobject wi_kobj;
struct iw_node_attr *nattrs[];
};
static struct sysfs_wi_group *wi_group;
...
static void wi_kobj_release(struct kobject *wi_kobj)
{
kfree(wi_group);
}
...
static int __init add_weighted_interleave_group(struct kobject *mempolicy_kobj)
{
int nid, err;
wi_group = kzalloc(struct_size(wi_group, nattrs, nr_node_ids),
GFP_KERNEL);
In this case, wi_kobj_release() is responsible for freeing the
container struct wi_group that includes the kobject.
Therefore, it seems more appropriate to use kobject_init_and_add
in this context.
I would appreciate your thoughts on this.
>
> 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.
For the mempolicy_kobj, I will update the code as you suggested
and use kobject_create_and_add().
With all your recommendations applied, Patch1 would now look like this:
@@ -3488,20 +3488,21 @@ static void iw_table_free(void)
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);
+ rcu_assign_pointer(iw_table, NULL);
+ mutex_unlock(&iw_table_lock);
- synchronize_rcu();
- kfree(old);
- } else
- mutex_unlock(&iw_table_lock);
+ synchronize_rcu();
+ kfree(old);
}
-static void wi_kobj_release(struct kobject *wi_kobj)
-{
+static void wi_cleanup(struct kobject *wi_kobj) {
+ sysfs_wi_node_delete_all(wi_kobj);
iw_table_free();
kfree(node_attrs);
+}
+
+static void wi_kobj_release(struct kobject *wi_kobj)
+{
kfree(wi_kobj);
}
@@ -3575,45 +3576,30 @@ static int add_weighted_interleave_group(struct kobject *root_kobj)
return 0;
err_cleanup_kobj:
- sysfs_wi_node_delete_all(wi_kobj);
+ wi_cleanup(wi_kobj);
kobject_del(wi_kobj);
err_put_kobj:
kobject_put(wi_kobj);
return err;
}
-static void mempolicy_kobj_release(struct kobject *kobj)
-{
- kfree(kobj);
-}
-
-static const struct kobj_type mempolicy_ktype = {
- .release = mempolicy_kobj_release
-};
-
static int __init mempolicy_sysfs_init(void)
{
int err;
static struct kobject *mempolicy_kobj;
- mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL);
+ mempolicy_kobj = kobject_create_and_add("mempolicy", mm_kobj);
if (!mempolicy_kobj)
return -ENOMEM;
- err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj,
- "mempolicy");
- if (err)
- goto err_put_kobj;
-
err = add_weighted_interleave_group(mempolicy_kobj);
if (err)
- goto err_del_kobj;
+ goto err_kobj;
return 0;
-err_del_kobj:
+err_kobj:
kobject_del(mempolicy_kobj);
-err_put_kobj:
kobject_put(mempolicy_kobj);
return err;
}
Rakie
>
> 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