13697 zfs change-key does not follow clones, data loss ensues

Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed by: Alek Pinchuk <apinchuk@datto.com>
Reviewed by: Andy Fiddaman <andy@omnios.org>
Reviewed by: Vitaliy Gusev <gusev.vitaliy@icloud.com>
Portions contributed by: Alex Wilson <alex@cooperi.net>
Portions contributed by: Jason King <jason.brian.king@gmail.com>
Approved by: Dan McDonald <danmcd@joyent.com>

8 files changed, 155 insertions, 27 deletions

diff --git a/usr/src/pkg/manifests/system-test-zfstest.mf b/usr/src/pkg/manifests/system-test-zfstest.mf
index 5b9783052c..c6b7b17987 100644
--- a/usr/src/pkg/manifests/system-test-zfstest.mf
+++ b/usr/src/pkg/manifests/system-test-zfstest.mf
@@ -738,6 +738,9 @@ file \
     path=opt/zfs-tests/tests/functional/cli_root/zfs_change-key/zfs_change-key_child \
     mode=0555
 file \
+    path=opt/zfs-tests/tests/functional/cli_root/zfs_change-key/zfs_change-key_clones \
+    mode=0555
+file \
     path=opt/zfs-tests/tests/functional/cli_root/zfs_change-key/zfs_change-key_format \
     mode=0555
 file \
