[PATCH] PM: Fix dev_pm_put_subsys_data() to not call kfree() while holding device power lock

[PATCH] PM: Fix dev_pm_put_subsys_data() to not call kfree() while holding device power lock

[Shuah Khan]

dev_pm_put_subsys_data() calls kfree() while holding device power lock, when
the reference count is 0. Fix it to call kfree() after releasing the lock.

Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
---
 drivers/base/power/common.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index 39c3252..da05fe2 100644
--- a/drivers/base/power/common.c
+++ b/drivers/base/power/common.c
@@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
 
 	if (--psd->refcount == 0) {
 		dev->power.subsys_data = NULL;
-		kfree(psd);
 		ret = 1;
 	}
 
  out:
 	spin_unlock_irq(&dev->power.lock);
 
+	if (ret == 1) {
+		/* kfree() verifies that its argument is nonzero. */
+		kfree(psd);
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_put_subsys_data);
-- 
1.7.10.4

Re: [PATCH] PM: Fix dev_pm_put_subsys_data() to not call kfree() while holding device power lock

[Pavel Machek]

Hi!

> dev_pm_put_subsys_data() calls kfree() while holding device power lock, when
> the reference count is 0. Fix it to call kfree() after releasing the lock.
> 
> Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
> ---
>  drivers/base/power/common.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
w> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> index 39c3252..da05fe2 100644
> --- a/drivers/base/power/common.c
> +++ b/drivers/base/power/common.c
> @@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
>  
>  	if (--psd->refcount == 0) {
>  		dev->power.subsys_data = NULL;
> -		kfree(psd);
>  		ret = 1;
>  	}
>  
>   out:
>  	spin_unlock_irq(&dev->power.lock);
>  
> +	if (ret == 1) {
> +		/* kfree() verifies that its argument is nonzero. */
> +		kfree(psd);
> +	}
> +
>  	return ret;
>  }

Wow, that function is fragile. It returns 0/1/-EINVAL, while being
documented for 0/1...

Patch does not look obviously wrong, but maybe 

@@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
 
 	if (--psd->refcount == 0) {
 		dev->power.subsys_data = NULL;
+		spin_unlock_irq(&dev->power.lock);
-		kfree(psd);
- 		ret = 1;
+		return 1;
 	}

Would be cleaner.
									Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] PM: Fix dev_pm_put_subsys_data() to not call kfree() while holding device power lock

[Rafael J. Wysocki]

On Saturday, May 04, 2013 02:51:16 PM Pavel Machek wrote:
> Hi!
> 
> > dev_pm_put_subsys_data() calls kfree() while holding device power lock, when
> > the reference count is 0. Fix it to call kfree() after releasing the lock.
> > 
> > Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
> > ---
> >  drivers/base/power/common.c |    6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> w> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> > index 39c3252..da05fe2 100644
> > --- a/drivers/base/power/common.c
> > +++ b/drivers/base/power/common.c
> > @@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
> >  
> >  	if (--psd->refcount == 0) {
> >  		dev->power.subsys_data = NULL;
> > -		kfree(psd);
> >  		ret = 1;
> >  	}
> >  
> >   out:
> >  	spin_unlock_irq(&dev->power.lock);
> >  
> > +	if (ret == 1) {
> > +		/* kfree() verifies that its argument is nonzero. */
> > +		kfree(psd);
> > +	}
> > +
> >  	return ret;
> >  }
> 
> Wow, that function is fragile. It returns 0/1/-EINVAL, while being
> documented for 0/1...

Oh, it generally should return 1 for !psd.

> Patch does not look obviously wrong, but maybe 
> 
> @@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
>  
>  	if (--psd->refcount == 0) {
>  		dev->power.subsys_data = NULL;
> +		spin_unlock_irq(&dev->power.lock);
> -		kfree(psd);
> - 		ret = 1;
> +		return 1;
>  	}
> 
> Would be cleaner.

What about this:

---
 drivers/base/power/common.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/base/power/common.c
===================================================================
--- linux-pm.orig/drivers/base/power/common.c
+++ linux-pm/drivers/base/power/common.c
@@ -61,24 +61,24 @@ EXPORT_SYMBOL_GPL(dev_pm_get_subsys_data
 int dev_pm_put_subsys_data(struct device *dev)
 {
 	struct pm_subsys_data *psd;
-	int ret = 0;
+	int ret = 1;
 
 	spin_lock_irq(&dev->power.lock);
 
 	psd = dev_to_psd(dev);
-	if (!psd) {
-		ret = -EINVAL;
+	if (!psd)
 		goto out;
-	}
 
 	if (--psd->refcount == 0) {
 		dev->power.subsys_data = NULL;
-		kfree(psd);
-		ret = 1;
+	} else {
+		psd = NULL;
+		ret = 0;
 	}
 
  out:
 	spin_unlock_irq(&dev->power.lock);
+	kfree(psd);
 
 	return ret;
 }


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] PM: Fix dev_pm_put_subsys_data() to not call kfree() while holding device power lock

