linux-kernel - Re: [PATCH v3 1/3] mm/mempolicy: Fix memory leaks in weighted interleave sysfs

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250320164532.1313581-1-joshua.hahnjy@gmail.com>
Date: Thu, 20 Mar 2025 09:45:31 -0700
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: Rakie Kim <rakie.kim@...com>
Cc: gourry@...rry.net,
	akpm@...ux-foundation.org,
	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,
	Jonathan.Cameron@...wei.com,
	kernel_team@...ynix.com,
	honggyu.kim@...com,
	yunjeong.mun@...com
Subject: Re: [PATCH v3 1/3] mm/mempolicy: Fix memory leaks in weighted interleave sysfs

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@...com> 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@...com>
> ---
>  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)