linux-kernel - Re: [PATCH v2 1/4] mm/mempolicy: Fix memory leaks in mempolicy_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] [day] [month] [year] [list]

Message-ID: <20250317082430.829-1-rakie.kim@sk.com>
Date: Mon, 17 Mar 2025 17:24:23 +0900
From: Rakie Kim <rakie.kim@...com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Rakie Kim <rakie.kim@...com>,
	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,
	kernel_team@...ynix.com,
	honggyu.kim@...com,
	yunjeong.mun@...com,
	Gregory Price <gourry@...rry.net>
Subject: Re: [PATCH v2 1/4] mm/mempolicy: Fix memory leaks in mempolicy_sysfs_init()

On Fri, 14 Mar 2025 10:55:00 +0000 Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:
> On Thu, 13 Mar 2025 11:52:18 -0400
> Gregory Price <gourry@...rry.net> wrote:
> 
> > On Thu, Mar 13, 2025 at 03:31:38PM +0900, Rakie Kim wrote:
> > > > Is this correct? If kobject_init_and_add fails, from other examples we
> > > > need only free the mempolicy_kobj - because it failed to initialize and
> > > > therefore should not have any references.  I think this causes an
> > > > underflow.  
> > > 
> > > Regarding the reordering of mempolicy_kobj allocation:
> > > 1) In kobject_init_and_add(), kobject_init() is always called, which  
> > 
> > Quite right, mea culpa.
> > 
> > > 
> > > 2) The release function for mempolicy_kobj is responsible for freeing
> > >    associated memory:
> > > 
> > >    static void mempolicy_kobj_release(struct kobject *kobj)
> > >    {
> > >        ...
> > >        kfree(ngrp->nattrs);
> > >        kfree(ngrp);
> > >        kfree(kobj);
> > >    }
> > >   
> > 
> > I see what you're trying to do now after looking at the free-ordering
> > at little closer.
> > 
> > Lets do the following:
> > 
> > 1) allocate node_attrs and mempolicy_kobj up front and keep your
> >    reordering, this lets us clean up allocations on failure before
> >    kobject_init is called
> > 
> > 2) after this remove all the other code and just let
> >    mempolicy_kobj_release clean up node_attrs
> > 
> > 3) Add a (%d) to the error message to differentiate failures
> 
> Given how unlikely (and noisy) a memory allocation failure is,
> maybe just drop the printing at all in those paths - allowing
> early returns.
> 
> The lifetime rules around node_attrs in here are making readability
> poor. It is implicitly owned by the mempolicy_kobj, but no direct association.
> Maybe just encapsulating the kobject in a structure that contains
> this as a [] array at the end.  Then we end up with single allocation of
> stuff that is effectively one thing.
> 

Hi Jonathan

Thank you for your response regarding this patch.
Your suggestions seem very appropriate. As you recommended, I will proceed to
encapsulate node_attrs and mempolicy_kobj into a single structure.

Rakie

> 
> > 
> > This is a little bit cleaner and is a bit less code. (Not built or
> > tested, just a recommendation).
> > 
> > I'd recommend submitting this patch by itself to mm-stable, since the
> > remainder of the patch line changes functionality and this fixes a bug
> > in LTS kernels.
> > 
> > ~Gregory
> > 
> > ---
> > 
> > 
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index 530e71fe9147..05a410db08b4 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -3541,38 +3541,34 @@ static int __init mempolicy_sysfs_init(void)
> >  	int err;
> >  	static struct kobject *mempolicy_kobj;
> > 
> > -	mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL);
> > -	if (!mempolicy_kobj) {
> > +	node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *),
> > +			     GFP_KERNEL);
> > +	if (!node_attrs) {
> >  		err = -ENOMEM;
> >  		goto err_out;
> >  	}
> > 
> > -	node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *),
> > -			     GFP_KERNEL);
> > -	if (!node_attrs) {
> > +	mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL);
> > +	if (!mempolicy_kobj) {
> >  		err = -ENOMEM;
> > -		goto mempol_out;
> > +		kfree(node_attrs);
> > +		goto err_out;
> >  	}
> > 
> >  	err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj,
> >  				   "mempolicy");
> >  	if (err)
> > -		goto node_out;
> > +		goto mempol_out;
> > 
> >  	err = add_weighted_interleave_group(mempolicy_kobj);
> > -	if (err) {
> > -		pr_err("mempolicy sysfs structure failed to initialize\n");
> > -		kobject_put(mempolicy_kobj);
> > -		return err;
> > -	}
> > +	if (err)
> > +		goto mempol_out;
> > 
> > -	return err;
> > -node_out:
> > -	kfree(node_attrs);
> > +	return 0;
> >  mempol_out:
> > -	kfree(mempolicy_kobj);
> > +	kobject_put(mempolicy_kobj);
> >  err_out:
> > -	pr_err("failed to add mempolicy kobject to the system\n");
> > +	pr_err("mempolicy sysfs structure failed to initialize (%d)\n", err);
> >  	return err;
> >  }
> > 
> > 
>