[Pavel Machek]

Hi!

> > Wow, that function is fragile. It returns 0/1/-EINVAL, while being
> > documented for 0/1...
> 
> Oh, it generally should return 1 for !psd.
> 
> > Patch does not look obviously wrong, but maybe 
> > 
> > @@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
> >  
> >  	if (--psd->refcount == 0) {
> >  		dev->power.subsys_data = NULL;
> > +		spin_unlock_irq(&dev->power.lock);
> > -		kfree(psd);
> > - 		ret = 1;
> > +		return 1;
> >  	}
> > 
> > Would be cleaner.
> 
> What about this:

Looks good to me.

Reviewed-by: Pavel Machek <pavel@ucw.cz>

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] PM: Fix dev_pm_put_subsys_data() to not call kfree() while holding device power lock

[Shuah Khan]

On Mon, 2013-05-06 at 14:09 +0200, Pavel Machek wrote:
> Hi!
> 
> > > Wow, that function is fragile. It returns 0/1/-EINVAL, while being
> > > documented for 0/1...
> > 
> > Oh, it generally should return 1 for !psd.
> > 
> > > Patch does not look obviously wrong, but maybe 
> > > 
> > > @@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
> > >  
> > >  	if (--psd->refcount == 0) {
> > >  		dev->power.subsys_data = NULL;
> > > +		spin_unlock_irq(&dev->power.lock);
> > > -		kfree(psd);
> > > - 		ret = 1;
> > > +		return 1;
> > >  	}
> > > 
> > > Would be cleaner.
> > 
> > What about this:
> 
> Looks good to me.
> 
> Reviewed-by: Pavel Machek <pavel@ucw.cz>

Rafael/Pavel,

I redid the patch based on Pavel's comments and just about to send it
and then I saw your exchange. This version looks good to me. Do you want
me to test the patch and resend?

thanks,
-- Shuah

Re: [PATCH] PM: Fix dev_pm_put_subsys_data() to not call kfree() while holding device power lock

[Shuah Khan]

On Mon, 2013-05-06 at 08:00 -0600, Shuah Khan wrote:
> On Mon, 2013-05-06 at 14:09 +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > > Wow, that function is fragile. It returns 0/1/-EINVAL, while being
> > > > documented for 0/1...
> > > 
> > > Oh, it generally should return 1 for !psd.
> > > 
> > > > Patch does not look obviously wrong, but maybe 
> > > > 
> > > > @@ -73,13 +73,17 @@ int dev_pm_put_subsys_data(struct device *dev)
> > > >  
> > > >  	if (--psd->refcount == 0) {
> > > >  		dev->power.subsys_data = NULL;
> > > > +		spin_unlock_irq(&dev->power.lock);
> > > > -		kfree(psd);
> > > > - 		ret = 1;
> > > > +		return 1;
> > > >  	}
> > > > 
> > > > Would be cleaner.
> > > 
> > > What about this:
> > 
> > Looks good to me.
> > 
> > Reviewed-by: Pavel Machek <pavel@ucw.cz>
> 
> Rafael/Pavel,
> 
> I redid the patch based on Pavel's comments and just about to send it
> and then I saw your exchange. This version looks good to me. Do you want
> me to test the patch and resend?
> 
> thanks,
> -- Shuah

I sent v2 patch that incorporates the comments from Rafael and Pavel.

thanks,

-- Shuah

Shuah Khan Lead Kernel Developer - Open Source Group 
Samsung Research America (Silicon Valley)
shuah.kh@samsung.com | (970) 672-0658

[PATCH v2] PM: Fix dev_pm_put_subsys_data() to not call kfree() while holding device power lock

[Shuah Khan]

dev_pm_put_subsys_data() calls kfree() while holding device power lock, when
the reference count is 0. Fix it to call kfree() after releasing the lock.

Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Rafael Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/common.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index 39c3252..e5b99f7 100644
--- a/drivers/base/power/common.c
+++ b/drivers/base/power/common.c
@@ -61,24 +61,26 @@ EXPORT_SYMBOL_GPL(dev_pm_get_subsys_data);
 int dev_pm_put_subsys_data(struct device *dev)
 {
 	struct pm_subsys_data *psd;
-	int ret = 0;
+	int ret = 1;
 
 	spin_lock_irq(&dev->power.lock);
 
 	psd = dev_to_psd(dev);
 	if (!psd) {
-		ret = -EINVAL;
 		goto out;
 	}
 
 	if (--psd->refcount == 0) {
 		dev->power.subsys_data = NULL;
-		kfree(psd);
 		ret = 1;
+	} else {
+		psd = NULL;
+		ret = 0;
 	}
 
  out:
 	spin_unlock_irq(&dev->power.lock);
+	kfree(psd);
 
 	return ret;
 }