diff --git a/usr/src/test/zfs-tests/runfiles/delphix.run b/usr/src/test/zfs-tests/runfiles/delphix.run
index 3d1e71cb36..a4297b7c13 100644
--- a/usr/src/test/zfs-tests/runfiles/delphix.run
+++ b/usr/src/test/zfs-tests/runfiles/delphix.run
@@ -116,7 +116,7 @@ tests = ['zfs_001_neg', 'zfs_002_pos', 'zfs_003_neg']
 [/opt/zfs-tests/tests/functional/cli_root/zfs_change-key]
 tests = ['zfs_change-key', 'zfs_change-key_child', 'zfs_change-key_format',
     'zfs_change-key_inherit', 'zfs_change-key_load', 'zfs_change-key_location',
-    'zfs_change-key_pbkdf2iters']
+    'zfs_change-key_pbkdf2iters', 'zfs_change-key_clones']
 
 [/opt/zfs-tests/tests/functional/cli_root/zfs_clone]
 tests = ['zfs_clone_001_neg', 'zfs_clone_002_pos', 'zfs_clone_003_pos',
diff --git a/usr/src/test/zfs-tests/runfiles/omnios.run b/usr/src/test/zfs-tests/runfiles/omnios.run
index 6db4f62e66..0b19ca7163 100644
--- a/usr/src/test/zfs-tests/runfiles/omnios.run
+++ b/usr/src/test/zfs-tests/runfiles/omnios.run
@@ -118,7 +118,7 @@ tests = ['zfs_001_neg', 'zfs_002_pos', 'zfs_003_neg']
 [/opt/zfs-tests/tests/functional/cli_root/zfs_change-key]
 tests = ['zfs_change-key', 'zfs_change-key_child', 'zfs_change-key_format',
     'zfs_change-key_inherit', 'zfs_change-key_load', 'zfs_change-key_location',
-    'zfs_change-key_pbkdf2iters']
+    'zfs_change-key_pbkdf2iters', 'zfs_change-key_clones']
 
 [/opt/zfs-tests/tests/functional/cli_root/zfs_clone]
 tests = ['zfs_clone_001_neg', 'zfs_clone_002_pos', 'zfs_clone_003_pos',
diff --git a/usr/src/test/zfs-tests/runfiles/openindiana.run b/usr/src/test/zfs-tests/runfiles/openindiana.run
index 426b215a60..8d003a8110 100644
--- a/usr/src/test/zfs-tests/runfiles/openindiana.run
+++ b/usr/src/test/zfs-tests/runfiles/openindiana.run
@@ -118,7 +118,7 @@ tests = ['zfs_001_neg', 'zfs_002_pos', 'zfs_003_neg']
 [/opt/zfs-tests/tests/functional/cli_root/zfs_change-key]
 tests = ['zfs_change-key', 'zfs_change-key_child', 'zfs_change-key_format',
     'zfs_change-key_inherit', 'zfs_change-key_load', 'zfs_change-key_location',
-    'zfs_change-key_pbkdf2iters']
+    'zfs_change-key_pbkdf2iters', 'zfs_change-key_clones']
 
 [/opt/zfs-tests/tests/functional/cli_root/zfs_clone]
 tests = ['zfs_clone_001_neg', 'zfs_clone_002_pos', 'zfs_clone_003_pos',
diff --git a/usr/src/test/zfs-tests/runfiles/smartos.run b/usr/src/test/zfs-tests/runfiles/smartos.run
index 375e894870..d68b6d1250 100644
--- a/usr/src/test/zfs-tests/runfiles/smartos.run
+++ b/usr/src/test/zfs-tests/runfiles/smartos.run
@@ -80,7 +80,7 @@ tests = ['zfs_001_neg', 'zfs_002_pos', 'zfs_003_neg']
 [/opt/zfs-tests/tests/functional/cli_root/zfs_change-key]
 tests = ['zfs_change-key', 'zfs_change-key_child', 'zfs_change-key_format',
     'zfs_change-key_inherit', 'zfs_change-key_load', 'zfs_change-key_location',
-    'zfs_change-key_pbkdf2iters']
+    'zfs_change-key_pbkdf2iters', 'zfs_change-key_clones']
 
 [/opt/zfs-tests/tests/functional/cli_root/zfs_clone]
 tests = ['zfs_clone_001_neg', 'zfs_clone_002_pos', 'zfs_clone_003_pos',
diff --git a/usr/src/test/zfs-tests/tests/functional/cli_root/zfs_change-key/zfs_change-key_clones.ksh b/usr/src/test/zfs-tests/tests/functional/cli_root/zfs_change-key/zfs_change-key_clones.ksh
new file mode 100755
index 0000000000..497fb99c81
--- /dev/null
+++ b/usr/src/test/zfs-tests/tests/functional/cli_root/zfs_change-key/zfs_change-key_clones.ksh
@@ -0,0 +1,80 @@
+#!/bin/ksh -p
+#
+# CDDL HEADER START
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# http://www.illumos.org/license/CDDL.
+#
+# CDDL HEADER END
+#
+
+#
+# Copyright (c) 2017 Datto, Inc. All rights reserved.
+#
+
+. $STF_SUITE/include/libtest.shlib
+. $STF_SUITE/tests/functional/cli_root/zfs_load-key/zfs_load-key_common.kshlib
+
+#
+# DESCRIPTION:
+# 'zfs change-key' should correctly update encryption roots with clones.
+#
+# STRATEGY:
+# 1. Create an encrypted dataset
+# 2. Create an encryption root child of the first dataset
+# 3. Clone the child encryption root twice
+# 4. Add inheriting children to the encryption root and each of the clones
+# 5. Verify the encryption roots
+# 6. Have the child encryption root inherit from its parent
+# 7. Verify the encryption root for all datasets is now the parent dataset
+#
+
+verify_runnable "both"
+
+function cleanup
+{
+	datasetexists $TESTPOOL/$TESTFS1 && \
+		log_must zfs destroy -Rf $TESTPOOL/$TESTFS1
+}
+
+log_onexit cleanup
+
+log_assert "'zfs change-key' should correctly update encryption " \
+	"roots with clones"
+
+log_must eval "echo $PASSPHRASE1 | zfs create -o encryption=on" \
+	"-o keyformat=passphrase -o keylocation=prompt $TESTPOOL/$TESTFS1"
+log_must eval "echo $PASSPHRASE2 | zfs create -o encryption=on" \
+	"-o keyformat=passphrase -o keylocation=prompt $TESTPOOL/$TESTFS1/child"
+log_must zfs snapshot $TESTPOOL/$TESTFS1/child@1
+log_must zfs clone $TESTPOOL/$TESTFS1/child@1 $TESTPOOL/$TESTFS1/clone1
+log_must zfs clone $TESTPOOL/$TESTFS1/child@1 $TESTPOOL/$TESTFS1/clone2
+log_must zfs create $TESTPOOL/$TESTFS1/child/A
+log_must zfs create $TESTPOOL/$TESTFS1/clone1/B
+log_must zfs create $TESTPOOL/$TESTFS1/clone2/C
+
+log_must verify_encryption_root $TESTPOOL/$TESTFS1 $TESTPOOL/$TESTFS1
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/child $TESTPOOL/$TESTFS1/child
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/clone1 $TESTPOOL/$TESTFS1/child
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/clone2 $TESTPOOL/$TESTFS1/child
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/child/A $TESTPOOL/$TESTFS1/child
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/clone1/B $TESTPOOL/$TESTFS1/child
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/clone2/C $TESTPOOL/$TESTFS1/child
+
+log_must zfs change-key -i $TESTPOOL/$TESTFS1/child
+
+log_must verify_encryption_root $TESTPOOL/$TESTFS1 $TESTPOOL/$TESTFS1
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/child $TESTPOOL/$TESTFS1
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/clone1 $TESTPOOL/$TESTFS1
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/clone2 $TESTPOOL/$TESTFS1
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/child/A $TESTPOOL/$TESTFS1
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/clone1/B $TESTPOOL/$TESTFS1
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/clone2/C $TESTPOOL/$TESTFS1
+
+log_pass "'zfs change-key' correctly updates encryption roots with clones"
diff --git a/usr/src/test/zfs-tests/tests/functional/cli_root/zfs_promote/zfs_promote_encryptionroot.ksh b/usr/src/test/zfs-tests/tests/functional/cli_root/zfs_promote/zfs_promote_encryptionroot.ksh
index 336c7b2538..2c7584d354 100644
--- a/usr/src/test/zfs-tests/tests/functional/cli_root/zfs_promote/zfs_promote_encryptionroot.ksh
+++ b/usr/src/test/zfs-tests/tests/functional/cli_root/zfs_promote/zfs_promote_encryptionroot.ksh
@@ -29,11 +29,12 @@
 # 1. Create an encrypted dataset
 # 2. Clone the encryption root
 # 3. Clone the clone
-# 4. Verify the encryption root of all three datasets is the origin
+# 4. Add children to each of these three datasets
+# 4. Verify the encryption root of all datasets is the origin
 # 5. Promote the clone of the clone
-# 6. Verify the encryption root of all three datasets is still the origin
-# 7. Promote the clone of the original encryption root
-# 8. Verify the encryption root of all three datasets is the promoted dataset
+# 6. Verify the encryption root of all datasets is still the origin
+# 7. Promote the dataset again, so it is now the encryption root
+# 8. Verify the encryption root of all datasets is the promoted dataset
 #
 
 verify_runnable "both"
@@ -62,19 +63,31 @@ log_must zfs snap $snaproot
 log_must zfs clone $snaproot $TESTPOOL/clone1
 log_must zfs snap $snapclone
 log_must zfs clone $snapclone $TESTPOOL/clone2
+log_must zfs create $TESTPOOL/$TESTFS1/child0
+log_must zfs create $TESTPOOL/clone1/child1
+log_must zfs create $TESTPOOL/clone2/child2
 
 log_must verify_encryption_root $TESTPOOL/$TESTFS1 $TESTPOOL/$TESTFS1
 log_must verify_encryption_root $TESTPOOL/clone1 $TESTPOOL/$TESTFS1
 log_must verify_encryption_root $TESTPOOL/clone2 $TESTPOOL/$TESTFS1
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/child0 $TESTPOOL/$TESTFS1
+log_must verify_encryption_root $TESTPOOL/clone1/child1 $TESTPOOL/$TESTFS1
+log_must verify_encryption_root $TESTPOOL/clone2/child2 $TESTPOOL/$TESTFS1
 
 log_must zfs promote $TESTPOOL/clone2
 log_must verify_encryption_root $TESTPOOL/$TESTFS1 $TESTPOOL/$TESTFS1
 log_must verify_encryption_root $TESTPOOL/clone1 $TESTPOOL/$TESTFS1
 log_must verify_encryption_root $TESTPOOL/clone2 $TESTPOOL/$TESTFS1
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/child0 $TESTPOOL/$TESTFS1
+log_must verify_encryption_root $TESTPOOL/clone1/child1 $TESTPOOL/$TESTFS1
+log_must verify_encryption_root $TESTPOOL/clone2/child2 $TESTPOOL/$TESTFS1
 
 log_must zfs promote $TESTPOOL/clone2
 log_must verify_encryption_root $TESTPOOL/$TESTFS1 $TESTPOOL/clone2
 log_must verify_encryption_root $TESTPOOL/clone1 $TESTPOOL/clone2
 log_must verify_encryption_root $TESTPOOL/clone2 $TESTPOOL/clone2
+log_must verify_encryption_root $TESTPOOL/$TESTFS1/child0 $TESTPOOL/clone2
+log_must verify_encryption_root $TESTPOOL/clone1/child1 $TESTPOOL/clone2
+log_must verify_encryption_root $TESTPOOL/clone2/child2 $TESTPOOL/clone2
 
 log_pass "ZFS promotes clones of an encryption root"
diff --git a/usr/src/uts/common/fs/zfs/dsl_crypt.c b/usr/src/uts/common/fs/zfs/dsl_crypt.c
index c9d02e1c57..cb412151e7 100644
--- a/usr/src/uts/common/fs/zfs/dsl_crypt.c
+++ b/usr/src/uts/common/fs/zfs/dsl_crypt.c
@@ -1396,10 +1396,17 @@ error:
 	return (ret);
 }
 
