Re: [PATCH] SCSI: Fix NULL pointer dereference in runtime PM

Re: [PATCH] SCSI: Fix NULL pointer dereference in runtime PM

[Alan Stern]

On Mon, 7 Sep 2015, Xue, Ken wrote:

> Hi Alan and James,
> I found a bug about sr device runtime suspend & resume which is introduced by your patch about fixing NULL pointer dereference in runtime PM.
> You know that sr device only has runtime suspend routine but doesn't have resume routine.
> After your patch, blk_ *_ runtime_suspend  will be called when runtime suspend. But blk_ *_ runtime_resume cannot be called when resume.
> So, sr device cannot work correctly after 1st runtime suspend.

Argh.  Things just get more and more complicated...

> I want to make a dummy runtime resume routine in sr.c for fixing this runtime issue of sr device.
> If you guys think a dummy runtime routine for resume is acceptable, I can submit patch later.

A dummy routine isn't the greatest solution.  Eventually someone will 
see it, wonder why it's there, and remove it.

The real issue we need to address here is whether the driver has asked
for request-queue-oriented runtime PM by calling blk_pm_runtime_init().  
If it hasn't then we need to skip at least the calls to
blk_{pre|post}_runtime_{suspend|resume}.

The patch I wrote uses the existence of runtime-PM callbacks as an 
indicator for this, but evidently it isn't adequate.  A more direct 
approach would be to test whether sdev->request_queue->dev is non-NULL, 
but this would be a layering violation.

Should we add a SCSI-level flag to indicate that blk_pm_runtime_init() 
has been called?

Alan Stern

Re: [PATCH] SCSI: Fix NULL pointer dereference in runtime PM

[Ken Xue]

On Mon, 2015-09-07 at 10:48 -0400, Alan Stern wrote:
> On Mon, 7 Sep 2015, Xue, Ken wrote:
> 
> > Hi Alan and James,
> > I found a bug about sr device runtime suspend & resume which is introduced by your patch about fixing NULL pointer dereference in runtime PM.
> > You know that sr device only has runtime suspend routine but doesn't have resume routine.
> > After your patch, blk_ *_ runtime_suspend  will be called when runtime suspend. But blk_ *_ runtime_resume cannot be called when resume.
> > So, sr device cannot work correctly after 1st runtime suspend.
> 
> Argh.  Things just get more and more complicated...
> 
> > I want to make a dummy runtime resume routine in sr.c for fixing this runtime issue of sr device.
> > If you guys think a dummy runtime routine for resume is acceptable, I can submit patch later.
> 
> A dummy routine isn't the greatest solution.  Eventually someone will 
> see it, wonder why it's there, and remove it.
> 
> The real issue we need to address here is whether the driver has asked
> for request-queue-oriented runtime PM by calling blk_pm_runtime_init().  
> If it hasn't then we need to skip at least the calls to
> blk_{pre|post}_runtime_{suspend|resume}.
> 
> The patch I wrote uses the existence of runtime-PM callbacks as an 
> indicator for this, but evidently it isn't adequate.  A more direct 
> approach would be to test whether sdev->request_queue->dev is non-NULL, 
> but this would be a layering violation.
> 
> Should we add a SCSI-level flag to indicate that blk_pm_runtime_init() 
> has been called?
 Can we check whether sdev->request_queue->dev is non-NULL in blk_{pre|
post}_runtime_{suspend|resume}?

just use simple codes like:
int blk_pre_runtime_suspend(struct request_queue *q)
{
...
	if(!q->dev)
	  return -ENODEV;
..
}

> 
> Alan Stern
> 

Re: [PATCH] SCSI: Fix NULL pointer dereference in runtime PM

[Alan Stern]

On Tue, 8 Sep 2015, Ken Xue wrote:

