9330 stack overflow when creating a deeply nested dataset

Reviewed by: John Kennedy <john.kennedy@delphix.com>
Reviewed by: Matt Ahrens <matt@delphix.com>
Approved by: Garrett D'Amore <garrett@damore.org>

usr/src/cmd/mdb/common/modules/zfs/zfs.c

@@ -576,6 +576,7 @@ zfs_params(uintptr_t addr, uint_t flags, int argc, const mdb_arg_t *argv)
 		"zvol_maxphys",
 		"zvol_unmap_enabled",
 		"zvol_unmap_sync_enabled",
+		"zfs_max_dataset_nesting",
 	};
 
 	for (int i = 0; i < sizeof (params) / sizeof (params[0]); i++) {

usr/src/common/zfs/zfs_namecheck.c

@@ -23,7 +23,7 @@
  * Use is subject to license terms.
  */
 /*
- * Copyright (c) 2013 by Delphix. All rights reserved.
+ * Copyright (c) 2013, 2016 by Delphix. All rights reserved.
  */
 
 /*
@@ -34,8 +34,6 @@
  * name is invalid.  In the kernel, we only care whether it's valid or not.
  * Each routine therefore takes a 'namecheck_err_t' which describes exactly why
  * the name failed to validate.
- *
- * Each function returns 0 on success, -1 on error.
  */
 
 #if defined(_KERNEL)
@@ -50,6 +48,14 @@
 #include "zfs_namecheck.h"
 #include "zfs_deleg.h"
 
+/*
+ * Deeply nested datasets can overflow the stack, so we put a limit
+ * in the amount of nesting a path can have. zfs_max_dataset_nesting
+ * can be tuned temporarily to fix existing datasets that exceed our
+ * predefined limit.
+ */
+int zfs_max_dataset_nesting = 50;
+
 static int
 valid_char(char c)
 {
@@ -59,11 +65,36 @@ valid_char(char c)
 	    c == '-' || c == '_' || c == '.' || c == ':' || c == ' ');
 }
 
+/*
+ * Looks at a path and returns its level of nesting (depth).
+ */
+int
+get_dataset_depth(const char *path)
+{
+	const char *loc = path;
+	int nesting = 0;
+
+	/*
+	 * Keep track of nesting until you hit the end of the
+	 * path or found the snapshot/bookmark seperator.
+	 */
+	for (int i = 0; loc[i] != '\0' &&
+	    loc[i] != '@' &&
+	    loc[i] != '#'; i++) {
+		if (loc[i] == '/')
+			nesting++;
+	}
+
+	return (nesting);
+}
+
 /*
  * Snapshot names must be made up of alphanumeric characters plus the following
  * characters:
  *
- * 	[-_.: ]
+ *	[-_.: ]
+ *
+ * Returns 0 on success, -1 on error.
  */
 int
 zfs_component_namecheck(const char *path, namecheck_err_t *why, char *what)
@@ -99,6 +130,8 @@ zfs_component_namecheck(const char *path, namecheck_err_t *why, char *what)
  * Permissions set name must start with the letter '@' followed by the
  * same character restrictions as snapshot names, except that the name
  * cannot exceed 64 characters.
+ *
+ * Returns 0 on success, -1 on error.
  */
 int
 permset_namecheck(const char *path, namecheck_err_t *why, char *what)
@@ -120,29 +153,41 @@ permset_namecheck(const char *path, namecheck_err_t *why, char *what)
 	return (zfs_component_namecheck(&path[1], why, what));
 }
 
+/*
+ * Dataset paths should not be deeper than zfs_max_dataset_nesting
+ * in terms of nesting.
+ *
+ * Returns 0 on success, -1 on error.
+ */
+int
+dataset_nestcheck(const char *path)
+{
+	return ((get_dataset_depth(path) < zfs_max_dataset_nesting) ? 0 : -1);
+}
+
 /*
  * Entity names must be of the following form:
  *
- * 	[component/]*[component][(@|#)component]?
+ *	[component/]*[component][(@|#)component]?
  *
  * Where each component is made up of alphanumeric characters plus the following
  * characters:
  *
- * 	[-_.:%]
+ *	[-_.:%]
  *
  * We allow '%' here as we use that character internally to create unique
  * names for temporary clones (for online recv).
+ *
+ * Returns 0 on success, -1 on error.
  */
 int
 entity_namecheck(const char *path, namecheck_err_t *why, char *what)
 {
-	const char *start, *end;
-	int found_delim;
+	const char *end;
 
 	/*
 	 * Make sure the name is not too long.
 	 */
-
 	if (strlen(path) >= ZFS_MAX_DATASET_NAME_LEN) {
 		if (why)
 			*why = NAME_ERR_TOOLONG;
@@ -162,8 +207,8 @@ entity_namecheck(const char *path, namecheck_err_t *why, char *what)
 		return (-1);
 	}
 
-	start = path;
-	found_delim = 0;
+	const char *start = path;
+	boolean_t found_delim = B_FALSE;
 	for (;;) {
 		/* Find the end of this component */
 		end = start;
@@ -198,7 +243,7 @@ entity_namecheck(const char *path, namecheck_err_t *why, char *what)
 				return (-1);
 			}
 
-			found_delim = 1;
+			found_delim = B_TRUE;
 		}
 
 		/* Zero-length components are not allowed */
