From 0a02bce554ea50930c68fc1e103c76c1e36135c1 Mon Sep 17 00:00:00 2001 From: ilovezfs Date: Mon, 25 Jan 2016 23:41:11 -0800 Subject: [PATCH 1/4] 6541 dedup=[cksum],verify defeated feature check zio_checksum_to_feature() expects a zio_checksum enum not a raw property intval, so the new checksums weren't being detected when the ZIO_CHECKSUM_VERIFY flag got in the way. Given a pool without feature@sha512, zfs create -o dedup=sha512 naughty/fivetwelve_noverify_ds would fail as expected since the raw intval would indeed be equal to SPA_FEATURE_SHA512. However, zfs create -o dedup=sha512,verify naughty/fivetwelve_verify_ds would incorrectly succeed because ZIO_CHECKSUM_VERIFY would be in the way, the raw intval would not be a member of the enum, and zio_checksum_to_feature() would return SPA_FEATURE_NONE, with the result that spa_feature_is_enabled() would never be called. This was first detected with edonr, since in that case verify is required. This commit clears the ZIO_CHECKSUM_VERIFY flag before calling zio_checksum_to_feature() using the ZIO_CHECKSUM_MASK and verifies in zio_checksum_to_feature() that ZIO_CHECKSUM_MASK has been applied by the caller to attempt to prevent the same bug from occurring again in the future. --- usr/src/uts/common/fs/zfs/zfs_ioctl.c | 2 +- usr/src/uts/common/fs/zfs/zio_checksum.c | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/usr/src/uts/common/fs/zfs/zfs_ioctl.c b/usr/src/uts/common/fs/zfs/zfs_ioctl.c index bc04c51cbf6a..109f839c17a7 100644 --- a/usr/src/uts/common/fs/zfs/zfs_ioctl.c +++ b/usr/src/uts/common/fs/zfs/zfs_ioctl.c @@ -3885,7 +3885,7 @@ zfs_check_settable(const char *dsname, nvpair_t *pair, cred_t *cr) return (SET_ERROR(EINVAL)); /* check prop value is enabled in features */ - feature = zio_checksum_to_feature(intval); + feature = zio_checksum_to_feature(intval & ZIO_CHECKSUM_MASK); if (feature == SPA_FEATURE_NONE) break; diff --git a/usr/src/uts/common/fs/zfs/zio_checksum.c b/usr/src/uts/common/fs/zfs/zio_checksum.c index 4bef6a3e112d..8bd7e02bef38 100644 --- a/usr/src/uts/common/fs/zfs/zio_checksum.c +++ b/usr/src/uts/common/fs/zfs/zio_checksum.c @@ -136,9 +136,15 @@ zio_checksum_info_t zio_checksum_table[ZIO_CHECKSUM_FUNCTIONS] = { ZCHECKSUM_FLAG_NOPWRITE, "edonr"}, }; +/* + * The flag corresponding to the "verify" in dedup=[checksum,]verify + * must be cleared first, so callers should use ZIO_CHECKSUM_MASK. + */ spa_feature_t zio_checksum_to_feature(enum zio_checksum cksum) { + VERIFY((cksum & ~ZIO_CHECKSUM_MASK) == 0); + switch (cksum) { case ZIO_CHECKSUM_SHA512: return (SPA_FEATURE_SHA512); From d5b4849147f774efdc9bb8fc170169c5c9ccbd36 Mon Sep 17 00:00:00 2001 From: ilovezfs Date: Thu, 28 Jan 2016 04:51:19 -0800 Subject: [PATCH 2/4] 6585 sha512, skein, edonr need extensible dataset In any pool without the extensible dataset feature flag already enabled, creating a dataset with dedup set to use one of the new checksums would result in the following panic as soon as any data was added: panic[cpu0]/thread=ffffff0006761c40: feature_get_refcount(spa, feature, &refcount) != 48 (0x30 != 0x30), file: ../../common/fs/zfs/zfeature.c line 390 Inpsection showed that feature->fi_feature was 7, which is the value of SPA_FEATURE_EXTENSIBLE_DATASET in the spa_feature enum. This commit adds extensible dataset as a dependency for the sha512, edonr, and skein feature flags, which prevents the panic. --- usr/src/common/zfs/zfeature_common.c | 20 +++++++++++++++++--- usr/src/man/man5/zpool-features.5 | 6 +++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/usr/src/common/zfs/zfeature_common.c b/usr/src/common/zfs/zfeature_common.c index a00125528978..5c17d4b6df49 100644 --- a/usr/src/common/zfs/zfeature_common.c +++ b/usr/src/common/zfs/zfeature_common.c @@ -233,16 +233,30 @@ zpool_feature_init(void) "Support for blocks larger than 128KB.", ZFEATURE_FLAG_PER_DATASET, large_blocks_deps); + static const spa_feature_t sha512_deps[] = { + SPA_FEATURE_EXTENSIBLE_DATASET, + SPA_FEATURE_NONE + }; zfeature_register(SPA_FEATURE_SHA512, "org.illumos:sha512", "sha512", "SHA-512/256 hash algorithm.", - ZFEATURE_FLAG_PER_DATASET, NULL); + ZFEATURE_FLAG_PER_DATASET, sha512_deps); + + static const spa_feature_t skein_deps[] = { + SPA_FEATURE_EXTENSIBLE_DATASET, + SPA_FEATURE_NONE + }; zfeature_register(SPA_FEATURE_SKEIN, "org.illumos:skein", "skein", "Skein hash algorithm.", - ZFEATURE_FLAG_PER_DATASET, NULL); + ZFEATURE_FLAG_PER_DATASET, skein_deps); + + static const spa_feature_t edonr_deps[] = { + SPA_FEATURE_EXTENSIBLE_DATASET, + SPA_FEATURE_NONE + }; zfeature_register(SPA_FEATURE_EDONR, "org.illumos:edonr", "edonr", "Edon-R hash algorithm.", - ZFEATURE_FLAG_PER_DATASET, NULL); + ZFEATURE_FLAG_PER_DATASET, edonr_deps); } diff --git a/usr/src/man/man5/zpool-features.5 b/usr/src/man/man5/zpool-features.5 index a80fc3479ef2..58ba0458a811 100644 --- a/usr/src/man/man5/zpool-features.5 +++ b/usr/src/man/man5/zpool-features.5 @@ -454,7 +454,7 @@ filesystems that have ever had their recordsize larger than 128KB are destroyed. l l . GUID org.illumos:sha512 READ\-ONLY COMPATIBLE no -DEPENDENCIES none +DEPENDENCIES extensible_dataset .TE This feature enables the use of the SHA-512/256 truncated hash algorithm @@ -487,7 +487,7 @@ the updated GRUB stage2 module is installed). l l . GUID org.illumos:skein READ\-ONLY COMPATIBLE no -DEPENDENCIES none +DEPENDENCIES extensible_dataset .TE This feature enables the use of the Skein hash algorithm for checksum @@ -523,7 +523,7 @@ error. l l . GUID org.illumos:edonr READ\-ONLY COMPATIBLE no -DEPENDENCIES none +DEPENDENCIES extensible_dataset .TE This feature enables the use of the Edon-R hash algorithm for checksum, From 6f72ca2766d970d59bf8f741034a79535b3cfefc Mon Sep 17 00:00:00 2001 From: ilovezfs Date: Thu, 28 Jan 2016 04:52:17 -0800 Subject: [PATCH 3/4] 6586 Use tabs in feature deps arrays consistenly In zpool_feature_init() in zfeature_common.c the hole_birth_deps array and the filesystem_limits_deps array were formatted with four space continuations, unlike the bookmarks_deps array and the large_blocks_deps array, which indent the elements with tabs, as is generally the custom for arrays throughout the codebase. --- usr/src/common/zfs/zfeature_common.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/usr/src/common/zfs/zfeature_common.c b/usr/src/common/zfs/zfeature_common.c index 5c17d4b6df49..9824aac2248e 100644 --- a/usr/src/common/zfs/zfeature_common.c +++ b/usr/src/common/zfs/zfeature_common.c @@ -187,8 +187,10 @@ zpool_feature_init(void) "Record txg at which a feature is enabled", ZFEATURE_FLAG_READONLY_COMPAT, NULL); - static spa_feature_t hole_birth_deps[] = { SPA_FEATURE_ENABLED_TXG, - SPA_FEATURE_NONE }; + static spa_feature_t hole_birth_deps[] = { + SPA_FEATURE_ENABLED_TXG, + SPA_FEATURE_NONE + }; zfeature_register(SPA_FEATURE_HOLE_BIRTH, "com.delphix:hole_birth", "hole_birth", "Retain hole birth txg for more precise zfs send", @@ -210,8 +212,8 @@ zpool_feature_init(void) ZFEATURE_FLAG_READONLY_COMPAT, bookmarks_deps); static const spa_feature_t filesystem_limits_deps[] = { - SPA_FEATURE_EXTENSIBLE_DATASET, - SPA_FEATURE_NONE + SPA_FEATURE_EXTENSIBLE_DATASET, + SPA_FEATURE_NONE }; zfeature_register(SPA_FEATURE_FS_SS_LIMIT, "com.joyent:filesystem_limits", "filesystem_limits", From d71cb5a6cf5f476642c5e19c58e508e36ed9b800 Mon Sep 17 00:00:00 2001 From: ilovezfs Date: Wed, 3 Feb 2016 22:31:48 -0800 Subject: [PATCH 4/4] 6603 Verify feature-flag-per-ds -> extensible-ds zfeature_register() should verify that if ZFEATURE_FLAG_PER_DATASET is set, then the feature depends on SPA_FEATURE_EXTENSIBLE_DATASET. The converse need not be true. This should help to prevent bugs like illumos 6585 in the future. --- usr/src/common/zfs/zfeature_common.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/usr/src/common/zfs/zfeature_common.c b/usr/src/common/zfs/zfeature_common.c index 9824aac2248e..1eb3a1bef10f 100644 --- a/usr/src/common/zfs/zfeature_common.c +++ b/usr/src/common/zfs/zfeature_common.c @@ -129,6 +129,16 @@ zfeature_depends_on(spa_feature_t fid, spa_feature_t check) return (B_FALSE); } +static boolean_t +deps_contains_feature(const spa_feature_t *deps, const spa_feature_t feature) +{ + for (int i = 0; deps[i] != SPA_FEATURE_NONE; i++) + if (deps[i] == feature) + return (B_TRUE); + + return (B_FALSE); +} + static void zfeature_register(spa_feature_t fid, const char *guid, const char *name, const char *desc, zfeature_flags_t flags, const spa_feature_t *deps) @@ -146,6 +156,9 @@ zfeature_register(spa_feature_t fid, const char *guid, const char *name, if (deps == NULL) deps = nodeps; + VERIFY(((flags & ZFEATURE_FLAG_PER_DATASET) == 0) || + (deps_contains_feature(deps, SPA_FEATURE_EXTENSIBLE_DATASET))); + feature->fi_feature = fid; feature->fi_guid = guid; feature->fi_uname = name;