[PATCH v3 0/3] ovl: metacopy/verity fixes and improvements

[PATCH v3 0/3] ovl: metacopy/verity fixes and improvements

[Miklos Szeredi]

The main purpose of this patchset is allowing metadata/data-only layers to
be usable in user namespaces (without super user privs).  The main use case
is composefs in unprivileged containers.

Will post xfstests testcases shortly.

v3:
 - consistently refuse following redirect/metacopy for upper found through
   index (dropped RVB's due to this change)
 - move redirect/metacopy check into helper
 - remove verity -> metacopy dependency (Amir)
 - stable fixes moved to git://git.kernel.org/pub/scm/linux/kernel/git/overlayfs/vfs.git#ovl-fixes

v2:
 - drop broken hunk in param.c (Amir)
 - patch header improvements (Amir)


---
Miklos Szeredi (3):
  ovl: make redirect/metacopy rejection consistent
  ovl: relax redirect/metacopy requirements for lower -> data redirect
  ovl: don't require "metacopy=on" for "verity"

 Documentation/filesystems/overlayfs.rst |  7 ++
 fs/overlayfs/namei.c                    | 89 +++++++++++++++++--------
 fs/overlayfs/params.c                   | 31 +--------
 3 files changed, 71 insertions(+), 56 deletions(-)

-- 
2.49.0

[PATCH v3 1/3] ovl: make redirect/metacopy rejection consistent

[Miklos Szeredi]

When overlayfs finds a file with metacopy and/or redirect attributes and
the metacopy and/or redirect features are not enabled, then it refuses to
act on those attributes while also issuing a warning.

There was an inconsistency in not checking metacopy found from the index.

And also only warning on an upper metacopy if it found the next file on the
lower layer, while always warning for metacopy found on a lower layer.

Fix these inconsistencies and make the logic more straightforward, paving
the way for following patches to change when dataredirects are allowed.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/namei.c | 81 +++++++++++++++++++++++++++++++-------------
 1 file changed, 57 insertions(+), 24 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index be5c65d6f848..5cebdd05ab3a 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -16,6 +16,7 @@
 
 struct ovl_lookup_data {
 	struct super_block *sb;
+	struct dentry *dentry;
 	const struct ovl_layer *layer;
 	struct qstr name;
 	bool is_dir;
@@ -23,6 +24,8 @@ struct ovl_lookup_data {
 	bool xwhiteouts;
 	bool stop;
 	bool last;
+	bool nextredirect;
+	bool nextmetacopy;
 	char *redirect;
 	int metacopy;
 	/* Referring to last redirect xattr */
@@ -1024,6 +1027,31 @@ int ovl_verify_lowerdata(struct dentry *dentry)
 	return ovl_maybe_validate_verity(dentry);
 }
 
+/*
+ * Following redirects/metacopy can have security consequences: it's like a
+ * symlink into the lower layer without the permission checks.
+ *
+ * This is only a problem if the upper layer is untrusted (e.g comes from an USB
+ * drive).  This can allow a non-readable file or directory to become readable.
+ *
+ * Only following redirects when redirects are enabled disables this attack
+ * vector when not necessary.
+ */
+static bool ovl_check_nextredirect(struct ovl_lookup_data *d)
+{
+	struct ovl_fs *ofs = OVL_FS(d->sb);
+
+	if (d->nextmetacopy && !ofs->config.metacopy) {
+		pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", d->dentry);
+		return false;
+	}
+	if (d->nextredirect && !ovl_redirect_follow(ofs)) {
+		pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", d->dentry);
+		return false;
+	}
+	return true;
+}
+
 struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			  unsigned int flags)
 {
@@ -1047,6 +1075,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	int metacopy_size = 0;
 	struct ovl_lookup_data d = {
 		.sb = dentry->d_sb,
+		.dentry = dentry,
 		.name = dentry->d_name,
 		.is_dir = false,
 		.opaque = false,
@@ -1054,6 +1083,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		.last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
 		.redirect = NULL,
 		.metacopy = 0,
+		.nextredirect = false,
+		.nextmetacopy = false,
 	};
 
 	if (dentry->d_name.len > ofs->namelen)
@@ -1087,8 +1118,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			if (err)
 				goto out_put_upper;
 
-			if (d.metacopy)
+			if (d.metacopy) {
 				uppermetacopy = true;
+				d.nextmetacopy = true;
+			}
 			metacopy_size = d.metacopy;
 		}
 
@@ -1099,6 +1132,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 				goto out_put_upper;
 			if (d.redirect[0] == '/')
 				poe = roe;
+			d.nextredirect = true;
 		}
 		upperopaque = d.opaque;
 	}
@@ -1113,6 +1147,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	for (i = 0; !d.stop && i < ovl_numlower(poe); i++) {
 		struct ovl_path lower = ovl_lowerstack(poe)[i];
 
+		if (!ovl_check_nextredirect(&d)) {
+			err = -EPERM;
+			goto out_put;
+		}
+
 		if (!ovl_redirect_follow(ofs))
 			d.last = i == ovl_numlower(poe) - 1;
 		else if (d.is_dir || !ofs->numdatalayer)
@@ -1126,12 +1165,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		if (!this)
 			continue;
 
-		if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) {
-			dput(this);
-			err = -EPERM;
-			pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
-			goto out_put;
-		}
+		if (d.metacopy)
+			d.nextmetacopy = true;
 
 		/*
 		 * If no origin fh is stored in upper of a merge dir, store fh
@@ -1185,22 +1220,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			ctr++;
 		}
 
-		/*
-		 * Following redirects can have security consequences: it's like
-		 * a symlink into the lower layer without the permission checks.
-		 * This is only a problem if the upper layer is untrusted (e.g
-		 * comes from an USB drive).  This can allow a non-readable file
-		 * or directory to become readable.
-		 *
-		 * Only following redirects when redirects are enabled disables
-		 * this attack vector when not necessary.
-		 */
-		err = -EPERM;
-		if (d.redirect && !ovl_redirect_follow(ofs)) {
-			pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n",
-					    dentry);
-			goto out_put;
-		}
+		if (d.redirect)
+			d.nextredirect = true;
 
 		if (d.stop)
 			break;
