Fix zvol_state_t->zv_open_count race

5559ba0 added zv_state_lock to protect zvol_state_t internal data:
this, however, doesn't guard zv->zv_open_count and
zv->zv_disk->private_data in zvol_remove_minors_impl().

Fix this by taking zv->zv_state_lock before we check its zv_open_count.

P1 (z_zvol)                       P2 (systemd-udevd)
---                               ---
zvol_remove_minors_impl()
: zv->zv_open_count==0
                                  zvol_open()
                                  ->mutex_enter(zv_state_lock)
                                  : zv->zv_open_count++
                                  ->mutex_exit(zv_state_lock)
->mutex_enter(zv->zv_state_lock)
->zvol_remove(zv)
->mutex_exit(zv->zv_state_lock)
: zv->zv_disk->private_data = NULL
->zvol_free()
-->ASSERT(zv->zv_open_count==0) *
                                  zvol_release()
                                  : zv = disk->private_data
                                  ->ASSERT(zv && zv->zv_open_count>0) *
---                               ---
* ASSERT() fails

Reviewed by: Boris Protopopov <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]
Signed-off-by: loli10K <[email protected]>
Closes #6213 

1 files changed, 21 insertions, 11 deletions

diff --git a/module/zfs/zvol.c b/module/zfs/zvol.c
index b2392305b..7730c28c6 100644
--- a/module/zfs/zvol.c
+++ b/module/zfs/zvol.c
@@ -1964,22 +1964,27 @@ zvol_remove_minors_impl(const char *name)
 		    (strncmp(zv->zv_name, name, namelen) == 0 &&
 		    (zv->zv_name[namelen] == '/' ||
 		    zv->zv_name[namelen] == '@'))) {
-
-			/* If in use, leave alone */
-			if (zv->zv_open_count > 0 ||
-			    atomic_read(&zv->zv_suspend_ref))
-				continue;
 			/*
 			 * By taking zv_state_lock here, we guarantee that no
 			 * one is currently using this zv
 			 */
 			mutex_enter(&zv->zv_state_lock);
+
+			/* If in use, leave alone */
+			if (zv->zv_open_count > 0 ||
+			    atomic_read(&zv->zv_suspend_ref)) {
+				mutex_exit(&zv->zv_state_lock);
+				continue;
+			}
+
 			zvol_remove(zv);
-			mutex_exit(&zv->zv_state_lock);
 
 			/* clear this so zvol_open won't open it */
 			zv->zv_disk->private_data = NULL;
 
+			/* Drop zv_state_lock before zvol_free() */
+			mutex_exit(&zv->zv_state_lock);
+
 			/* try parallel zv_free, if failed do it in place */
 			t = taskq_dispatch(system_taskq, zvol_free, zv,
 			    TQ_SLEEP);
@@ -2021,19 +2026,24 @@ zvol_remove_minor_impl(const char *name)
 		zv_next = list_next(&zvol_state_list, zv);
 
 		if (strcmp(zv->zv_name, name) == 0) {
-			/* If in use, leave alone */
-			if (zv->zv_open_count > 0 ||
-			    atomic_read(&zv->zv_suspend_ref))
-				continue;
 			/*
 			 * By taking zv_state_lock here, we guarantee that no
 			 * one is currently using this zv
 			 */
 			mutex_enter(&zv->zv_state_lock);
+
+			/* If in use, leave alone */
+			if (zv->zv_open_count > 0 ||
+			    atomic_read(&zv->zv_suspend_ref)) {
+				mutex_exit(&zv->zv_state_lock);
+				continue;
+			}
 			zvol_remove(zv);
-			mutex_exit(&zv->zv_state_lock);
+
 			/* clear this so zvol_open won't open it */
 			zv->zv_disk->private_data = NULL;
+
+			mutex_exit(&zv->zv_state_lock);
 			break;
 		}
 	}