Improvements to the 'compatibility' property

Several improvements to the operation of the 'compatibility' property:

1) Improved handling of unrecognized features:
Change the way unrecognized features in compatibility files are handled.

 * invalid features in files under /usr/share/zfs/compatibility.d
   only get a warning (as these may refer to future features not yet in
   the library),
 * invalid features in files under /etc/zfs/compatibility.d
   get an error (as these are presumed to refer to the current system).

2) Improved error reporting from zpool_load_compat.
Note: slight ABI change to zpool_load_compat for better error reporting.

3) compatibility=legacy inhibits all 'zpool upgrade' operations.

4) Detect when features are enabled outside current compatibility set
   * zpool set compatibility=foo <-- print a warning
   * zpool set feature@xxx=enabled <-- error
   * zpool status <-- indicate this state

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Colm Buckley <colm@tuatha.org>
Closes #11861

cmd/zpool/zpool_main.c

@@ -786,7 +786,7 @@ add_prop_list(const char *propname, char *propval, nvlist_t **props,
 
 	if (poolprop) {
 		const char *vname = zpool_prop_to_name(ZPOOL_PROP_VERSION);
-		const char *fname =
+		const char *cname =
 		    zpool_prop_to_name(ZPOOL_PROP_COMPATIBILITY);
 
 		if ((prop = zpool_name_to_prop(propname)) == ZPOOL_PROP_INVAL &&
@@ -811,16 +811,19 @@ add_prop_list(const char *propname, char *propval, nvlist_t **props,
 		}
 
 		/*
-		 * compatibility property and version should not be specified
+		 * if version is specified, only "legacy" compatibility
-		 * at the same time.
+		 * may be requested
 		 */
 		if ((prop == ZPOOL_PROP_COMPATIBILITY &&
+		    strcmp(propval, ZPOOL_COMPAT_LEGACY) != 0 &&
 		    nvlist_exists(proplist, vname)) ||
 		    (prop == ZPOOL_PROP_VERSION &&
-		    nvlist_exists(proplist, fname))) {
+		    nvlist_exists(proplist, cname) &&
-			(void) fprintf(stderr, gettext("'compatibility' and "
+		    strcmp(fnvlist_lookup_string(proplist, cname),
-			    "'version' properties cannot be specified "
+		    ZPOOL_COMPAT_LEGACY) != 0)) {
-			    "together\n"));
+			(void) fprintf(stderr, gettext("when 'version' is "
+			    "specified, the 'compatibility' feature may only "
+			    "be set to '" ZPOOL_COMPAT_LEGACY "'\n"));
 			return (2);
 		}
 
@@ -1674,6 +1677,7 @@ zpool_do_create(int argc, char **argv)
 		 *    - enable_pool_features (ie: no '-d' or '-o version')
 		 *    - it's supported by the kernel module
 		 *    - it's in the requested feature set
+		 *  - warn if it's enabled but not in compat
 		 */
 		for (spa_feature_t i = 0; i < SPA_FEATURES; i++) {
 			char propname[MAXPATHLEN];
@@ -1687,6 +1691,14 @@ zpool_do_create(int argc, char **argv)
 				if (strcmp(propval, ZFS_FEATURE_DISABLED) == 0)
 					(void) nvlist_remove_all(props,
 					    propname);
+				if (strcmp(propval,
+				    ZFS_FEATURE_ENABLED) == 0 &&
+				    !requested_features[i])
+					(void) fprintf(stderr, gettext(
+					    "Warning: feature \"%s\" enabled "
+					    "but is not in specified "
+					    "'compatibility' feature set.\n"),
+					    feat->fi_uname);
 			} else if (
 			    enable_pool_features &&
 			    feat->fi_zfs_mod_supported &&
@@ -2717,8 +2729,10 @@ show_import(nvlist_t *config, boolean_t report_error)
 
 	case ZPOOL_STATUS_FEAT_DISABLED:
 		printf_color(ANSI_BOLD, gettext("status: "));
-		printf_color(ANSI_YELLOW, gettext("Some supported and "
+		printf_color(ANSI_YELLOW, gettext("Some supported "
-		    "requested features are not enabled on the pool.\n"));
+		    "features are not enabled on the pool.\n\t"
+		    "(Note that they may be intentionally disabled "
+		    "if the\n\t'compatibility' property is set.)\n"));
 		break;
 
 	case ZPOOL_STATUS_COMPATIBILITY_ERR:
@@ -2728,6 +2742,13 @@ show_import(nvlist_t *config, boolean_t report_error)
 		    "property.\n"));
 		break;
 
+	case ZPOOL_STATUS_INCOMPATIBLE_FEAT:
+		printf_color(ANSI_BOLD, gettext("status: "));
+		printf_color(ANSI_YELLOW, gettext("One or more features "
+		    "are enabled on the pool despite not being\n"
+		    "requested by the 'compatibility' property.\n"));
+		break;
+
 	case ZPOOL_STATUS_UNSUP_FEAT_READ:
 		printf_color(ANSI_BOLD, gettext("status: "));
 		printf_color(ANSI_YELLOW, gettext("The pool uses the following "
@@ -8055,7 +8076,8 @@ status_callback(zpool_handle_t *zhp, void *data)
 	    (reason == ZPOOL_STATUS_OK ||
 	    reason == ZPOOL_STATUS_VERSION_OLDER ||
 	    reason == ZPOOL_STATUS_FEAT_DISABLED ||
-	    reason == ZPOOL_STATUS_COMPATIBILITY_ERR)) {
+	    reason == ZPOOL_STATUS_COMPATIBILITY_ERR ||
+	    reason == ZPOOL_STATUS_INCOMPATIBLE_FEAT)) {
 		if (!cbp->cb_allpools) {
 			(void) printf(gettext("pool '%s' is healthy\n"),
 			    zpool_get_name(zhp));
@@ -8254,6 +8276,18 @@ status_callback(zpool_handle_t *zhp, void *data)
 		    ZPOOL_DATA_COMPAT_D ".\n"));
 		break;
 
+	case ZPOOL_STATUS_INCOMPATIBLE_FEAT:
+		printf_color(ANSI_BOLD, gettext("status: "));
+		printf_color(ANSI_YELLOW, gettext("One or more features "
+		    "are enabled on the pool despite not being\n\t"
+		    "requested by the 'compatibility' property.\n"));
+		printf_color(ANSI_BOLD, gettext("action: "));
+		printf_color(ANSI_YELLOW, gettext("Consider setting "
+		    "'compatibility' to an appropriate value, or\n\t"
+		    "adding needed features to the relevant file in\n\t"
+		    ZPOOL_SYSCONF_COMPAT_D " or " ZPOOL_DATA_COMPAT_D ".\n"));
+		break;
+
 	case ZPOOL_STATUS_UNSUP_FEAT_READ:
 		printf_color(ANSI_BOLD, gettext("status: "));
 		printf_color(ANSI_YELLOW, gettext("The pool cannot be accessed "
@@ -8713,6 +8747,11 @@ upgrade_version(zpool_handle_t *zhp, uint64_t version)
 	verify(nvlist_lookup_uint64(config, ZPOOL_CONFIG_VERSION,
 	    &oldversion) == 0);
 
+	char compat[ZFS_MAXPROPLEN];
+	if (zpool_get_prop(zhp, ZPOOL_PROP_COMPATIBILITY, compat,
+	    ZFS_MAXPROPLEN, NULL, B_FALSE) != 0)
+		compat[0] = '\0';
+
 	assert(SPA_VERSION_IS_SUPPORTED(oldversion));
 	assert(oldversion < version);
 
@@ -8727,6 +8766,13 @@ upgrade_version(zpool_handle_t *zhp, uint64_t version)
 		return (1);
 	}
 
+	if (strcmp(compat, ZPOOL_COMPAT_LEGACY) == 0) {
+		(void) fprintf(stderr, gettext("Upgrade not performed because "
+		    "'compatibility' property set to '"
+		    ZPOOL_COMPAT_LEGACY "'.\n"));
+		return (1);
+	}
+
 	ret = zpool_upgrade(zhp, version);
 	if (ret != 0)
 		return (ret);
@@ -8868,7 +8914,10 @@ upgrade_list_older_cb(zpool_handle_t *zhp, void *arg)
 			    "be upgraded to use feature flags.  After "
 			    "being upgraded, these pools\nwill no "
 			    "longer be accessible by software that does not "
-			    "support feature\nflags.\n\n"));
+			    "support feature\nflags.\n\n"
+			    "Note that setting a pool's 'compatibility' "
+			    "feature to '" ZPOOL_COMPAT_LEGACY "' will\n"
+			    "inhibit upgrades.\n\n"));
 			(void) printf(gettext("VER  POOL\n"));
 			(void) printf(gettext("---  ------------\n"));
 			cbp->cb_first = B_FALSE;
