diff mbox series

[v2,1/4] mm/mempolicy: Fix memory leaks in mempolicy_sysfs_init()

Message ID 20250312075628.648-1-rakie.kim@sk.com
State New
Headers show
Series [v2,1/4] mm/mempolicy: Fix memory leaks in mempolicy_sysfs_init() | expand

Commit Message

Rakie Kim March 12, 2025, 7:56 a.m. UTC
Improper cleanup of sysfs attributes caused kobject and memory leaks when
initialization failed or nodes were removed.

This patch ensures proper deallocation of kobjects and memory, preventing
resource leaks and improving stability.

Fixes: dce41f5ae253 ("mm/mempolicy: implement the sysfs-based weighted_interleave interface")
Signed-off-by: Rakie Kim <rakie.kim@sk.com>
---
 mm/mempolicy.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)


base-commit: 80e54e84911a923c40d7bee33a34c1b4be148d7a

Comments

Gregory Price March 12, 2025, 3:49 p.m. UTC | #1
On Wed, Mar 12, 2025 at 04:56:24PM +0900, Rakie Kim wrote:
> Improper cleanup of sysfs attributes caused kobject and memory leaks when
> initialization failed or nodes were removed.
> 
> This patch ensures proper deallocation of kobjects and memory, preventing
> resource leaks and improving stability.
>

This patch does multiple things, please split these changes into
multiple patches.

> Fixes: dce41f5ae253 ("mm/mempolicy: implement the sysfs-based weighted_interleave interface")
> Signed-off-by: Rakie Kim <rakie.kim@sk.com>
> ---
>  mm/mempolicy.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index bbaadbeeb291..1691748badb2 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -3541,39 +3541,40 @@ 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;
> +		goto node_out;
>  	}

It's not clear to me why you re-ordered these allocations, it seems
superfluous.

>  
>  	err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj,
>  				   "mempolicy");
> -	if (err)
> -		goto node_out;
> +	if (err) {
> +		kobject_put(mempolicy_kobj);

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.

> +		goto err_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;
> +		goto err_out;
>  	}
>  
> -	return err;
> +	return 0;
> +

Please keep functional changes and refactors separate.  There's more
churn in this patch than is needed to accomplish what the changelog
states is the intent.

>  node_out:
>  	kfree(node_attrs);
> -mempol_out:
> -	kfree(mempolicy_kobj);
>  err_out:
> -	pr_err("failed to add mempolicy kobject to the system\n");
> +	pr_err("mempolicy sysfs structure failed to initialize\n");
>  	return err;
> +
>  }
>  
>  late_initcall(mempolicy_sysfs_init);
> 
> base-commit: 80e54e84911a923c40d7bee33a34c1b4be148d7a
> -- 
> 2.34.1
>
Rakie Kim March 13, 2025, 6:31 a.m. UTC | #2
On Wed, 12 Mar 2025 11:49:06 -0400 Gregory Price <gourry@gourry.net> wrote:

Hi Gregory
Thank you for your response regarding this patch.

> On Wed, Mar 12, 2025 at 04:56:24PM +0900, Rakie Kim wrote:
> > Improper cleanup of sysfs attributes caused kobject and memory leaks when
> > initialization failed or nodes were removed.
> > 
> > This patch ensures proper deallocation of kobjects and memory, preventing
> > resource leaks and improving stability.
> >
> 
> This patch does multiple things, please split these changes into
> multiple patches.

This patch should remain as a single patch since all changes address
kobject-related memory issues in mempolicy_sysfs_init(). If you still
believe it should be split, I would appreciate your suggestion on
which parts should be separated.

> 
> > Fixes: dce41f5ae253 ("mm/mempolicy: implement the sysfs-based weighted_interleave interface")
> > Signed-off-by: Rakie Kim <rakie.kim@sk.com>
> > ---
> >  mm/mempolicy.c | 29 +++++++++++++++--------------
> >  1 file changed, 15 insertions(+), 14 deletions(-)
> > 
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index bbaadbeeb291..1691748badb2 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -3541,39 +3541,40 @@ 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;
> > +		goto node_out;
> >  	}
> 
> It's not clear to me why you re-ordered these allocations, it seems
> superfluous.
> 
> >  
> >  	err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj,
> >  				   "mempolicy");
> > -	if (err)
> > -		goto node_out;
> > +	if (err) {
> > +		kobject_put(mempolicy_kobj);
> 
> 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
   increments the kobject's refcount. Therefore, even if
   kobject_init_and_add() fails, kobject_put() must be called to ensure
   proper memory cleanup.

   int kobject_init_and_add(struct kobject *kobj, const struct kobj_type *ktype,
                           struct kobject *parent, const char *fmt, ...)
   {
   ...
       kobject_init(kobj, ktype);
       retval = kobject_add_varg(kobj, parent, fmt, args);
   ...
       return retval;
   }

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);
   }

   Once mempolicy_kobj is passed to kobject_init_and_add(), the memory
   for ngrp->attrs and ngrp should be released via mempolicy_kobj_release().
   The allocation order was changed to ensure that kobject_put() properly
   invokes mempolicy_kobj_release() when required.