@@ -1218,6 +1239,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		ctr++;
 	}
 
+	if (!ovl_check_nextredirect(&d)) {
+		err = -EPERM;
+		goto out_put;
+	}
+
 	/*
 	 * For regular non-metacopy upper dentries, there is no lower
 	 * path based lookup, hence ctr will be zero. If a dentry is found
@@ -1307,11 +1333,18 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			upperredirect = NULL;
 			goto out_free_oe;
 		}
+		d.nextredirect = upperredirect;
+
 		err = ovl_check_metacopy_xattr(ofs, &upperpath, NULL);
 		if (err < 0)
 			goto out_free_oe;
-		uppermetacopy = err;
+		d.nextmetacopy = uppermetacopy = err;
 		metacopy_size = err;
+
+		if (!ovl_check_nextredirect(&d)) {
+			err = -EPERM;
+			goto out_free_oe;
+		}
 	}
 
 	if (upperdentry || ctr) {
-- 
2.49.0

[PATCH v3 2/3] ovl: relax redirect/metacopy requirements for lower -> data redirect

[Miklos Szeredi]

Allow the special case of a redirect from a lower layer to a data layer
without having to turn on metacopy.  This makes the feature work with
userxattr, which in turn allows data layers to be usable in user
namespaces.

Minimize the risk by only enabling redirect from a single lower layer to a
data layer iff a data layer is specified.  The only way to access a data
layer is to enable this, so there's really no reason not to enable this.

This can be used safely if the lower layer is read-only and the
user.overlay.redirect xattr cannot be modified.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 Documentation/filesystems/overlayfs.rst |  7 +++++++
 fs/overlayfs/namei.c                    | 14 ++++++++------
 fs/overlayfs/params.c                   |  5 -----
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index 2db379b4b31e..4133a336486d 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -443,6 +443,13 @@ Only the data of the files in the "data-only" lower layers may be visible
 when a "metacopy" file in one of the lower layers above it, has a "redirect"
 to the absolute path of the "lower data" file in the "data-only" lower layer.
 
+Instead of explicitly enabling "metacopy=on" it is sufficient to specify at
+least one data-only layer to enable redirection of data to a data-only layer.
+In this case other forms of metacopy are rejected.  Note: this way data-only
+layers may be used toghether with "userxattr", in which case careful attention
+must be given to privileges needed to change the "user.overlay.redirect" xattr
+to prevent misuse.
+
 Since kernel version v6.8, "data-only" lower layers can also be added using
 the "datadir+" mount options and the fsconfig syscall from new mount api.
 For example::
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 5cebdd05ab3a..3d99e5fe5cfc 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1068,6 +1068,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	struct inode *inode = NULL;
 	bool upperopaque = false;
 	char *upperredirect = NULL;
+	bool check_redirect = (ovl_redirect_follow(ofs) || ofs->numdatalayer);
 	struct dentry *this;
 	unsigned int i;
 	int err;
@@ -1080,7 +1081,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		.is_dir = false,
 		.opaque = false,
 		.stop = false,
-		.last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
+		.last = check_redirect ? false : !ovl_numlower(poe),
 		.redirect = NULL,
 		.metacopy = 0,
 		.nextredirect = false,
@@ -1152,7 +1153,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			goto out_put;
 		}
 
-		if (!ovl_redirect_follow(ofs))
+		if (!check_redirect)
 			d.last = i == ovl_numlower(poe) - 1;
 		else if (d.is_dir || !ofs->numdatalayer)
 			d.last = lower.layer->idx == ovl_numlower(roe);
@@ -1233,13 +1234,14 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 	}
 
-	/* Defer lookup of lowerdata in data-only layers to first access */
+	/*
+	 * Defer lookup of lowerdata in data-only layers to first access.
+	 * Don't require redirect=follow and metacopy=on in this case.
+	 */
 	if (d.metacopy && ctr && ofs->numdatalayer && d.absolute_redirect) {
 		d.metacopy = 0;
 		ctr++;
-	}
-
-	if (!ovl_check_nextredirect(&d)) {
+	} else if (!ovl_check_nextredirect(&d)) {
 		err = -EPERM;
 		goto out_put;
 	}
diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index 6759f7d040c8..2468b436bb13 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -1025,11 +1025,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
 		 */
 	}
 
-	if (ctx->nr_data > 0 && !config->metacopy) {
-		pr_err("lower data-only dirs require metacopy support.\n");
-		return -EINVAL;
-	}
-
 	return 0;
 }
 
-- 
2.49.0

[PATCH v3 3/3] ovl: don't require "metacopy=on" for "verity"

[Miklos Szeredi]

This allows the "verity" mount option to be used with "userxattr" data-only
layer(s).

Also it allows dropping the "metacopy=on" option when the "datadir+" option
is to be used.  This cleanly separates the two features that have been
lumped together under "metacopy=on":

 - data-redirect: data access is redirected to the data-only layer

 - meta-copy: copy up metadata only if possible

Previous patches made sure that with "userxattr" metacopy only works in the
lower -> data scenario.

