Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 2 Aug 2018 21:19:36 +0000 (UTC)
From:      Alexander Motin <mav@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r337183 - in head: cddl/contrib/opensolaris/cmd/zfs cddl/contrib/opensolaris/lib/libzfs/common sys/cddl/contrib/opensolaris/common/zfs sys/cddl/contrib/opensolaris/uts/common/fs/zfs
Message-ID:  <201808022119.w72LJaNY044357@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: mav
Date: Thu Aug  2 21:19:35 2018
New Revision: 337183
URL: https://svnweb.freebsd.org/changeset/base/337183

Log:
  MFV r337182: 9330 stack overflow when creating a deeply nested dataset
  
  Datasets that are deeply nested (~100 levels) are impractical. We just put
  a limit of 50 levels to newly created datasets. Existing datasets should
  work without a problem.
  
  illumos/illumos-gate@5ac95da7d61660aa299c287a39277cb0372be959
  
  Reviewed by: John Kennedy <john.kennedy@delphix.com>
  Reviewed by: Matt Ahrens <matt@delphix.com>
  Approved by: Garrett D'Amore <garrett@damore.org>
  Author:     Serapheim Dimitropoulos <serapheim.dimitro@delphix.com>

Modified:
  head/cddl/contrib/opensolaris/cmd/zfs/zfs.8
  head/cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c
  head/sys/cddl/contrib/opensolaris/common/zfs/zfs_namecheck.c
  head/sys/cddl/contrib/opensolaris/common/zfs/zfs_namecheck.h
  head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dmu_objset.c
  head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dir.c
Directory Properties:
  head/cddl/contrib/opensolaris/   (props changed)
  head/sys/cddl/contrib/opensolaris/   (props changed)

Modified: head/cddl/contrib/opensolaris/cmd/zfs/zfs.8
==============================================================================
--- head/cddl/contrib/opensolaris/cmd/zfs/zfs.8	Thu Aug  2 21:12:52 2018	(r337182)
+++ head/cddl/contrib/opensolaris/cmd/zfs/zfs.8	Thu Aug  2 21:19:35 2018	(r337183)
@@ -320,7 +320,8 @@ namespace. For example:
 .Pp
 where the maximum length of a dataset name is
 .Dv MAXNAMELEN
-(256 bytes).
+(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 -hang -width 12n

Modified: head/cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c
==============================================================================
--- head/cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c	Thu Aug  2 21:12:52 2018	(r337182)
+++ head/cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c	Thu Aug  2 21:19:35 2018	(r337183)
@@ -3449,8 +3449,22 @@ zfs_create_ancestors(libzfs_handle_t *hdl, const char 
 {
 	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);
 
@@ -3486,6 +3500,12 @@ zfs_create(libzfs_handle_t *hdl, const char *path, zfs
 	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);
@@ -4286,6 +4306,7 @@ zfs_rename(zfs_handle_t *zhp, const char *source, cons
 				    errbuf));
 			}
 		}
