[PATCH v2] scsi: avoid a permanent stop of the scsi device's request queue

[PATCH v2] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

A scan work can run simultaneously with fc_remote_port_delete().
If a scsi device is added to the ->__devices list in the scan work,
it can be touched and will be blocked in scsi_target_block(), which
will be called in fc_remote_port_delete(), and QUEUE_FLAG_STOPPED
flag will be setted to the scsi device's request queue.

The scsi device is being setted to the SDEV_RUNNING state in
scsi_sysfs_add_sdev() at the end of the scan work. When the remote
port reappears, scsi_target_unblock() will be called, but the
QUEUE_FLAG_STOPPED flag will not be cleared, since
scsi_internal_device_unblock() ignores SCSI devices in SDEV_RUNNING
state. It results in a permanent stop of the scsi device's request
queue. Every requests sended to it will be blocked.

Since the scsi device shouldn't be unblocked in this case, fix
it by removing scsi_device_set_state() in scsi_sysfs_add_sdev().

Reported-and-tested-by: Zengxi Chen <chenzengxi@huawei.com>
Signed-off-by: Wei Fang <fangwei1@huawei.com>
---
Changes v1->v2:
- don't modify scsi_internal_device_unblock(), just remove changing
  state to SDEV_RUNNING in scsi_sysfs_add_sdev(), suggested by
  James Bottomley and Ewan D. Milne.

 drivers/scsi/scsi_sysfs.c  | 4 ----
 include/scsi/scsi_device.h | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 0734927..82dfe07 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1204,10 +1204,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 	struct request_queue *rq = sdev->request_queue;
 	struct scsi_target *starget = sdev->sdev_target;
 
-	error = scsi_device_set_state(sdev, SDEV_RUNNING);
-	if (error)
-		return error;
-
 	error = scsi_target_add(starget);
 	if (error)
 		return error;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 8990e58..5c53cf5 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -31,7 +31,7 @@ struct scsi_mode_data {
 enum scsi_device_state {
 	SDEV_CREATED = 1,	/* device created but not added to sysfs
 				 * Only internal commands allowed (for inq) */
-	SDEV_RUNNING,		/* device properly configured
+	SDEV_RUNNING,		/* device properly initialized
 				 * All commands allowed */
 	SDEV_CANCEL,		/* beginning to delete device
 				 * Only error handler commands allowed */
-- 
2.4.11

Re: [PATCH v2] scsi: avoid a permanent stop of the scsi device's request queue

[James Bottomley]

On Fri, 2016-12-09 at 17:35 +0800, Wei Fang wrote:
	
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 8990e58..5c53cf5 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -31,7 +31,7 @@ struct scsi_mode_data {
>  enum scsi_device_state {
>  	SDEV_CREATED = 1,	/* device created but not added to
> sysfs
>  				 * Only internal commands allowed
> (for inq) */
> -	SDEV_RUNNING,		/* device properly configured
> +	SDEV_RUNNING,		/* device properly initialized
>  				 * All commands allowed */
>  	SDEV_CANCEL,		/* beginning to delete device
>  				 * Only error handler commands
> allowed */

This hunk is still pointless.  What even is the difference between
initialized and configured to someone reading the comments?

The reason for not having pointless changes is to make this as clean as
possible for a backport to stable.

James

Re: [PATCH v2] scsi: avoid a permanent stop of the scsi device's request queue

[James Bottomley]

On Fri, 2016-12-09 at 17:35 +0800, Wei Fang wrote:
> A scan work can run simultaneously with fc_remote_port_delete().
> If a scsi device is added to the ->__devices list in the scan work,
> it can be touched and will be blocked in scsi_target_block(), which
> will be called in fc_remote_port_delete(), and QUEUE_FLAG_STOPPED
> flag will be setted to the scsi device's request queue.
>  
> The scsi device is being setted to the SDEV_RUNNING state in
> scsi_sysfs_add_sdev() at the end of the scan work. When the remote
> port reappears, scsi_target_unblock() will be called, but the
> QUEUE_FLAG_STOPPED flag will not be cleared, since
> scsi_internal_device_unblock() ignores SCSI devices in SDEV_RUNNING
> state. It results in a permanent stop of the scsi device's request
> queue. Every requests sended to it will be blocked.

This is a bit unclear as a description of the problem

> Since the scsi device shouldn't be unblocked in this case, fix
> it by removing scsi_device_set_state() in scsi_sysfs_add_sdev().

So is this as a description of the solution, because the reader doesn't
know there's a prior place where SDEV_RUNNING was previously set.

How about

---
A race between scanning and fc_remote_port_delete() may result in a
permanent stop if the device gets blocked before scsi_sysfs_add_lun()
and unblocked after.  The reason is that blocking a device sets both
the SDEV_BLOCKED state and the QUEUE_FLAG_STOPPED.  However,
scsi_sysfs_add_sdev() unconditionally sets SDEV_RUNNING which causes
the device to be ignored by scsi_target_unblock() and thus never have
its QUEUE_FLAG_STOPPED cleared leading to a device which is apparently
running but has a stopped queue.

We actually have two places where SDEV_RUNNING is set: once in
scsi_add_lun() which respects the blocked flag and once in
scsi_sysfs_add_sdev() which doesn't.  Since the second set is entirely
spurious, simply remove it to fix the problem.
---

James

Re: [PATCH v2] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

Hi, James,

On 2016/12/10 0:02, James Bottomley wrote:
> On Fri, 2016-12-09 at 17:35 +0800, Wei Fang wrote:
>> A scan work can run simultaneously with fc_remote_port_delete().
>> If a scsi device is added to the ->__devices list in the scan work,
>> it can be touched and will be blocked in scsi_target_block(), which
>> will be called in fc_remote_port_delete(), and QUEUE_FLAG_STOPPED
>> flag will be setted to the scsi device's request queue.
>>  
>> The scsi device is being setted to the SDEV_RUNNING state in
>> scsi_sysfs_add_sdev() at the end of the scan work. When the remote
>> port reappears, scsi_target_unblock() will be called, but the
>> QUEUE_FLAG_STOPPED flag will not be cleared, since
>> scsi_internal_device_unblock() ignores SCSI devices in SDEV_RUNNING
>> state. It results in a permanent stop of the scsi device's request
>> queue. Every requests sended to it will be blocked.
> 
> This is a bit unclear as a description of the problem
> 
>> Since the scsi device shouldn't be unblocked in this case, fix
>> it by removing scsi_device_set_state() in scsi_sysfs_add_sdev().
> 
> So is this as a description of the solution, because the reader doesn't
> know there's a prior place where SDEV_RUNNING was previously set.
> 
> How about
> 
> ---
> A race between scanning and fc_remote_port_delete() may result in a
> permanent stop if the device gets blocked before scsi_sysfs_add_lun()
> and unblocked after.  The reason is that blocking a device sets both
> the SDEV_BLOCKED state and the QUEUE_FLAG_STOPPED.  However,
> scsi_sysfs_add_sdev() unconditionally sets SDEV_RUNNING which causes
> the device to be ignored by scsi_target_unblock() and thus never have
> its QUEUE_FLAG_STOPPED cleared leading to a device which is apparently
> running but has a stopped queue.
> 
> We actually have two places where SDEV_RUNNING is set: once in
> scsi_add_lun() which respects the blocked flag and once in
> scsi_sysfs_add_sdev() which doesn't.  Since the second set is entirely
> spurious, simply remove it to fix the problem.

That's much clearer, thanks. I'll use this in patch v3.

Thanks,
Wei