@@ -250,6 +295,8 @@ dataset_namecheck(const char *path, namecheck_err_t *why, char *what)
  * mountpoint names must be of the following form:
  *
  *	/[component][/]*[component][/]
+ *
+ * Returns 0 on success, -1 on error.
  */
 int
 mountpoint_namecheck(const char *path, namecheck_err_t *why)
@@ -294,6 +341,8 @@ mountpoint_namecheck(const char *path, namecheck_err_t *why)
  * dataset names, with the additional restriction that the pool name must begin
  * with a letter.  The pool names 'raidz' and 'mirror' are also reserved names
  * that cannot be used.
+ *
+ * Returns 0 on success, -1 on error.
  */
 int
 pool_namecheck(const char *pool, namecheck_err_t *why, char *what)

usr/src/common/zfs/zfs_namecheck.h

@@ -23,7 +23,7 @@
  * Use is subject to license terms.
  */
 /*
- * Copyright (c) 2013 by Delphix. All rights reserved.
+ * Copyright (c) 2013, 2016 by Delphix. All rights reserved.
  */
 
 #ifndef	_ZFS_NAMECHECK_H
@@ -48,9 +48,13 @@ typedef enum {
 
 #define	ZFS_PERMSET_MAXLEN	64
 
+extern int zfs_max_dataset_nesting;
+
+int get_dataset_depth(const char *);
 int pool_namecheck(const char *, namecheck_err_t *, char *);
 int entity_namecheck(const char *, namecheck_err_t *, char *);
 int dataset_namecheck(const char *, namecheck_err_t *, char *);
+int dataset_nestcheck(const char *);
 int mountpoint_namecheck(const char *, namecheck_err_t *);
 int zfs_component_namecheck(const char *, namecheck_err_t *, char *);
 int permset_namecheck(const char *, namecheck_err_t *, char *);

usr/src/lib/libzfs/common/libzfs_dataset.c

@@ -3409,8 +3409,22 @@ zfs_create_ancestors(libzfs_handle_t *hdl, const char *path)
 {
 	int prefix;
 	char *path_copy;
+	char errbuf[1024];
 	int rc = 0;
 
+	(void) snprintf(errbuf, sizeof (errbuf), dgettext(TEXT_DOMAIN,
+	    "cannot create '%s'"), path);
+
+	/*
+	 * Check that we are not passing the nesting limit
+	 * before we start creating any ancestors.
+	 */
+	if (dataset_nestcheck(path) != 0) {
+		zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
+		    "maximum name nesting depth exceeded"));
+		return (zfs_error(hdl, EZFS_INVALIDNAME, errbuf));
+	}
+
 	if (check_parents(hdl, path, NULL, B_TRUE, &prefix) != 0)
 		return (-1);
 
@@ -3446,6 +3460,12 @@ zfs_create(libzfs_handle_t *hdl, const char *path, zfs_type_t type,
 	if (!zfs_validate_name(hdl, path, type, B_TRUE))
 		return (zfs_error(hdl, EZFS_INVALIDNAME, errbuf));
 
+	if (dataset_nestcheck(path) != 0) {
+		zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
+		    "maximum name nesting depth exceeded"));
+		return (zfs_error(hdl, EZFS_INVALIDNAME, errbuf));
+	}
+
 	/* validate parents exist */
 	if (check_parents(hdl, path, &zoned, B_FALSE, NULL) != 0)
 		return (-1);
@@ -4233,6 +4253,7 @@ zfs_rename(zfs_handle_t *zhp, const char *target, boolean_t recursive,
 				    errbuf));
 			}
 		}
