Fix clone handling with encryption roots

Currently, spa_keystore_change_key_sync_impl() does not recurse
into clones when updating encryption roots for either a call to
'zfs promote' or 'zfs change-key'. This can cause children of
these clones to end up in a state where they point to the wrong
dataset as the encryption root. It can also trigger ASSERTs in
some cases where the code checks reference counts on wrapping
keys. This patch fixes this issue by ensuring that this function
properly recurses into clones during processing.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alek Pinchuk <[email protected]>
Signed-off-by: Tom Caputi <[email protected]>
Closes #9267 
Closes #9294 

1 files changed, 50 insertions, 19 deletions

diff --git a/module/zfs/dsl_crypt.c b/module/zfs/dsl_crypt.c
index abe724eed..327d3ee91 100644
--- a/module/zfs/dsl_crypt.c
+++ b/module/zfs/dsl_crypt.c
@@ -1418,10 +1418,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)
 {
 	zap_cursor_t *zc;
 	zap_attribute_t *za;
@@ -1435,7 +1442,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;
@@ -1446,7 +1453,7 @@ spa_keystore_change_key_sync_impl(uint64_t rddobj, uint64_t ddobj,
 	 * or if this dd is a clone.
 	 */
 	VERIFY0(dsl_dir_get_encryption_root_ddobj(dd, &curr_rddobj));
-	if (curr_rddobj != rddobj || dsl_dir_is_clone(dd)) {
+	if (!skip && (curr_rddobj != rddobj || dsl_dir_is_clone(dd))) {
 		dsl_dir_rele(dd, FTAG);
 		return;
 	}
@@ -1454,19 +1461,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);
@@ -1478,7 +1489,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);
 
@@ -1558,7 +1589,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
@@ -1736,7 +1767,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);