+
 		if (!zfs_validate_name(hdl, target, zhp->zfs_type, B_TRUE))
 			return (zfs_error(hdl, EZFS_INVALIDNAME, errbuf));
 	} else {

Modified: head/sys/cddl/contrib/opensolaris/common/zfs/zfs_namecheck.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/common/zfs/zfs_namecheck.c	Thu Aug  2 21:12:52 2018	(r337182)
+++ head/sys/cddl/contrib/opensolaris/common/zfs/zfs_namecheck.c	Thu Aug  2 21:19:35 2018	(r337183)
@@ -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)
 {
@@ -60,10 +66,35 @@ valid_char(char 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_er
  * 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)
@@ -121,28 +154,40 @@ permset_namecheck(const char *path, namecheck_err_t *w
 }
 
 /*
+ * 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 *wh
 		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 *wh
 				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 *w
  * 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
  * 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)

Modified: head/sys/cddl/contrib/opensolaris/common/zfs/zfs_namecheck.h
==============================================================================
--- head/sys/cddl/contrib/opensolaris/common/zfs/zfs_namecheck.h	Thu Aug  2 21:12:52 2018	(r337182)
+++ head/sys/cddl/contrib/opensolaris/common/zfs/zfs_namecheck.h	Thu Aug  2 21:19:35 2018	(r337183)
@@ -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 *);

Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dmu_objset.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dmu_objset.c	Thu Aug  2 21:12:52 2018	(r337182)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dmu_objset.c	Thu Aug  2 21:19:35 2018	(r337183)
@@ -54,6 +54,7 @@
 #include <sys/dsl_destroy.h>
 #include <sys/vdev.h>
 #include <sys/zfeature.h>
+#include "zfs_namecheck.h"
 
 /*
  * Needed to close a window in dnode_move() that allows the objset to be freed
@@ -911,6 +912,9 @@ dmu_objset_create_check(void *arg, dmu_tx_t *tx)
 		return (SET_ERROR(EINVAL));
 
 	if (strlen(doca->doca_name) >= ZFS_MAX_DATASET_NAME_LEN)
+		return (SET_ERROR(ENAMETOOLONG));
+
+	if (dataset_nestcheck(doca->doca_name) != 0)
 		return (SET_ERROR(ENAMETOOLONG));
 
 	error = dsl_dir_hold(dp, doca->doca_name, FTAG, &pdd, &tail);

Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dir.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dir.c	Thu Aug  2 21:12:52 2018	(r337182)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/dsl_dir.c	Thu Aug  2 21:19:35 2018	(r337183)
@@ -1819,17 +1819,29 @@ typedef struct dsl_dir_rename_arg {
 	cred_t *ddra_cred;
 } dsl_dir_rename_arg_t;
 
+typedef struct dsl_valid_rename_arg {
+	int char_delta;
+	int nest_delta;
+} dsl_valid_rename_arg_t;
+
 /* ARGSUSED */
 static int
 dsl_valid_rename(dsl_pool_t *dp, dsl_dataset_t *ds, void *arg)
 {
-	int *deltap = arg;
+	dsl_valid_rename_arg_t *dvra = arg;
 	char namebuf[ZFS_MAX_DATASET_NAME_LEN];
 
 	dsl_dataset_name(ds, namebuf);
 
-	if (strlen(namebuf) + *deltap >= ZFS_MAX_DATASET_NAME_LEN)
+	ASSERT3U(strnlen(namebuf, ZFS_MAX_DATASET_NAME_LEN),
+	    <, ZFS_MAX_DATASET_NAME_LEN);
+	int namelen = strlen(namebuf) + dvra->char_delta;
+	int depth = get_dataset_depth(namebuf) + dvra->nest_delta;
+
+	if (namelen >= ZFS_MAX_DATASET_NAME_LEN)
 		return (SET_ERROR(ENAMETOOLONG));
+	if (dvra->nest_delta > 0 && depth >= zfs_max_dataset_nesting)
+		return (SET_ERROR(ENAMETOOLONG));
 	return (0);
 }
 
@@ -1839,9 +1851,9 @@ dsl_dir_rename_check(void *arg, dmu_tx_t *tx)
 	dsl_dir_rename_arg_t *ddra = arg;
 	dsl_pool_t *dp = dmu_tx_pool(tx);
 	dsl_dir_t *dd, *newparent;
+	dsl_valid_rename_arg_t dvra;
 	const char *mynewname;
 	int error;
-	int delta = strlen(ddra->ddra_newname) - strlen(ddra->ddra_oldname);
 
 	/* target dir should exist */
 	error = dsl_dir_hold(dp, ddra->ddra_oldname, FTAG, &dd, NULL);
@@ -1870,10 +1882,19 @@ dsl_dir_rename_check(void *arg, dmu_tx_t *tx)
 		return (SET_ERROR(EEXIST));
 	}
 
+	ASSERT3U(strnlen(ddra->ddra_newname, ZFS_MAX_DATASET_NAME_LEN),
+	    <, ZFS_MAX_DATASET_NAME_LEN);
+	ASSERT3U(strnlen(ddra->ddra_oldname, ZFS_MAX_DATASET_NAME_LEN),
+	    <, ZFS_MAX_DATASET_NAME_LEN);
+	dvra.char_delta = strlen(ddra->ddra_newname)
+	    - strlen(ddra->ddra_oldname);
+	dvra.nest_delta = get_dataset_depth(ddra->ddra_newname)
+	    - get_dataset_depth(ddra->ddra_oldname);
+
 	/* if the name length is growing, validate child name lengths */
-	if (delta > 0) {
+	if (dvra.char_delta > 0 || dvra.nest_delta > 0) {
 		error = dmu_objset_find_dp(dp, dd->dd_object, dsl_valid_rename,
-		    &delta, DS_FIND_CHILDREN | DS_FIND_SNAPSHOTS);
+		    &dvra, DS_FIND_CHILDREN | DS_FIND_SNAPSHOTS);
 		if (error != 0) {
 			dsl_dir_rele(newparent, FTAG);
 			dsl_dir_rele(dd, FTAG);



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201808022119.w72LJaNY044357>