From 8f1d1f1c1cb653f2dd749a9550b93a6b33be4a95 Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Thu, 9 Sep 2010 20:20:36 +0000 Subject: [PATCH] Possible deadlocks with sysfs ref counting in sysfs works fixed (it is waiting for the last put, but the last ref counter holder is waiting for scst_mutex). git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@2078 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_lib.c | 17 +++++- scst/src/scst_main.c | 118 +++++++++++++++++++++++++++--------------- scst/src/scst_priv.h | 5 -- scst/src/scst_sysfs.c | 51 ++++++++++++++++++ 4 files changed, 143 insertions(+), 48 deletions(-) diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 2c5346027..d16de7bed 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -4027,7 +4027,14 @@ void scst_free_session(struct scst_session *sess) scst_sess_free_tgt_devs(sess); +#ifndef CONFIG_SCST_PROC + /* tgt will stay alive at least until its sysfs alive */ + kobject_get(&sess->tgt->tgt_kobj); + + mutex_unlock(&scst_mutex); scst_sess_sysfs_del(sess); + mutex_lock(&scst_mutex); +#endif /* * The lists delete must be after sysfs del. Otherwise it would break @@ -4039,14 +4046,22 @@ void scst_free_session(struct scst_session *sess) TRACE_DBG("Removing session %p from acg %s", sess, sess->acg->acg_name); list_del(&sess->acg_sess_list_entry); +#ifdef CONFIG_SCST_PROC /* Called under lock to protect from too early tgt release */ wake_up_all(&sess->tgt->unreg_waitQ); +#endif mutex_unlock(&scst_mutex); - kfree(sess->transport_id); +#ifndef CONFIG_SCST_PROC + wake_up_all(&sess->tgt->unreg_waitQ); + kobject_put(&sess->tgt->tgt_kobj); +#endif + + kfree(sess->transport_id); kfree(sess->initiator_name); + kmem_cache_free(scst_sess_cachep, sess); TRACE_EXIT(); diff --git a/scst/src/scst_main.c b/scst/src/scst_main.c index 852e54010..7ae3c7c44 100644 --- a/scst/src/scst_main.c +++ b/scst/src/scst_main.c @@ -550,16 +550,16 @@ out: #ifndef CONFIG_SCST_PROC out_sysfs_del: + mutex_unlock(&scst_mutex); scst_tgt_sysfs_del(tgt); + goto out_free_tgt; #endif out_unlock: mutex_unlock(&scst_mutex); out_free_tgt: - /* - * In case of error tgt_name will be freed in scst_free_tgt(). - */ + /* In case of error tgt_name will be freed in scst_free_tgt() */ scst_free_tgt(tgt); tgt = NULL; goto out; @@ -633,13 +633,15 @@ again: acg_list_entry) { scst_del_free_acg(acg); } - - scst_tgt_sysfs_del(tgt); #endif mutex_unlock(&scst_mutex); scst_resume_activity(); +#ifndef CONFIG_SCST_PROC + scst_tgt_sysfs_del(tgt); +#endif + PRINT_INFO("Target %s for template %s unregistered successfully", tgt->tgt_name, vtt->name); @@ -843,13 +845,19 @@ static int scst_register_device(struct scsi_device *scsidp) TRACE_ENTRY(); +#ifdef CONFIG_SCST_PROC res = scst_suspend_activity(true); if (res != 0) goto out; +#endif if (mutex_lock_interruptible(&scst_mutex) != 0) { res = -EINTR; +#ifdef CONFIG_SCST_PROC goto out_resume; +#else + goto out; +#endif } res = scst_alloc_device(GFP_KERNEL, &dev); @@ -904,15 +912,17 @@ static int scst_register_device(struct scsi_device *scsidp) break; } } + + mutex_unlock(&scst_mutex); + scst_resume_activity(); #else + mutex_unlock(&scst_mutex); + res = scst_dev_sysfs_create(dev); if (res != 0) goto out_del; #endif - mutex_unlock(&scst_mutex); - scst_resume_activity(); - PRINT_INFO("Attached to scsi%d, channel %d, id %d, lun %d, " "type %d", scsidp->host->host_no, scsidp->channel, scsidp->id, scsidp->lun, scsidp->type); @@ -932,9 +942,10 @@ out_free_dev: out_unlock: mutex_unlock(&scst_mutex); - +#ifdef CONFIG_SCST_PROC out_resume: scst_resume_activity(); +#endif goto out; } @@ -971,8 +982,6 @@ static void scst_unregister_device(struct scsi_device *scsidp) scst_acg_del_lun(acg_dev->acg, acg_dev->lun, true); } - scst_dev_sysfs_del(dev); - mutex_unlock(&scst_mutex); #if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 18) @@ -981,6 +990,8 @@ static void scst_unregister_device(struct scsi_device *scsidp) scst_resume_activity(); + scst_dev_sysfs_del(dev); + PRINT_INFO("Detached from scsi%d, channel %d, id %d, lun %d, type %d", scsidp->host->host_no, scsidp->channel, scsidp->id, scsidp->lun, scsidp->type); @@ -1060,6 +1071,7 @@ int scst_register_virtual_device(struct scst_dev_type *dev_handler, { int res, rc; struct scst_device *dev, *d; + bool sysfs_del = false; TRACE_ENTRY(); @@ -1093,14 +1105,6 @@ int scst_register_virtual_device(struct scst_dev_type *dev_handler, goto out_resume; } - list_for_each_entry(d, &scst_dev_list, dev_list_entry) { - if (strcmp(d->virt_name, dev_name) == 0) { - PRINT_ERROR("Device %s already exists", dev_name); - res = -EEXIST; - goto out_unlock; - } - } - res = scst_alloc_device(GFP_KERNEL, &dev); if (res != 0) goto out_unlock; @@ -1122,26 +1126,46 @@ int scst_register_virtual_device(struct scst_dev_type *dev_handler, scst_virt_dev_last_id = 1; } - list_add_tail(&dev->dev_list_entry, &scst_dev_list); - res = dev->virt_id; rc = scst_pr_init_dev(dev); if (rc != 0) { res = rc; - goto out_del; + goto out_free_dev; } +#ifndef CONFIG_SCST_PROC + /* + * We can drop scst_mutex, because we have not yet added the dev in + * scst_dev_list, so it "doesn't exist" yet. + */ + mutex_unlock(&scst_mutex); + res = scst_dev_sysfs_create(dev); if (res != 0) - goto out_pr_clear_dev; + goto out_lock_pr_clear_dev; + + mutex_lock(&scst_mutex); +#endif + + list_for_each_entry(d, &scst_dev_list, dev_list_entry) { + if (strcmp(d->virt_name, dev_name) == 0) { + PRINT_ERROR("Device %s already exists", dev_name); + res = -EEXIST; + sysfs_del = true; + goto out_pr_clear_dev; + } + } rc = scst_assign_dev_handler(dev, dev_handler); if (rc != 0) { res = rc; - goto out_sysfs_del; + sysfs_del = true; + goto out_pr_clear_dev; } + list_add_tail(&dev->dev_list_entry, &scst_dev_list); + mutex_unlock(&scst_mutex); scst_resume_activity(); @@ -1154,17 +1178,20 @@ out: TRACE_EXIT_RES(res); return res; -out_sysfs_del: - scst_dev_sysfs_del(dev); +#ifndef CONFIG_SCST_PROC +out_lock_pr_clear_dev: + mutex_lock(&scst_mutex); +#endif out_pr_clear_dev: scst_pr_clear_dev(dev); -out_del: - list_del(&dev->dev_list_entry); - out_free_dev: + mutex_unlock(&scst_mutex); + if (sysfs_del) + scst_dev_sysfs_del(dev); scst_free_device(dev); + goto out_resume; out_unlock: mutex_unlock(&scst_mutex); @@ -1212,11 +1239,11 @@ void scst_unregister_virtual_device(int id) scst_acg_del_lun(acg_dev->acg, acg_dev->lun, true); } - scst_dev_sysfs_del(dev); - mutex_unlock(&scst_mutex); scst_resume_activity(); + scst_dev_sysfs_del(dev); + PRINT_INFO("Detached from virtual device %s (id %d)", dev->virt_name, dev->virt_id); @@ -1287,13 +1314,19 @@ int __scst_register_dev_driver(struct scst_dev_type *dev_type, } #endif /* !defined(SCSI_EXEC_REQ_FIFO_DEFINED) */ +#ifdef CONFIG_SCST_PROC res = scst_suspend_activity(true); if (res != 0) goto out; +#endif if (mutex_lock_interruptible(&scst_mutex) != 0) { res = -EINTR; +#ifdef CONFIG_SCST_PROC goto out_resume; +#else + goto out; +#endif } exist = 0; @@ -1308,23 +1341,17 @@ int __scst_register_dev_driver(struct scst_dev_type *dev_type, if (exist) goto out_unlock; + list_add_tail(&dev_type->dev_type_list_entry, &scst_dev_type_list); + #ifdef CONFIG_SCST_PROC if (!dev_type->no_proc) { res = scst_build_proc_dev_handler_dir_entries(dev_type); if (res < 0) goto out_unlock; } -#else - res = scst_devt_sysfs_create(dev_type); - if (res < 0) - goto out_unlock; -#endif - list_add_tail(&dev_type->dev_type_list_entry, &scst_dev_type_list); - -#ifdef CONFIG_SCST_PROC /* - * Let's don't attach to all devices by default, but keep this code in + * Let's don't attach to all devices by default, but keep this code * for compatibility in the proc build only. */ list_for_each_entry(dev, &scst_dev_list, dev_list_entry) { @@ -1333,10 +1360,16 @@ int __scst_register_dev_driver(struct scst_dev_type *dev_type, if (dev->scsi_dev->type == dev_type->type) scst_assign_dev_handler(dev, dev_type); } -#endif mutex_unlock(&scst_mutex); scst_resume_activity(); +#else + mutex_unlock(&scst_mutex); + + res = scst_devt_sysfs_create(dev_type); + if (res < 0) + goto out; +#endif PRINT_INFO("Device handler \"%s\" for type %d registered " "successfully", dev_type->name, dev_type->type); @@ -1347,9 +1380,10 @@ out: out_unlock: mutex_unlock(&scst_mutex); - +#ifdef CONFIG_SCST_PROC out_resume: scst_resume_activity(); +#endif goto out; } EXPORT_SYMBOL_GPL(__scst_register_dev_driver); diff --git a/scst/src/scst_priv.h b/scst/src/scst_priv.h index f2858e80b..585fbf31d 100644 --- a/scst/src/scst_priv.h +++ b/scst/src/scst_priv.h @@ -459,10 +459,6 @@ static inline int scst_devt_dev_sysfs_create(struct scst_device *dev) } static inline void scst_devt_dev_sysfs_del(struct scst_device *dev) { } -static inline int scst_dev_sysfs_create(struct scst_device *dev) -{ - return 0; -} static inline void scst_dev_sysfs_del(struct scst_device *dev) { } static inline int scst_tgt_dev_sysfs_create(struct scst_tgt_dev *tgt_dev) @@ -475,7 +471,6 @@ static inline int scst_sess_sysfs_create(struct scst_session *sess) { return 0; } -static inline void scst_sess_sysfs_del(struct scst_session *sess) { } static inline int scst_acg_dev_sysfs_create(struct scst_acg_dev *acg_dev, struct kobject *parent) diff --git a/scst/src/scst_sysfs.c b/scst/src/scst_sysfs.c index 4334772e4..98925f2ee 100644 --- a/scst/src/scst_sysfs.c +++ b/scst/src/scst_sysfs.c @@ -869,6 +869,11 @@ out_del: goto out; } +/* + * Must not be called under scst_mutex, due to possible deadlock with + * sysfs ref counting in sysfs works (it is waiting for the last put, but + * the last ref counter holder is waiting for scst_mutex) + */ void scst_tgtt_sysfs_del(struct scst_tgt_template *tgtt) { int rc; @@ -1083,6 +1088,10 @@ static struct kobj_attribute tgt_enable_attr = __ATTR(enabled, S_IRUGO | S_IWUSR, scst_tgt_enable_show, scst_tgt_enable_store); +/* + * Supposed to be called under scst_mutex. In case of error will drop, + * then reacquire it. + */ int scst_tgt_sysfs_create(struct scst_tgt *tgt) { int res; @@ -1192,10 +1201,17 @@ out_nomem: res = -ENOMEM; out_err: + mutex_unlock(&scst_mutex); scst_tgt_sysfs_del(tgt); + mutex_lock(&scst_mutex); goto out; } +/* + * Must not be called under scst_mutex, due to possible deadlock with + * sysfs ref counting in sysfs works (it is waiting for the last put, but + * the last ref counter holder is waiting for scst_mutex) + */ void scst_tgt_sysfs_del(struct scst_tgt *tgt) { int rc; @@ -1639,6 +1655,10 @@ static struct kobj_type scst_dev_ktype = { .default_attrs = scst_dev_attrs, }; +/* + * Must not be called under scst_mutex, because it can call + * scst_dev_sysfs_del() + */ int scst_dev_sysfs_create(struct scst_device *dev) { int res = 0; @@ -1694,6 +1714,11 @@ out_del: goto out; } +/* + * Must not be called under scst_mutex, due to possible deadlock with + * sysfs ref counting in sysfs works (it is waiting for the last put, but + * the last ref counter holder is waiting for scst_mutex) + */ void scst_dev_sysfs_del(struct scst_device *dev) { int rc; @@ -1905,6 +1930,13 @@ out: return res; } +/* + * Called with scst_mutex held. + * + * !! No sysfs works must use kobject_get() to protect tgt_dev, due to possible + * !! deadlock with scst_mutex (it is waiting for the last put, but + * !! the last ref counter holder is waiting for scst_mutex) + */ void scst_tgt_dev_sysfs_del(struct scst_tgt_dev *tgt_dev) { int rc; @@ -2407,6 +2439,11 @@ out_free: return res; } +/* + * Must not be called under scst_mutex, due to possible deadlock with + * sysfs ref counting in sysfs works (it is waiting for the last put, but + * the last ref counter holder is waiting for scst_mutex) + */ void scst_sess_sysfs_del(struct scst_session *sess) { int rc; @@ -2482,6 +2519,13 @@ static struct kobj_type acg_dev_ktype = { .default_attrs = lun_attrs, }; +/* + * Called with scst_mutex held. + * + * !! No sysfs works must use kobject_get() to protect acg_dev, due to possible + * !! deadlock with scst_mutex (it is waiting for the last put, but + * !! the last ref counter holder is waiting for scst_mutex) + */ void scst_acg_dev_sysfs_del(struct scst_acg_dev *acg_dev) { int rc; @@ -3098,6 +3142,13 @@ out: return res; } +/* + * Called with scst_mutex held. + * + * !! No sysfs works must use kobject_get() to protect acg, due to possible + * !! deadlock with scst_mutex (it is waiting for the last put, but + * !! the last ref counter holder is waiting for scst_mutex) + */ void scst_acg_sysfs_del(struct scst_acg *acg) { int rc;