| 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: <20250416074951.610-1-rakie.kim@sk.com>
Date: Wed, 16 Apr 2025 16:49:32 +0900
From: Rakie Kim <rakie.kim@...com>
To: Honggyu Kim <honggyu.kim@...com>
Cc: kernel_team@...ynix.com,
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,
yunjeong.mun@...com,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Rakie Kim <rakie.kim@...com>
Subject: Re: [PATCH v7 3/3] mm/mempolicy: Support memory hotplug in weighted interleave
On Wed, 16 Apr 2025 13:04:32 +0900 Honggyu Kim <honggyu.kim@...com> wrote:
Hi Jonathan and Honggyu,
Thank you for reviewing this patch and for offering valuable ideas to
address the issues. I have accepted all of your suggestions and am
currently preparing a new patch series, version v8.
> Hi Jonathan,
>
> Thanks for reviewing our patches.
>
> I have a few comments and the rest will be addressed by Rakie.
>
> On 4/16/2025 1:00 AM, Jonathan Cameron wrote:
> > On Tue, 8 Apr 2025 16:32:42 +0900
> > Rakie Kim <rakie.kim@...com> wrote:
> >
> >> @@ -3470,13 +3472,24 @@ static ssize_t node_store(struct kobject *kobj, struct kobj_attribute *attr,
> >>
> >> static void sysfs_wi_node_delete(int nid)
> >> {
> >> - if (!wi_group->nattrs[nid])
> >> + struct iw_node_attr *attr;
> >> +
> >> + if (nid < 0 || nid >= nr_node_ids)
> >> + return;
> >> +
> >> + mutex_lock(&wi_group->kobj_lock);
> >> + attr = wi_group->nattrs[nid];
> >> + if (!attr) {
> >> + mutex_unlock(&wi_group->kobj_lock);
> >> return;
> >> + }
> >> +
> >> + wi_group->nattrs[nid] = NULL;
> >> + mutex_unlock(&wi_group->kobj_lock);
> >>
> >> - sysfs_remove_file(&wi_group->wi_kobj,
> >> - &wi_group->nattrs[nid]->kobj_attr.attr);
> >> - kfree(wi_group->nattrs[nid]->kobj_attr.attr.name);
> >> - kfree(wi_group->nattrs[nid]);
> >> + sysfs_remove_file(&wi_group->wi_kobj, &attr->kobj_attr.attr);
> >> + kfree(attr->kobj_attr.attr.name);
> >> + kfree(attr);
> > Here you go through a careful dance to not touch wi_group->nattrs[nid]
> > except under the lock, but later you are happy to do so in the
> > error handling paths. Maybe better to do similar to here and
> > set it to NULL under the lock but do the freeing on a copy taken
> > under that lock.
I have updated the error handling path in sysfs_wi_node_add() as you
suggested.
> > .
> >> }
> >>
> >> static void sysfs_wi_release(struct kobject *wi_kobj)
> >> @@ -3495,35 +3508,77 @@ static const struct kobj_type wi_ktype = {
> >>
> >> static int sysfs_wi_node_add(int nid)
> >> {
> >> - struct iw_node_attr *node_attr;
> >> + int ret = 0;
> >
> > Trivial but isn't ret always set when it is used? So no need to initialize
> > here.
In the updated code for v8, I retained the initialization of `ret = 0`
because it is required for proper cleanup handling in the current
version.
>
> If we don't initialize it, then this kind of trivial fixup might be needed later
> so I think there is no reason not to initialize it.
> https://lore.kernel.org/mm-commits/20240705010631.46743C4AF07@smtp.kernel.org
>
> >
> >> char *name;
> >> + struct iw_node_attr *new_attr = NULL;
> >
> > This is also always set before use so I'm not seeing a
> > reason to initialize it to NULL.
>
> Ditto.
I also removed the unnecessary `= NULL` initializer for `new_attr`,
as it is always assigned before use.
>
> >
> >
> >>
> >> - node_attr = kzalloc(sizeof(*node_attr), GFP_KERNEL);
> >> - if (!node_attr)
> >> + if (nid < 0 || nid >= nr_node_ids) {
> >> + pr_err("Invalid node id: %d\n", nid);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + new_attr = kzalloc(sizeof(struct iw_node_attr), GFP_KERNEL);
> >
> > I'd prefer sizeof(*new_attr) because I'm lazy and don't like checking
> > types for allocation sizes :) Local style seems to be a bit
> > of a mix though.
>
> Agreed.
As you recommended, I changed the allocation from
`sizeof(struct iw_node_attr)` to `sizeof(*new_attr)` for better
readability and consistency.
>
> >
> >> + if (!new_attr)
> >> return -ENOMEM;
> >>
> >> name = kasprintf(GFP_KERNEL, "node%d", nid);
> >> if (!name) {
> >> - kfree(node_attr);
> >> + kfree(new_attr);
> >> return -ENOMEM;
> >> }
> >>
> >> - sysfs_attr_init(&node_attr->kobj_attr.attr);
> >> - node_attr->kobj_attr.attr.name = name;
> >> - node_attr->kobj_attr.attr.mode = 0644;
> >> - node_attr->kobj_attr.show = node_show;
> >> - node_attr->kobj_attr.store = node_store;
> >> - node_attr->nid = nid;
> >> + mutex_lock(&wi_group->kobj_lock);
> >> + if (wi_group->nattrs[nid]) {
> >> + mutex_unlock(&wi_group->kobj_lock);
> >> + pr_info("Node [%d] already exists\n", nid);
> >> + kfree(new_attr);
> >> + kfree(name);
> >> + return 0;
> >> + }
> >> + wi_group->nattrs[nid] = new_attr;
>
> This set can be done after all the "wi_group->nattrs[nid]" related set is done.
>
> >>
> >> - if (sysfs_create_file(&wi_group->wi_kobj, &node_attr->kobj_attr.attr)) {
> >> - kfree(node_attr->kobj_attr.attr.name);
> >> - kfree(node_attr);
> >> - pr_err("failed to add attribute to weighted_interleave\n");
> >> - return -ENOMEM;
> >> + sysfs_attr_init(&wi_group->nattrs[nid]->kobj_attr.attr);
> >
> > I'd have been tempted to use the new_attr pointer but perhaps
> > this brings some documentation like advantages.
>
> +1
Additionally, I replaced all usage of `wi_group->nattrs[nid]` in
sysfs_wi_node_add() with the `new_attr` pointer to simplify the logic
and improve clarity. This also aligns with your suggestion to treat
`new_attr` consistently throughout the function.
>
> >
> >> + wi_group->nattrs[nid]->kobj_attr.attr.name = name;
> >> + wi_group->nattrs[nid]->kobj_attr.attr.mode = 0644;
> >> + wi_group->nattrs[nid]->kobj_attr.show = node_show;
> >> + wi_group->nattrs[nid]->kobj_attr.store = node_store;
> >> + wi_group->nattrs[nid]->nid = nid;
>
> As Jonathan mentioned, all the "wi_group->nattrs[nid]" here is better to be
> "new_attr" for simplicity.
>
> Thanks,
> Honggyu
>
> >> +
> >> + ret = sysfs_create_file(&wi_group->wi_kobj,
> >> + &wi_group->nattrs[nid]->kobj_attr.attr);
> >> + if (ret) {
> >> + kfree(wi_group->nattrs[nid]->kobj_attr.attr.name);
> >
> > See comment above on the rather different handling here to in
> > sysfs_wi_node_delete() where you set it to NULL first, release the lock and tidy up.
> > new_attrand name are still set so you could even combine the handling with the
> > if (wi_group->nattrs[nid]) above via appropriate gotos.
I agree with your observation regarding the difference in error
handling between sysfs_wi_node_add() and sysfs_wi_node_delete(), so I
refactored sysfs_wi_node_add() to follow the same structure.
I will apply all of these updates in the new v8 series. Thank you
again for your thoughtful and detailed feedback.
Below is the revised code after incorporating your feedback.
Rakie
@@ -3532,14 +3532,14 @@ static int sysfs_wi_node_add(int nid)
{
int ret = 0;
char *name;
- struct iw_node_attr *new_attr = NULL;
+ struct iw_node_attr *new_attr;
if (nid < 0 || nid >= nr_node_ids) {
- pr_err("Invalid node id: %d\n", nid);
+ pr_err("invalid node id: %d\n", nid);
return -EINVAL;
}
- new_attr = kzalloc(sizeof(struct iw_node_attr), GFP_KERNEL);
+ new_attr = kzalloc(sizeof(*new_attr), GFP_KERNEL);
if (!new_attr)
return -ENOMEM;
@@ -3549,33 +3549,32 @@ static int sysfs_wi_node_add(int nid)
return -ENOMEM;
}
+ sysfs_attr_init(&new_attr->kobj_attr.attr);
+ new_attr->kobj_attr.attr.name = name;
+ new_attr->kobj_attr.attr.mode = 0644;
+ new_attr->kobj_attr.show = node_show;
+ new_attr->kobj_attr.store = node_store;
+ new_attr->nid = nid;
+
mutex_lock(&wi_group->kobj_lock);
if (wi_group->nattrs[nid]) {
mutex_unlock(&wi_group->kobj_lock);
- pr_info("Node [%d] already exists\n", nid);
- kfree(new_attr);
- kfree(name);
- return 0;
+ pr_info("node%d already exists\n", nid);
+ goto out;
}
- wi_group->nattrs[nid] = new_attr;
-
- sysfs_attr_init(&wi_group->nattrs[nid]->kobj_attr.attr);
- pr_info("Node [%d] already exists\n", nid);
- kfree(new_attr);
- kfree(name);
- return 0;
+ pr_info("node%d already exists\n", nid);
+ goto out;
}
- wi_group->nattrs[nid] = new_attr;
-
- sysfs_attr_init(&wi_group->nattrs[nid]->kobj_attr.attr);
- wi_group->nattrs[nid]->kobj_attr.attr.name = name;
- wi_group->nattrs[nid]->kobj_attr.attr.mode = 0644;
- wi_group->nattrs[nid]->kobj_attr.show = node_show;
- wi_group->nattrs[nid]->kobj_attr.store = node_store;
- wi_group->nattrs[nid]->nid = nid;
- ret = sysfs_create_file(&wi_group->wi_kobj,
- &wi_group->nattrs[nid]->kobj_attr.attr);
+ ret = sysfs_create_file(&wi_group->wi_kobj, &new_attr->kobj_attr.attr);
if (ret) {
- kfree(wi_group->nattrs[nid]->kobj_attr.attr.name);
- kfree(wi_group->nattrs[nid]);
- wi_group->nattrs[nid] = NULL;
- pr_err("Failed to add attribute to weighted_interleave: %d\n", ret);
+ mutex_unlock(&wi_group->kobj_lock);
+ goto out;
}
+ wi_group->nattrs[nid] = new_attr;
mutex_unlock(&wi_group->kobj_lock);
+ return 0;
+out:
+ kfree(new_attr->kobj_attr.attr.name);
+ kfree(new_attr);
return ret;
}
> >
> >> + kfree(wi_group->nattrs[nid]);
> >> + wi_group->nattrs[nid] = NULL;
> >> + pr_err("Failed to add attribute to weighted_interleave: %d\n", ret);
> >> }
> >> + mutex_unlock(&wi_group->kobj_lock);
> >>
> >> - wi_group->nattrs[nid] = node_attr;
> >> - return 0;
> >> + return ret;
> >> +}
> >
> >
Powered by blists - more mailing lists