> On Mon, 2015-09-07 at 10:48 -0400, Alan Stern wrote:
> > On Mon, 7 Sep 2015, Xue, Ken wrote:
> > 
> > > Hi Alan and James,
> > > I found a bug about sr device runtime suspend & resume which is introduced by your patch about fixing NULL pointer dereference in runtime PM.
> > > You know that sr device only has runtime suspend routine but doesn't have resume routine.
> > > After your patch, blk_ *_ runtime_suspend  will be called when runtime suspend. But blk_ *_ runtime_resume cannot be called when resume.
> > > So, sr device cannot work correctly after 1st runtime suspend.
> > 
> > Argh.  Things just get more and more complicated...
> > 
> > > I want to make a dummy runtime resume routine in sr.c for fixing this runtime issue of sr device.
> > > If you guys think a dummy runtime routine for resume is acceptable, I can submit patch later.
> > 
> > A dummy routine isn't the greatest solution.  Eventually someone will 
> > see it, wonder why it's there, and remove it.
> > 
> > The real issue we need to address here is whether the driver has asked
> > for request-queue-oriented runtime PM by calling blk_pm_runtime_init().  
> > If it hasn't then we need to skip at least the calls to
> > blk_{pre|post}_runtime_{suspend|resume}.
> > 
> > The patch I wrote uses the existence of runtime-PM callbacks as an 
> > indicator for this, but evidently it isn't adequate.  A more direct 
> > approach would be to test whether sdev->request_queue->dev is non-NULL, 
> > but this would be a layering violation.
> > 
> > Should we add a SCSI-level flag to indicate that blk_pm_runtime_init() 
> > has been called?
>  Can we check whether sdev->request_queue->dev is non-NULL in blk_{pre|
> post}_runtime_{suspend|resume}?
> 
> just use simple codes like:
> int blk_pre_runtime_suspend(struct request_queue *q)
> {
> ...
> 	if(!q->dev)

Missing space between "if" and "(".  :-)

> 	  return -ENODEV;

Return ret (i.e., 0), not -ENODEV.  If, for some reason, a device uses
runtime PM that's not based on the state of the request queue, we don't
want to subvert that other mechanism.

> ..
> }

I agree, this appears to be the best solution.  (That, plus reverting
my earlier patch.)  I originally avoided putting in this test because 
it seemed like unnecessary overhead.  Now we see that the overhead is 
necessary after all.

Alan Stern

Re: [PATCH] SCSI: Fix NULL pointer dereference in runtime PM

[Alan Stern]

On Mon, 17 Aug 2015, Alan Stern wrote:

> The routines in scsi_rpm.c assume that if a runtime-PM callback is
> invoked for a SCSI device, it can only mean that the device's driver 
> has asked the block layer to handle the runtime power management (by
> calling blk_pm_runtime_init(), which among other things sets q->dev).
> 
> However, this assumption turns out to be wrong for things like the ses
> driver.  Normally ses devices are not allowed to do runtime PM, but
> userspace can override this setting.  If this happens, the kernel gets
> a NULL pointer dereference when blk_post_runtime_resume() tries to use
> the uninitialized q->dev pointer.
> 
> This patch fixes the problem by calling the block layer's runtime-PM
> routines only if the device's driver really does have a runtime-PM
> callback routine.  Since ses doesn't define any such callbacks, the
> crash won't occur.
> 
> This fixes Bugzilla #101371.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Reported-by: Stanisław Pitucha <viraptor@gmail.com>
> Reported-by: Ilan Cohen <ilanco@gmail.com>
> Tested-by: Ilan Cohen <ilanco@gmail.com>

James:

I forgot to include a

CC: <stable@vger.kernel.org>

tag.  Can you add that in when you merge this patch?  Thanks.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SCSI: Fix NULL pointer dereference in runtime PM

[Johannes Thumshirn]

Alan Stern <stern@rowland.harvard.edu> writes:

> The routines in scsi_rpm.c assume that if a runtime-PM callback is
> invoked for a SCSI device, it can only mean that the device's driver 
> has asked the block layer to handle the runtime power management (by
> calling blk_pm_runtime_init(), which among other things sets q->dev).
>
> However, this assumption turns out to be wrong for things like the ses
> driver.  Normally ses devices are not allowed to do runtime PM, but
> userspace can override this setting.  If this happens, the kernel gets
> a NULL pointer dereference when blk_post_runtime_resume() tries to use
> the uninitialized q->dev pointer.
>
> This patch fixes the problem by calling the block layer's runtime-PM
> routines only if the device's driver really does have a runtime-PM
> callback routine.  Since ses doesn't define any such callbacks, the
> crash won't occur.
>
> This fixes Bugzilla #101371.
>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Reported-by: Stanisław Pitucha <viraptor@gmail.com>
> Reported-by: Ilan Cohen <ilanco@gmail.com>
> Tested-by: Ilan Cohen <ilanco@gmail.com>
>
> ---
>
>
> [as1784]
>
>
>  drivers/scsi/scsi_pm.c |   22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> Index: usb-4.0/drivers/scsi/scsi_pm.c
> ===================================================================
> --- usb-4.0.orig/drivers/scsi/scsi_pm.c
> +++ usb-4.0/drivers/scsi/scsi_pm.c
> @@ -217,15 +217,15 @@ static int sdev_runtime_suspend(struct d
>  {
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  	struct scsi_device *sdev = to_scsi_device(dev);
> -	int err;
> +	int err = 0;
>  
> -	err = blk_pre_runtime_suspend(sdev->request_queue);
> -	if (err)
> -		return err;
> -	if (pm && pm->runtime_suspend)
> +	if (pm && pm->runtime_suspend) {
> +		err = blk_pre_runtime_suspend(sdev->request_queue);
> +		if (err)
> +			return err;
>  		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 de
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>  	int err = 0;
>  
> -	blk_pre_runtime_resume(sdev->request_queue);
> -	if (pm && pm->runtime_resume)
> +	if (pm && pm->runtime_resume) {
> +		blk_pre_runtime_resume(sdev->request_queue);
>  		err = pm->runtime_resume(dev);
> -	blk_post_runtime_resume(sdev->request_queue, err);
> -
> +		blk_post_runtime_resume(sdev->request_queue, err);
> +	}
>  	return err;
>  }
>  
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                           Storage
jthumshirn@suse.de                                 +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600  D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] SCSI: Fix NULL pointer dereference in runtime PM

[Alan Stern]

The routines in scsi_rpm.c assume that if a runtime-PM callback is
invoked for a SCSI device, it can only mean that the device's driver 
has asked the block layer to handle the runtime power management (by
calling blk_pm_runtime_init(), which among other things sets q->dev).

However, this assumption turns out to be wrong for things like the ses
driver.  Normally ses devices are not allowed to do runtime PM, but
userspace can override this setting.  If this happens, the kernel gets
a NULL pointer dereference when blk_post_runtime_resume() tries to use
the uninitialized q->dev pointer.

This patch fixes the problem by calling the block layer's runtime-PM
routines only if the device's driver really does have a runtime-PM
callback routine.  Since ses doesn't define any such callbacks, the
crash won't occur.

This fixes Bugzilla #101371.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-by: Stanisław Pitucha <viraptor@gmail.com>
Reported-by: Ilan Cohen <ilanco@gmail.com>
Tested-by: Ilan Cohen <ilanco@gmail.com>

---


[as1784]


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

Index: usb-4.0/drivers/scsi/scsi_pm.c
===================================================================
--- usb-4.0.orig/drivers/scsi/scsi_pm.c
+++ usb-4.0/drivers/scsi/scsi_pm.c
@@ -217,15 +217,15 @@ static int sdev_runtime_suspend(struct d
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	struct scsi_device *sdev = to_scsi_device(dev);
-	int err;
+	int err = 0;
 
-	err = blk_pre_runtime_suspend(sdev->request_queue);
-	if (err)
-		return err;
-	if (pm && pm->runtime_suspend)
+	if (pm && pm->runtime_suspend) {
+		err = blk_pre_runtime_suspend(sdev->request_queue);
+		if (err)
+			return err;
 		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 de
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int err = 0;
 
-	blk_pre_runtime_resume(sdev->request_queue);
-	if (pm && pm->runtime_resume)
+	if (pm && pm->runtime_resume) {
+		blk_pre_runtime_resume(sdev->request_queue);
 		err = pm->runtime_resume(dev);
-	blk_post_runtime_resume(sdev->request_queue, err);
-
+		blk_post_runtime_resume(sdev->request_queue, err);
+	}
 	return err;
 }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html