@@ -8914,7 +8963,11 @@ upgrade_list_disabled_cb(zpool_handle_t *zhp, void *arg)
 					    "software\nthat does not support "
 					    "the feature. See "
 					    "zpool-features(5) for "
-					    "details.\n\n"));
+					    "details.\n\n"
+					    "Note that the pool "
+					    "'compatibility' feature can be "
+					    "used to inhibit\nfeature "
+					    "upgrades.\n\n"));
 					(void) printf(gettext("POOL  "
 					    "FEATURE\n"));
 					(void) printf(gettext("------"
@@ -9970,6 +10023,63 @@ set_callback(zpool_handle_t *zhp, void *data)
 	int error;
 	set_cbdata_t *cb = (set_cbdata_t *)data;
 
+	/* Check if we have out-of-bounds features */
+	if (strcmp(cb->cb_propname, ZPOOL_CONFIG_COMPATIBILITY) == 0) {
+		boolean_t features[SPA_FEATURES];
+		if (zpool_do_load_compat(cb->cb_value, features) !=
+		    ZPOOL_COMPATIBILITY_OK)
+			return (-1);
+
+		nvlist_t *enabled = zpool_get_features(zhp);
+		spa_feature_t i;
+		for (i = 0; i < SPA_FEATURES; i++) {
+			const char *fguid = spa_feature_table[i].fi_guid;
+			if (nvlist_exists(enabled, fguid) && !features[i])
+				break;
+		}
+		if (i < SPA_FEATURES)
+			(void) fprintf(stderr, gettext("Warning: one or "
+			    "more features already enabled on pool '%s'\n"
+			    "are not present in this compatibility set.\n"),
+			    zpool_get_name(zhp));
+	}
+
+	/* if we're setting a feature, check it's in compatibility set */
+	if (zpool_prop_feature(cb->cb_propname) &&
+	    strcmp(cb->cb_value, ZFS_FEATURE_ENABLED) == 0) {
+		char *fname = strchr(cb->cb_propname, '@') + 1;
+		spa_feature_t f;
+
+		if (zfeature_lookup_name(fname, &f) == 0) {
+			char compat[ZFS_MAXPROPLEN];
+			if (zpool_get_prop(zhp, ZPOOL_PROP_COMPATIBILITY,
+			    compat, ZFS_MAXPROPLEN, NULL, B_FALSE) != 0)
+				compat[0] = '\0';
+
+			boolean_t features[SPA_FEATURES];
+			if (zpool_do_load_compat(compat, features) !=
+			    ZPOOL_COMPATIBILITY_OK) {
+				(void) fprintf(stderr, gettext("Error: "
+				    "cannot enable feature '%s' on pool '%s'\n"
+				    "because the pool's 'compatibility' "
+				    "property cannot be parsed.\n"),
+				    fname, zpool_get_name(zhp));
+				return (-1);
+			}
+
+			if (!features[f]) {
+				(void) fprintf(stderr, gettext("Error: "
+				    "cannot enable feature '%s' on pool '%s'\n"
+				    "as it is not specified in this pool's "
+				    "current compatibility set.\n"
+				    "Consider setting 'compatibility' to a "
+				    "less restrictive set, or to 'off'.\n"),
+				    fname, zpool_get_name(zhp));
+				return (-1);
+			}
+		}
+	}
+
 	error = zpool_set_prop(zhp, cb->cb_propname, cb->cb_value);
 
 	if (!error)
@@ -10492,28 +10602,25 @@ zpool_do_version(int argc, char **argv)
 static zpool_compat_status_t
 zpool_do_load_compat(const char *compat, boolean_t *list)
 {
-	char badword[ZFS_MAXPROPLEN];
+	char report[1024];
-	char badfile[MAXPATHLEN];
+
 	zpool_compat_status_t ret;
 
-	switch (ret = zpool_load_compat(compat, list, badword, badfile)) {
+	ret = zpool_load_compat(compat, list, report, 1024);
+	switch (ret) {
+
 	case ZPOOL_COMPATIBILITY_OK:
 		break;
-	case ZPOOL_COMPATIBILITY_READERR:
+
-		(void) fprintf(stderr, gettext("error reading compatibility "
-		    "file '%s'\n"), badfile);
-		break;
-	case ZPOOL_COMPATIBILITY_BADFILE:
-		(void) fprintf(stderr, gettext("compatibility file '%s' "
-		    "too large or not newline-terminated\n"), badfile);
-		break;
-	case ZPOOL_COMPATIBILITY_BADWORD:
-		(void) fprintf(stderr, gettext("unknown feature '%s' in "
-		    "compatibility file '%s'\n"), badword, badfile);
-		break;
 	case ZPOOL_COMPATIBILITY_NOFILES:
-		(void) fprintf(stderr, gettext("no compatibility files "
+	case ZPOOL_COMPATIBILITY_BADFILE:
-		    "specified\n"));
+	case ZPOOL_COMPATIBILITY_BADTOKEN:
+		(void) fprintf(stderr, "Error: %s\n", report);
+		break;
+
+	case ZPOOL_COMPATIBILITY_WARNTOKEN:
+		(void) fprintf(stderr, "Warning: %s\n", report);
+		ret = ZPOOL_COMPATIBILITY_OK;
 		break;
 	}
 	return (ret);

include/libzfs.h

@@ -393,6 +393,7 @@ typedef enum {
 	ZPOOL_STATUS_REBUILD_SCRUB,	/* recommend scrubbing the pool */
 	ZPOOL_STATUS_NON_NATIVE_ASHIFT,	/* (e.g. 512e dev with ashift of 9) */
 	ZPOOL_STATUS_COMPATIBILITY_ERR,	/* bad 'compatibility' property */
+	ZPOOL_STATUS_INCOMPATIBLE_FEAT,	/* feature set outside compatibility */
 
 	/*
 	 * Finally, the following indicates a healthy pool.
@@ -922,14 +923,14 @@ extern int zpool_disable_datasets(zpool_handle_t *, boolean_t);
  */
 typedef enum {
 	ZPOOL_COMPATIBILITY_OK,
-	ZPOOL_COMPATIBILITY_READERR,
+	ZPOOL_COMPATIBILITY_WARNTOKEN,
+	ZPOOL_COMPATIBILITY_BADTOKEN,
 	ZPOOL_COMPATIBILITY_BADFILE,
-	ZPOOL_COMPATIBILITY_BADWORD,
 	ZPOOL_COMPATIBILITY_NOFILES
 } zpool_compat_status_t;
 
 extern zpool_compat_status_t zpool_load_compat(const char *,
-    boolean_t *, char *, char *);
+    boolean_t *, char *, size_t);
 
 #ifdef __FreeBSD__
 

lib/libzfs/libzfs_pool.c

@@ -467,8 +467,7 @@ zpool_valid_proplist(libzfs_handle_t *hdl, const char *poolname,
 	char *slash, *check;
 	struct stat64 statbuf;
 	zpool_handle_t *zhp;
-	char badword[ZFS_MAXPROPLEN];
+	char report[1024];
-	char badfile[MAXPATHLEN];
 
 	if (nvlist_alloc(&retprops, NV_UNIQUE_NAME, 0) != 0) {
 		(void) no_memory(hdl);
@@ -679,33 +678,14 @@ zpool_valid_proplist(libzfs_handle_t *hdl, const char *poolname,
 			break;
 
 		case ZPOOL_PROP_COMPATIBILITY:
-			switch (zpool_load_compat(strval, NULL,
+			switch (zpool_load_compat(strval, NULL, report, 1024)) {
-			    badword, badfile)) {
 			case ZPOOL_COMPATIBILITY_OK:
+			case ZPOOL_COMPATIBILITY_WARNTOKEN:
 				break;
-			case ZPOOL_COMPATIBILITY_READERR:
-				zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
-				    "error reading feature file '%s'"),
-				    badfile);
-				(void) zfs_error(hdl, EZFS_BADPROP, errbuf);
-				goto error;
 			case ZPOOL_COMPATIBILITY_BADFILE:
-				zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
+			case ZPOOL_COMPATIBILITY_BADTOKEN:
-				    "feature file '%s' too large or not "
-				    "newline-terminated"),
-				    badfile);
-				(void) zfs_error(hdl, EZFS_BADPROP, errbuf);
-				goto error;
-			case ZPOOL_COMPATIBILITY_BADWORD:
-				zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
-				    "unknown feature '%s' in feature "
-				    "file '%s'"),
-				    badword, badfile);
-				(void) zfs_error(hdl, EZFS_BADPROP, errbuf);
-				goto error;
 			case ZPOOL_COMPATIBILITY_NOFILES:
-				zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
+				zfs_error_aux(hdl, report);
-				    "no feature files specified"));
 				(void) zfs_error(hdl, EZFS_BADPROP, errbuf);
 				goto error;
 			}
