On Thu, 20 Mar 2025 09:45:31 -0700 Joshua Hahn <joshua.hahnjy@xxxxxxxxx> wrote:
Hi Joshua
Thank you for your response regarding this patch.
> 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.
Yes, this patch does both: it replaces kfree with kobject_put and, as you
also mentioned, it moves the actual kfree logic into the release function.
I agree the original explanation may have been unclear, so I will review
and revise the commit message accordingly.
>
> > 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)
The reason for moving node_attrs is that it should be tied to wi_kobj
rather than mempolicy_kobj. Since node_attrs must be freed together
with wi_kobj, the allocation was relocated. I believe this behavior
is more clearly expressed in Patch 2.
>
> > + 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 : -)
This is also being cleaned up in Patch 2. In fact, once Patch 2 is
applied, the node_out label becomes unnecessary.
>
> > + 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
As you mentioned, I agree that Patch 1 may be a bit unclear.
In fact, Patch 1 and Patch 2 share similar goals, and in my view,
they only provide complete functionality when applied together.
Initially, I thought that Patch 1 was the fix for the original issue and
considered it the candidate for a backport.
However, upon further reflection, I believe that all changes in Patch 1
through Patch 3 are necessary to fully address the underlying problem.
Therefore, I now think it makes more sense to merge Patch 1 and Patch 2
into a single patch, then renumber the current Patch 3 as Patch 2,
and treat the entire set as a proper -stable backport candidate.
I'd appreciate your thoughts on this suggestion.
Rakie
>
> Sent using hkml (https://github.com/sjp38/hackermail)
>
