Fix "snapdev" property issues

When inheriting the "snapdev" property to we don't always call
zfs_prop_set_special(): this prevents device nodes from being created in
certain situations. Because "snapdev" is the only *special* property
that is also inheritable we need to call zfs_prop_set_special() even
when we're not reverting it to the received value ('zfs inherit -S').

Additionally, fix a NULL pointer dereference accidentally introduced in
5559ba0 that can be triggered when setting the "snapdev" property to
the value "hidden" twice.

Finally, add a new test case "zvol_misc_snapdev" to the ZFS Test Suite.

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

5 files changed, 248 insertions, 64 deletions

diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c
index c6a532123..4fda36c7f 100644
--- a/module/zfs/zfs_ioctl.c
+++ b/module/zfs/zfs_ioctl.c
@@ -2761,54 +2761,12 @@ zfs_ioc_inherit_prop(zfs_cmd_t *zc)
 	zprop_source_t source = (received
 	    ? ZPROP_SRC_NONE		/* revert to received value, if any */
 	    : ZPROP_SRC_INHERITED);	/* explicitly inherit */
+	nvlist_t *dummy;
+	nvpair_t *pair;
+	zprop_type_t type;
+	int err;
 
-	if (received) {
-		nvlist_t *dummy;
-		nvpair_t *pair;
-		zprop_type_t type;
-		int err;
-
-		/*
-		 * zfs_prop_set_special() expects properties in the form of an
-		 * nvpair with type info.
-		 */
-		if (prop == ZPROP_INVAL) {
-			if (!zfs_prop_user(propname))
-				return (SET_ERROR(EINVAL));
-
-			type = PROP_TYPE_STRING;
-		} else if (prop == ZFS_PROP_VOLSIZE ||
-		    prop == ZFS_PROP_VERSION) {
-			return (SET_ERROR(EINVAL));
-		} else {
-			type = zfs_prop_get_type(prop);
-		}
-
-		VERIFY(nvlist_alloc(&dummy, NV_UNIQUE_NAME, KM_SLEEP) == 0);
-
-		switch (type) {
-		case PROP_TYPE_STRING:
-			VERIFY(0 == nvlist_add_string(dummy, propname, ""));
-			break;
-		case PROP_TYPE_NUMBER:
-		case PROP_TYPE_INDEX:
-			VERIFY(0 == nvlist_add_uint64(dummy, propname, 0));
-			break;
-		default:
-			nvlist_free(dummy);
-			return (SET_ERROR(EINVAL));
-		}
-
-		pair = nvlist_next_nvpair(dummy, NULL);
-		if (pair == NULL) {
-			nvlist_free(dummy);
-			return (SET_ERROR(EINVAL));
-		}
-		err = zfs_prop_set_special(zc->zc_name, source, pair);
-		nvlist_free(dummy);
-		if (err != -1)
-			return (err); /* special property already handled */
-	} else {
+	if (!received) {
 		/*
 		 * Only check this in the non-received case. We want to allow
 		 * 'inherit -S' to revert non-inheritable properties like quota
@@ -2819,8 +2777,49 @@ zfs_ioc_inherit_prop(zfs_cmd_t *zc)
 			return (SET_ERROR(EINVAL));
 	}
 
-	/* property name has been validated by zfs_secpolicy_inherit_prop() */
-	return (dsl_prop_inherit(zc->zc_name, zc->zc_value, source));
+	if (prop == ZPROP_INVAL) {
+		if (!zfs_prop_user(propname))
+			return (SET_ERROR(EINVAL));
+
+		type = PROP_TYPE_STRING;
+	} else if (prop == ZFS_PROP_VOLSIZE || prop == ZFS_PROP_VERSION) {
+		return (SET_ERROR(EINVAL));
+	} else {
+		type = zfs_prop_get_type(prop);
+	}
+
+	/*
+	 * zfs_prop_set_special() expects properties in the form of an
+	 * nvpair with type info.
+	 */
+	dummy = fnvlist_alloc();
+
+	switch (type) {
+	case PROP_TYPE_STRING:
+		VERIFY(0 == nvlist_add_string(dummy, propname, ""));
+		break;
+	case PROP_TYPE_NUMBER:
+	case PROP_TYPE_INDEX:
+		VERIFY(0 == nvlist_add_uint64(dummy, propname, 0));
+		break;
+	default:
+		err = SET_ERROR(EINVAL);
+		goto errout;
+	}
+
+	pair = nvlist_next_nvpair(dummy, NULL);
+	if (pair == NULL) {
+		err = SET_ERROR(EINVAL);
+	} else {
+		err = zfs_prop_set_special(zc->zc_name, source, pair);
+		if (err == -1) /* property is not "special", needs handling */
+			err = dsl_prop_inherit(zc->zc_name, zc->zc_value,
+			    source);
+	}
+
+errout:
+	nvlist_free(dummy);
+	return (err);
 }
 
 static int
diff --git a/module/zfs/zvol.c b/module/zfs/zvol.c
index cff3da8b4..1a86cd3cd 100644
--- a/module/zfs/zvol.c
+++ b/module/zfs/zvol.c
@@ -2018,7 +2018,7 @@ zvol_remove_minors_impl(const char *name)
 static void
 zvol_remove_minor_impl(const char *name)
 {
-	zvol_state_t *zv, *zv_next;
+	zvol_state_t *zv = NULL, *zv_next;
 
 	if (zvol_inhibit_dev)
 		return;
@@ -2049,12 +2049,11 @@ zvol_remove_minor_impl(const char *name)
 		}
 	}
 