@@ -4742,8 +4722,8 @@ zpool_get_bootenv(zpool_handle_t *zhp, nvlist_t **nvlp)
  * Arguments:
  *  compatibility : string containing feature filenames
  *  features : either NULL or pointer to array of boolean
- *  badtoken : either NULL or pointer to char[ZFS_MAXPROPLEN]
+ *  report : either NULL or pointer to string buffer
- *  badfile : either NULL or pointer to char[MAXPATHLEN]
+ *  rlen : length of "report" buffer
  *
  * compatibility is NULL (unset), "", "off", "legacy", or list of
  * comma-separated filenames. filenames should either be absolute,
@@ -4752,48 +4732,56 @@ zpool_get_bootenv(zpool_handle_t *zhp, nvlist_t **nvlp)
  *   2) ZPOOL_DATA_COMPAT_D (eg: /usr/share/zfs/compatibility.d).
  * (Unset), "" or "off" => enable all features
  * "legacy" => disable all features
+ *
  * Any feature names read from files which match unames in spa_feature_table
  * will have the corresponding boolean set in the features array (if non-NULL).
  * If more than one feature set specified, only features present in *all* of
  * them will be set.
  *
- * An unreadable filename will be strlcpy'd to badfile (if non-NULL).
+ * "report" if not NULL will be populated with a suitable status message.
- * An unrecognized feature will be strlcpy'd to badtoken (if non-NULL).
  *
  * Return values:
  *   ZPOOL_COMPATIBILITY_OK : files read and parsed ok
