Fix issues found with zfs diff

Two deadlocks / ASSERT failures were introduced in a2c2ed1b which
would occur whenever arc_buf_fill() failed to decrypt a block of
data. This occurred because the call to arc_buf_destroy() which
was responsible for cleaning up the newly created buffer would
attempt to take out the hdr lock that it was already holding. This
was resolved by calling the underlying functions directly without
retaking the lock.

In addition, the dmu_diff() code did not properly ensure that keys
were loaded and mapped before begining dataset traversal. It turns
out that this code does not need to look at any encrypted values,
so the code was altered to perform raw IO only.

Reviewed by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Tom Caputi <tcaputi@datto.com>
Closes #7354
Closes #7456

diff --git a/module/zfs/arc.c b/module/zfs/arc.c
index 040e943655f44614623b0c1087fe9f8a4e57bd11..fa7f62d9906a6a3d994d233ec1a928e3347189ad 100644 (file)
--- a/module/zfs/arc.c
+++ b/module/zfs/arc.c
@@ -2132,8 +2132,10 @@ arc_buf_fill(arc_buf_t *buf, spa_t *spa, uint64_t dsobj, arc_fill_flags_t flags)
        if (HDR_PROTECTED(hdr)) {
                error = arc_fill_hdr_crypt(hdr, hash_lock, spa,
                    dsobj, !!(flags & ARC_FILL_NOAUTH));
-               if (error != 0)
+               if (error != 0) {
+                       arc_hdr_set_flags(hdr, ARC_FLAG_IO_ERROR);
                        return (error);
+               }
        }
 
        /*
@@ -2234,6 +2236,7 @@ arc_buf_fill(arc_buf_t *buf, spa_t *spa, uint64_t dsobj, arc_fill_flags_t flags)
                                    "hdr %p, compress %d, psize %d, lsize %d",
                                    hdr, arc_hdr_get_compress(hdr),
                                    HDR_GET_PSIZE(hdr), HDR_GET_LSIZE(hdr));
+                               arc_hdr_set_flags(hdr, ARC_FLAG_IO_ERROR);
                                return (SET_ERROR(EIO));
                        }
                }
@@ -5815,7 +5818,9 @@ arc_read_done(zio_t *zio)
                    acb->acb_compressed, acb->acb_noauth, B_TRUE,
                    &acb->acb_buf);
                if (error != 0) {
-                       arc_buf_destroy(acb->acb_buf, acb->acb_private);
+                       (void) remove_reference(hdr, hash_lock,
+                           acb->acb_private);
+                       arc_buf_destroy_impl(acb->acb_buf);
                        acb->acb_buf = NULL;
                }
 
@@ -6058,7 +6063,9 @@ top:
                                    spa, NULL, zb, NULL, 0, 0);
                        }
                        if (rc != 0) {
-                               arc_buf_destroy(buf, private);
+                               (void) remove_reference(hdr, hash_lock,
+                                   private);
+                               arc_buf_destroy_impl(buf);
                                buf = NULL;
                        }
 

diff --git a/module/zfs/dmu_diff.c b/module/zfs/dmu_diff.c
index 982b96132cc8f497a73820a5798c95f1c8d665e5..76c32b1264232dda0b757f43a6869eebce236743 100644 (file)
--- a/module/zfs/dmu_diff.c
+++ b/module/zfs/dmu_diff.c
@@ -131,11 +131,14 @@ diff_cb(spa_t *spa, zilog_t *zilog, const blkptr_t *bp,
                arc_buf_t *abuf;
                arc_flags_t aflags = ARC_FLAG_WAIT;
                int blksz = BP_GET_LSIZE(bp);
+               int zio_flags = ZIO_FLAG_CANFAIL;
                int i;
 
+               if (BP_IS_PROTECTED(bp))
+                       zio_flags |= ZIO_FLAG_RAW;
+
                if (arc_read(NULL, spa, bp, arc_getbuf_func, &abuf,
-                   ZIO_PRIORITY_ASYNC_READ, ZIO_FLAG_CANFAIL,
-                   &aflags, zb) != 0)
+                   ZIO_PRIORITY_ASYNC_READ, zio_flags, &aflags, zb) != 0)
                        return (SET_ERROR(EIO));
 
                blk = abuf->b_data;
@@ -206,8 +209,17 @@ dmu_diff(const char *tosnap_name, const char *fromsnap_name,
        da.da_ddr.ddr_first = da.da_ddr.ddr_last = 0;
        da.da_err = 0;
 
+       /*
+        * Since zfs diff only looks at dnodes which are stored in plaintext
+        * (other than bonus buffers), we don't technically need to decrypt
+        * the dataset to perform this operation. However, the command line
+        * utility will still fail if the keys are not loaded because the
+        * dataset isn't mounted and because it will fail when it attempts to
+        * call the ZFS_IOC_OBJ_TO_STATS ioctl.
+        */
        error = traverse_dataset(tosnap, fromtxg,
-           TRAVERSE_PRE | TRAVERSE_PREFETCH_METADATA, diff_cb, &da);
+           TRAVERSE_PRE | TRAVERSE_PREFETCH_METADATA | TRAVERSE_NO_DECRYPT,
+           diff_cb, &da);
 
        if (error != 0) {
                da.da_err = error;

diff --git a/tests/runfiles/linux.run b/tests/runfiles/linux.run
index 6dc4df96cffe7f3ddfdb98ef022ecfefbf5b73e0..62dfcf44e5746bfd6988e42bb4de996ba3eca807 100644 (file)
--- a/tests/runfiles/linux.run
+++ b/tests/runfiles/linux.run
@@ -150,7 +150,7 @@ tags = ['functional', 'cli_root', 'zfs_destroy']
 
 [tests/functional/cli_root/zfs_diff]
 tests = ['zfs_diff_changes', 'zfs_diff_cliargs', 'zfs_diff_timestamp',
-    'zfs_diff_types']
+    'zfs_diff_types', 'zfs_diff_encrypted']
 tags = ['functional', 'cli_root', 'zfs_diff']
 
 [tests/functional/cli_root/zfs_get]

diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_diff/Makefile.am b/tests/zfs-tests/tests/functional/cli_root/zfs_diff/Makefile.am
index afb45371ab2b2747fcd6ec76e7fbafaebaaae6de..db90e058559d94601ab202b2500e56d176c51113 100644 (file)
--- a/tests/zfs-tests/tests/functional/cli_root/zfs_diff/Makefile.am
+++ b/tests/zfs-tests/tests/functional/cli_root/zfs_diff/Makefile.am
@@ -7,6 +7,7 @@ dist_pkgdata_SCRIPTS = \
        setup.ksh \
        zfs_diff_changes.ksh \
        zfs_diff_cliargs.ksh \
+       zfs_diff_encrypted.ksh \
        zfs_diff_timestamp.ksh \
        zfs_diff_types.ksh
 

diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_diff/zfs_diff_encrypted.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_diff/zfs_diff_encrypted.ksh
new file mode 100755 (executable)
index 0000000..471e9ca
--- /dev/null
+++ b/tests/zfs-tests/tests/functional/cli_root/zfs_diff/zfs_diff_encrypted.ksh
@@ -0,0 +1,53 @@
+#!/bin/ksh -p
+#
+# 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.
+#
+
+#
+# Copyright 2018, Datto Inc.
+#
+
+. $STF_SUITE/include/libtest.shlib
+
+#
+# DESCRIPTION:
+# 'zfs diff' should work with encrypted datasets
+#
+# STRATEGY:
+# 1. Create an encrypted dataset
+# 2. Create two snapshots of the dataset
+# 3. Perform 'zfs diff -Ft' and verify no errors occur
+#
+
+verify_runnable "both"
+
+function cleanup
+{
+       datasetexists $TESTPOOL/$TESTFS1 && \
+               log_must zfs destroy -r $TESTPOOL/$TESTFS1
+}
+
+log_assert "'zfs diff' should work with encrypted datasets"
+log_onexit cleanup
+
+# 1. Create an encrypted dataset
+log_must eval "echo 'password' | zfs create -o encryption=on \
+       -o keyformat=passphrase $TESTPOOL/$TESTFS1"
+MNTPOINT="$(get_prop mountpoint $TESTPOOL/$TESTFS1)"
+
+# 2. Create two snapshots of the dataset
+log_must zfs snapshot $TESTPOOL/$TESTFS1@snap1
+log_must touch "$MNTPOINT/file"
+log_must zfs snapshot $TESTPOOL/$TESTFS1@snap2
+
+# 3. Perform 'zfs diff' and verify no errors occur
+log_must zfs diff -Ft $TESTPOOL/$TESTFS1@snap1 $TESTPOOL/$TESTFS1@snap2
+
+log_pass "'zfs diff' works with encrypted datasets"