-- 
1.7.10.4

Re: [PATCH v2] PM: Fix dev_pm_put_subsys_data() to not call kfree() while holding device power lock

[gregkh]

On Mon, May 06, 2013 at 07:04:53PM +0000, Shuah Khan wrote:
> dev_pm_put_subsys_data() calls kfree() while holding device power lock, when
> the reference count is 0. Fix it to call kfree() after releasing the lock.
> 
> Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Rafael Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/common.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Is this causing a problem now, and it should go into 3.10 and earlier
kernels (if so, which ones?), or can it just wait until 3.11 as it's
just a cleanup fix?

thanks,

greg k-h

Re: [PATCH v2] PM: Fix dev_pm_put_subsys_data() to not call kfree() while holding device power lock

[Shuah Khan]

On Mon, 2013-05-06 at 12:41 -0700, gregkh@linuxfoundation.org wrote:
> On Mon, May 06, 2013 at 07:04:53PM +0000, Shuah Khan wrote:
> > dev_pm_put_subsys_data() calls kfree() while holding device power lock, when
> > the reference count is 0. Fix it to call kfree() after releasing the lock.
> > 
> > Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
> > Reviewed-by: Pavel Machek <pavel@ucw.cz>
> > Reviewed-by: Rafael Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/common.c |    8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> Is this causing a problem now, and it should go into 3.10 and earlier
> kernels (if so, which ones?), or can it just wait until 3.11 as it's
> just a cleanup fix?

It can wait until 3.11. At this point I don't have any evidence that
this is actually causing problems. So please treat this as a cleanup fix
found during code review.

thanks,
-- Shuah

-- 
Shuah Khan Lead Kernel Developer - Open Source Group 
Samsung Research America (Silicon Valley)
shuah.kh@samsung.com | (970) 672-0658

Re: [PATCH v2] PM: Fix dev_pm_put_subsys_data() to not call kfree() while holding device power lock

[Rafael J. Wysocki]

On Monday, May 06, 2013 12:41:19 PM gregkh@linuxfoundation.org wrote:
> On Mon, May 06, 2013 at 07:04:53PM +0000, Shuah Khan wrote:
> > dev_pm_put_subsys_data() calls kfree() while holding device power lock, when
> > the reference count is 0. Fix it to call kfree() after releasing the lock.
> > 
> > Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
> > Reviewed-by: Pavel Machek <pavel@ucw.cz>
> > Reviewed-by: Rafael Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/common.c |    8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> Is this causing a problem now, and it should go into 3.10 and earlier
> kernels (if so, which ones?), or can it just wait until 3.11 as it's
> just a cleanup fix?

I think it's OK to push the fix for 3.10, but I'll take care of this (I have
a bunch of fixes for 3.10 anyway, but I'll send a pull request after -rc1,
because people are sending me fixes pretty much continuously ATM).

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2] PM: Fix dev_pm_put_subsys_data() to not call kfree() while holding device power lock

[gregkh]

On Mon, May 06, 2013 at 09:56:56PM +0200, Rafael J. Wysocki wrote:
> On Monday, May 06, 2013 12:41:19 PM gregkh@linuxfoundation.org wrote:
> > On Mon, May 06, 2013 at 07:04:53PM +0000, Shuah Khan wrote:
> > > dev_pm_put_subsys_data() calls kfree() while holding device power lock, when
> > > the reference count is 0. Fix it to call kfree() after releasing the lock.
> > > 
> > > Signed-off-by: Shuah Khan <shuah.kh@samsung.com>
> > > Reviewed-by: Pavel Machek <pavel@ucw.cz>
> > > Reviewed-by: Rafael Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/base/power/common.c |    8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > Is this causing a problem now, and it should go into 3.10 and earlier
> > kernels (if so, which ones?), or can it just wait until 3.11 as it's
> > just a cleanup fix?
> 
> I think it's OK to push the fix for 3.10, but I'll take care of this (I have
> a bunch of fixes for 3.10 anyway, but I'll send a pull request after -rc1,
> because people are sending me fixes pretty much continuously ATM).

Ok, feel free to add:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>