| 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: <20250411072135.588-1-rakie.kim@sk.com>
Date: Fri, 11 Apr 2025 16:21:28 +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 v7 2/3] mm/mempolicy: Prepare weighted interleave sysfs for memory hotplug
On Tue, 8 Apr 2025 20:54:48 -0700 Dan Williams <dan.j.williams@...el.com> wrote:
> Dan Williams wrote:
> > >
> > > +struct sysfs_wi_group {
> > > + struct kobject wi_kobj;
> > > + struct iw_node_attr *nattrs[];
> > > +};
> > > +
> > > +static struct sysfs_wi_group *wi_group;
> > > +
> > > static ssize_t node_show(struct kobject *kobj, struct kobj_attribute *attr,
> > > char *buf)
> > > {
> > > @@ -3461,27 +3468,24 @@ static ssize_t node_store(struct kobject *kobj, struct kobj_attribute *attr,
> > > return count;
> > > }
> > >
> > > -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(int nid)
> > > {
> > > - if (!node_attr)
> > > + if (!wi_group->nattrs[nid])
> > > return;
> > > - sysfs_remove_file(parent, &node_attr->kobj_attr.attr);
> > > - kfree(node_attr->kobj_attr.attr.name);
> > > - kfree(node_attr);
> > > +
> > > + sysfs_remove_file(&wi_group->wi_kobj,
> > > + &wi_group->nattrs[nid]->kobj_attr.attr);
> >
> > This still looks broken to me, but I think this is more a problem that
> > was present in the original code.
> >
> > At this point @wi_group's reference count is zero because
> > sysfs_wi_release() has been called. However, it can only be zero if it has
> > properly transitioned through kobject_del() and final kobject_put(). It
> > follows that kobject_del() arranges for kobj->sd to be NULL. That means
> > that this *should* be hitting the WARN() in kernfs_remove_by_name_ns()
> > for the !parent case.
> >
> > So, either you are not triggering that path, or testing that path, but
> > sys_remove_file() of the child attributes should be happening *before*
> > sysfs_wi_release().
> >
> > Did I miss something?
>
> I think the missing change is that sysfs_wi_node_add() failures need to
> be done with a sysfs_wi_node_delete() of the added attrs *before* the
> kobject_del() of @wi_group.
Hi Dan,
Thank you for pointing out this issue.
As you suggested, I believe the most appropriate way to handle this is
to incorporate your feedback into Patch 1
(mm/mempolicy: Fix memory leaks in weighted interleave sysfs).
To ensure that sysfs_remove_file() is called before kobject_del(), I
have restructured the code as follows:
<Previously>
static void sysfs_wi_release(struct kobject *wi_kobj)
{
int nid;
for (nid = 0; nid < nr_node_ids; nid++)
sysfs_wi_node_delete(node_attrs[nid], wi_kobj);
-> ERROR: sysfs_remove_file called here
kfree(node_attrs);
kfree(wi_kobj);
}
<Now>
static void sysfs_wi_node_delete_all(struct kobject *wi_kobj)
{
int nid;
for (nid = 0; nid < nr_node_ids; nid++)
sysfs_wi_node_delete(node_attrs[nid], wi_kobj);
-> sysfs_remove_file called here
}
static void sysfs_wi_release(struct kobject *wi_kobj)
{
kfree(node_attrs);
kfree(wi_kobj);
}
In addition, I call sysfs_wi_node_delete_all() before kobject_del()
during error handling:
+err_cleanup_kobj:
+ sysfs_wi_node_delete_all(wi_kobj);
kobject_del(wi_kobj);
I believe this resolves the issue you raised.
That said, I have a follow-up question. With this structure, when the
system is shutting down, sysfs_remove_file() will not be called. Based
on my review of other kernel subsystems, it seems that sysfs_remove_file()
is only called during module_exit() in driver code, and not in other
built-in subsystems.
Is this an acceptable practice? If you happen to know the expected
behavior in such cases, I would appreciate your insights.
Below is the full content of the updated Patch 1.
@@ -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,13 +3473,16 @@ 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);
+ for (nid = 0; nid < nr_node_ids; nid++)
+ sysfs_wi_node_delete(node_attrs[nid], wi_kobj);
+}
+static void sysfs_wi_release(struct kobject *wi_kobj)
+{
kfree(node_attrs);
kfree(wi_kobj);
}
@@ -3547,13 +3550,14 @@ static int add_weighted_interleave_group(struct kobject *root_kobj)
err = add_weight_node(nid, wi_kobj);
if (err) {
pr_err("failed to add sysfs [node%d]\n", nid);
- goto err_del_kobj;
+ goto err_cleanup_kobj;
}
}
return 0;
-err_del_kobj:
+err_cleanup_kobj:
+ sysfs_wi_node_delete_all(wi_kobj);
kobject_del(wi_kobj);
err_put_kobj:
kobject_put(wi_kobj);
Thank you again for your helpful feedback.
Rakie
Powered by blists - more mailing lists