Handle and detect #13709's unlock regression (#14161)

In #13709, as in #11294 before it, it turns out that 63a26454 still had
the same failure mode as when it was first landed as d1d47691, and
fails to unlock certain datasets that formerly worked.

Rather than reverting it again, let's add handling to just throw out
the accounting metadata that failed to unlock when that happens, as
well as a test with a pre-broken pool image to ensure that we never get
bitten by this again.

Fixes: #13709
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>

diff --git a/module/zfs/dsl_crypt.c b/module/zfs/dsl_crypt.c
index ce2e6ce742a2a11a64c0b1dbd802704f9f354b04..382de208b01d1ef88e837a1cb6e2e53332a5c6a2 100644 (file)
--- a/module/zfs/dsl_crypt.c
+++ b/module/zfs/dsl_crypt.c
@@ -2671,6 +2671,7 @@ spa_do_crypt_objset_mac_abd(boolean_t generate, spa_t *spa, uint64_t dsobj,
        objset_phys_t *osp = buf;
        uint8_t portable_mac[ZIO_OBJSET_MAC_LEN];
        uint8_t local_mac[ZIO_OBJSET_MAC_LEN];
+       const uint8_t zeroed_mac[ZIO_OBJSET_MAC_LEN] = {0};
 
        /* look up the key from the spa's keystore */
        ret = spa_keystore_lookup_key(spa, dsobj, FTAG, &dck);
@@ -2696,8 +2697,21 @@ spa_do_crypt_objset_mac_abd(boolean_t generate, spa_t *spa, uint64_t dsobj,
        if (memcmp(portable_mac, osp->os_portable_mac,
            ZIO_OBJSET_MAC_LEN) != 0 ||
            memcmp(local_mac, osp->os_local_mac, ZIO_OBJSET_MAC_LEN) != 0) {
-               abd_return_buf(abd, buf, datalen);
-               return (SET_ERROR(ECKSUM));
+               /*
+                * If the MAC is zeroed out, we failed to decrypt it.
+                * This should only arise, at least on Linux,
+                * if we hit edge case handling for useraccounting, since we
+                * shouldn't get here without bailing out on error earlier
+                * otherwise.
+                *
+                * So if we're in that case, we can just fall through and
+                * special-casing noticing that it's zero will handle it
+                * elsewhere, since we can just regenerate it.
+                */
+               if (memcmp(local_mac, zeroed_mac, ZIO_OBJSET_MAC_LEN) != 0) {
+                       abd_return_buf(abd, buf, datalen);
+                       return (SET_ERROR(ECKSUM));
+               }
        }
 
        abd_return_buf(abd, buf, datalen);

diff --git a/tests/runfiles/common.run b/tests/runfiles/common.run
index 323c37a3d17a6b43eaf12b12b19bfcc44826765f..73ca69993149699c325d5da714e2d95fb58ef410 100644 (file)
--- a/tests/runfiles/common.run
+++ b/tests/runfiles/common.run
@@ -911,7 +911,7 @@ tests = [
     'userquota_007_pos', 'userquota_008_pos', 'userquota_009_pos',
     'userquota_010_pos', 'userquota_011_pos', 'userquota_012_neg',
     'userspace_001_pos', 'userspace_002_pos', 'userspace_encrypted',
-    'userspace_send_encrypted']
+    'userspace_send_encrypted', 'userspace_encrypted_13709']
 tags = ['functional', 'userquota']
 
 [tests/functional/vdev_zaps]

diff --git a/tests/zfs-tests/tests/Makefile.am b/tests/zfs-tests/tests/Makefile.am
index 5b8458b73180f04107abb21406628235cc24781b..39eb44f73171d47d236b5b502ccf0a23a94ddbd5 100644 (file)
--- a/tests/zfs-tests/tests/Makefile.am
+++ b/tests/zfs-tests/tests/Makefile.am
@@ -368,6 +368,7 @@ nobase_dist_datadir_zfs_tests_tests_DATA += \
        functional/upgrade/upgrade_common.kshlib \
        functional/user_namespace/user_namespace.cfg \
        functional/user_namespace/user_namespace_common.kshlib \
+       functional/userquota/13709_reproducer.bz2 \
        functional/userquota/userquota.cfg \
        functional/userquota/userquota_common.kshlib \
        functional/vdev_zaps/vdev_zaps.kshlib \
@@ -1935,6 +1936,7 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \
        functional/userquota/userspace_003_pos.ksh \
        functional/userquota/userspace_encrypted.ksh \
        functional/userquota/userspace_send_encrypted.ksh \
+       functional/userquota/userspace_encrypted_13709.ksh \
        functional/vdev_zaps/cleanup.ksh \
        functional/vdev_zaps/setup.ksh \
        functional/vdev_zaps/vdev_zaps_001_pos.ksh \

diff --git a/tests/zfs-tests/tests/functional/userquota/13709_reproducer.bz2 b/tests/zfs-tests/tests/functional/userquota/13709_reproducer.bz2
new file mode 100644 (file)
index 0000000..9c31682
Binary files /dev/null and b/tests/zfs-tests/tests/functional/userquota/13709_reproducer.bz2 differ

diff --git a/tests/zfs-tests/tests/functional/userquota/userspace_encrypted_13709.ksh b/tests/zfs-tests/tests/functional/userquota/userspace_encrypted_13709.ksh
new file mode 100755 (executable)
index 0000000..9c1d847
--- /dev/null
+++ b/tests/zfs-tests/tests/functional/userquota/userspace_encrypted_13709.ksh
@@ -0,0 +1,45 @@
+#!/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.
+#
+
+. $STF_SUITE/include/libtest.shlib
+. $STF_SUITE/tests/functional/userquota/userquota_common.kshlib
+
+#
+# DESCRIPTION:
+# Avoid allowing #11294/#13709 to recur a third time.
+#
+# So we hardcode a copy of a pool with this bug, try unlocking it,
+# and fail on error. Simple.
+
+function cleanup
+{
+       destroy_pool $POOLNAME
+       rm -f $FILEDEV
+}
+
+log_onexit cleanup
+
+FILEDEV="$TEST_BASE_DIR/userspace_13709"
+POOLNAME="testpool_13709"
+
+log_assert "ZFS should be able to unlock pools with #13709's failure mode"
+
+log_must bzcat $STF_SUITE/tests/functional/userquota/13709_reproducer.bz2 > $FILEDEV
+
+log_must zpool import -d $FILEDEV $POOLNAME
+
+echo -e 'password\npassword\n' | log_must zfs mount -al
+
+# Cleanup
+cleanup
+
+log_pass "#13709 not happening here"