On Thu, 2016-12-08 at 14:38 +0800, Wei Fang wrote:
> Hi, James, Ewan, Bart,
>
> On 2016/12/8 11:22, Wei Fang wrote:
> > I looked through those code and found that if we fix this bug
> > by removing setting the state in scsi_sysfs_add_sdev(), it
> > can't be fixed completely:
> >
> > scsi_device_set_state(sdev, SDEV_RUNNING) in scsi_add_lun() and
> > scsi_device_set_state(sdev, SDEV_CREATED_BLOCK) in scsi_internal_device_block()
> > can be called simultaneously. Because there is no synchronization
> > between scsi_device_set_state(), those calls may both return
> > success, and the state may be SDEV_RUNNING after that, and the
> > device queue is stopped.
>
> Can we fix it in this way:
>
> Add a state lock to make sure the result of simultaneously calling
> of scsi_device_set_state() is not unpredictable.
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 253ee74..80cb493 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2457,10 +2457,16 @@ EXPORT_SYMBOL(scsi_test_unit_ready);
> int
> scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
> {
> - enum scsi_device_state oldstate = sdev->sdev_state;
> + enum scsi_device_state oldstate;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&sdev->state_lock, flags);
> + oldstate = sdev->sdev_state;
>
> - if (state == oldstate)
> + if (state == oldstate) {
> + spin_unlock_irqrestore(&sdev->state_lock, flags);
> return 0;
> + }
>
> switch (state) {
> case SDEV_CREATED:
> @@ -2558,9 +2564,11 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
>
> }
> sdev->sdev_state = state;
> + spin_unlock_irqrestore(&sdev->state_lock, flags);
> return 0;
>
> illegal:
> + spin_unlock_irqrestore(&sdev->state_lock, flags);
> SCSI_LOG_ERROR_RECOVERY(1,
> sdev_printk(KERN_ERR, sdev,
> "Illegal state transition %s->%s",
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 6f7128f..ba2f38f 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -238,6 +238,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
> INIT_LIST_HEAD(&sdev->starved_entry);
> INIT_LIST_HEAD(&sdev->event_list);
> spin_lock_init(&sdev->list_lock);
> + spin_lock_init(&sdev->state_lock);
> mutex_init(&sdev->inquiry_mutex);
> INIT_WORK(&sdev->event_work, scsi_evt_thread);
> INIT_WORK(&sdev->requeue_work, scsi_requeue_run_queue);
> 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..e00764e 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 */
> @@ -207,6 +207,7 @@ struct scsi_device {
> void *handler_data;
>
> unsigned char access_state;
> + spinlock_t state_lock;
> enum scsi_device_state sdev_state;
> unsigned long sdev_data[0];
> } __attribute__((aligned(sizeof(unsigned long))));
>
> Haven't tested yet. Sending this for your opinion.
>
> Thanks,
> Wei
>
You would presumably need to take your lock in scsi_internal_device_unblock()
as well, since it also checks and updates sdev_state directly. There are also
places like scsi_device_resume() that examine the state before deciding to
call scsi_device_set_state().
-Ewan
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
