[PATCH]SCSI:Refine the way to fix NULL pointer dereference in runtime PM

[PATCH]SCSI:Refine the way to fix NULL pointer dereference in runtime PM

[Ken Xue]

>From 844ebfcecad7ddaf7206e0474c600b0146b4ef21 Mon Sep 17 00:00:00 2001
From: Ken Xue <Ken.Xue@amd.com>
Date: Wed, 9 Sep 2015 14:55:21 +0800
Subject: [PATCH] SCSI:Refine the way to fix NULL pointer dereference in
 runtime PM

There was a patch about Bugzilla #101371. That patch introduced a
possibility that can not call blk_{pre|post}_runtime_suspend and
blk_{pre|post}_runtime_resume in pairs.

Signed-off-by: Ken Xue <Ken.Xue@amd.com>
---
 block/blk-core.c       | 12 ++++++++++++
 drivers/scsi/scsi_pm.c | 20 ++++++++++----------
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 60912e9..a07ab18 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -3280,6 +3280,9 @@ int blk_pre_runtime_suspend(struct request_queue *q)
 {
 	int ret = 0;
 
+	if (!q->dev)
+		return ret;
+
 	spin_lock_irq(q->queue_lock);
 	if (q->nr_pending) {
 		ret = -EBUSY;
@@ -3307,6 +3310,9 @@ EXPORT_SYMBOL(blk_pre_runtime_suspend);
  */
 void blk_post_runtime_suspend(struct request_queue *q, int err)
 {
+	if (!q->dev)
+		return;
+
 	spin_lock_irq(q->queue_lock);
 	if (!err) {
 		q->rpm_status = RPM_SUSPENDED;
@@ -3331,6 +3337,9 @@ EXPORT_SYMBOL(blk_post_runtime_suspend);
  */
 void blk_pre_runtime_resume(struct request_queue *q)
 {
+	if (!q->dev)
+		return;
+
 	spin_lock_irq(q->queue_lock);
 	q->rpm_status = RPM_RESUMING;
 	spin_unlock_irq(q->queue_lock);
@@ -3353,6 +3362,9 @@ EXPORT_SYMBOL(blk_pre_runtime_resume);
  */
 void blk_post_runtime_resume(struct request_queue *q, int err)
 {
+	if (!q->dev)
+		return;
+
 	spin_lock_irq(q->queue_lock);
 	if (!err) {
 		q->rpm_status = RPM_ACTIVE;
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index e4b7998..459abe1 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -219,13 +219,13 @@ static int sdev_runtime_suspend(struct device *dev)
 	struct scsi_device *sdev = to_scsi_device(dev);
 	int err = 0;
 
-	if (pm && pm->runtime_suspend) {
-		err = blk_pre_runtime_suspend(sdev->request_queue);
-		if (err)
-			return err;
+	err = blk_pre_runtime_suspend(sdev->request_queue);
+	if (err)
+		return err;
+	if (pm && pm->runtime_suspend)
 		err = pm->runtime_suspend(dev);
-		blk_post_runtime_suspend(sdev->request_queue, err);
-	}
+	blk_post_runtime_suspend(sdev->request_queue, err);
+
 	return err;
 }
 
@@ -248,11 +248,11 @@ static int sdev_runtime_resume(struct device *dev)
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int err = 0;
 
-	if (pm && pm->runtime_resume) {
-		blk_pre_runtime_resume(sdev->request_queue);
+	blk_pre_runtime_resume(sdev->request_queue);
+	if (pm && pm->runtime_resume)
 		err = pm->runtime_resume(dev);
-		blk_post_runtime_resume(sdev->request_queue, err);
-	}
+	blk_post_runtime_resume(sdev->request_queue, err);
+
 	return err;
 }
 
-- 
1.9.1

Re: [PATCH]SCSI:Refine the way to fix NULL pointer dereference in runtime PM

[Huang Rui]

On Wed, Sep 09, 2015 at 03:02:11PM +0800, Ken Xue wrote:
> From 844ebfcecad7ddaf7206e0474c600b0146b4ef21 Mon Sep 17 00:00:00 2001
> From: Ken Xue <Ken.Xue@amd.com>
> Date: Wed, 9 Sep 2015 14:55:21 +0800
> Subject: [PATCH] SCSI:Refine the way to fix NULL pointer dereference in
>  runtime PM
> 
> There was a patch about Bugzilla #101371.

Which Bugzilla? I guess you should put a link or oops message of NULL
pointer dereference here. Then other guys can know detail infomation
of this fix.

Thanks,
Rui

> That patch introduced a possibility that can not call
> blk_{pre|post}_runtime_suspend and blk_{pre|post}_runtime_resume in
> pairs.
> 
> Signed-off-by: Ken Xue <Ken.Xue@amd.com>
> ---
>  block/blk-core.c       | 12 ++++++++++++
>  drivers/scsi/scsi_pm.c | 20 ++++++++++----------
>  2 files changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 60912e9..a07ab18 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -3280,6 +3280,9 @@ int blk_pre_runtime_suspend(struct request_queue *q)
>  {
>  	int ret = 0;
>  
> +	if (!q->dev)
> +		return ret;
> +
>  	spin_lock_irq(q->queue_lock);
>  	if (q->nr_pending) {
>  		ret = -EBUSY;
> @@ -3307,6 +3310,9 @@ EXPORT_SYMBOL(blk_pre_runtime_suspend);
>   */
>  void blk_post_runtime_suspend(struct request_queue *q, int err)
>  {
> +	if (!q->dev)
> +		return;
> +
>  	spin_lock_irq(q->queue_lock);
>  	if (!err) {
>  		q->rpm_status = RPM_SUSPENDED;
> @@ -3331,6 +3337,9 @@ EXPORT_SYMBOL(blk_post_runtime_suspend);
>   */
>  void blk_pre_runtime_resume(struct request_queue *q)
>  {
> +	if (!q->dev)
> +		return;
> +
>  	spin_lock_irq(q->queue_lock);
>  	q->rpm_status = RPM_RESUMING;
>  	spin_unlock_irq(q->queue_lock);
> @@ -3353,6 +3362,9 @@ EXPORT_SYMBOL(blk_pre_runtime_resume);
>   */
>  void blk_post_runtime_resume(struct request_queue *q, int err)
>  {
> +	if (!q->dev)
> +		return;
> +
>  	spin_lock_irq(q->queue_lock);
>  	if (!err) {
>  		q->rpm_status = RPM_ACTIVE;
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index e4b7998..459abe1 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -219,13 +219,13 @@ static int sdev_runtime_suspend(struct device *dev)
>  	struct scsi_device *sdev = to_scsi_device(dev);
>  	int err = 0;
>  
> -	if (pm && pm->runtime_suspend) {
> -		err = blk_pre_runtime_suspend(sdev->request_queue);
> -		if (err)
> -			return err;
> +	err = blk_pre_runtime_suspend(sdev->request_queue);
> +	if (err)
> +		return err;
> +	if (pm && pm->runtime_suspend)
>  		err = pm->runtime_suspend(dev);
> -		blk_post_runtime_suspend(sdev->request_queue, err);
> -	}
> +	blk_post_runtime_suspend(sdev->request_queue, err);
> +
>  	return err;
>  }
>  
> @@ -248,11 +248,11 @@ static int sdev_runtime_resume(struct device *dev)
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  	int err = 0;
>  
> -	if (pm && pm->runtime_resume) {
> -		blk_pre_runtime_resume(sdev->request_queue);
> +	blk_pre_runtime_resume(sdev->request_queue);
> +	if (pm && pm->runtime_resume)
>  		err = pm->runtime_resume(dev);
> -		blk_post_runtime_resume(sdev->request_queue, err);
> -	}
> +	blk_post_runtime_resume(sdev->request_queue, err);
> +
>  	return err;
>  }
>  
> -- 
> 1.9.1
> 
> 
> 

Re: [PATCH]SCSI:Refine the way to fix NULL pointer dereference in runtime PM

[Ken Xue]

On Wed, 2015-09-09 at 16:30 +0800, Huang Rui wrote:
> On Wed, Sep 09, 2015 at 03:02:11PM +0800, Ken Xue wrote:
> > From 844ebfcecad7ddaf7206e0474c600b0146b4ef21 Mon Sep 17 00:00:00 2001
> > From: Ken Xue <Ken.Xue@amd.com>
> > Date: Wed, 9 Sep 2015 14:55:21 +0800
> > Subject: [PATCH] SCSI:Refine the way to fix NULL pointer dereference in
> >  runtime PM
> > 
> > There was a patch about Bugzilla #101371.
> 
> Which Bugzilla? I guess you should put a link or oops message of NULL
> pointer dereference here. Then other guys can know detail infomation
> of this fix.
> 
You can find out previous patch with commit ID 49718f.
You also can find log with below link
https://bugzilla.kernel.org/show_bug.cgi?id=101371

Re: [PATCH]SCSI:Refine the way to fix NULL pointer dereference in runtime PM

[Huang Rui]

On Wed, Sep 09, 2015 at 04:40:07PM +0800, Ken Xue wrote:
> On Wed, 2015-09-09 at 16:30 +0800, Huang Rui wrote:
> > On Wed, Sep 09, 2015 at 03:02:11PM +0800, Ken Xue wrote:
> > > From 844ebfcecad7ddaf7206e0474c600b0146b4ef21 Mon Sep 17 00:00:00 2001
> > > From: Ken Xue <Ken.Xue@amd.com>
> > > Date: Wed, 9 Sep 2015 14:55:21 +0800
> > > Subject: [PATCH] SCSI:Refine the way to fix NULL pointer dereference in
> > >  runtime PM
> > > 
> > > There was a patch about Bugzilla #101371.
> > 
> > Which Bugzilla? I guess you should put a link or oops message of NULL
> > pointer dereference here. Then other guys can know detail infomation
> > of this fix.
> > 
> You can find out previous patch with commit ID 49718f.
> You also can find log with below link
> https://bugzilla.kernel.org/show_bug.cgi?id=101371
> 

I see. So maybe we would better mark the commit ID. :)

Alan, could you please take a look?

Thanks,
Rui

Re: [PATCH]SCSI:Refine the way to fix NULL pointer dereference in runtime PM

[Alan Stern]

On Wed, 9 Sep 2015, Huang Rui wrote:

> On Wed, Sep 09, 2015 at 04:40:07PM +0800, Ken Xue wrote:
> > On Wed, 2015-09-09 at 16:30 +0800, Huang Rui wrote:
> > > On Wed, Sep 09, 2015 at 03:02:11PM +0800, Ken Xue wrote:
> > > > From 844ebfcecad7ddaf7206e0474c600b0146b4ef21 Mon Sep 17 00:00:00 2001
> > > > From: Ken Xue <Ken.Xue@amd.com>
> > > > Date: Wed, 9 Sep 2015 14:55:21 +0800
> > > > Subject: [PATCH] SCSI:Refine the way to fix NULL pointer dereference in
> > > >  runtime PM
> > > > 
> > > > There was a patch about Bugzilla #101371.
> > > 
> > > Which Bugzilla? I guess you should put a link or oops message of NULL
> > > pointer dereference here. Then other guys can know detail infomation
> > > of this fix.
> > > 
> > You can find out previous patch with commit ID 49718f.
> > You also can find log with below link
> > https://bugzilla.kernel.org/show_bug.cgi?id=101371
> > 
> 
> I see. So maybe we would better mark the commit ID. :)
> 
> Alan, could you please take a look?

Yes.

Ken, your fix was good but the description was bad.  The description 
did not explain what the problem was or how you were going to fix it.

In fact, this change should be split up into two patches.  The first 
patch should simply revert commit 49718f0fb8c9 ("SCSI: Fix NULL pointer 
dereference in runtime PM") and the description should explain why that 
commit didn't work properly for the sr driver.

The second patch should make the new changes to the block core.  The 
description should explain why the changes are needed, and it can 
reference Bugzilla #101371.

Both patches should be tagged for the -stable kernels.  When you have 
done all that, you can add:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

to both patches.

Alan Stern

[PATCH 1/2]Revert "SCSI: Fix NULL pointer dereference in runtime PM"

[Ken Xue]

Revert "SCSI: Fix NULL pointer dereference in runtime PM"

This reverts commit 49718f0fb8c9 ("SCSI: Fix NULL pointer dereference in
runtime PM")

The old commit may lead to a issue that blk_{pre|post}_runtime_suspend and
blk_{pre|post}_runtime_resume can not be called in pairs.

Take sr device as example, when sr device goes to runtime suspend, 
blk_{pre|post}_runtime_suspend will be called since sr device defined
pm->runtime_suspend. But blk_{pre|post}_runtime_resume will not be called
since sr device doesn't have pm->runtime_resume. Then sr device can not
resume correctly anymore.

Signed-off-by: Ken Xue <Ken.Xue@amd.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Cc: stable@vger.kernel.org

---
 drivers/scsi/scsi_pm.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index e4b7998..459abe1 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -219,13 +219,13 @@ static int sdev_runtime_suspend(struct device *dev)
 	struct scsi_device *sdev = to_scsi_device(dev);
 	int err = 0;
 
-	if (pm && pm->runtime_suspend) {
-		err = blk_pre_runtime_suspend(sdev->request_queue);
-		if (err)
-			return err;
+	err = blk_pre_runtime_suspend(sdev->request_queue);
+	if (err)
+		return err;
+	if (pm && pm->runtime_suspend)
 		err = pm->runtime_suspend(dev);
-		blk_post_runtime_suspend(sdev->request_queue, err);
-	}
+	blk_post_runtime_suspend(sdev->request_queue, err);
+
 	return err;
 }
 
@@ -248,11 +248,11 @@ static int sdev_runtime_resume(struct device *dev)
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int err = 0;
 
-	if (pm && pm->runtime_resume) {
-		blk_pre_runtime_resume(sdev->request_queue);
+	blk_pre_runtime_resume(sdev->request_queue);
+	if (pm && pm->runtime_resume)
 		err = pm->runtime_resume(dev);
-		blk_post_runtime_resume(sdev->request_queue, err);
-	}
+	blk_post_runtime_resume(sdev->request_queue, err);
+
 	return err;
 }
 
-- 
1.9.1

Re: [PATCH 1/2]Revert "SCSI: Fix NULL pointer dereference in runtime PM"

[Xue, Ken]

On 四, 2015-09-10 at 10:23 +0800, Ken Xue wrote:

Can someone help to apply this patch series? Thanks.
http://marc.info/?l=linux-scsi&m=144185206825609&w=2
http://marc.info/?l=linux-scsi&m=144185208525611&w=2 

> Revert "SCSI: Fix NULL pointer dereference in runtime PM"
> 
> This reverts commit 49718f0fb8c9 ("SCSI: Fix NULL pointer dereference
> in
> runtime PM")
> 
> The old commit may lead to a issue that
> blk_{pre|post}_runtime_suspend and
> blk_{pre|post}_runtime_resume can not be called in pairs.
> 
> Take sr device as example, when sr device goes to runtime suspend, 
> blk_{pre|post}_runtime_suspend will be called since sr device defined
> pm->runtime_suspend. But blk_{pre|post}_runtime_resume will not be
> called
> since sr device doesn't have pm->runtime_resume. Then sr device can
> not
> resume correctly anymore.
> 
> Signed-off-by: Ken Xue <Ken.Xue@amd.com>
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> Cc: stable@vger.kernel.org
> 
> ---
>  drivers/scsi/scsi_pm.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index e4b7998..459abe1 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -219,13 +219,13 @@ static int sdev_runtime_suspend(struct device
> *dev)
>  	struct scsi_device *sdev = to_scsi_device(dev);
>  	int err = 0;
>  
> -	if (pm && pm->runtime_suspend) {
> -		err = blk_pre_runtime_suspend(sdev->request_queue);
> -		if (err)
> -			return err;
> +	err = blk_pre_runtime_suspend(sdev->request_queue);
> +	if (err)
> +		return err;
> +	if (pm && pm->runtime_suspend)
>  		err = pm->runtime_suspend(dev);
> -		blk_post_runtime_suspend(sdev->request_queue, err);
> -	}
> +	blk_post_runtime_suspend(sdev->request_queue, err);
> +
>  	return err;
>  }
>  
> @@ -248,11 +248,11 @@ static int sdev_runtime_resume(struct device
> *dev)
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm
> : NULL;
>  	int err = 0;
>  
> -	if (pm && pm->runtime_resume) {
> -		blk_pre_runtime_resume(sdev->request_queue);
> +	blk_pre_runtime_resume(sdev->request_queue);
> +	if (pm && pm->runtime_resume)
>  		err = pm->runtime_resume(dev);
> -		blk_post_runtime_resume(sdev->request_queue, err);
> -	}
> +	blk_post_runtime_resume(sdev->request_queue, err);
> +
>  	return err;
>  }
>  

[PATCH 1/2]Revert "SCSI: Fix NULL pointer dereference in runtime PM"

[Ken Xue]

This reverts commit 49718f0fb8c9 ("SCSI: Fix NULL pointer dereference in
runtime PM")

The old commit may lead to a issue that blk_{pre|post}_runtime_suspend and
blk_{pre|post}_runtime_resume may not be called in pairs.

Take sr device as example, when sr device goes to runtime suspend, 
blk_{pre|post}_runtime_suspend will be called since sr device defined
pm->runtime_suspend. But blk_{pre|post}_runtime_resume will not be called
since sr device doesn't have pm->runtime_resume. so, sr device can not
resume correctly anymore.

More discussion can be found from below link.
http://marc.info/?l=linux-scsi&m=144163730531875&w=2

Signed-off-by: Ken Xue <Ken.Xue@amd.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Cc: Xiangliang Yu <Xiangliang.Yu@amd.com>
Cc: James E.J. Bottomley <JBottomley@odin.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Michael Terry <Michael.terry@canonical.com>
Cc: stable@vger.kernel.org

---
 drivers/scsi/scsi_pm.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index e4b7998..459abe1 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -219,13 +219,13 @@ static int sdev_runtime_suspend(struct device *dev)
 	struct scsi_device *sdev = to_scsi_device(dev);
 	int err = 0;
 
-	if (pm && pm->runtime_suspend) {
-		err = blk_pre_runtime_suspend(sdev->request_queue);
-		if (err)
-			return err;
+	err = blk_pre_runtime_suspend(sdev->request_queue);
+	if (err)
+		return err;
+	if (pm && pm->runtime_suspend)
 		err = pm->runtime_suspend(dev);
-		blk_post_runtime_suspend(sdev->request_queue, err);
-	}
+	blk_post_runtime_suspend(sdev->request_queue, err);
+
 	return err;
 }
 
@@ -248,11 +248,11 @@ static int sdev_runtime_resume(struct device *dev)
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int err = 0;
 
-	if (pm && pm->runtime_resume) {
-		blk_pre_runtime_resume(sdev->request_queue);
+	blk_pre_runtime_resume(sdev->request_queue);
+	if (pm && pm->runtime_resume)
 		err = pm->runtime_resume(dev);
-		blk_post_runtime_resume(sdev->request_queue, err);
-	}
+	blk_post_runtime_resume(sdev->request_queue, err);
+
 	return err;
 }
 
-- 
1.9.1