+
 		if (!zfs_validate_name(hdl, target, zhp->zfs_type, B_TRUE))
 			return (zfs_error(hdl, EZFS_INVALIDNAME, errbuf));
 	} else {

usr/src/man/man1m/zfs.1m

@@ -289,7 +289,8 @@ pool/{filesystem,volume,snapshot}
 .Pp
 where the maximum length of a dataset name is
 .Dv MAXNAMELEN
-.Pq 256 bytes .
+.Pq 256 bytes
+and the maximum amount of nesting allowed in a path is 50 levels deep.
 .Pp
 A dataset can be one of the following:
 .Bl -tag -width "file system"

usr/src/pkg/manifests/system-test-zfstest.mf

@@ -1011,6 +1011,9 @@ file \
 file \
     path=opt/zfs-tests/tests/functional/cli_root/zfs_rename/zfs_rename_013_pos \
     mode=0555
+file \
+    path=opt/zfs-tests/tests/functional/cli_root/zfs_rename/zfs_rename_014_neg \
+    mode=0555
 file path=opt/zfs-tests/tests/functional/cli_root/zfs_reservation/cleanup \
     mode=0555
 file path=opt/zfs-tests/tests/functional/cli_root/zfs_reservation/setup \

usr/src/test/zfs-tests/runfiles/delphix.run

@@ -170,7 +170,7 @@ tests = ['zfs_rename_001_pos', 'zfs_rename_002_pos', 'zfs_rename_003_pos',
     'zfs_rename_004_neg', 'zfs_rename_005_neg', 'zfs_rename_006_pos',
     'zfs_rename_007_pos', 'zfs_rename_008_pos', 'zfs_rename_009_neg',
     'zfs_rename_010_neg', 'zfs_rename_011_pos', 'zfs_rename_012_neg',
-    'zfs_rename_013_pos']
+    'zfs_rename_013_pos', 'zfs_rename_014_neg']
 
 [/opt/zfs-tests/tests/functional/cli_root/zfs_reservation]
 tests = ['zfs_reservation_001_pos', 'zfs_reservation_002_pos']

usr/src/test/zfs-tests/runfiles/omnios.run

@@ -160,7 +160,7 @@ tests = ['zfs_rename_001_pos', 'zfs_rename_002_pos', 'zfs_rename_003_pos',
     'zfs_rename_004_neg', 'zfs_rename_005_neg', 'zfs_rename_006_pos',
     'zfs_rename_007_pos', 'zfs_rename_008_pos', 'zfs_rename_009_neg',
     'zfs_rename_010_neg', 'zfs_rename_011_pos', 'zfs_rename_012_neg',
-    'zfs_rename_013_pos']
+    'zfs_rename_013_pos', 'zfs_rename_014_neg']
 
 [/opt/zfs-tests/tests/functional/cli_root/zfs_remap]
 tests = ['zfs_remap_cliargs', 'zfs_remap_obsolete_counts']

usr/src/test/zfs-tests/runfiles/openindiana.run

@@ -160,7 +160,7 @@ tests = ['zfs_rename_001_pos', 'zfs_rename_002_pos', 'zfs_rename_003_pos',
     'zfs_rename_004_neg', 'zfs_rename_005_neg', 'zfs_rename_006_pos',
     'zfs_rename_007_pos', 'zfs_rename_008_pos', 'zfs_rename_009_neg',
     'zfs_rename_010_neg', 'zfs_rename_011_pos', 'zfs_rename_012_neg',
-    'zfs_rename_013_pos']
+    'zfs_rename_013_pos', 'zfs_rename_014_neg']
 
 [/opt/zfs-tests/tests/functional/cli_root/zfs_remap]
 tests = ['zfs_remap_cliargs', 'zfs_remap_obsolete_counts']

usr/src/test/zfs-tests/tests/functional/cli_root/zfs_create/zfs_create.cfg

@@ -25,7 +25,7 @@
 #
 
 #
-# Copyright (c) 2012 by Delphix. All rights reserved.
+# Copyright (c) 2012, 2016 by Delphix. All rights reserved.
 #
 
 export BYND_MAX_NAME="byondmaxnamelength\
@@ -38,6 +38,12 @@ export BYND_MAX_NAME="byondmaxnamelength\
 012345678901234567890123456789\
 012345678901234567890123456789"
 
+export BYND_NEST_LIMIT="a/a/a/\
+a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
+a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
+a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/\
+a/a"
+
 # There're 3 different prompt messages while create
 # a volume that great than 1TB on 32-bit
 #       - volume size exceeds limit for this system. (happy gate)

usr/src/test/zfs-tests/tests/functional/cli_root/zfs_create/zfs_create_009_neg.ksh

@@ -43,6 +43,7 @@
 #	*Beyond maximal name length.
 #	*Same property set multiple times via '-o property=value'
 #	*Volume's property set on filesystem
+#	*Exceeding maximum name nesting
 #
 # STRATEGY:
 # 1. Create an array of <filesystem> arguments
@@ -89,7 +90,7 @@ set -A args  "$TESTPOOL/" "$TESTPOOL//blah" "$TESTPOOL/@blah" \
 	"$TESTPOOL/blah*blah" "$TESTPOOL/blah blah" \
 	"-s $TESTPOOL/$TESTFS1" "-b 1092 $TESTPOOL/$TESTFS1" \
 	"-b 64k $TESTPOOL/$TESTFS1" "-s -b 32k $TESTPOOL/$TESTFS1" \
-	"$TESTPOOL/$BYND_MAX_NAME"
+	"$TESTPOOL/$BYND_MAX_NAME" "$TESTPOOL/$BYND_NEST_LIMIT"
 
 log_assert "Verify 'zfs create <filesystem>' fails with bad <filesystem> argument."