From: Bart Van Assche Date: Fri, 2 Jun 2017 21:21:55 +0000 (-0700) Subject: scsi: Protect SCSI device state changes with a mutex X-Git-Tag: v4.1.12-124.31.3~1129 X-Git-Url: https://www.infradead.org/git/?a=commitdiff_plain;h=2e5602ab8d7d9ebc330af282571a1baa8a358b35;p=users%2Fjedix%2Flinux-maple.git scsi: Protect SCSI device state changes with a mutex Serializing SCSI device state changes avoids that two state changes can occur concurrently, e.g. the state changes in scsi_target_block() and __scsi_remove_device(). This serialization is essential to make patch "Make __scsi_remove_device go straight from BLOCKED to DEL" work reliably. Enable this mechanism for all scsi_target_*block() callers but not for the scsi_internal_device_unblock() calls from the mpt3sas driver because that driver can call scsi_internal_device_unblock() from atomic context. Signed-off-by: Bart Van Assche Cc: Christoph Hellwig Cc: Hannes Reinecke Cc: Johannes Thumshirn Signed-off-by: Martin K. Petersen Orabug: 27546768 (cherry picked from commit 0db6ca8a5e1ea585795db3643ec7d50fc8cb1aff) Signed-off-by: Kyle Fortin Reviewed-by: Martin K. Petersen Conflicts: drivers/scsi/scsi_error.c drivers/scsi/scsi_lib.c include/scsi/scsi_device.h Use kmalloc'd state_mutex_kabi in scsi_device to fit new field state_mutex in KABI reserved space. --- diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index 841fdf745fcf..fe741661765d 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -1687,16 +1687,17 @@ static void scsi_eh_offline_sdevs(struct list_head *work_q, struct list_head *done_q) { struct scsi_cmnd *scmd, *next; + struct scsi_device *sdev; list_for_each_entry_safe(scmd, next, work_q, eh_entry) { sdev_printk(KERN_INFO, scmd->device, "Device offlined - " "not ready after error recovery\n"); - scsi_device_set_state(scmd->device, SDEV_OFFLINE); - if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD) { - /* - * FIXME: Handle lost cmds. - */ - } + sdev = scmd->device; + + mutex_lock(sdev->state_mutex_kabi); + scsi_device_set_state(sdev, SDEV_OFFLINE); + mutex_unlock(sdev->state_mutex_kabi); + scsi_eh_finish_cmd(scmd, done_q); } return; diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index ab4d4f022d81..df9a7428168b 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -2862,7 +2862,12 @@ EXPORT_SYMBOL_GPL(sdev_evt_send_simple); int scsi_device_quiesce(struct scsi_device *sdev) { - int err = scsi_device_set_state(sdev, SDEV_QUIESCE); + int err; + + mutex_lock(sdev->state_mutex_kabi); + err = scsi_device_set_state(sdev, SDEV_QUIESCE); + mutex_unlock(sdev->state_mutex_kabi); + if (err) return err; @@ -2890,10 +2895,11 @@ void scsi_device_resume(struct scsi_device *sdev) * so assume the state is being managed elsewhere (for example * device deleted during suspend) */ - if (sdev->sdev_state != SDEV_QUIESCE || - scsi_device_set_state(sdev, SDEV_RUNNING)) - return; - scsi_run_queue(sdev->request_queue); + mutex_lock(sdev->state_mutex_kabi); + if (sdev->sdev_state == SDEV_QUIESCE && + scsi_device_set_state(sdev, SDEV_RUNNING) == 0) + scsi_run_queue(sdev->request_queue); + mutex_unlock(sdev->state_mutex_kabi); } EXPORT_SYMBOL(scsi_device_resume); @@ -3031,7 +3037,9 @@ EXPORT_SYMBOL_GPL(scsi_internal_device_unblock); static void device_block(struct scsi_device *sdev, void *data) { + mutex_lock(sdev->state_mutex_kabi); scsi_internal_device_block(sdev); + mutex_unlock(sdev->state_mutex_kabi); } static int @@ -3057,7 +3065,9 @@ EXPORT_SYMBOL_GPL(scsi_target_block); static void device_unblock(struct scsi_device *sdev, void *data) { + mutex_lock(sdev->state_mutex_kabi); scsi_internal_device_unblock(sdev, *(enum scsi_device_state *)data); + mutex_unlock(sdev->state_mutex_kabi); } static int diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index f605548054ba..253cd8591d8a 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -220,6 +220,13 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, if (!sdev) goto out; + sdev->state_mutex_kabi = kzalloc(sizeof(*(sdev->state_mutex_kabi)), + GFP_ATOMIC); + if (!sdev->state_mutex_kabi) { + kfree(sdev); + goto out; + } + sdev->vendor = scsi_null_device_strs; sdev->model = scsi_null_device_strs; sdev->rev = scsi_null_device_strs; @@ -228,6 +235,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, sdev->id = starget->id; sdev->lun = lun; sdev->channel = starget->channel; + mutex_init(sdev->state_mutex_kabi); sdev->sdev_state = SDEV_CREATED; INIT_LIST_HEAD(&sdev->siblings); INIT_LIST_HEAD(&sdev->same_target_siblings); @@ -268,6 +276,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, /* release fn is set up in scsi_sysfs_device_initialise, so * have to free and put manually here */ put_device(&starget->dev); + kfree(sdev->state_mutex_kabi); kfree(sdev); goto out; } @@ -935,16 +944,17 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, /* set the device running here so that slave configure * may do I/O */ + mutex_lock(sdev->state_mutex_kabi); ret = scsi_device_set_state(sdev, SDEV_RUNNING); - if (ret) { + if (ret) ret = scsi_device_set_state(sdev, SDEV_BLOCK); + mutex_unlock(sdev->state_mutex_kabi); - if (ret) { - sdev_printk(KERN_ERR, sdev, - "in wrong state %s to complete scan\n", - scsi_device_state_name(sdev->sdev_state)); - return SCSI_SCAN_NO_RESPONSE; - } + if (ret) { + sdev_printk(KERN_ERR, sdev, + "in wrong state %s to complete scan\n", + scsi_device_state_name(sdev->sdev_state)); + return SCSI_SCAN_NO_RESPONSE; } if (*bflags & BLIST_MS_192_BYTES_FOR_3F) diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index 9a7c0bf43195..78fbc35326a2 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -424,6 +424,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work) kfree(sdev->vpd_pg83); kfree(sdev->vpd_pg80); kfree(sdev->inquiry); + kfree(sdev->state_mutex_kabi); kfree(sdev); if (parent) @@ -686,7 +687,7 @@ static ssize_t store_state_field(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { - int i; + int i, ret; struct scsi_device *sdev = to_scsi_device(dev); enum scsi_device_state state = 0; @@ -701,9 +702,11 @@ store_state_field(struct device *dev, struct device_attribute *attr, if (!state) return -EINVAL; - if (scsi_device_set_state(sdev, state)) - return -EINVAL; - return count; + mutex_lock(sdev->state_mutex_kabi); + ret = scsi_device_set_state(sdev, state); + mutex_unlock(sdev->state_mutex_kabi); + + return ret == 0 ? count : -EINVAL; } static ssize_t @@ -1062,6 +1065,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev) void __scsi_remove_device(struct scsi_device *sdev) { struct device *dev = &sdev->sdev_gendev; + int res; /* * This cleanup path is not reentrant and while it is impossible @@ -1072,7 +1076,15 @@ void __scsi_remove_device(struct scsi_device *sdev) return; if (sdev->is_visible) { - if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0) + /* + * If scsi_internal_target_block() is running concurrently, + * wait until it has finished before changing the device state. + */ + mutex_lock(sdev->state_mutex_kabi); + res = scsi_device_set_state(sdev, SDEV_CANCEL); + mutex_unlock(sdev->state_mutex_kabi); + + if (res != 0) return; bsg_unregister_queue(sdev->request_queue); @@ -1087,7 +1099,10 @@ void __scsi_remove_device(struct scsi_device *sdev) * scsi_run_queue() invocations have finished before tearing down the * device. */ + mutex_lock(sdev->state_mutex_kabi); scsi_device_set_state(sdev, SDEV_DEL); + mutex_unlock(sdev->state_mutex_kabi); + blk_cleanup_queue(sdev->request_queue); cancel_work_sync(&sdev->requeue_work); diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c index f115f67a6ba5..ef734d68a1f1 100644 --- a/drivers/scsi/scsi_transport_srp.c +++ b/drivers/scsi/scsi_transport_srp.c @@ -586,11 +586,12 @@ int srp_reconnect_rport(struct srp_rport *rport) * invoking scsi_target_unblock() won't change the state of * these devices into running so do that explicitly. */ - spin_lock_irq(shost->host_lock); - __shost_for_each_device(sdev, shost) + shost_for_each_device(sdev, shost) { + mutex_lock(sdev->state_mutex_kabi); if (sdev->sdev_state == SDEV_OFFLINE) sdev->sdev_state = SDEV_RUNNING; - spin_unlock_irq(shost->host_lock); + mutex_unlock(sdev->state_mutex_kabi); + } } else if (rport->state == SRP_RPORT_RUNNING) { /* * srp_reconnect_rport() has been invoked with fast_io_fail diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 94f48fcc2715..9cd46eca3482 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1565,8 +1565,9 @@ static const struct block_device_operations sd_fops = { static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp) { struct scsi_disk *sdkp = scsi_disk(scmd->request->rq_disk); + struct scsi_device *sdev = scmd->device; - if (!scsi_device_online(scmd->device) || + if (!scsi_device_online(sdev) || !scsi_medium_access_command(scmd) || host_byte(scmd->result) != DID_TIME_OUT || eh_disp != SUCCESS) @@ -1589,7 +1590,9 @@ static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp) if (sdkp->medium_access_timed_out >= sdkp->max_medium_access_timeouts) { scmd_printk(KERN_ERR, scmd, "Medium access timeout failure. Offlining disk!\n"); - scsi_device_set_state(scmd->device, SDEV_OFFLINE); + mutex_lock(sdev->state_mutex_kabi); + scsi_device_set_state(sdev, SDEV_OFFLINE); + mutex_unlock(sdev->state_mutex_kabi); return FAILED; } diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 7cb449b18cec..a55cb48340f7 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -202,8 +202,13 @@ struct scsi_device { * * The following padding has been inserted before ABI freeze to * allow extending the structure while preserving ABI. + * + * The state_mutex struct wouldn't fit in KABI reserved space. + * Use a kmalloc'd storage for it and make it a pointer named + * as state_mutex_kabi to make future cherry-picks referencing + * state_mutex more obvious earlier. */ - UEK_KABI_RESERVED(1) + UEK_KABI_USE(1, struct mutex *state_mutex_kabi) UEK_KABI_RESERVED(2) unsigned long sdev_data[0]; } __attribute__((aligned(sizeof(unsigned long))));