- *   ZPOOL_COMPATIBILITY_READERR : file could not be opened / mmap'd
  *   ZPOOL_COMPATIBILITY_BADFILE : file too big or not a text file
- *   ZPOOL_COMPATIBILITY_BADWORD : file contains invalid feature name
+ *   ZPOOL_COMPATIBILITY_BADTOKEN : SYSCONF file contains invalid feature name
- *   ZPOOL_COMPATIBILITY_NOFILES  : no file names found
+ *   ZPOOL_COMPATIBILITY_WARNTOKEN : DATA file contains invalid feature name
+ *   ZPOOL_COMPATIBILITY_NOFILES : no feature files found
  */
 zpool_compat_status_t
-zpool_load_compat(const char *compatibility,
+zpool_load_compat(const char *compat, boolean_t *features, char *report,
-    boolean_t *features, char *badtoken, char *badfile)
+    size_t rlen)
 {
 	int sdirfd, ddirfd, featfd;
-	int i;
 	struct stat fs;
-	char *fc;			/* mmap of file */
+	char *fc;
-	char *ps, *ls, *ws;		/* strtok state */
+	char *ps, *ls, *ws;
 	char *file, *line, *word;
-	char filenames[ZFS_MAXPROPLEN];
+
-	int filecount = 0;
+	char l_compat[ZFS_MAXPROPLEN];
+
+	boolean_t ret_nofiles = B_TRUE;
+	boolean_t ret_badfile = B_FALSE;
+	boolean_t ret_badtoken = B_FALSE;
+	boolean_t ret_warntoken = B_FALSE;
 
 	/* special cases (unset), "" and "off" => enable all features */
-	if (compatibility == NULL || compatibility[0] == '\0' ||
+	if (compat == NULL || compat[0] == '\0' ||
-	    strcmp(compatibility, ZPOOL_COMPAT_OFF) == 0) {
+	    strcmp(compat, ZPOOL_COMPAT_OFF) == 0) {
 		if (features != NULL)
-			for (i = 0; i < SPA_FEATURES; i++)
+			for (uint_t i = 0; i < SPA_FEATURES; i++)
 				features[i] = B_TRUE;
+		if (report != NULL)
+			strlcpy(report, gettext("all features enabled"), rlen);
 		return (ZPOOL_COMPATIBILITY_OK);
 	}
 
 	/* Final special case "legacy" => disable all features */
-	if (strcmp(compatibility, ZPOOL_COMPAT_LEGACY) == 0) {
+	if (strcmp(compat, ZPOOL_COMPAT_LEGACY) == 0) {
 		if (features != NULL)
-			for (i = 0; i < SPA_FEATURES; i++)
+			for (uint_t i = 0; i < SPA_FEATURES; i++)
 				features[i] = B_FALSE;
+		if (report != NULL)
+			strlcpy(report, gettext("all features disabled"), rlen);
 		return (ZPOOL_COMPATIBILITY_OK);
 	}
 
@@ -4801,9 +4789,12 @@ zpool_load_compat(const char *compatibility,
 	 * Start with all true; will be ANDed with results from each file
 	 */
 	if (features != NULL)
-		for (i = 0; i < SPA_FEATURES; i++)
+		for (uint_t i = 0; i < SPA_FEATURES; i++)
 			features[i] = B_TRUE;
 
+	char err_badfile[1024] = "";
+	char err_badtoken[1024] = "";
+
 	/*
 	 * We ignore errors from the directory open()
 	 * as they're only needed if the filename is relative
@@ -4815,32 +4806,33 @@ zpool_load_compat(const char *compatibility,
 	sdirfd = open(ZPOOL_SYSCONF_COMPAT_D, O_DIRECTORY | O_PATH | O_CLOEXEC);
 	ddirfd = open(ZPOOL_DATA_COMPAT_D, O_DIRECTORY | O_PATH | O_CLOEXEC);
 
-	(void) strlcpy(filenames, compatibility, ZFS_MAXPROPLEN);
+	(void) strlcpy(l_compat, compat, ZFS_MAXPROPLEN);
-	file = strtok_r(filenames, ",", &ps);
+
-	while (file != NULL) {
+	for (file = strtok_r(l_compat, ",", &ps);
-		boolean_t features_local[SPA_FEATURES];
+	    file != NULL;
+	    file = strtok_r(NULL, ",", &ps)) {
+
+		boolean_t l_features[SPA_FEATURES];
+
+		enum { Z_SYSCONF, Z_DATA } source;
 
 		/* try sysconfdir first, then datadir */
-		if ((featfd = openat(sdirfd, file, 0, O_RDONLY)) < 0)
+		source = Z_SYSCONF;
+		if ((featfd = openat(sdirfd, file, 0, O_RDONLY)) < 0) {
 			featfd = openat(ddirfd, file, 0, O_RDONLY);
-
+			source = Z_DATA;
-		if (featfd < 0 || fstat(featfd, &fs) < 0) {
-			(void) close(featfd);
-			(void) close(sdirfd);
-			(void) close(ddirfd);
-			if (badfile != NULL)
-				(void) strlcpy(badfile, file, MAXPATHLEN);
-			return (ZPOOL_COMPATIBILITY_READERR);
 		}
 
-		/* Too big or too small */
+		/* File readable and correct size? */
-		if (fs.st_size < 1 || fs.st_size > ZPOOL_COMPAT_MAXSIZE) {
+		if (featfd < 0 ||
+		    fstat(featfd, &fs) < 0 ||
+		    fs.st_size < 1 ||
+		    fs.st_size > ZPOOL_COMPAT_MAXSIZE) {
 			(void) close(featfd);
-			(void) close(sdirfd);
+			strlcat(err_badfile, file, ZFS_MAXPROPLEN);
-			(void) close(ddirfd);
+			strlcat(err_badfile, " ", ZFS_MAXPROPLEN);
-			if (badfile != NULL)
+			ret_badfile = B_TRUE;
-				(void) strlcpy(badfile, file, MAXPATHLEN);
+			continue;
-			return (ZPOOL_COMPATIBILITY_BADFILE);
 		}
 
 		/* private mmap() so we can strtok safely */
@@ -4848,73 +4840,99 @@ zpool_load_compat(const char *compatibility,
 		    PROT_READ|PROT_WRITE, MAP_PRIVATE, featfd, 0);
 		(void) close(featfd);
 
-		if (fc < 0) {
+		/* map ok, and last character == newline? */
-			(void) close(sdirfd);
+		if (fc < 0 || fc[fs.st_size - 1] != '\n') {
-			(void) close(ddirfd);
-			if (badfile != NULL)
-				(void) strlcpy(badfile, file, MAXPATHLEN);
-			return (ZPOOL_COMPATIBILITY_READERR);
-		}
-
-		/* Text file sanity check - last char should be newline */
-		if (fc[fs.st_size - 1] != '\n') {
 			(void) munmap((void *) fc, fs.st_size);
-			(void) close(sdirfd);
+			strlcat(err_badfile, file, ZFS_MAXPROPLEN);
-			(void) close(ddirfd);
+			strlcat(err_badfile, " ", ZFS_MAXPROPLEN);
-			if (badfile != NULL)
+			ret_badfile = B_TRUE;
-				(void) strlcpy(badfile, file, MAXPATHLEN);
+			continue;
-			return (ZPOOL_COMPATIBILITY_BADFILE);
 		}
 
-		/* replace with NUL to ensure we have a delimiter */
+		ret_nofiles = B_FALSE;
+
+		for (uint_t i = 0; i < SPA_FEATURES; i++)
+			l_features[i] = B_FALSE;
+
+		/* replace last char with NUL to ensure we have a delimiter */
 		fc[fs.st_size - 1] = '\0';
 
-		for (i = 0; i < SPA_FEATURES; i++)
+		for (line = strtok_r(fc, "\n", &ls);
-			features_local[i] = B_FALSE;
+		    line != NULL;
-
+		    line = strtok_r(NULL, "\n", &ls)) {
-		line = strtok_r(fc, "\n", &ls);
-		while (line != NULL) {
 			/* discard comments */
 			*(strchrnul(line, '#')) = '\0';
 
-			word = strtok_r(line, ", \t", &ws);
+			for (word = strtok_r(line, ", \t", &ws);
-			while (word != NULL) {
+			    word != NULL;
+			    word = strtok_r(NULL, ", \t", &ws)) {
 				/* Find matching feature name */
-				for (i = 0; i < SPA_FEATURES; i++) {
+				uint_t f;
+				for (f = 0; f < SPA_FEATURES; f++) {
 					zfeature_info_t *fi =
-					    &spa_feature_table[i];
+					    &spa_feature_table[f];
 					if (strcmp(word, fi->fi_uname) == 0) {
-						features_local[i] = B_TRUE;
+						l_features[f] = B_TRUE;
 						break;
 					}
 				}
-				if (i == SPA_FEATURES) {
+				if (f < SPA_FEATURES)
-					if (badtoken != NULL)
+					continue;
-						(void) strlcpy(badtoken, word,
+
-						    ZFS_MAXPROPLEN);
+				/* found an unrecognized word */
-					if (badfile != NULL)
+				/* lightly sanitize it */
-						(void) strlcpy(badfile, file,
+				if (strlen(word) > 32)
-						    MAXPATHLEN);
+					word[32] = '\0';
-					(void) munmap((void *) fc, fs.st_size);
+				for (char *c = word; *c != '\0'; c++)
-					(void) close(sdirfd);
+					if (!isprint(*c))
-					(void) close(ddirfd);
+						*c = '?';
-					return (ZPOOL_COMPATIBILITY_BADWORD);
+
-				}
+				strlcat(err_badtoken, word, ZFS_MAXPROPLEN);
-				word = strtok_r(NULL, ", \t", &ws);
+				strlcat(err_badtoken, " ", ZFS_MAXPROPLEN);
+				if (source == Z_SYSCONF)
+					ret_badtoken = B_TRUE;
+				else
+					ret_warntoken = B_TRUE;
 			}
-			line = strtok_r(NULL, "\n", &ls);
 		}
 		(void) munmap((void *) fc, fs.st_size);
-		if (features != NULL) {
+
-			for (i = 0; i < SPA_FEATURES; i++)
+		if (features != NULL)
-				features[i] &= features_local[i];
+			for (uint_t i = 0; i < SPA_FEATURES; i++)
-		}
+				features[i] &= l_features[i];
-		filecount++;
-		file = strtok_r(NULL, ",", &ps);
 	}
 	(void) close(sdirfd);
 	(void) close(ddirfd);
-	if (filecount == 0)
+
+	/* Return the most serious error */
+	if (ret_badfile) {
+		if (report != NULL)
+			snprintf(report, rlen, gettext("could not read/"
+			    "parse feature file(s): %s"), err_badfile);
+		return (ZPOOL_COMPATIBILITY_BADFILE);
+	}
+	if (ret_nofiles) {
+		if (report != NULL)
+			strlcpy(report,
+			    gettext("no valid compatibility files specified"),
+			    rlen);
 		return (ZPOOL_COMPATIBILITY_NOFILES);
+	}
+	if (ret_badtoken) {
+		if (report != NULL)
+			snprintf(report, rlen, gettext("invalid feature "
+			    "name(s) in local compatibility files: %s"),
+			    err_badtoken);
+		return (ZPOOL_COMPATIBILITY_BADTOKEN);
+	}
+	if (ret_warntoken) {
+		if (report != NULL)
+			snprintf(report, rlen, gettext("unrecognized feature "
+			    "name(s) in distribution compatibility files: %s"),
+			    err_badtoken);
+		return (ZPOOL_COMPATIBILITY_WARNTOKEN);
+	}
+	if (report != NULL)
+		strlcpy(report, gettext("compatibility set ok"), rlen);
 	return (ZPOOL_COMPATIBILITY_OK);
 }

lib/libzfs/libzfs_status.c

@@ -89,6 +89,7 @@ static char *zfs_msgid_table[] = {
 	 *	ZPOOL_STATUS_REBUILDING
 	 *	ZPOOL_STATUS_REBUILD_SCRUB
 	 *	ZPOOL_STATUS_COMPATIBILITY_ERR
+	 *	ZPOOL_STATUS_INCOMPATIBLE_FEAT
 	 *	ZPOOL_STATUS_OK
 	 */
 };
@@ -453,11 +454,17 @@ check_status(nvlist_t *config, boolean_t isimport,
 	/*
 	 * Outdated, but usable, version
 	 */
-	if (SPA_VERSION_IS_SUPPORTED(version) && version != SPA_VERSION)
+	if (SPA_VERSION_IS_SUPPORTED(version) && version != SPA_VERSION) {
-		return (ZPOOL_STATUS_VERSION_OLDER);
+		/* "legacy" compatibility disables old version reporting */
+		if (compat != NULL && strcmp(compat, ZPOOL_COMPAT_LEGACY) == 0)
+			return (ZPOOL_STATUS_OK);
+		else
+			return (ZPOOL_STATUS_VERSION_OLDER);
+	}
 
 	/*
-	 * Usable pool with disabled features
+	 * Usable pool with disabled or superfluous features
+	 * (superfluous = beyond what's requested by 'compatibility')
 	 */
 	if (version >= SPA_VERSION_FEATURES) {
 		int i;
@@ -475,18 +482,23 @@ check_status(nvlist_t *config, boolean_t isimport,
 		}
 
 		/* check against all features, or limited set? */
-		boolean_t pool_features[SPA_FEATURES];
+		boolean_t c_features[SPA_FEATURES];
 
-		if (zpool_load_compat(compat, pool_features, NULL, NULL) !=
+		switch (zpool_load_compat(compat, c_features, NULL, 0)) {
-		    ZPOOL_COMPATIBILITY_OK)
+		case ZPOOL_COMPATIBILITY_OK:
+		case ZPOOL_COMPATIBILITY_WARNTOKEN:
+			break;
+		default:
 			return (ZPOOL_STATUS_COMPATIBILITY_ERR);
+		}
 		for (i = 0; i < SPA_FEATURES; i++) {
 			zfeature_info_t *fi = &spa_feature_table[i];
 			if (!fi->fi_zfs_mod_supported)
 				continue;
-			if (pool_features[i] &&
+			if (c_features[i] && !nvlist_exists(feat, fi->fi_guid))
-			    !nvlist_exists(feat, fi->fi_guid))
 				return (ZPOOL_STATUS_FEAT_DISABLED);
+			if (!c_features[i] && nvlist_exists(feat, fi->fi_guid))
+				return (ZPOOL_STATUS_INCOMPATIBLE_FEAT);
 		}
 	}
 

man/man5/zpool-features.5

@@ -166,6 +166,12 @@ enabled when using \fBzpool upgrade\fR. \fBzpool status\fR
 will not show a warning about disabled features which are not part
 of the requested feature set.
 .LP
+The special value \fBlegacy\fR prevents any features from being enabled,
+either via \fBzpool upgrade\fR or via \fBzpool set feature@XX=enabled\fR.
+This setting also prevents pools from being upgraded to newer on-disk
+versions. This is a safety measure to prevent new features from being
+accidentally enabled, breaking compatibility.
+.LP
 By convention, compatibility files in \fB/usr/share/zfs/compatibility.d\fR
 are provided by the distribution package, and include feature sets
 supported by important versions of popular distributions, and feature
@@ -173,6 +179,15 @@ sets commonly supported at the start of each year.  Compatibility files
 in \fB/etc/zfs/compatibility.d\fR, if present, will take precedence over
 files with the same name in \fB/usr/share/zfs/compatibility.d\fR.
 .LP
+If an unrecognized feature is found in these files, an error message will
+be shown. If the unrecognized feature is in a file in
+\fB/etc/zfs/compatibility.d\fR, this is treated as an error and processing
+will stop. If the unrecognized feature is under
+\fB/usr/share/zfs/compatibility.d\fR, this is treated as a warning and
+processing will continue. This difference is to allow distributions to
+include features which might not be recognized by the currently-installed
+binaries.
+.LP
 Compatibility files may include comments; any text from \fB#\fR to the end
 of the line is ignored.
 .LP

man/man8/zpool-upgrade.8

@@ -55,11 +55,9 @@ formatted using a legacy ZFS version number.
 These pools can continue to be used, but some features may not be available.
 Use
 .Nm zpool Cm upgrade Fl a
-to enable all features on all pools. (If a pool has specified compatibility
+to enable all features on all pools (subject to the
-feature sets using the
 .Fl o Ar compatibility
-property, only the features present in all requested compatibility sets will
+property).
-be enabled on that pool.)
 .It Xo
 .Nm zpool
 .Cm upgrade
@@ -75,11 +73,15 @@ for a description of feature flags features supported by the current software.
 .Op Fl V Ar version
 .Fl a Ns | Ns Ar pool Ns ...
 .Xc
-Enables all supported features on the given pool. (If the pool has specified
+Enables all supported features on the given pool.
-compatibility feature sets using the
+.Pp
+If the pool has specified compatibility feature sets using the
 .Fl o Ar compatibility
 property, only the features present in all requested compatibility sets will be
-enabled.)
+enabled. If this property is set to 
+.Ar legacy
+then no upgrade will take place.
+.Pp
 Once this is done, the pool will no longer be accessible on systems that do not
 support feature flags.
 See