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

@@ -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();

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);

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)

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;