+	/* Drop zvol_state_lock before calling zvol_free() */
 	mutex_exit(&zvol_state_lock);
 
-	/*
-	 * Drop zvol_state_lock before calling zvol_free()
-	 */
-	zvol_free(zv);
+	if (zv != NULL)
+		zvol_free(zv);
 }
 
 /*
@@ -2224,20 +2223,18 @@ zvol_set_snapdev_check(void *arg, dmu_tx_t *tx)
 	return (error);
 }
 
+/* ARGSUSED */
 static int
 zvol_set_snapdev_sync_cb(dsl_pool_t *dp, dsl_dataset_t *ds, void *arg)
 {
-	zvol_set_snapdev_arg_t *zsda = arg;
 	char dsname[MAXNAMELEN];
 	zvol_task_t *task;
+	uint64_t snapdev;
 
 	dsl_dataset_name(ds, dsname);
-	dsl_prop_set_sync_impl(ds, zfs_prop_to_name(ZFS_PROP_SNAPDEV),
-	    zsda->zsda_source, sizeof (zsda->zsda_value), 1,
-	    &zsda->zsda_value, zsda->zsda_tx);
-
-	task = zvol_task_alloc(ZVOL_ASYNC_SET_SNAPDEV, dsname,
-	    NULL, zsda->zsda_value);
+	if (dsl_prop_get_int_ds(ds, "snapdev", &snapdev) != 0)
+		return (0);
+	task = zvol_task_alloc(ZVOL_ASYNC_SET_SNAPDEV, dsname, NULL, snapdev);
 	if (task == NULL)
 		return (0);
 
@@ -2247,7 +2244,11 @@ zvol_set_snapdev_sync_cb(dsl_pool_t *dp, dsl_dataset_t *ds, void *arg)
 }
 
 /*
- * Traverse all child snapshot datasets and apply snapdev appropriately.
+ * Traverse all child datasets and apply snapdev appropriately.
+ * We call dsl_prop_set_sync_impl() here to set the value only on the toplevel
+ * dataset and read the effective "snapdev" on every child in the callback
+ * function: this is because the value is not guaranteed to be the same in the
+ * whole dataset hierarchy.
  */
 static void
 zvol_set_snapdev_sync(void *arg, dmu_tx_t *tx)
@@ -2255,10 +2256,19 @@ zvol_set_snapdev_sync(void *arg, dmu_tx_t *tx)
 	zvol_set_snapdev_arg_t *zsda = arg;
 	dsl_pool_t *dp = dmu_tx_pool(tx);
 	dsl_dir_t *dd;
+	dsl_dataset_t *ds;
+	int error;
 
 	VERIFY0(dsl_dir_hold(dp, zsda->zsda_name, FTAG, &dd, NULL));
 	zsda->zsda_tx = tx;
 
+	error = dsl_dataset_hold(dp, zsda->zsda_name, FTAG, &ds);
+	if (error == 0) {
+		dsl_prop_set_sync_impl(ds, zfs_prop_to_name(ZFS_PROP_SNAPDEV),
+		    zsda->zsda_source, sizeof (zsda->zsda_value), 1,
+		    &zsda->zsda_value, zsda->zsda_tx);
+		dsl_dataset_rele(ds, FTAG);
+	}
 	dmu_objset_find_dp(dp, dd->dd_object, zvol_set_snapdev_sync_cb,
 	    zsda, DS_FIND_CHILDREN);
 
