Re: [PATCH] ovl: fix regression in showing lowerdir mount option

[Miklos Szeredi <miklos@szeredi.hu>]

[-- Attachment #1: Type: text/plain, Size: 296 bytes --]

On Sat, 14 Oct 2023 at 19:31, Miklos Szeredi <miklos@szeredi.hu> wrote:

> If you can code this up quickly, that's good.  I can have a go at it
> on Monday, but my PoC patch needs splitting up and so it's not ready
> for 6.6.

Attaching my current patch (against your 3 patches).

Thanks,
Miklos

[-- Attachment #2: ovl-layer-lookup.patch --]
[-- Type: text/x-patch, Size: 16361 bytes --]

Index: linux/fs/overlayfs/params.c
===================================================================
--- linux.orig/fs/overlayfs/params.c	2023-10-14 20:18:24.690696842 +0200
+++ linux/fs/overlayfs/params.c	2023-10-14 20:18:31.318713470 +0200
@@ -43,8 +43,10 @@ module_param_named(metacopy, ovl_metacop
 MODULE_PARM_DESC(metacopy,
 		 "Default to on or off for the metadata only copy up feature");
 
-enum {
+enum ovl_opt {
 	Opt_lowerdir,
+	Opt_lowerdir_add,
+	Opt_datadir_add,
 	Opt_upperdir,
 	Opt_workdir,
 	Opt_default_permissions,
@@ -140,10 +142,13 @@ static int ovl_verity_mode_def(void)
 #define fsparam_string_empty(NAME, OPT) \
 	__fsparam(fs_param_is_string, NAME, OPT, fs_param_can_be_empty, NULL)
 
+
 const struct fs_parameter_spec ovl_parameter_spec[] = {
 	fsparam_string_empty("lowerdir",    Opt_lowerdir),
-	fsparam_string("upperdir",          Opt_upperdir),
-	fsparam_string("workdir",           Opt_workdir),
+	fsparam_path("lowerdir+",           Opt_lowerdir_add),
+	fsparam_path("datadir+",            Opt_datadir_add),
+	fsparam_path("upperdir",            Opt_upperdir),
+	fsparam_path("workdir",             Opt_workdir),
 	fsparam_flag("default_permissions", Opt_default_permissions),
 	fsparam_enum("redirect_dir",        Opt_redirect_dir, ovl_parameter_redirect_dir),
 	fsparam_enum("index",               Opt_index, ovl_parameter_bool),
@@ -225,36 +230,6 @@ static ssize_t ovl_parse_param_split_low
 	return nr_layers;
 }
 
-static int ovl_mount_dir_noesc(const char *name, struct path *path)
-{
-	int err = -EINVAL;
-
-	if (!*name) {
-		pr_err("empty lowerdir\n");
-		goto out;
-	}
-	err = kern_path(name, LOOKUP_FOLLOW, path);
-	if (err) {
-		pr_err("failed to resolve '%s': %i\n", name, err);
-		goto out;
-	}
-	err = -EINVAL;
-	if (ovl_dentry_weird(path->dentry)) {
-		pr_err("filesystem on '%s' not supported\n", name);
-		goto out_put;
-	}
-	if (!d_is_dir(path->dentry)) {
-		pr_err("'%s' not a directory\n", name);
-		goto out_put;
-	}
-	return 0;
-
-out_put:
-	path_put_init(path);
-out:
-	return err;
-}
-
 static void ovl_unescape(char *s)
 {
 	char *d = s;
@@ -268,70 +243,179 @@ static void ovl_unescape(char *s)
 	}
 }
 
-static int ovl_mount_dir(const char *name, struct path *path, bool upper)
+static int ovl_mount_dir_check(struct fs_context *fc, const struct path *path,
+			       enum ovl_opt layer, const char *name)
 {
-	int err = -ENOMEM;
-	char *tmp = kstrdup(name, GFP_KERNEL);
+	struct ovl_fs_context *ctx = fc->fs_private;
 
-	if (tmp) {
-		ovl_unescape(tmp);
-		err = ovl_mount_dir_noesc(tmp, path);
+	if (ovl_dentry_weird(path->dentry))
+		return invalfc(fc, "filesystem on %s not supported", name);
 
-		if (!err && upper && path->dentry->d_flags & DCACHE_OP_REAL) {
-			pr_err("filesystem on '%s' not supported as upperdir\n",
-			       tmp);
-			path_put_init(path);
-			err = -EINVAL;
+	if (!d_is_dir(path->dentry))
+		return invalfc(fc, "%s is not a directory", name);
+
+	if (layer == Opt_upperdir || layer == Opt_workdir) {
+		if (path->dentry->d_flags & DCACHE_OP_REAL)
+			return invalfc(fc, "filesystem not supported as %s", name);
+		/*
+		 * Check whether upper path is read-only here to report failures
+		 * early. Don't forget to recheck when the superblock is created
+		 * as the mount attributes could change.
+		 */
+		if (__mnt_is_readonly(path->mnt))
+			return invalfc(fc, "%s is read-only", name);
+	} else {
+		if (ctx->nr == OVL_MAX_STACK)
+			return invalfc(fc, "too many lower directories, limit is %d", OVL_MAX_STACK);
+		if (ctx->nr_data && layer == Opt_lowerdir_add)
+			return invalfc(fc, "regular lower layers cannot follow data layers");
+	}
+	return 0;
+}
+
+static char *escape_colons(char *s, char *p)
+{
+	char *ret = s;
+
+	for (;;) {
+		char c = *p++;
+		if (c != ':') {
+			*s++ = c;
+			if (!c)
+				return ret;
+		} else if (s + 2 > p) {
+			return ERR_PTR(-ENAMETOOLONG);
+		} else {
+			*s++ = '\\';
+			*s++ = c;
 		}
+	}
+}
+
+static char *ovl_layer_path(struct fs_context *fc, const struct path *path,
+			    bool lower, const char *name)
+{
+	char *tmp, *p = NULL;
+
+	tmp = kmalloc(PATH_MAX, GFP_KERNEL);
+	if (tmp) {
+		p = d_path(path, tmp, PATH_MAX);
+		if (!IS_ERR(p) && lower)
+			p = escape_colons(tmp, p);
+		if (!IS_ERR(p))
+			p = kstrdup(p, GFP_KERNEL);
 		kfree(tmp);
 	}
-	return err;
+	if (IS_ERR(p))
+		errorfc(fc, "failed to generate %s pathname", name);
+
+	return p ?: ERR_PTR(-ENOMEM);
 }
 
-static int ovl_parse_param_upperdir(const char *name, struct fs_context *fc,
-				    bool workdir)
+static int ovl_ctx_realloc_lower(struct fs_context *fc)
+{
+	struct ovl_fs_context *ctx = fc->fs_private;
+	struct ovl_fs_context_layer *l;
+	size_t nr;
+
+	if (ctx->nr < ctx->capacity)
+		return 0;
+
+	nr = min(max(4096 / sizeof(*l), ctx->capacity * 2), (size_t) OVL_MAX_STACK);
+	l = krealloc_array(ctx->lower, nr, sizeof(*l), GFP_KERNEL_ACCOUNT);
+	if (!l)
+		return -ENOMEM;
+
+	ctx->lower = l;
+	ctx->capacity = nr;
+	return 0;
+}
+
+static void ovl_add_layer(struct fs_context *fc, enum ovl_opt layer,
+			  struct path *path, char **pp)
 {
-	int err;
 	struct ovl_fs *ofs = fc->s_fs_info;
 	struct ovl_config *config = &ofs->config;
 	struct ovl_fs_context *ctx = fc->fs_private;
+	struct ovl_fs_context_layer *l;
+
+	switch (layer) {
+	case Opt_workdir:
+		swap(config->workdir, *pp);
+		swap(ctx->work, *path);
+		break;
+	case Opt_upperdir:
+		swap(config->upperdir, *pp);
+		swap(ctx->upper, *path);
+		break;
+	case Opt_datadir_add:
+		ctx->nr_data++;
+		fallthrough;
+	case Opt_lowerdir_add:
+		WARN_ON(ctx->nr >= ctx->capacity);
+		l = &ctx->lower[ctx->nr++];
+		memset(l, 0, sizeof(*l));
+		swap(l->name, *pp);
+		swap(l->path, *path);
+		break;
+	default:
+		WARN_ON(1);
+	}
+}
+
+static int ovl_parse_layer(struct fs_context *fc, struct fs_parameter *param,
+			   enum ovl_opt layer)
+{
+	char *pathname = NULL, *tmp = kstrdup(param->string, GFP_KERNEL);
+	bool lower = (layer == Opt_lowerdir_add || layer == Opt_datadir_add);
 	struct path path;
-	char *dup;
+	int err;
 
-	err = ovl_mount_dir(name, &path, true);
+	if (param->type == fs_value_is_string) {
+		if (!tmp)
+			return -ENOMEM;
+		/* look up with unescaped value */
+		ovl_unescape(tmp);
+		swap(param->string, tmp);
+	}
+	err = fs_lookup_param(fc, param, false, LOOKUP_FOLLOW, &path);
+	if (param->type == fs_value_is_string)
+		swap(param->string, tmp);
+	kfree(tmp);
 	if (err)
 		return err;
 
+	err = ovl_mount_dir_check(fc, &path, layer, param->key);
+	if (err)
+		goto put_path;
+
 	/*
-	 * Check whether upper path is read-only here to report failures
-	 * early. Don't forget to recheck when the superblock is created
-	 * as the mount attributes could change.
+	 * If configuring with FSCONFIG_SET_PATH/FSCONFIG_SET_PATH_EMPTY, then
+	 * need to generate a path for mount option display.
 	 */
-	if (__mnt_is_readonly(path.mnt)) {
-		path_put(&path);
-		return -EINVAL;
-	}
+	if (param->type == fs_value_is_string)
+		swap(param->string, pathname);
 
-	dup = kstrdup(name, GFP_KERNEL);
-	if (!dup) {
-		path_put(&path);
-		return -ENOMEM;
-	}
-
-	if (workdir) {
-		kfree(config->workdir);
-		config->workdir = dup;
-		path_put(&ctx->work);
-		ctx->work = path;
-	} else {
-		kfree(config->upperdir);
-		config->upperdir = dup;
-		path_put(&ctx->upper);
-		ctx->upper = path;
-	}
-	return 0;
+	if (!pathname) {
+		pathname = ovl_layer_path(fc, &path, lower, param->key);
+		err = PTR_ERR(pathname);
+		if (IS_ERR(pathname))
+			goto put_path;
+
+		pr_info("Generated pathname: <%s>\n", pathname);
+	}
+	err = 0;
+	if (lower)
+		err = ovl_ctx_realloc_lower(fc);
+	if (!err)
+		ovl_add_layer(fc, layer, &path, &pathname);
+	kfree(pathname);
+put_path:
+	path_put(&path);
+	return err;
 }
 
+
 static void ovl_parse_param_drop_lowerdir(struct ovl_fs_context *ctx)
 {
 	for (size_t nr = 0; nr < ctx->nr; nr++) {
@@ -344,225 +428,51 @@ static void ovl_parse_param_drop_lowerdi
 }
 
 /*
- * Parse lowerdir= mount option:
+ * Parse lowerdir= mount option in old monolithic style:
  *
  * (1) lowerdir=/lower1:/lower2:/lower3::/data1::/data2
  *     Set "/lower1", "/lower2", and "/lower3" as lower layers and
  *     "/data1" and "/data2" as data lower layers. Any existing lower
  *     layers are replaced.
- * (2) lowerdir=:/lower4
- *     Append "/lower4" to current stack of lower layers. This requires
- *     that there already is at least one lower layer configured.
- * (3) lowerdir=::/lower5
- *     Append data "/lower5" as data lower layer. This requires that
- *     there's at least one regular lower layer present.
  */
-static int ovl_parse_param_lowerdir(const char *name, struct fs_context *fc)
+static int ovl_parse_lower_layers(struct fs_context *fc,
+				  struct fs_parameter *param)
 {
-	int err;
+	int err = 0;
 	struct ovl_fs_context *ctx = fc->fs_private;
-	struct ovl_fs_context_layer *l;
-	char *dup = NULL, *dup_iter;
-	ssize_t nr_lower = 0, nr = 0, nr_data = 0;
-	bool append = false, data_layer = false;
-
-	/*
-	 * Ensure we're backwards compatible with mount(2)
-	 * by allowing relative paths.
-	 */
-
-	/* drop all existing lower layers */
-	if (!*name) {
-		ovl_parse_param_drop_lowerdir(ctx);
-		return 0;
-	}
-
-	if (strncmp(name, "::", 2) == 0) {
-		/*
-		 * This is a data layer.
-		 * There must be at least one regular lower layer
-		 * specified.
-		 */
-		if (ctx->nr == 0) {
-			pr_err("data lower layers without regular lower layers not allowed");
-			return -EINVAL;
-		}
-
-		/* Skip the leading "::". */
-		name += 2;
-		data_layer = true;
-		/*
-		 * A data layer is automatically an append as there
-		 * must've been at least one regular lower layer.
-		 */
-		append = true;
-	} else if (*name == ':') {
-		/*
-		 * This is a regular lower layer.
-		 * If users want to append a layer enforce that they
-		 * have already specified a first layer before. It's
-		 * better to be strict.
-		 */
-		if (ctx->nr == 0) {
-			pr_err("cannot append layer if no previous layer has been specified");
-			return -EINVAL;
-		}
-
-		/*
-		 * Once a sequence of data layers has started regular
-		 * lower layers are forbidden.
-		 */
-		if (ctx->nr_data > 0) {
-			pr_err("regular lower layers cannot follow data lower layers");
-			return -EINVAL;
-		}
-
-		/* Skip the leading ":". */
-		name++;
-		append = true;
-	}
+	char *iter;
+	ssize_t nr_lower, nr;
+	enum ovl_opt layer = Opt_lowerdir_add;
 
-	dup = kstrdup(name, GFP_KERNEL);
-	if (!dup)
-		return -ENOMEM;
+	ovl_parse_param_drop_lowerdir(ctx);
 
-	err = -EINVAL;
-	nr_lower = ovl_parse_param_split_lowerdirs(dup);
+	nr_lower = ovl_parse_param_split_lowerdirs(param->string);
 	if (nr_lower < 0)
-		goto out_err;
+		return nr_lower;
 
-	if ((nr_lower > OVL_MAX_STACK) ||
-	    (append && (size_add(ctx->nr, nr_lower) > OVL_MAX_STACK))) {
-		pr_err("too many lower directories, limit is %d\n", OVL_MAX_STACK);
-		goto out_err;
-	}
-
-	if (!append)
-		ovl_parse_param_drop_lowerdir(ctx);
+	iter = param->string;
+	for (nr = 0; nr < nr_lower; nr++) {
+		struct fs_parameter p = {
+			.key = (layer == Opt_lowerdir_add) ? "lowerdir" : "datadir",
+			.type = fs_value_is_string,
+			.string = kstrdup(iter, GFP_KERNEL),
+		};
 
-	/*
-	 * (1) append
-	 *
-	 * We want nr <= nr_lower <= capacity We know nr > 0 and nr <=
-	 * capacity. If nr == 0 this wouldn't be append. If nr +
-	 * nr_lower is <= capacity then nr <= nr_lower <= capacity
-	 * already holds. If nr + nr_lower exceeds capacity, we realloc.
-	 *
-	 * (2) replace
-	 *
-	 * Ensure we're backwards compatible with mount(2) which allows
-	 * "lowerdir=/a:/b:/c,lowerdir=/d:/e:/f" causing the last
-	 * specified lowerdir mount option to win.
-	 *
-	 * We want nr <= nr_lower <= capacity We know either (i) nr == 0
-	 * or (ii) nr > 0. We also know nr_lower > 0. The capacity
-	 * could've been changed multiple times already so we only know
-	 * nr <= capacity. If nr + nr_lower > capacity we realloc,
-	 * otherwise nr <= nr_lower <= capacity holds already.
-	 */
-	nr_lower += ctx->nr;
-	if (nr_lower > ctx->capacity) {
-		err = -ENOMEM;
-		l = krealloc_array(ctx->lower, nr_lower, sizeof(*ctx->lower),
-				   GFP_KERNEL_ACCOUNT);
-		if (!l)
-			goto out_err;
+		if (!p.string)
+			return -ENOMEM;
 
-		ctx->lower = l;
-		ctx->capacity = nr_lower;
-	}
-
-	/*
-	 *   (3) By (1) and (2) we know nr <= nr_lower <= capacity.
-	 *   (4) If ctx->nr == 0 => replace
-	 *       We have verified above that the lowerdir mount option
-	 *       isn't an append, i.e., the lowerdir mount option
-	 *       doesn't start with ":" or "::".
-	 * (4.1) The lowerdir mount options only contains regular lower
-	 *       layers ":".
-	 *       => Nothing to verify.
-	 * (4.2) The lowerdir mount options contains regular ":" and
-	 *       data "::" layers.
-	 *       => We need to verify that data lower layers "::" aren't
-	 *          followed by regular ":" lower layers
-	 *   (5) If ctx->nr > 0 => append
-	 *       We know that there's at least one regular layer
-	 *       otherwise we would've failed when parsing the previous
-	 *       lowerdir mount option.
-	 * (5.1) The lowerdir mount option is a regular layer ":" append
-	 *       => We need to verify that no data layers have been
-	 *          specified before.
-	 * (5.2) The lowerdir mount option is a data layer "::" append
-	 *       We know that there's at least one regular layer or
-	 *       other data layers. => There's nothing to verify.
-	 */
-	dup_iter = dup;
-	for (nr = ctx->nr; nr < nr_lower; nr++) {
-		l = &ctx->lower[nr];
-		memset(l, 0, sizeof(*l));
-
-		err = ovl_mount_dir(dup_iter, &l->path, false);
+		err = ovl_parse_layer(fc, &p, layer);
+		kfree(p.string);
 		if (err)
-			goto out_put;
-
-		err = -ENOMEM;
-		l->name = kstrdup(dup_iter, GFP_KERNEL_ACCOUNT);
-		if (!l->name)
-			goto out_put;
-
-		if (data_layer)
-			nr_data++;
-
-		/* Calling strchr() again would overrun. */
-		if ((nr + 1) == nr_lower)
 			break;
 
-		err = -EINVAL;
-		dup_iter = strchr(dup_iter, '\0') + 1;
-		if (*dup_iter) {
-			/*
-			 * This is a regular layer so we require that
-			 * there are no data layers.
-			 */
-			if ((ctx->nr_data + nr_data) > 0) {
-				pr_err("regular lower layers cannot follow data lower layers");
-				goto out_put;
-			}
-
-			data_layer = false;
-			continue;
+		layer = Opt_lowerdir_add;
+		iter = strchr(iter, '\0') + 1;
+		if (!*iter) {
+			layer = Opt_datadir_add;
+			iter++;
 		}
-
-		/* This is a data lower layer. */
-		data_layer = true;
-		dup_iter++;
-	}
-	ctx->nr = nr_lower;
-	ctx->nr_data += nr_data;
-	kfree(dup);
-	return 0;
-
-out_put:
-	/*
-	 * We know nr >= ctx->nr < nr_lower. If we failed somewhere
-	 * we want to undo until nr == ctx->nr. This is correct for
-	 * both ctx->nr == 0 and ctx->nr > 0.
-	 */
-	for (; nr >= ctx->nr; nr--) {
-		l = &ctx->lower[nr];
-		kfree(l->name);
-		l->name = NULL;
-		path_put(&l->path);
-
-		/* don't overflow */
-		if (nr == 0)
-			break;
 	}
-
-out_err:
-	kfree(dup);
-
-	/* Intentionally don't realloc to a smaller size. */
 	return err;
 }
 
@@ -600,13 +510,13 @@ static int ovl_parse_param(struct fs_con
 
 	switch (opt) {
 	case Opt_lowerdir:
-		err = ovl_parse_param_lowerdir(param->string, fc);
+		err = ovl_parse_lower_layers(fc, param);
 		break;
+	case Opt_lowerdir_add:
+	case Opt_datadir_add:
 	case Opt_upperdir:
-		fallthrough;
 	case Opt_workdir:
-		err = ovl_parse_param_upperdir(param->string, fc,
-					       (Opt_workdir == opt));
+		err = ovl_parse_layer(fc, param, opt);
 		break;
 	case Opt_default_permissions:
 		config->default_permissions = true;
Index: linux/fs/fs_parser.c
===================================================================
--- linux.orig/fs/fs_parser.c	2023-10-14 20:18:25.548698995 +0200
+++ linux/fs/fs_parser.c	2023-10-14 20:18:30.464711328 +0200
@@ -150,6 +150,7 @@ int fs_lookup_param(struct fs_context *f
 	struct filename *f;
 	bool put_f;
 	int ret;
+	int dfd = AT_FDCWD;
 
 	switch (param->type) {
 	case fs_value_is_string:
@@ -160,13 +161,14 @@ int fs_lookup_param(struct fs_context *f
 		break;
 	case fs_value_is_filename:
 		f = param->name;
+		dfd = param->dirfd;
 		put_f = false;
 		break;
 	default:
 		return invalf(fc, "%s: not usable as path", param->key);
 	}
 
-	ret = filename_lookup(param->dirfd, f, flags, _path, NULL);
+	ret = filename_lookup(dfd, f, flags, _path, NULL);
 	if (ret < 0) {
 		errorf(fc, "%s: Lookup failure for '%s'", param->key, f->name);
 		goto out;