In this scenario the lower (metadata) layer must be secured against
tampering, in which case the verity checksums contained in this layer can
ensure integrity of data even in the case of an untrusted data layer.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/params.c | 26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index 2468b436bb13..e297681ecac7 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -871,18 +871,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
 		config->uuid = OVL_UUID_NULL;
 	}
 
-	/* Resolve verity -> metacopy dependency */
-	if (config->verity_mode && !config->metacopy) {
-		/* Don't allow explicit specified conflicting combinations */
-		if (set.metacopy) {
-			pr_err("conflicting options: metacopy=off,verity=%s\n",
-			       ovl_verity_mode(config));
-			return -EINVAL;
-		}
-		/* Otherwise automatically enable metacopy. */
-		config->metacopy = true;
-	}
-
 	/*
 	 * This is to make the logic below simpler.  It doesn't make any other
 	 * difference, since redirect_dir=on is only used for upper.
@@ -890,18 +878,13 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
 	if (!config->upperdir && config->redirect_mode == OVL_REDIRECT_FOLLOW)
 		config->redirect_mode = OVL_REDIRECT_ON;
 
-	/* Resolve verity -> metacopy -> redirect_dir dependency */
+	/* metacopy -> redirect_dir dependency */
 	if (config->metacopy && config->redirect_mode != OVL_REDIRECT_ON) {
 		if (set.metacopy && set.redirect) {
 			pr_err("conflicting options: metacopy=on,redirect_dir=%s\n",
 			       ovl_redirect_mode(config));
 			return -EINVAL;
 		}
-		if (config->verity_mode && set.redirect) {
-			pr_err("conflicting options: verity=%s,redirect_dir=%s\n",
-			       ovl_verity_mode(config), ovl_redirect_mode(config));
-			return -EINVAL;
-		}
 		if (set.redirect) {
 			/*
 			 * There was an explicit redirect_dir=... that resulted
@@ -970,7 +953,7 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
 	}
 
 
-	/* Resolve userxattr -> !redirect && !metacopy && !verity dependency */
+	/* Resolve userxattr -> !redirect && !metacopy dependency */
 	if (config->userxattr) {
 		if (set.redirect &&
 		    config->redirect_mode != OVL_REDIRECT_NOFOLLOW) {
@@ -982,11 +965,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
 			pr_err("conflicting options: userxattr,metacopy=on\n");
 			return -EINVAL;
 		}
-		if (config->verity_mode) {
-			pr_err("conflicting options: userxattr,verity=%s\n",
-			       ovl_verity_mode(config));
-			return -EINVAL;
-		}
 		/*
 		 * Silently disable default setting of redirect and metacopy.
 		 * This shall be the default in the future as well: these
-- 
2.49.0

Re: [PATCH v3 1/3] ovl: make redirect/metacopy rejection consistent

[Amir Goldstein]

On Tue, Apr 8, 2025 at 5:40 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> When overlayfs finds a file with metacopy and/or redirect attributes and
> the metacopy and/or redirect features are not enabled, then it refuses to
> act on those attributes while also issuing a warning.
>
> There was an inconsistency in not checking metacopy found from the index.
>
> And also only warning on an upper metacopy if it found the next file on the
> lower layer, while always warning for metacopy found on a lower layer.
>
> Fix these inconsistencies and make the logic more straightforward, paving
> the way for following patches to change when dataredirects are allowed.

missing space: dataredirects

>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/overlayfs/namei.c | 81 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 57 insertions(+), 24 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index be5c65d6f848..5cebdd05ab3a 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -16,6 +16,7 @@
>
>  struct ovl_lookup_data {
>         struct super_block *sb;
> +       struct dentry *dentry;
>         const struct ovl_layer *layer;
>         struct qstr name;
>         bool is_dir;
> @@ -23,6 +24,8 @@ struct ovl_lookup_data {
>         bool xwhiteouts;
>         bool stop;
>         bool last;
> +       bool nextredirect;
> +       bool nextmetacopy;

I think these are not needed

>         char *redirect;
>         int metacopy;
>         /* Referring to last redirect xattr */
> @@ -1024,6 +1027,31 @@ int ovl_verify_lowerdata(struct dentry *dentry)
>         return ovl_maybe_validate_verity(dentry);
>  }
>
> +/*
> + * Following redirects/metacopy can have security consequences: it's like a
> + * symlink into the lower layer without the permission checks.
> + *
> + * This is only a problem if the upper layer is untrusted (e.g comes from an USB
> + * drive).  This can allow a non-readable file or directory to become readable.
> + *
> + * Only following redirects when redirects are enabled disables this attack
> + * vector when not necessary.
> + */
> +static bool ovl_check_nextredirect(struct ovl_lookup_data *d)

Looks much better with the helper.
May I suggest ovl_check_follow_redirect()

> +{
> +       struct ovl_fs *ofs = OVL_FS(d->sb);
> +
> +       if (d->nextmetacopy && !ofs->config.metacopy) {

Should be equivalent to
       if (d->metacopy && !ofs->config.metacopy) {

In current code

> +               pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", d->dentry);
> +               return false;
> +       }
> +       if (d->nextredirect && !ovl_redirect_follow(ofs)) {

Should be equivalent to
       if (d->redirect && !ovl_redirect_follow(ofs)) {

With minor changes to index lookup code


> +               pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", d->dentry);
> +               return false;
> +       }
> +       return true;
> +}
> +
>  struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                           unsigned int flags)
>  {
> @@ -1047,6 +1075,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         int metacopy_size = 0;
>         struct ovl_lookup_data d = {
>                 .sb = dentry->d_sb,
> +               .dentry = dentry,
>                 .name = dentry->d_name,
>                 .is_dir = false,
>                 .opaque = false,
> @@ -1054,6 +1083,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 .last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
>                 .redirect = NULL,
>                 .metacopy = 0,
> +               .nextredirect = false,
> +               .nextmetacopy = false,
>         };
>
>         if (dentry->d_name.len > ofs->namelen)
> @@ -1087,8 +1118,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         if (err)
>                                 goto out_put_upper;
>
> -                       if (d.metacopy)
> +                       if (d.metacopy) {
>                                 uppermetacopy = true;
> +                               d.nextmetacopy = true;

always set IFF (d.metacopy)

> +                       }
>                         metacopy_size = d.metacopy;
>                 }
>
> @@ -1099,6 +1132,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                                 goto out_put_upper;
>                         if (d.redirect[0] == '/')
>                                 poe = roe;
> +                       d.nextredirect = true;

mostly set IFF (d.redirect)

>                 }
>                 upperopaque = d.opaque;
>         }
> @@ -1113,6 +1147,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         for (i = 0; !d.stop && i < ovl_numlower(poe); i++) {
>                 struct ovl_path lower = ovl_lowerstack(poe)[i];
>
> +               if (!ovl_check_nextredirect(&d)) {
> +                       err = -EPERM;
> +                       goto out_put;
> +               }
> +
>                 if (!ovl_redirect_follow(ofs))
>                         d.last = i == ovl_numlower(poe) - 1;
>                 else if (d.is_dir || !ofs->numdatalayer)
> @@ -1126,12 +1165,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 if (!this)
>                         continue;
>
> -               if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) {
> -                       dput(this);
> -                       err = -EPERM;
> -                       pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
> -                       goto out_put;
> -               }
> +               if (d.metacopy)
> +                       d.nextmetacopy = true;
>
>                 /*
>                  * If no origin fh is stored in upper of a merge dir, store fh
> @@ -1185,22 +1220,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         ctr++;
>                 }
>
> -               /*
> -                * Following redirects can have security consequences: it's like
> -                * a symlink into the lower layer without the permission checks.
> -                * This is only a problem if the upper layer is untrusted (e.g
> -                * comes from an USB drive).  This can allow a non-readable file
> -                * or directory to become readable.
> -                *
> -                * Only following redirects when redirects are enabled disables
> -                * this attack vector when not necessary.
> -                */
> -               err = -EPERM;
> -               if (d.redirect && !ovl_redirect_follow(ofs)) {
> -                       pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n",
> -                                           dentry);
> -                       goto out_put;
> -               }
> +               if (d.redirect)
> +                       d.nextredirect = true;
>
>                 if (d.stop)
>                         break;
> @@ -1218,6 +1239,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 ctr++;
>         }
>
> +       if (!ovl_check_nextredirect(&d)) {
> +               err = -EPERM;
> +               goto out_put;
> +       }
> +
>         /*
>          * For regular non-metacopy upper dentries, there is no lower
>          * path based lookup, hence ctr will be zero. If a dentry is found
> @@ -1307,11 +1333,18 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         upperredirect = NULL;
>                         goto out_free_oe;
>                 }
> +               d.nextredirect = upperredirect;
> +
>                 err = ovl_check_metacopy_xattr(ofs, &upperpath, NULL);
>                 if (err < 0)
>                         goto out_free_oe;
> -               uppermetacopy = err;
> +               d.nextmetacopy = uppermetacopy = err;

Could be changed to:
+               d.metacopy = uppermetacopy = err;


>                 metacopy_size = err;
> +
> +               if (!ovl_check_nextredirect(&d)) {
> +                       err = -EPERM;
> +                       goto out_free_oe;
> +               }
>         }
>


We never really follow redirect from index
All upperredirect is ever used for is to suppress ovl_set_redirect()
after copy up of another lower hardlink and rename,
but also in that case, upperredirect is not going to be followed
(or trusted for that matter) until a new lookup of the copied up
alias and at that point  ovl_check_follow_redirect() will be
called when upperdentry is found.

I think we do not need to check follow of redirect from index
I think it is enough to check follow of metacopy from index.

Therefore, I think there d.nextmetacopy and d.nextredirect are
completely implied from d.metacopy and d.redirect.

Thanks,
Amir.

Re: [PATCH v3 2/3] ovl: relax redirect/metacopy requirements for lower -> data redirect

[Amir Goldstein]

On Tue, Apr 8, 2025 at 5:40 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> Allow the special case of a redirect from a lower layer to a data layer
> without having to turn on metacopy.  This makes the feature work with
> userxattr, which in turn allows data layers to be usable in user
> namespaces.
>
> Minimize the risk by only enabling redirect from a single lower layer to a
> data layer iff a data layer is specified.  The only way to access a data
> layer is to enable this, so there's really no reason not to enable this.
>
> This can be used safely if the lower layer is read-only and the
> user.overlay.redirect xattr cannot be modified.
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  Documentation/filesystems/overlayfs.rst |  7 +++++++
>  fs/overlayfs/namei.c                    | 14 ++++++++------
>  fs/overlayfs/params.c                   |  5 -----
>  3 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index 2db379b4b31e..4133a336486d 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -443,6 +443,13 @@ Only the data of the files in the "data-only" lower layers may be visible
>  when a "metacopy" file in one of the lower layers above it, has a "redirect"
>  to the absolute path of the "lower data" file in the "data-only" lower layer.
>
> +Instead of explicitly enabling "metacopy=on" it is sufficient to specify at
> +least one data-only layer to enable redirection of data to a data-only layer.
> +In this case other forms of metacopy are rejected.  Note: this way data-only
> +layers may be used toghether with "userxattr", in which case careful attention
> +must be given to privileges needed to change the "user.overlay.redirect" xattr
> +to prevent misuse.
> +
>  Since kernel version v6.8, "data-only" lower layers can also be added using
>  the "datadir+" mount options and the fsconfig syscall from new mount api.
>  For example::
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 5cebdd05ab3a..3d99e5fe5cfc 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1068,6 +1068,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         struct inode *inode = NULL;
>         bool upperopaque = false;
>         char *upperredirect = NULL;
> +       bool check_redirect = (ovl_redirect_follow(ofs) || ofs->numdatalayer);
>         struct dentry *this;
>         unsigned int i;
>         int err;
> @@ -1080,7 +1081,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 .is_dir = false,
>                 .opaque = false,
>                 .stop = false,
> -               .last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
> +               .last = check_redirect ? false : !ovl_numlower(poe),
>                 .redirect = NULL,
>                 .metacopy = 0,
>                 .nextredirect = false,
> @@ -1152,7 +1153,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         goto out_put;
>                 }
>
> -               if (!ovl_redirect_follow(ofs))
> +               if (!check_redirect)
>                         d.last = i == ovl_numlower(poe) - 1;
>                 else if (d.is_dir || !ofs->numdatalayer)
>                         d.last = lower.layer->idx == ovl_numlower(roe);
> @@ -1233,13 +1234,14 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 }
>         }
>
> -       /* Defer lookup of lowerdata in data-only layers to first access */
> +       /*
> +        * Defer lookup of lowerdata in data-only layers to first access.
> +        * Don't require redirect=follow and metacopy=on in this case.
> +        */
>         if (d.metacopy && ctr && ofs->numdatalayer && d.absolute_redirect) {
>                 d.metacopy = 0;
>                 ctr++;
> -       }
> -
> -       if (!ovl_check_nextredirect(&d)) {
> +       } else if (!ovl_check_nextredirect(&d)) {
>                 err = -EPERM;
>                 goto out_put;
>         }
> diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> index 6759f7d040c8..2468b436bb13 100644
> --- a/fs/overlayfs/params.c
> +++ b/fs/overlayfs/params.c
> @@ -1025,11 +1025,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
>                  */
>         }
>
> -       if (ctx->nr_data > 0 && !config->metacopy) {
> -               pr_err("lower data-only dirs require metacopy support.\n");
> -               return -EINVAL;
> -       }
> -
>         return 0;
>  }
>
> --
> 2.49.0
>