diff --git a/tests/runfiles/linux.run b/tests/runfiles/linux.run
index 7e64c6714..674684f1f 100644
--- a/tests/runfiles/linux.run
+++ b/tests/runfiles/linux.run
@@ -564,7 +564,8 @@ tests = ['zvol_cli_001_pos', 'zvol_cli_002_pos', 'zvol_cli_003_neg']
 
 [tests/functional/zvol/zvol_misc]
 tests = ['zvol_misc_001_neg', 'zvol_misc_002_pos', 'zvol_misc_003_neg',
-    'zvol_misc_004_pos', 'zvol_misc_005_neg', 'zvol_misc_006_pos']
+    'zvol_misc_004_pos', 'zvol_misc_005_neg', 'zvol_misc_006_pos',
+    'zvol_misc_snapdev']
 
 [tests/functional/zvol/zvol_swap]
 tests = ['zvol_swap_001_pos', 'zvol_swap_002_pos', 'zvol_swap_003_pos',
diff --git a/tests/zfs-tests/tests/functional/zvol/zvol_misc/Makefile.am b/tests/zfs-tests/tests/functional/zvol/zvol_misc/Makefile.am
index b52088ccc..f72970490 100644
--- a/tests/zfs-tests/tests/functional/zvol/zvol_misc/Makefile.am
+++ b/tests/zfs-tests/tests/functional/zvol/zvol_misc/Makefile.am
@@ -7,4 +7,5 @@ dist_pkgdata_SCRIPTS = \
 	zvol_misc_003_neg.ksh \
 	zvol_misc_004_pos.ksh \
 	zvol_misc_005_neg.ksh \
-	zvol_misc_006_pos.ksh
+	zvol_misc_006_pos.ksh \
+	zvol_misc_snapdev.ksh
diff --git a/tests/zfs-tests/tests/functional/zvol/zvol_misc/zvol_misc_snapdev.ksh b/tests/zfs-tests/tests/functional/zvol/zvol_misc/zvol_misc_snapdev.ksh
new file mode 100755
index 000000000..57002fe65
--- /dev/null
+++ b/tests/zfs-tests/tests/functional/zvol/zvol_misc/zvol_misc_snapdev.ksh
@@ -0,0 +1,173 @@
+#!/bin/ksh -p
+#
+# CDDL HEADER START
+#
+# The contents of this file are subject to the terms of the
+# Common Development and Distribution License (the "License").
+# You may not use this file except in compliance with the License.
+#
+# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
+# or http://www.opensolaris.org/os/licensing.
+# See the License for the specific language governing permissions
+# and limitations under the License.
+#
+# When distributing Covered Code, include this CDDL HEADER in each
+# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
+# If applicable, add the following below this CDDL HEADER, with the
+# fields enclosed by brackets "[]" replaced with your own identifying
+# information: Portions Copyright [yyyy] [name of copyright owner]
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright 2017, loli10K <[email protected]>. All rights reserved.
+#
+
+. $STF_SUITE/include/libtest.shlib
+. $STF_SUITE/tests/functional/cli_root/zfs_set/zfs_set_common.kshlib
+. $STF_SUITE/tests/functional/zvol/zvol_common.shlib
+
+#
+# DESCRIPTION:
+# Verify that ZFS volume property "snapdev" works as intended.
+#
+# STRATEGY:
+# 1. Verify "snapdev" property does not accept invalid values
+# 2. Verify "snapdev" adds and removes device nodes when updated
+# 3. Verify "snapdev" is inherited correctly
+#
+
+verify_runnable "global"
+
+function cleanup
+{
+	datasetexists $VOLFS && log_must zfs destroy -r $VOLFS
+	datasetexists $ZVOL && log_must zfs destroy -r $ZVOL
+	log_must zfs inherit snapdev $TESTPOOL
+	block_device_wait
+}
+
+#
+# Verify $device exists and is a block device
+#
+function blockdev_exists # device
+{
+	typeset device="$1"
+
+	# we wait here instead of doing it in a wrapper around 'zfs set snapdev'
+	# because there are other commands (zfs snap, zfs inherit, zfs destroy)
+	# that can affect device nodes
+	block_device_wait
+
+	if [[ ! -b "$device" ]]; then
+		log_fail "$device does not exist as a block device"
+	fi
+}
+
+#
+# Verify $device does not exist
+#
+function check_missing # device
+{
+	typeset device="$1"
+
+	# we wait here instead of doing it in a wrapper around 'zfs set snapdev'
+	# because there are other commands (zfs snap, zfs inherit, zfs destroy)
+	# that can affect device nodes
+	block_device_wait
+
+	if [[ -e "$device" ]]; then
+		log_fail "$device exists when not expected"
+	fi
+}
+
+#
+# Verify $property on $dataset is inherited by $parent and is set to $value
+#
+function verify_inherited # property value dataset parent
+{
+	typeset property="$1"
+	typeset value="$2"
+	typeset dataset="$3"
+	typeset parent="$4"
+
+	typeset val=$(get_prop "$property" "$dataset")
+	typeset src=$(get_source "$property" "$dataset")
+	if [[ "$val" != "$value" || "$src" != "inherited from $parent" ]]
+	then
+		log_fail "Dataset $dataset did not inherit $property properly:"\
+		    "expected=$value, value=$val, source=$src."
+	fi
+
+}
+
+log_assert "Verify that ZFS volume property 'snapdev' works as expected."
+log_onexit cleanup
+
+VOLFS="$TESTPOOL/volfs"
+ZVOL="$TESTPOOL/vol"
+SNAP="$ZVOL@snap"
+SNAPDEV="${ZVOL_DEVDIR}/$SNAP"
+SUBZVOL="$VOLFS/subvol"
+SUBSNAP="$SUBZVOL@snap"
+SUBSNAPDEV="${ZVOL_DEVDIR}/$SUBSNAP"
+
+log_must zfs create -o mountpoint=none $VOLFS
+log_must zfs create -V $VOLSIZE -s $ZVOL
+log_must zfs create -V $VOLSIZE -s $SUBZVOL
+
+# 1. Verify "snapdev" property does not accept invalid values
+typeset badvals=("off" "on" "1" "nope" "-")
+for badval in ${badvals[@]}
+do
+	log_mustnot zfs set snapdev="$badval" $ZVOL
+done
+
+# 2. Verify "snapdev" adds and removes device nodes when updated
+# 2.1 First create a snapshot then change snapdev property
+log_must zfs snapshot $SNAP
+log_must zfs set snapdev=visible $ZVOL
+blockdev_exists $SNAPDEV
+log_must zfs set snapdev=hidden $ZVOL
+check_missing $SNAPDEV
+log_must zfs destroy $SNAP
+# 2.2 First set snapdev property then create a snapshot
+log_must zfs set snapdev=visible $ZVOL
+log_must zfs snapshot $SNAP
+blockdev_exists $SNAPDEV
+log_must zfs destroy $SNAP
+check_missing $SNAPDEV
+# 2.3 Verify setting to the same value multiple times does not lead to issues
+log_must zfs snapshot $SNAP
+log_must zfs set snapdev=visible $ZVOL
+blockdev_exists $SNAPDEV
+log_must zfs set snapdev=visible $ZVOL
+blockdev_exists $SNAPDEV
+log_must zfs set snapdev=hidden $ZVOL
+check_missing $SNAPDEV
+log_must zfs set snapdev=hidden $ZVOL
+check_missing $SNAPDEV
+log_must zfs destroy $SNAP
+
+# 3. Verify "snapdev" is inherited correctly
+# 3.1 Check snapdev=visible case
+log_must zfs snapshot $SNAP
+log_must zfs inherit snapdev $ZVOL
+log_must zfs set snapdev=visible $TESTPOOL
+verify_inherited 'snapdev' 'visible' $ZVOL $TESTPOOL
+blockdev_exists $SNAPDEV
+# 3.2 Check snapdev=hidden case
+log_must zfs set snapdev=hidden $TESTPOOL
+verify_inherited 'snapdev' 'hidden' $ZVOL $TESTPOOL
+check_missing $SNAPDEV
+# 3.3 Check inheritance on multiple levels
+log_must zfs snapshot $SUBSNAP
+log_must zfs inherit snapdev $SUBZVOL
+log_must zfs set snapdev=hidden $VOLFS
+log_must zfs set snapdev=visible $TESTPOOL
+verify_inherited 'snapdev' 'hidden' $SUBZVOL $VOLFS
+check_missing $SUBSNAPDEV
+blockdev_exists $SNAPDEV
+
+log_pass "ZFS volume property 'snapdev' works as expected"