From ff61d1a4959065aa99d52489438f6737765987c6 Mon Sep 17 00:00:00 2001 From: LOLi Date: Wed, 29 Mar 2017 02:21:11 +0200 Subject: [PATCH] Check ashift validity in 'zpool add' df83110 added the ability to specify a custom "ashift" value from the command line in 'zpool add' and 'zpool attach'. This commit adds additional checks to the provided ashift to prevent invalid values from being used, which could result in disastrous consequences for the whole pool. Additionally provide ASHIFT_MAX and ASHIFT_MIN definitions in spa.h. Reviewed-by: Giuseppe Di Natale Reviewed-by: Brian Behlendorf Signed-off-by: loli10K Closes #5878 --- cmd/zpool/zpool_vdev.c | 13 ++- include/sys/fs/zfs.h | 3 +- include/sys/spa.h | 11 ++ lib/libzfs/libzfs_pool.c | 9 +- module/zfs/vdev.c | 14 ++- tests/runfiles/linux.run | 3 +- .../functional/cli_root/zpool_add/Makefile.am | 3 +- .../cli_root/zpool_add/add-o_ashift.ksh | 106 ++++++++++++++++++ 8 files changed, 153 insertions(+), 9 deletions(-) create mode 100644 tests/zfs-tests/tests/functional/cli_root/zpool_add/add-o_ashift.ksh diff --git a/cmd/zpool/zpool_vdev.c b/cmd/zpool/zpool_vdev.c index c0d3076d2..ed607ec85 100644 --- a/cmd/zpool/zpool_vdev.c +++ b/cmd/zpool/zpool_vdev.c @@ -70,6 +70,7 @@ #include #include #include +#include #include #include #include @@ -721,8 +722,18 @@ make_leaf_vdev(nvlist_t *props, const char *arg, uint64_t is_log) char *value = NULL; if (nvlist_lookup_string(props, - zpool_prop_to_name(ZPOOL_PROP_ASHIFT), &value) == 0) + zpool_prop_to_name(ZPOOL_PROP_ASHIFT), &value) == 0) { zfs_nicestrtonum(NULL, value, &ashift); + if (ashift != 0 && + (ashift < ASHIFT_MIN || ashift > ASHIFT_MAX)) { + (void) fprintf(stderr, + gettext("invalid 'ashift=%" PRIu64 "' " + "property: only values between %" PRId32 " " + "and %" PRId32 " are allowed.\n"), + ashift, ASHIFT_MIN, ASHIFT_MAX); + return (NULL); + } + } } /* diff --git a/include/sys/fs/zfs.h b/include/sys/fs/zfs.h index 962698c2f..ba6487612 100644 --- a/include/sys/fs/zfs.h +++ b/include/sys/fs/zfs.h @@ -732,7 +732,8 @@ typedef enum vdev_aux { VDEV_AUX_IO_FAILURE, /* experienced I/O failure */ VDEV_AUX_BAD_LOG, /* cannot read log chain(s) */ VDEV_AUX_EXTERNAL, /* external diagnosis */ - VDEV_AUX_SPLIT_POOL /* vdev was split off into another pool */ + VDEV_AUX_SPLIT_POOL, /* vdev was split off into another pool */ + VDEV_AUX_BAD_ASHIFT /* vdev ashift is invalid */ } vdev_aux_t; /* diff --git a/include/sys/spa.h b/include/sys/spa.h index 0f05d04ad..b6e124faa 100644 --- a/include/sys/spa.h +++ b/include/sys/spa.h @@ -121,6 +121,17 @@ _NOTE(CONSTCOND) } while (0) #define SPA_OLD_MAXBLOCKSIZE (1ULL << SPA_OLD_MAXBLOCKSHIFT) #define SPA_MAXBLOCKSIZE (1ULL << SPA_MAXBLOCKSHIFT) +/* + * Alignment Shift (ashift) is an immutable, internal top-level vdev property + * which can only be set at vdev creation time. Physical writes are always done + * according to it, which makes 2^ashift the smallest possible IO on a vdev. + * + * We currently allow values ranging from 512 bytes (2^9 = 512) to 8 KiB + * (2^13 = 8,192). + */ +#define ASHIFT_MIN 9 +#define ASHIFT_MAX 13 + /* * Size of block to hold the configuration data (a packed nvlist) */ diff --git a/lib/libzfs/libzfs_pool.c b/lib/libzfs/libzfs_pool.c index 616f5061f..16fff89e6 100644 --- a/lib/libzfs/libzfs_pool.c +++ b/lib/libzfs/libzfs_pool.c @@ -538,10 +538,13 @@ zpool_valid_proplist(libzfs_handle_t *hdl, const char *poolname, goto error; } - if (intval != 0 && (intval < 9 || intval > 13)) { + if (intval != 0 && + (intval < ASHIFT_MIN || intval > ASHIFT_MAX)) { zfs_error_aux(hdl, dgettext(TEXT_DOMAIN, - "property '%s' number %d is invalid."), - propname, intval); + "invalid '%s=%d' property: only values " + "between %" PRId32 " and %" PRId32 " " + "are allowed.\n"), + propname, intval, ASHIFT_MIN, ASHIFT_MAX); (void) zfs_error(hdl, EZFS_BADPROP, errbuf); goto error; } diff --git a/module/zfs/vdev.c b/module/zfs/vdev.c index e741a6998..613a9b51e 100644 --- a/module/zfs/vdev.c +++ b/module/zfs/vdev.c @@ -1345,8 +1345,15 @@ vdev_open(vdev_t *vd) */ vd->vdev_asize = asize; vd->vdev_max_asize = max_asize; - if (vd->vdev_ashift == 0) - vd->vdev_ashift = ashift; + if (vd->vdev_ashift == 0) { + vd->vdev_ashift = ashift; /* use detected value */ + } + if (vd->vdev_ashift != 0 && (vd->vdev_ashift < ASHIFT_MIN || + vd->vdev_ashift > ASHIFT_MAX)) { + vdev_set_state(vd, B_TRUE, VDEV_STATE_CANT_OPEN, + VDEV_AUX_BAD_ASHIFT); + return (SET_ERROR(EDOM)); + } } else { /* * Detect if the alignment requirement has increased. @@ -3487,6 +3494,9 @@ vdev_set_state(vdev_t *vd, boolean_t isopen, vdev_state_t state, vdev_aux_t aux) case VDEV_AUX_BAD_LABEL: class = FM_EREPORT_ZFS_DEVICE_BAD_LABEL; break; + case VDEV_AUX_BAD_ASHIFT: + class = FM_EREPORT_ZFS_DEVICE_BAD_ASHIFT; + break; default: class = FM_EREPORT_ZFS_DEVICE_UNKNOWN; } diff --git a/tests/runfiles/linux.run b/tests/runfiles/linux.run index a7b58474e..73f19ecfa 100644 --- a/tests/runfiles/linux.run +++ b/tests/runfiles/linux.run @@ -230,7 +230,8 @@ tests = ['zpool_001_neg', 'zpool_002_pos', 'zpool_003_pos'] # zpool_add_006_pos - https://github.com/zfsonlinux/zfs/issues/3484 [tests/functional/cli_root/zpool_add] tests = ['zpool_add_001_pos', 'zpool_add_002_pos', 'zpool_add_003_pos', - 'zpool_add_007_neg', 'zpool_add_008_neg', 'zpool_add_009_neg'] + 'zpool_add_007_neg', 'zpool_add_008_neg', 'zpool_add_009_neg', + 'add-o_ashift'] [tests/functional/cli_root/zpool_attach] tests = ['zpool_attach_001_neg'] diff --git a/tests/zfs-tests/tests/functional/cli_root/zpool_add/Makefile.am b/tests/zfs-tests/tests/functional/cli_root/zpool_add/Makefile.am index fef934325..60a16e102 100644 --- a/tests/zfs-tests/tests/functional/cli_root/zpool_add/Makefile.am +++ b/tests/zfs-tests/tests/functional/cli_root/zpool_add/Makefile.am @@ -12,4 +12,5 @@ dist_pkgdata_SCRIPTS = \ zpool_add_006_pos.ksh \ zpool_add_007_neg.ksh \ zpool_add_008_neg.ksh \ - zpool_add_009_neg.ksh + zpool_add_009_neg.ksh \ + add-o_ashift.ksh diff --git a/tests/zfs-tests/tests/functional/cli_root/zpool_add/add-o_ashift.ksh b/tests/zfs-tests/tests/functional/cli_root/zpool_add/add-o_ashift.ksh new file mode 100644 index 000000000..a7180b5c5 --- /dev/null +++ b/tests/zfs-tests/tests/functional/cli_root/zpool_add/add-o_ashift.ksh @@ -0,0 +1,106 @@ +#!/bin/ksh -p +# +# CDDL HEADER START +# +# The contents of this file are subject to the terms of the +# Common Development and Distribution License (the "License"). +# You may not use this file except in compliance with the License. +# +# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE +# or http://www.opensolaris.org/os/licensing. +# See the License for the specific language governing permissions +# and limitations under the License. +# +# When distributing Covered Code, include this CDDL HEADER in each +# file and include the License file at usr/src/OPENSOLARIS.LICENSE. +# If applicable, add the following below this CDDL HEADER, with the +# fields enclosed by brackets "[]" replaced with your own identifying +# information: Portions Copyright [yyyy] [name of copyright owner] +# +# CDDL HEADER END +# + +# +# Copyright 2017, loli10K. All rights reserved. +# + +. $STF_SUITE/include/libtest.shlib +. $STF_SUITE/tests/functional/cli_root/zpool_create/zpool_create.shlib + +# +# DESCRIPTION: +# 'zpool add -o ashift= ...' should work with different ashift +# values. +# +# STRATEGY: +# 1. Create a pool with default values. +# 2. Verify 'zpool add -o ashift=' works with allowed values (9-13). +# 3. Verify 'zpool add -o ashift=' doesn't accept other invalid values. +# + +verify_runnable "global" + +function cleanup +{ + poolexists $TESTPOOL && destroy_pool $TESTPOOL + log_must $RM $disk1 $disk2 +} + +# +# Verify every label in device $1 contains ashift value $2 +# $1 device +# $2 ashift value +# +function verify_device_ashift +{ + typeset device=$1 + typeset value=$2 + typeset ashift + + $ZDB -e -l $device | $GREP " ashift:" | { + while read ashift ; do + if [[ "ashift: $value" != "$ashift" ]]; then + return 1 + fi + done + } + + return 0 +} + +log_assert "zpool add -o ashift=' works with different ashift values" +log_onexit cleanup + +disk1=$TEST_BASE_DIR/$FILEDISK0 +disk2=$TEST_BASE_DIR/$FILEDISK1 +log_must $MKFILE $SIZE $disk1 +log_must $MKFILE $SIZE $disk2 + +typeset ashifts=("9" "10" "11" "12" "13") +for ashift in ${ashifts[@]} +do + log_must $ZPOOL create $TESTPOOL $disk1 + log_must $ZPOOL add -o ashift=$ashift $TESTPOOL $disk2 + verify_device_ashift $disk2 $ashift + if [[ $? -ne 0 ]] + then + log_fail "Device was added without setting ashift value to "\ + "$ashift" + fi + # clean things for the next run + log_must $ZPOOL destroy $TESTPOOL + log_must $ZPOOL labelclear $disk1 + log_must $ZPOOL labelclear $disk2 +done + +typeset badvals=("off" "on" "1" "8" "14" "1b" "ff" "-") +for badval in ${badvals[@]} +do + log_must $ZPOOL create $TESTPOOL $disk1 + log_mustnot $ZPOOL add -o ashift="$badval" $disk2 + log_must $ZPOOL destroy $TESTPOOL + log_must $ZPOOL labelclear $disk1 + log_must $ZPOOL labelclear $disk2 +done + +log_pass "zpool add -o ashift=' works with different ashift values"