Re: [PATCH v3 3/3] ovl: don't require "metacopy=on" for "verity"

[Amir Goldstein]

On Tue, Apr 8, 2025 at 5:40 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> This allows the "verity" mount option to be used with "userxattr" data-only
> layer(s).
>
> Also it allows dropping the "metacopy=on" option when the "datadir+" option
> is to be used.  This cleanly separates the two features that have been
> lumped together under "metacopy=on":
>
>  - data-redirect: data access is redirected to the data-only layer
>
>  - meta-copy: copy up metadata only if possible
>
> Previous patches made sure that with "userxattr" metacopy only works in the
> lower -> data scenario.
>
> In this scenario the lower (metadata) layer must be secured against
> tampering, in which case the verity checksums contained in this layer can
> ensure integrity of data even in the case of an untrusted data layer.
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>


> ---
>  fs/overlayfs/params.c | 26 ++------------------------
>  1 file changed, 2 insertions(+), 24 deletions(-)
>
> diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> index 2468b436bb13..e297681ecac7 100644
> --- a/fs/overlayfs/params.c
> +++ b/fs/overlayfs/params.c
> @@ -871,18 +871,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
>                 config->uuid = OVL_UUID_NULL;
>         }
>
> -       /* Resolve verity -> metacopy dependency */
> -       if (config->verity_mode && !config->metacopy) {
> -               /* Don't allow explicit specified conflicting combinations */
> -               if (set.metacopy) {
> -                       pr_err("conflicting options: metacopy=off,verity=%s\n",
> -                              ovl_verity_mode(config));
> -                       return -EINVAL;
> -               }
> -               /* Otherwise automatically enable metacopy. */
> -               config->metacopy = true;
> -       }
> -
>         /*
>          * This is to make the logic below simpler.  It doesn't make any other
>          * difference, since redirect_dir=on is only used for upper.
> @@ -890,18 +878,13 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
>         if (!config->upperdir && config->redirect_mode == OVL_REDIRECT_FOLLOW)
>                 config->redirect_mode = OVL_REDIRECT_ON;
>
> -       /* Resolve verity -> metacopy -> redirect_dir dependency */
> +       /* metacopy -> redirect_dir dependency */
>         if (config->metacopy && config->redirect_mode != OVL_REDIRECT_ON) {
>                 if (set.metacopy && set.redirect) {
>                         pr_err("conflicting options: metacopy=on,redirect_dir=%s\n",
>                                ovl_redirect_mode(config));
>                         return -EINVAL;
>                 }
> -               if (config->verity_mode && set.redirect) {
> -                       pr_err("conflicting options: verity=%s,redirect_dir=%s\n",
> -                              ovl_verity_mode(config), ovl_redirect_mode(config));
> -                       return -EINVAL;
> -               }
>                 if (set.redirect) {
>                         /*
>                          * There was an explicit redirect_dir=... that resulted
> @@ -970,7 +953,7 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
>         }
>
>
> -       /* Resolve userxattr -> !redirect && !metacopy && !verity dependency */
> +       /* Resolve userxattr -> !redirect && !metacopy dependency */
>         if (config->userxattr) {
>                 if (set.redirect &&
>                     config->redirect_mode != OVL_REDIRECT_NOFOLLOW) {
> @@ -982,11 +965,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
>                         pr_err("conflicting options: userxattr,metacopy=on\n");
>                         return -EINVAL;
>                 }
> -               if (config->verity_mode) {
> -                       pr_err("conflicting options: userxattr,verity=%s\n",
> -                              ovl_verity_mode(config));
> -                       return -EINVAL;
> -               }
>                 /*
>                  * Silently disable default setting of redirect and metacopy.
>                  * This shall be the default in the future as well: these
> --
> 2.49.0
>

Re: [PATCH v3 1/3] ovl: make redirect/metacopy rejection consistent

[Amir Goldstein]

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

On Wed, Apr 9, 2025 at 8:09 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Apr 8, 2025 at 5:40 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
> >
> > When overlayfs finds a file with metacopy and/or redirect attributes and
> > the metacopy and/or redirect features are not enabled, then it refuses to
> > act on those attributes while also issuing a warning.
> >
> > There was an inconsistency in not checking metacopy found from the index.
> >
> > And also only warning on an upper metacopy if it found the next file on the
> > lower layer, while always warning for metacopy found on a lower layer.
> >
> > Fix these inconsistencies and make the logic more straightforward, paving
> > the way for following patches to change when dataredirects are allowed.
>
> missing space: dataredirects
>
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> >  fs/overlayfs/namei.c | 81 +++++++++++++++++++++++++++++++-------------
> >  1 file changed, 57 insertions(+), 24 deletions(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index be5c65d6f848..5cebdd05ab3a 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -16,6 +16,7 @@
> >
> >  struct ovl_lookup_data {
> >         struct super_block *sb;
> > +       struct dentry *dentry;
> >         const struct ovl_layer *layer;
> >         struct qstr name;
> >         bool is_dir;
> > @@ -23,6 +24,8 @@ struct ovl_lookup_data {
> >         bool xwhiteouts;
> >         bool stop;
> >         bool last;
> > +       bool nextredirect;
> > +       bool nextmetacopy;
>
> I think these are not needed
>
> >         char *redirect;
> >         int metacopy;
> >         /* Referring to last redirect xattr */
> > @@ -1024,6 +1027,31 @@ int ovl_verify_lowerdata(struct dentry *dentry)
> >         return ovl_maybe_validate_verity(dentry);
> >  }
> >
> > +/*
> > + * Following redirects/metacopy can have security consequences: it's like a
> > + * symlink into the lower layer without the permission checks.
> > + *
> > + * This is only a problem if the upper layer is untrusted (e.g comes from an USB
> > + * drive).  This can allow a non-readable file or directory to become readable.
> > + *
> > + * Only following redirects when redirects are enabled disables this attack
> > + * vector when not necessary.
> > + */
> > +static bool ovl_check_nextredirect(struct ovl_lookup_data *d)
>
> Looks much better with the helper.
> May I suggest ovl_check_follow_redirect()
>
> > +{
> > +       struct ovl_fs *ofs = OVL_FS(d->sb);
> > +
> > +       if (d->nextmetacopy && !ofs->config.metacopy) {
>
> Should be equivalent to
>        if (d->metacopy && !ofs->config.metacopy) {
>
> In current code
>
> > +               pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", d->dentry);
> > +               return false;
> > +       }
> > +       if (d->nextredirect && !ovl_redirect_follow(ofs)) {
>
> Should be equivalent to
>        if (d->redirect && !ovl_redirect_follow(ofs)) {
>
> With minor changes to index lookup code
>
>
> > +               pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", d->dentry);
> > +               return false;
> > +       }
> > +       return true;
> > +}
> > +
> >  struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                           unsigned int flags)
> >  {
> > @@ -1047,6 +1075,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >         int metacopy_size = 0;
> >         struct ovl_lookup_data d = {
> >                 .sb = dentry->d_sb,
> > +               .dentry = dentry,
> >                 .name = dentry->d_name,
> >                 .is_dir = false,
> >                 .opaque = false,
> > @@ -1054,6 +1083,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                 .last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
> >                 .redirect = NULL,
> >                 .metacopy = 0,
> > +               .nextredirect = false,
> > +               .nextmetacopy = false,
> >         };
> >
> >         if (dentry->d_name.len > ofs->namelen)
> > @@ -1087,8 +1118,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                         if (err)
> >                                 goto out_put_upper;
> >
> > -                       if (d.metacopy)
> > +                       if (d.metacopy) {
> >                                 uppermetacopy = true;
> > +                               d.nextmetacopy = true;
>
> always set IFF (d.metacopy)
>
> > +                       }
> >                         metacopy_size = d.metacopy;
> >                 }
> >
> > @@ -1099,6 +1132,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                                 goto out_put_upper;
> >                         if (d.redirect[0] == '/')
> >                                 poe = roe;
> > +                       d.nextredirect = true;
>
> mostly set IFF (d.redirect)
>
> >                 }
> >                 upperopaque = d.opaque;
> >         }
> > @@ -1113,6 +1147,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >         for (i = 0; !d.stop && i < ovl_numlower(poe); i++) {
> >                 struct ovl_path lower = ovl_lowerstack(poe)[i];
> >
> > +               if (!ovl_check_nextredirect(&d)) {
> > +                       err = -EPERM;
> > +                       goto out_put;
> > +               }
> > +
> >                 if (!ovl_redirect_follow(ofs))
> >                         d.last = i == ovl_numlower(poe) - 1;
> >                 else if (d.is_dir || !ofs->numdatalayer)
> > @@ -1126,12 +1165,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                 if (!this)
> >                         continue;
> >
> > -               if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) {
> > -                       dput(this);
> > -                       err = -EPERM;
> > -                       pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
> > -                       goto out_put;
> > -               }
> > +               if (d.metacopy)
> > +                       d.nextmetacopy = true;
> >
> >                 /*
> >                  * If no origin fh is stored in upper of a merge dir, store fh
> > @@ -1185,22 +1220,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                         ctr++;
> >                 }
> >
> > -               /*
> > -                * Following redirects can have security consequences: it's like
> > -                * a symlink into the lower layer without the permission checks.
> > -                * This is only a problem if the upper layer is untrusted (e.g
> > -                * comes from an USB drive).  This can allow a non-readable file
> > -                * or directory to become readable.
> > -                *
> > -                * Only following redirects when redirects are enabled disables
> > -                * this attack vector when not necessary.
> > -                */
> > -               err = -EPERM;
> > -               if (d.redirect && !ovl_redirect_follow(ofs)) {
> > -                       pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n",
> > -                                           dentry);
> > -                       goto out_put;
> > -               }
> > +               if (d.redirect)
> > +                       d.nextredirect = true;
> >
> >                 if (d.stop)
> >                         break;
> > @@ -1218,6 +1239,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                 ctr++;
> >         }
> >
> > +       if (!ovl_check_nextredirect(&d)) {
> > +               err = -EPERM;
> > +               goto out_put;
> > +       }
> > +
> >         /*
> >          * For regular non-metacopy upper dentries, there is no lower
> >          * path based lookup, hence ctr will be zero. If a dentry is found
> > @@ -1307,11 +1333,18 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                         upperredirect = NULL;
> >                         goto out_free_oe;
> >                 }
> > +               d.nextredirect = upperredirect;
> > +
> >                 err = ovl_check_metacopy_xattr(ofs, &upperpath, NULL);
> >                 if (err < 0)
> >                         goto out_free_oe;
> > -               uppermetacopy = err;
> > +               d.nextmetacopy = uppermetacopy = err;
>
> Could be changed to:
> +               d.metacopy = uppermetacopy = err;
>
>
> >                 metacopy_size = err;
> > +
> > +               if (!ovl_check_nextredirect(&d)) {
> > +                       err = -EPERM;
> > +                       goto out_free_oe;
> > +               }
> >         }
> >
>
>
> We never really follow redirect from index
> All upperredirect is ever used for is to suppress ovl_set_redirect()
> after copy up of another lower hardlink and rename,
> but also in that case, upperredirect is not going to be followed
> (or trusted for that matter) until a new lookup of the copied up
> alias and at that point  ovl_check_follow_redirect() will be
> called when upperdentry is found.
>
> I think we do not need to check follow of redirect from index
> I think it is enough to check follow of metacopy from index.
>
> Therefore, I think there d.nextmetacopy and d.nextredirect are
> completely implied from d.metacopy and d.redirect.

On second thought, if unpriv user suppresses ovl_set_redirect()
by setting some mock redirect value on index maybe that lead to some
risk. Not worth overthinking about it.

Attached patch removed next* variables without this compromise.

Tested it squashed to patch 1 and minor rebase conflicts fixes in patch 2.
It passed your tests.

Thanks,
Amir.

[-- Attachment #2: ovl-remove-unneeded-nextredirect-nextmetacopy.patch --]
[-- Type: text/x-patch, Size: 5266 bytes --]

From 2305730bc1c4ee35fcf0b88305e4add828bdff0b Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Wed, 9 Apr 2025 09:02:43 +0200
Subject: [PATCH] ovl: remove unneeded nextredirect/nextmetacopy

---
 fs/overlayfs/namei.c | 48 +++++++++++++++++---------------------------
 1 file changed, 18 insertions(+), 30 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 5cebdd05ab3a..4fc2c47ebc55 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -24,9 +24,8 @@ struct ovl_lookup_data {
 	bool xwhiteouts;
 	bool stop;
 	bool last;
-	bool nextredirect;
-	bool nextmetacopy;
 	char *redirect;
+	char *upperredirect;
 	int metacopy;
 	/* Referring to last redirect xattr */
 	bool absolute_redirect;