-
+/*
+ * This function deals with the intricacies of updating wrapping
+ * key references and encryption roots recursively in the event
+ * of a call to 'zfs change-key' or 'zfs promote'. The 'skip'
+ * parameter should always be set to B_FALSE when called
+ * externally.
+ */
 static void
 spa_keystore_change_key_sync_impl(uint64_t rddobj, uint64_t ddobj,
-    uint64_t new_rddobj, dsl_wrapping_key_t *wkey, dmu_tx_t *tx)
+    uint64_t new_rddobj, dsl_wrapping_key_t *wkey, boolean_t skip,
+    dmu_tx_t *tx)
 {
 	int ret;
 	zap_cursor_t *zc;
@@ -1414,7 +1421,7 @@ spa_keystore_change_key_sync_impl(uint64_t rddobj, uint64_t ddobj,
 	/* hold the dd */
 	VERIFY0(dsl_dir_hold_obj(dp, ddobj, NULL, FTAG, &dd));
 
-	/* ignore hidden dsl dirs */
+	/* ignore special dsl dirs */
 	if (dd->dd_myname[0] == '$' || dd->dd_myname[0] == '%') {
 		dsl_dir_rele(dd, FTAG);
 		return;
@@ -1427,7 +1434,8 @@ spa_keystore_change_key_sync_impl(uint64_t rddobj, uint64_t ddobj,
 	 * Stop recursing if this dsl dir didn't inherit from the root
 	 * or if this dd is a clone.
 	 */
-	if (ret == ENOENT || curr_rddobj != rddobj || dsl_dir_is_clone(dd)) {
+	if (ret == ENOENT ||
+	    (!skip && (curr_rddobj != rddobj || dsl_dir_is_clone(dd)))) {
 		dsl_dir_rele(dd, FTAG);
 		return;
 	}
@@ -1435,19 +1443,23 @@ spa_keystore_change_key_sync_impl(uint64_t rddobj, uint64_t ddobj,
 	/*
 	 * If we don't have a wrapping key just update the dck to reflect the
 	 * new encryption root. Otherwise rewrap the entire dck and re-sync it
-	 * to disk.
+	 * to disk. If skip is set, we don't do any of this work.
 	 */
-	if (wkey == NULL) {
-		VERIFY0(zap_update(dp->dp_meta_objset, dd->dd_crypto_obj,
-		    DSL_CRYPTO_KEY_ROOT_DDOBJ, 8, 1, &new_rddobj, tx));
-	} else {
-		VERIFY0(spa_keystore_dsl_key_hold_dd(dp->dp_spa, dd,
-		    FTAG, &dck));
-		dsl_wrapping_key_hold(wkey, dck);
-		dsl_wrapping_key_rele(dck->dck_wkey, dck);
-		dck->dck_wkey = wkey;
-		dsl_crypto_key_sync(dck, tx);
-		spa_keystore_dsl_key_rele(dp->dp_spa, dck, FTAG);
+	if (!skip) {
+		if (wkey == NULL) {
+			VERIFY0(zap_update(dp->dp_meta_objset,
+			    dd->dd_crypto_obj,
+			    DSL_CRYPTO_KEY_ROOT_DDOBJ, 8, 1,
+			    &new_rddobj, tx));
+		} else {
+			VERIFY0(spa_keystore_dsl_key_hold_dd(dp->dp_spa, dd,
+			    FTAG, &dck));
+			dsl_wrapping_key_hold(wkey, dck);
+			dsl_wrapping_key_rele(dck->dck_wkey, dck);
+			dck->dck_wkey = wkey;
+			dsl_crypto_key_sync(dck, tx);
+			spa_keystore_dsl_key_rele(dp->dp_spa, dck, FTAG);
+		}
 	}
 
 	zc = kmem_alloc(sizeof (zap_cursor_t), KM_SLEEP);
@@ -1459,7 +1471,27 @@ spa_keystore_change_key_sync_impl(uint64_t rddobj, uint64_t ddobj,
 	    zap_cursor_retrieve(zc, za) == 0;
 	    zap_cursor_advance(zc)) {
 		spa_keystore_change_key_sync_impl(rddobj,
-		    za->za_first_integer, new_rddobj, wkey, tx);
+		    za->za_first_integer, new_rddobj, wkey, B_FALSE, tx);
+	}
+	zap_cursor_fini(zc);
+
+	/*
+	 * Recurse into all dsl dirs of clones. We utilize the skip parameter
+	 * here so that we don't attempt to process the clones directly. This
+	 * is because the clone and its origin share the same dck, which has
+	 * already been updated.
+	 */
+	for (zap_cursor_init(zc, dp->dp_meta_objset,
+	    dsl_dir_phys(dd)->dd_clones);
+	    zap_cursor_retrieve(zc, za) == 0;
+	    zap_cursor_advance(zc)) {
+		dsl_dataset_t *clone;
+
+		VERIFY0(dsl_dataset_hold_obj(dp, za->za_first_integer,
+		    FTAG, &clone));
+		spa_keystore_change_key_sync_impl(rddobj,
+		    clone->ds_dir->dd_object, new_rddobj, wkey, B_TRUE, tx);
+		dsl_dataset_rele(clone, FTAG);
 	}
 	zap_cursor_fini(zc);
 
@@ -1539,7 +1571,7 @@ spa_keystore_change_key_sync(void *arg, dmu_tx_t *tx)
 
 	/* recurse through all children and rewrap their keys */
 	spa_keystore_change_key_sync_impl(rddobj, ds->ds_dir->dd_object,
-	    new_rddobj, wkey, tx);
+	    new_rddobj, wkey, B_FALSE, tx);
 
 	/*
 	 * All references to the old wkey should be released now (if it
@@ -1713,7 +1745,7 @@ dsl_dataset_promote_crypt_sync(dsl_dir_t *target, dsl_dir_t *origin,
 
 	rw_enter(&dp->dp_spa->spa_keystore.sk_wkeys_lock, RW_WRITER);
 	spa_keystore_change_key_sync_impl(rddobj, origin->dd_object,
-	    target->dd_object, NULL, tx);
+	    target->dd_object, NULL, B_FALSE, tx);
 	rw_exit(&dp->dp_spa->spa_keystore.sk_wkeys_lock);
 
 	dsl_dataset_rele(targetds, FTAG);