> 
> > +		goto err_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;
> > +		goto err_out;
> >  	}
> >  
> > -	return err;
> > +	return 0;
> > +
> 
> Please keep functional changes and refactors separate.  There's more
> churn in this patch than is needed to accomplish what the changelog
> states is the intent.

As mentioned earlier, I believe this patch does not need to be split.
However, if you have further concerns or suggestions, I would
appreciate your input.

> 
> >  node_out:
> >  	kfree(node_attrs);
> > -mempol_out:
> > -	kfree(mempolicy_kobj);
> >  err_out:
> > -	pr_err("failed to add mempolicy kobject to the system\n");
> > +	pr_err("mempolicy sysfs structure failed to initialize\n");
> >  	return err;
> > +
> >  }
> >  
> >  late_initcall(mempolicy_sysfs_init);
> > 
> > base-commit: 80e54e84911a923c40d7bee33a34c1b4be148d7a
> > -- 
> > 2.34.1
> >
Gregory Price March 13, 2025, 3:52 p.m. UTC | #3
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

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;
 }
Rakie Kim March 14, 2025, 7:44 a.m. UTC | #4
On Thu, 13 Mar 2025 11:52:18 -0400 Gregory Price <gourry@gourry.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
> 
> 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
> 

I will make every effort to incorporate your suggestions.
Additionally, I will separate this patch from the current patch series
and create it as an independent patch.

Rakie

> 
> 
> 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;
>  }
>
Jonathan Cameron March 14, 2025, 10:55 a.m. UTC | #5
On Thu, 13 Mar 2025 11:52:18 -0400
Gregory Price <gourry@gourry.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.


> 
> 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;
>  }
> 
>
Gregory Price March 14, 2025, 1:42 p.m. UTC | #6
On Fri, Mar 14, 2025 at 10:55:00AM +0000, Jonathan Cameron wrote:
> > 
> > 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.
>

Even better recommendation, lets do as Jonathan suggests. <3

~Gregory
Rakie Kim March 17, 2025, 8:24 a.m. UTC | #7
On Fri, 14 Mar 2025 09:42:31 -0400 Gregory Price <gourry@gourry.net> wrote:
> On Fri, Mar 14, 2025 at 10:55:00AM +0000, Jonathan Cameron wrote:
> > > 
> > > 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.
> >
> 
> Even better recommendation, lets do as Jonathan suggests. <3
> 
> ~Gregory

Hi Gregory

I will revise the next version based on Jonathan's feedback.
Moreover, I'll separate this patch from the hotplug series and make it an
independent patch.

Rakie
Rakie Kim March 17, 2025, 8:24 a.m. UTC | #8
On Fri, 14 Mar 2025 10:55:00 +0000 Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> On Thu, 13 Mar 2025 11:52:18 -0400
> Gregory Price <gourry@gourry.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;
> >  }
> > 
> > 
>
diff mbox series

Patch

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index bbaadbeeb291..1691748badb2 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -3541,39 +3541,40 @@  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;
+		goto node_out;
 	}
 
 	err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj,
 				   "mempolicy");
-	if (err)
-		goto node_out;
+	if (err) {
+		kobject_put(mempolicy_kobj);
+		goto err_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;
+		goto err_out;
 	}
 
-	return err;
+	return 0;
+
 node_out:
 	kfree(node_attrs);
-mempol_out:
-	kfree(mempolicy_kobj);
 err_out:
-	pr_err("failed to add mempolicy kobject to the system\n");
+	pr_err("mempolicy sysfs structure failed to initialize\n");
 	return err;
+
 }
 
 late_initcall(mempolicy_sysfs_init);