@@ -1037,15 +1036,15 @@ int ovl_verify_lowerdata(struct dentry *dentry)
  * Only following redirects when redirects are enabled disables this attack
  * vector when not necessary.
  */
-static bool ovl_check_nextredirect(struct ovl_lookup_data *d)
+static bool ovl_check_follow_redirect(struct ovl_lookup_data *d)
 {
 	struct ovl_fs *ofs = OVL_FS(d->sb);
 
-	if (d->nextmetacopy && !ofs->config.metacopy) {
+	if (d->metacopy && !ofs->config.metacopy) {
 		pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", d->dentry);
 		return false;
 	}
-	if (d->nextredirect && !ovl_redirect_follow(ofs)) {
+	if ((d->redirect || d->upperredirect) && !ovl_redirect_follow(ofs)) {
 		pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", d->dentry);
 		return false;
 	}
@@ -1067,7 +1066,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	unsigned int ctr = 0;
 	struct inode *inode = NULL;
 	bool upperopaque = false;
-	char *upperredirect = NULL;
 	struct dentry *this;
 	unsigned int i;
 	int err;
@@ -1082,9 +1080,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		.stop = false,
 		.last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
 		.redirect = NULL,
+		.upperredirect = NULL,
 		.metacopy = 0,
-		.nextredirect = false,
-		.nextmetacopy = false,
 	};
 
 	if (dentry->d_name.len > ofs->namelen)
@@ -1120,19 +1117,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 			if (d.metacopy) {
 				uppermetacopy = true;
-				d.nextmetacopy = true;
 			}
 			metacopy_size = d.metacopy;
 		}
 
 		if (d.redirect) {
 			err = -ENOMEM;
-			upperredirect = kstrdup(d.redirect, GFP_KERNEL);
-			if (!upperredirect)
+			d.upperredirect = kstrdup(d.redirect, GFP_KERNEL);
+			if (!d.upperredirect)
 				goto out_put_upper;
 			if (d.redirect[0] == '/')
 				poe = roe;
-			d.nextredirect = true;
 		}
 		upperopaque = d.opaque;
 	}
