Hi Rakie, thank you for the new version! I have just a few questions / nits
about this patch.
On Thu, 20 Mar 2025 13:17:46 +0900 Rakie Kim <rakie.kim@xxxxxx> 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 `kobject_put()`, ensuring proper cleanup and preventing
> memory leaks.
>
> By correctly using `kobject_put()`, the release function now
> properly deallocates memory without causing resource leaks,
> thereby improving system stability.
>
> Fixes: dce41f5ae253 ("mm/mempolicy: implement the sysfs-based weighted_interleave interface")
> Signed-off-by: Rakie Kim <rakie.kim@xxxxxx>
> ---
> mm/mempolicy.c | 61 +++++++++++++++++++++++++-------------------------
> 1 file changed, 31 insertions(+), 30 deletions(-)
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index bbaadbeeb291..5950d5d5b85e 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);
> }
I think the intent here is to make mempolicy_sysfs_init call kobject_put, which
will then call sysfs_wi_release when the refcount is 0. So I think replacing
kobject_put with kfree makes a lot of sense here. However, I think it is a bit
confusing based on the commit message, which states that you are doing the
opposite (replacing kfree with kobject_put). Perhaps it makes more sense to
say that you are moving kfree() from sysfs_init to the release function, so
that the struct and the node_attrs struct is freed together by the last
reference holder.
> 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;
It's also not obvious to me why the allocation for node_attrs was moved to
add_weighted_interleave_group. Maybe this refactoring belongs in patch 2,
whose described intent is to consolidate the two objects into one (I expand
on this idea below)
> + 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,17 @@ static int add_weighted_interleave_group(struct kobject *root_kobj)
> break;
> }
> }
> - if (err)
> + if (err) {
> kobject_put(wi_kobj);
> + goto err_out;
> + }
> +
> return 0;
> +
> +node_out:
> + kfree(node_attrs);
> +err_out:
NIT: Is there a reason why we have a single line goto statement? Maybe it
is more readable to replace all `goto err_out` with `return err` and save
a few jumps : -)
> + return err;
> }
>
> static void mempolicy_kobj_release(struct kobject *kobj)
> @@ -3528,7 +3545,6 @@ static void mempolicy_kobj_release(struct kobject *kobj)
> mutex_unlock(&iw_table_lock);
> synchronize_rcu();
> kfree(old);
> - kfree(node_attrs);
I think the intent of this patch is slightly confusing. Viewing this patch
alone, it is not entirely obvious why the kfree for node_attrs is now being
moved from the release of mempolicy_kobj to wi_kobj. Of course, we know that
it is actually because this patch serves a secondary purpose of moving
the allocations / freeing of nattrs and wi_kobj together, so that in the
next patch they can be combined into a single struct.
I think one way to make this patch more readable and maintainable is to
separate it into (1) fixes, (as the Fixes: tag in your commit message suggests)
and (2) refactoring that prepares for the next patch.
Please let me know what you think -- these were just some thoughts that I had
while I was reading the patch. Thank you again for this new version!
Have a great day : -)
Joshua
Sent using hkml (https://github.com/sjp38/hackermail)
