From 4e5548c4098549cdb1f2eac1a2c1bf495180422e Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Mon, 12 Aug 2019 01:39:37 +0000 Subject: [PATCH 1/2] scst: Verify dev_exec_cmd_list protection at runtime git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@8500 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_targ.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scst/src/scst_targ.c b/scst/src/scst_targ.c index bee0ae2de..aa5c563b0 100644 --- a/scst/src/scst_targ.c +++ b/scst/src/scst_targ.c @@ -338,6 +338,8 @@ bool scst_do_check_blocked_dev(struct scst_cmd *cmd) TRACE_ENTRY(); + lockdep_assert_held(&dev->dev_lock); + /* * We want to have fairness between just unblocked previously blocked * SCSI atomic cmds and new cmds came after them. Otherwise, the new @@ -482,6 +484,8 @@ void __scst_check_unblock_dev(struct scst_cmd *cmd) TRACE_ENTRY(); + lockdep_assert_held(&dev->dev_lock); + /* * We might be called here as part of Copy Manager's check blocking * undo, so restore all flags in the previous state to allow From c6e8013f486497a262d20b4f5a56f08e4ce315f7 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Mon, 12 Aug 2019 01:41:26 +0000 Subject: [PATCH 2/2] scst: Only free a device after all associated commands and LUNs have finished Merge scst_free_device() and scst_finally_free_device() into a single function. Increase dev->refcnt when registering a device or virtual device. Kill and decrease dev->refcnt when unregistering a device or virtual device. These changes ensure that a scst_free_device() is only called after all users (commands and tgt_devs) have stopped accessing the SCST device. Fixes: 3f2d50b5894b ("scst: Do not suspend command processing when deleting a device"; r8067) git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@8501 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/include/scst.h | 2 -- scst/src/scst_lib.c | 57 +++++++++++++++----------------------------- scst/src/scst_main.c | 16 +++++++++---- scst/src/scst_priv.h | 1 - 4 files changed, 30 insertions(+), 46 deletions(-) diff --git a/scst/include/scst.h b/scst/include/scst.h index 35af18bd8..3f6a2e1d1 100644 --- a/scst/include/scst.h +++ b/scst/include/scst.h @@ -2894,8 +2894,6 @@ struct scst_device { struct work_struct free_work; - struct completion *dev_freed_cmpl; - /* * Maximum count of uncompleted commands that an initiator could * queue on this device. Then it will start getting TASK QUEUE FULL diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index b5b7289ef..2ce642255 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -4185,20 +4185,33 @@ static int scst_dif_none_type1(struct scst_cmd *cmd); #endif /* Called from thread context and hence may sleep. */ -static void scst_finally_free_device(struct work_struct *work) +static void scst_free_device(struct work_struct *work) { struct scst_device *dev = container_of(work, typeof(*dev), free_work); - struct completion *c = dev->dev_freed_cmpl; + + EXTRACHECKS_BUG_ON(dev->dev_scsi_atomic_cmd_active != 0); + EXTRACHECKS_BUG_ON(!list_empty(&dev->dev_exec_cmd_list)); + +#ifdef CONFIG_SCST_EXTRACHECKS + if (!list_empty(&dev->dev_tgt_dev_list) || + !list_empty(&dev->dev_acg_dev_list)) { + PRINT_CRIT_ERROR("%s: dev_tgt_dev_list or dev_acg_dev_list " + "is not empty!", __func__); + sBUG(); + } +#endif + + /* Ensure that ext_blockers_work is done */ + flush_work(&dev->ext_blockers_work); + + scst_deinit_threads(&dev->dev_cmd_threads); scst_pr_cleanup(dev); kfree(dev->virt_name); percpu_ref_exit(&dev->refcnt); kmem_cache_free(scst_dev_cachep, dev); - - if (c) - complete(c); } /* RCU callback. Must not sleep. */ @@ -4227,7 +4240,7 @@ int scst_alloc_device(gfp_t gfp_mask, int nodeid, struct scst_device **out_dev) memset(dev, 0, sizeof(*dev)); dev->handler = &scst_null_devtype; - INIT_WORK(&dev->free_work, scst_finally_free_device); + INIT_WORK(&dev->free_work, scst_free_device); res = percpu_ref_init(&dev->refcnt, scst_release_device, 0, GFP_KERNEL); if (res < 0) goto free_dev; @@ -4272,38 +4285,6 @@ free_dev: goto out; } -void scst_free_device(struct scst_device *dev) -{ - DECLARE_COMPLETION_ONSTACK(c); - - TRACE_ENTRY(); - - EXTRACHECKS_BUG_ON(dev->dev_scsi_atomic_cmd_active != 0); - EXTRACHECKS_BUG_ON(!list_empty(&dev->dev_exec_cmd_list)); - -#ifdef CONFIG_SCST_EXTRACHECKS - if (!list_empty(&dev->dev_tgt_dev_list) || - !list_empty(&dev->dev_acg_dev_list)) { - PRINT_CRIT_ERROR("%s: dev_tgt_dev_list or dev_acg_dev_list " - "is not empty!", __func__); - sBUG(); - } -#endif - - /* Ensure that ext_blockers_work is done */ - flush_work(&dev->ext_blockers_work); - - scst_deinit_threads(&dev->dev_cmd_threads); - - dev->dev_freed_cmpl = &c; - percpu_ref_kill(&dev->refcnt); - - wait_for_completion(&c); - - TRACE_EXIT(); - return; -} - bool scst_device_is_exported(struct scst_device *dev) { lockdep_assert_held(&scst_mutex); diff --git a/scst/src/scst_main.c b/scst/src/scst_main.c index ff3c4f12f..afc5f9184 100644 --- a/scst/src/scst_main.c +++ b/scst/src/scst_main.c @@ -1132,6 +1132,8 @@ static int scst_register_device(struct scsi_device *scsidp) if (res != 0) goto out_del_unlocked; + percpu_ref_get(&dev->refcnt); + PRINT_INFO("Attached to scsi%d, channel %d, id %d, lun %lld, type %d", scsidp->host->host_no, scsidp->channel, scsidp->id, (u64)scsidp->lun, scsidp->type); @@ -1145,7 +1147,7 @@ out_del_unlocked: list_del_init(&dev->dev_list_entry); mutex_unlock(&scst_mutex); - scst_free_device(dev); + percpu_ref_kill(&dev->refcnt); goto out; #ifdef CONFIG_SCST_FORWARD_MODE_PASS_THROUGH @@ -1153,7 +1155,7 @@ out_del_unlocked: #endif out_free_dev: - scst_free_device(dev); + percpu_ref_kill(&dev->refcnt); out_unlock: mutex_unlock(&scst_mutex); @@ -1213,7 +1215,8 @@ static void scst_unregister_device(struct scsi_device *scsidp) scsidp->host->host_no, scsidp->channel, scsidp->id, (u64)scsidp->lun, scsidp->type); - scst_free_device(dev); + percpu_ref_kill(&dev->refcnt); + percpu_ref_put(&dev->refcnt); out: TRACE_EXIT(); @@ -1393,6 +1396,8 @@ int scst_register_virtual_device_node(struct scst_dev_type *dev_handler, mutex_unlock(&scst_mutex); + percpu_ref_get(&dev->refcnt); + res = dev->virt_id; PRINT_INFO("Attached to virtual device %s (id %d)", dev_name, res); @@ -1416,7 +1421,7 @@ out_free_dev: mutex_unlock(&scst_mutex); if (sysfs_del) scst_dev_sysfs_del(dev); - scst_free_device(dev); + percpu_ref_kill(&dev->refcnt); goto out; out_unlock: @@ -1478,7 +1483,8 @@ void scst_unregister_virtual_device(int id, if (on_free) on_free(dev, arg); - scst_free_device(dev); + percpu_ref_kill(&dev->refcnt); + percpu_ref_put(&dev->refcnt); out: TRACE_EXIT(); diff --git a/scst/src/scst_priv.h b/scst/src/scst_priv.h index 9864e6971..20756def1 100644 --- a/scst/src/scst_priv.h +++ b/scst/src/scst_priv.h @@ -365,7 +365,6 @@ int scst_alloc_tgt(struct scst_tgt_template *tgtt, struct scst_tgt **tgt); void scst_free_tgt(struct scst_tgt *tgt); int scst_alloc_device(gfp_t gfp_mask, int nodeid, struct scst_device **out_dev); -void scst_free_device(struct scst_device *dev); bool scst_device_is_exported(struct scst_device *dev); int scst_alloc_add_acg(struct scst_tgt *tgt, const char *acg_name,