@@ -1147,7 +1142,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	for (i = 0; !d.stop && i < ovl_numlower(poe); i++) {
 		struct ovl_path lower = ovl_lowerstack(poe)[i];
 
-		if (!ovl_check_nextredirect(&d)) {
+		if (!ovl_check_follow_redirect(&d)) {
 			err = -EPERM;
 			goto out_put;
 		}
@@ -1165,9 +1160,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		if (!this)
 			continue;
 
-		if (d.metacopy)
-			d.nextmetacopy = true;
-
 		/*
 		 * If no origin fh is stored in upper of a merge dir, store fh
 		 * of lower dir and set upper parent "impure".
@@ -1220,9 +1212,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			ctr++;
 		}
 
-		if (d.redirect)
-			d.nextredirect = true;
-
 		if (d.stop)
 			break;
 
@@ -1239,7 +1228,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		ctr++;
 	}
 
-	if (!ovl_check_nextredirect(&d)) {
+	if (!ovl_check_follow_redirect(&d)) {
 		err = -EPERM;
 		goto out_put;
 	}
@@ -1324,24 +1313,23 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 		/*
 		 * It's safe to assign upperredirect here: the previous
-		 * assignment of happens only if upperdentry is non-NULL, and
+		 * assignment happens only if upperdentry is non-NULL, and
 		 * this one only if upperdentry is NULL.
 		 */
-		upperredirect = ovl_get_redirect_xattr(ofs, &upperpath, 0);
-		if (IS_ERR(upperredirect)) {
-			err = PTR_ERR(upperredirect);
-			upperredirect = NULL;
+		d.upperredirect = ovl_get_redirect_xattr(ofs, &upperpath, 0);
+		if (IS_ERR(d.upperredirect)) {
+			err = PTR_ERR(d.upperredirect);
+			d.upperredirect = NULL;
 			goto out_free_oe;
 		}
-		d.nextredirect = upperredirect;
 
 		err = ovl_check_metacopy_xattr(ofs, &upperpath, NULL);
 		if (err < 0)
 			goto out_free_oe;
-		d.nextmetacopy = uppermetacopy = err;
+		d.metacopy = uppermetacopy = err;
 		metacopy_size = err;
 
-		if (!ovl_check_nextredirect(&d)) {
+		if (!ovl_check_follow_redirect(&d)) {
 			err = -EPERM;
 			goto out_free_oe;
 		}
@@ -1352,7 +1340,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			.upperdentry = upperdentry,
 			.oe = oe,
 			.index = index,
-			.redirect = upperredirect,
+			.redirect = d.upperredirect,
 		};
 
 		/* Store lowerdata redirect for lazy lookup */
@@ -1394,7 +1382,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		kfree(origin_path);
 	}
 	dput(upperdentry);
-	kfree(upperredirect);
+	kfree(d.upperredirect);
 out:
 	kfree(d.redirect);
 	ovl_revert_creds(old_cred);
-- 
2.34.1

Re: [PATCH v3 1/3] ovl: make redirect/metacopy rejection consistent

[Miklos Szeredi]

On Wed, 9 Apr 2025 at 10:25, Amir Goldstein <amir73il@gmail.com> wrote:

> On second thought, if unpriv user suppresses ovl_set_redirect()
> by setting some mock redirect value on index maybe that lead to some
> risk. Not worth overthinking about it.
>
> Attached patch removed next* variables without this compromise.
>
> Tested it squashed to patch 1 and minor rebase conflicts fixes in patch 2.
> It passed your tests.

Thanks.

One more change:  in this patch we just want the consistency fix, not
the behavior change introduced in 2/3.  So move the
ovl_check_follow_redirect() to before the lazy-data check here and
restore the order in the next patch.

Pushed to overlayfs/vfs.git#overlayfs-next

Thanks,
Miklos

Re: [PATCH v3 1/3] ovl: make redirect/metacopy rejection consistent

[Amir Goldstein]

On Wed, Apr 9, 2025 at 1:12 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 9 Apr 2025 at 10:25, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > On second thought, if unpriv user suppresses ovl_set_redirect()
> > by setting some mock redirect value on index maybe that lead to some
> > risk. Not worth overthinking about it.
> >
> > Attached patch removed next* variables without this compromise.
> >
> > Tested it squashed to patch 1 and minor rebase conflicts fixes in patch 2.
> > It passed your tests.
>
> Thanks.
>
> One more change:  in this patch we just want the consistency fix, not
> the behavior change introduced in 2/3.  So move the
> ovl_check_follow_redirect() to before the lazy-data check here and
> restore the order in the next patch.

Right.

>
> Pushed to overlayfs/vfs.git#overlayfs-next

Looks good.

Thanks,
Amir.

FYI, I am going to be on vacation 6.15-rc3..6.15-rc5.