linux-kernel - Re: [PATCH v4 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: <67ec58c8c637_1d472949b@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 1 Apr 2025 14:21:12 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Rakie Kim <rakie.kim@...com>, <gourry@...rry.net>
CC: <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>, <rakie.kim@...com>
Subject: Re: [PATCH v4 1/3] mm/mempolicy: Fix memory leaks in weighted
 interleave sysfs

Rakie Kim 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>
> Signed-off-by: Honggyu Kim <honggyu.kim@...com>
> Signed-off-by: Yunjeong Mun <yunjeong.mun@...com>
> Reviewed-by: Gregory Price <gourry@...rry.net>
> ---
>  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);
>  }
>  
>  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;
>  
> +	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");

It would be nice if this could take advantage of scope-based cleanup to
avoid the new gotos. It would need a new:

DEFINE_FREE(kobject_put, struct kobject *, if (!IS_ERR_OR_NULL(_T)) kobject_put(_T))

...and a wrapper around kobject_init_and_add() to support auto cleanup:

struct kobject *kobject_init_and_add_or_errptr(struct kobject *kobj)
{
	int err = kobject_init_and_add(kobj...);

	if (err)
		return ERR_PTR(err);
	return kobj;
}

With those then you could do:

struct kobject *wi_kobj __free(kfree) = kzalloc(sizeof(struct kobject), GFP_KERNEL);
struct kobject *kobj __free(kobject_put) = kobject_init_and_add_or_errptr(no_free_ptr(wi_kobj), ...)

Otherwise, the patch does look good to me as is, but it seems like an
opportunity for further cleanups that might also help other
kobject_init_and_add() code paths.