[PATCH v2] ovl: check for incomapt features in work dir

[PATCH v2] ovl: check for incomapt features in work dir

[Amir Goldstein]

An incompatible feature is marked by a non-empty directory nested
2 levels deep under "work" dir, e.g.:
workdir/work/incompat/volatile.

This commit checks for marked incompat features, warns about them
and fails to mount the overlay, for example:
  overlayfs: overlay with incompat feature 'volatile' cannot be mounted

Very old kernels (i.e. v3.18) will fail to remove a non-empty "work"
dir and fail the mount.  Newer kernels will fail to remove a "work"
dir with entries nested 3 levels and fall back to read-only mount.

User mounting with old kernel will see a warning like these in dmesg:
  overlayfs: cleanup of 'incompat/...' failed (-39)
  overlayfs: cleanup of 'work/incompat' failed (-39)
  overlayfs: cleanup of 'ovl-work/work' failed (-39)
  overlayfs: failed to create directory /vdf/ovl-work/work (errno: 17);
             mounting read-only

These warnings should give the hint to the user that:
1. mount failure is caused by backward incompatible features
2. mount failure can be resolved by manually removing the "work" directory

There is nothing preventing users on old kernels from manually removing
workdir entirely or mounting overlay with a new workdir, so this is in
no way a full proof backward compatibility enforcement, but only a best
effort.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Vivek,

Re-posting with minor cleanups.
Also pushed to branch ovl-incompat.

Thanks,
Amir.

Changes since v1:
- Move check for "incompat" name to ovl_workdir_cleanup_recurse()


 fs/overlayfs/readdir.c | 30 +++++++++++++++++++++++++-----
 fs/overlayfs/super.c   | 25 ++++++++++++++++++-------
 2 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 6918b98faeb6..683c6f27ab77 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1051,7 +1051,9 @@ int ovl_check_d_type_supported(struct path *realpath)
 	return rdd.d_type_supported;
 }
 
-static void ovl_workdir_cleanup_recurse(struct path *path, int level)
+#define OVL_INCOMPATDIR_NAME "incompat"
+
+static int ovl_workdir_cleanup_recurse(struct path *path, int level)
 {
 	int err;
 	struct inode *dir = path->dentry->d_inode;
@@ -1065,6 +1067,15 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
 		.root = &root,
 		.is_lowest = false,
 	};
+	bool incompat = false;
+
+	/*
+	 * The "work/incompat" directory is treated specially - if it is not
+	 * empty, instead of printing a generic error and mounting read-only,
+	 * we will error about incompat features and fail the mount.
+	 */
+	if (level == 2 && !strcmp(path->dentry->d_name.name, OVL_INCOMPATDIR_NAME))
+		incompat = true;
 
 	err = ovl_dir_read(path, &rdd);
 	if (err)
@@ -1079,21 +1090,29 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
 				continue;
 			if (p->len == 2 && p->name[1] == '.')
 				continue;
+		} else if (incompat) {
+			pr_warn("overlay with incompat feature '%.*s' cannot be mounted\n",
+				p->len, p->name);
+			err = -EEXIST;
+			break;
 		}
 		dentry = lookup_one_len(p->name, path->dentry, p->len);
 		if (IS_ERR(dentry))
 			continue;
 		if (dentry->d_inode)
-			ovl_workdir_cleanup(dir, path->mnt, dentry, level);
+			err = ovl_workdir_cleanup(dir, path->mnt, dentry, level);
 		dput(dentry);
+		if (err)
+			break;
 	}
 	inode_unlock(dir);
 out:
 	ovl_cache_free(&list);
+	return err;
 }
 
 int ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
-			 struct dentry *dentry, int level)
+			struct dentry *dentry, int level)
 {
 	int err;
 
@@ -1106,9 +1125,10 @@ int ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
 		struct path path = { .mnt = mnt, .dentry = dentry };
 
 		inode_unlock(dir);
-		ovl_workdir_cleanup_recurse(&path, level + 1);
+		err = ovl_workdir_cleanup_recurse(&path, level + 1);
 		inode_lock_nested(dir, I_MUTEX_PARENT);
-		err = ovl_cleanup(dir, dentry);
+		if (!err)
+			err = ovl_cleanup(dir, dentry);
 	}
 
 	return err;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 4b38141c2985..3cd47e4b2eae 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -705,8 +705,12 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 				goto out_unlock;
 
 			retried = true;
-			ovl_workdir_cleanup(dir, mnt, work, 0);
+			err = ovl_workdir_cleanup(dir, mnt, work, 0);
 			dput(work);
+			if (err == -EEXIST) {
+				work = ERR_PTR(err);
+				goto out_unlock;
+			}
 			goto retry;
 		}
 
@@ -1203,7 +1207,7 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
 			    struct path *workpath)
 {
 	struct vfsmount *mnt = ovl_upper_mnt(ofs);
-	struct dentry *temp;
+	struct dentry *temp, *workdir;
 	bool rename_whiteout;
 	bool d_type;
 	int fh_type;
@@ -1213,10 +1217,13 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
 	if (err)
 		return err;
 
-	ofs->workdir = ovl_workdir_create(ofs, OVL_WORKDIR_NAME, false);
-	if (!ofs->workdir)
+	workdir = ovl_workdir_create(ofs, OVL_WORKDIR_NAME, false);
+	err = PTR_ERR(workdir);
+	if (IS_ERR_OR_NULL(workdir))
 		goto out;
 
+	ofs->workdir = workdir;
+
 	err = ovl_setup_trap(sb, ofs->workdir, &ofs->workdir_trap, "workdir");
 	if (err)
 		goto out;
@@ -1347,6 +1354,7 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
 			    struct ovl_entry *oe, struct path *upperpath)
 {
 	struct vfsmount *mnt = ovl_upper_mnt(ofs);
+	struct dentry *indexdir;
 	int err;
 
 	err = mnt_want_write(mnt);
@@ -1366,9 +1374,12 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
 	ofs->workdir_trap = NULL;
 	dput(ofs->workdir);
 	ofs->workdir = NULL;
-	ofs->indexdir = ovl_workdir_create(ofs, OVL_INDEXDIR_NAME, true);
-	if (ofs->indexdir) {
-		ofs->workdir = dget(ofs->indexdir);
+	indexdir = ovl_workdir_create(ofs, OVL_INDEXDIR_NAME, true);
+	if (IS_ERR(indexdir)) {
+		err = PTR_ERR(indexdir);
+	} else if (indexdir) {
+		ofs->indexdir = indexdir;
+		ofs->workdir = dget(indexdir);
 
 		err = ovl_setup_trap(sb, ofs->indexdir, &ofs->indexdir_trap,
 				     "indexdir");
-- 
2.17.1

Re: [PATCH v2] ovl: check for incomapt features in work dir

[Vivek Goyal]

On Sun, Aug 30, 2020 at 11:28:03PM +0300, Amir Goldstein wrote:

[..]
> @@ -1079,21 +1090,29 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
>  				continue;
>  			if (p->len == 2 && p->name[1] == '.')
>  				continue;
> +		} else if (incompat) {
> +			pr_warn("overlay with incompat feature '%.*s' cannot be mounted\n",
> +				p->len, p->name);
> +			err = -EEXIST;
> +			break;

Hi Amir,

Should above be pr_err() instead of pr_warn()?

Apart from above minor nit, this patch looks good to me. I did a quick
test and it works.

Vivek

Re: [PATCH v2] ovl: check for incomapt features in work dir

[Amir Goldstein]

On Mon, Aug 31, 2020 at 7:12 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Sun, Aug 30, 2020 at 11:28:03PM +0300, Amir Goldstein wrote:
>
> [..]
> > @@ -1079,21 +1090,29 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
> >                               continue;
> >                       if (p->len == 2 && p->name[1] == '.')
> >                               continue;
> > +             } else if (incompat) {
> > +                     pr_warn("overlay with incompat feature '%.*s' cannot be mounted\n",
> > +                             p->len, p->name);
> > +                     err = -EEXIST;
> > +                     break;
>
> Hi Amir,
>
> Should above be pr_err() instead of pr_warn()?
>

Sure.

I suppose Miklos can fix that on commit...

Thanks,
Amir.

Re: [PATCH v2] ovl: check for incomapt features in work dir

[Miklos Szeredi]

On Sun, Aug 30, 2020 at 10:28 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> An incompatible feature is marked by a non-empty directory nested
> 2 levels deep under "work" dir, e.g.:
> workdir/work/incompat/volatile.
>
> This commit checks for marked incompat features, warns about them
> and fails to mount the overlay, for example:
>   overlayfs: overlay with incompat feature 'volatile' cannot be mounted
>
> Very old kernels (i.e. v3.18) will fail to remove a non-empty "work"
> dir and fail the mount.  Newer kernels will fail to remove a "work"
> dir with entries nested 3 levels and fall back to read-only mount.
>
> User mounting with old kernel will see a warning like these in dmesg:
>   overlayfs: cleanup of 'incompat/...' failed (-39)
>   overlayfs: cleanup of 'work/incompat' failed (-39)
>   overlayfs: cleanup of 'ovl-work/work' failed (-39)
>   overlayfs: failed to create directory /vdf/ovl-work/work (errno: 17);
>              mounting read-only
>
> These warnings should give the hint to the user that:
> 1. mount failure is caused by backward incompatible features
> 2. mount failure can be resolved by manually removing the "work" directory
>
> There is nothing preventing users on old kernels from manually removing
> workdir entirely or mounting overlay with a new workdir, so this is in
> no way a full proof backward compatibility enforcement, but only a best
> effort.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Vivek,
>
> Re-posting with minor cleanups.
> Also pushed to branch ovl-incompat.
>
> Thanks,
> Amir.
>
> Changes since v1:
> - Move check for "incompat" name to ovl_workdir_cleanup_recurse()
>
>
>  fs/overlayfs/readdir.c | 30 +++++++++++++++++++++++++-----
>  fs/overlayfs/super.c   | 25 ++++++++++++++++++-------
>  2 files changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 6918b98faeb6..683c6f27ab77 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1051,7 +1051,9 @@ int ovl_check_d_type_supported(struct path *realpath)
>         return rdd.d_type_supported;
>  }
>
> -static void ovl_workdir_cleanup_recurse(struct path *path, int level)
> +#define OVL_INCOMPATDIR_NAME "incompat"
> +
> +static int ovl_workdir_cleanup_recurse(struct path *path, int level)
>  {
>         int err;
>         struct inode *dir = path->dentry->d_inode;
> @@ -1065,6 +1067,15 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
>                 .root = &root,
>                 .is_lowest = false,
>         };
> +       bool incompat = false;
> +
> +       /*
> +        * The "work/incompat" directory is treated specially - if it is not
> +        * empty, instead of printing a generic error and mounting read-only,
> +        * we will error about incompat features and fail the mount.
> +        */
> +       if (level == 2 && !strcmp(path->dentry->d_name.name, OVL_INCOMPATDIR_NAME))
> +               incompat = true;

Should the test be specific to "work/incompat"?  AFAICS this will
trigger under "index" as well...

>
>         err = ovl_dir_read(path, &rdd);
>         if (err)
> @@ -1079,21 +1090,29 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
>                                 continue;
>                         if (p->len == 2 && p->name[1] == '.')
>                                 continue;
> +               } else if (incompat) {
> +                       pr_warn("overlay with incompat feature '%.*s' cannot be mounted\n",
> +                               p->len, p->name);
> +                       err = -EEXIST;

EEXIST feels counterintuitive.  I'd rather opt for EINVAL.

Thanks,
Miklos

Re: [PATCH v2] ovl: check for incomapt features in work dir

[Amir Goldstein]

On Tue, Sep 1, 2020 at 12:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sun, Aug 30, 2020 at 10:28 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > An incompatible feature is marked by a non-empty directory nested
> > 2 levels deep under "work" dir, e.g.:
> > workdir/work/incompat/volatile.
> >
> > This commit checks for marked incompat features, warns about them
> > and fails to mount the overlay, for example:
> >   overlayfs: overlay with incompat feature 'volatile' cannot be mounted
> >
> > Very old kernels (i.e. v3.18) will fail to remove a non-empty "work"
> > dir and fail the mount.  Newer kernels will fail to remove a "work"
> > dir with entries nested 3 levels and fall back to read-only mount.
> >
> > User mounting with old kernel will see a warning like these in dmesg:
> >   overlayfs: cleanup of 'incompat/...' failed (-39)
> >   overlayfs: cleanup of 'work/incompat' failed (-39)
> >   overlayfs: cleanup of 'ovl-work/work' failed (-39)
> >   overlayfs: failed to create directory /vdf/ovl-work/work (errno: 17);
> >              mounting read-only
> >
> > These warnings should give the hint to the user that:
> > 1. mount failure is caused by backward incompatible features
> > 2. mount failure can be resolved by manually removing the "work" directory
> >
> > There is nothing preventing users on old kernels from manually removing
> > workdir entirely or mounting overlay with a new workdir, so this is in
> > no way a full proof backward compatibility enforcement, but only a best
> > effort.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Vivek,
> >
> > Re-posting with minor cleanups.
> > Also pushed to branch ovl-incompat.
> >
> > Thanks,
> > Amir.
> >
> > Changes since v1:
> > - Move check for "incompat" name to ovl_workdir_cleanup_recurse()
> >
> >
> >  fs/overlayfs/readdir.c | 30 +++++++++++++++++++++++++-----
> >  fs/overlayfs/super.c   | 25 ++++++++++++++++++-------
> >  2 files changed, 43 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > index 6918b98faeb6..683c6f27ab77 100644
> > --- a/fs/overlayfs/readdir.c
> > +++ b/fs/overlayfs/readdir.c
> > @@ -1051,7 +1051,9 @@ int ovl_check_d_type_supported(struct path *realpath)
> >         return rdd.d_type_supported;
> >  }
> >
> > -static void ovl_workdir_cleanup_recurse(struct path *path, int level)
> > +#define OVL_INCOMPATDIR_NAME "incompat"
> > +
> > +static int ovl_workdir_cleanup_recurse(struct path *path, int level)
> >  {
> >         int err;
> >         struct inode *dir = path->dentry->d_inode;
> > @@ -1065,6 +1067,15 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
> >                 .root = &root,
> >                 .is_lowest = false,
> >         };
> > +       bool incompat = false;
> > +
> > +       /*
> > +        * The "work/incompat" directory is treated specially - if it is not
> > +        * empty, instead of printing a generic error and mounting read-only,
> > +        * we will error about incompat features and fail the mount.
> > +        */
> > +       if (level == 2 && !strcmp(path->dentry->d_name.name, OVL_INCOMPATDIR_NAME))
> > +               incompat = true;
>
> Should the test be specific to "work/incompat"?  AFAICS this will
> trigger under "index" as well...

When called from ovl_indexdir_cleanup(), path->dentry->d_name.name[0] == '#',
because cleanup starts at level 1 and ovl_workdir_cleanup_recurse() is called
with level 2.

>
> >
> >         err = ovl_dir_read(path, &rdd);
> >         if (err)
> > @@ -1079,21 +1090,29 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
> >                                 continue;
> >                         if (p->len == 2 && p->name[1] == '.')
> >                                 continue;
> > +               } else if (incompat) {
> > +                       pr_warn("overlay with incompat feature '%.*s' cannot be mounted\n",
> > +                               p->len, p->name);
> > +                       err = -EEXIST;
>
> EEXIST feels counterintuitive.  I'd rather opt for EINVAL.

I usually prefer to use EINVAL for illegal user input and this is a border line,
so I prefer errors that indicate the state of the object, like EEXIST
or ENOTEMPTY,
but because these errors are not expected on mount, I can live with EINVAL.

Assuming that you agree with my response to ovl_indexdir_cleanup(), let
me know if you need me to post v3 or if you can change the choice of error
and s/pr_warn/pr_err on commit.

Thanks,
Amir.

Re: [PATCH v2] ovl: check for incomapt features in work dir

[Miklos Szeredi]

On Tue, Sep 1, 2020 at 11:44 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Sep 1, 2020 at 12:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Sun, Aug 30, 2020 at 10:28 PM Amir Goldstein <amir73il@gmail.com> wrote:

> > > +        * The "work/incompat" directory is treated specially - if it is not
> > > +        * empty, instead of printing a generic error and mounting read-only,
> > > +        * we will error about incompat features and fail the mount.
> > > +        */
> > > +       if (level == 2 && !strcmp(path->dentry->d_name.name, OVL_INCOMPATDIR_NAME))
> > > +               incompat = true;
> >
> > Should the test be specific to "work/incompat"?  AFAICS this will
> > trigger under "index" as well...
>
> When called from ovl_indexdir_cleanup(), path->dentry->d_name.name[0] == '#',
> because cleanup starts at level 1 and ovl_workdir_cleanup_recurse() is called
> with level 2.

Okay.  I'll add a comment.

> >
> > >
> > >         err = ovl_dir_read(path, &rdd);
> > >         if (err)
> > > @@ -1079,21 +1090,29 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
> > >                                 continue;
> > >                         if (p->len == 2 && p->name[1] == '.')
> > >                                 continue;
> > > +               } else if (incompat) {
> > > +                       pr_warn("overlay with incompat feature '%.*s' cannot be mounted\n",
> > > +                               p->len, p->name);
> > > +                       err = -EEXIST;
> >
> > EEXIST feels counterintuitive.  I'd rather opt for EINVAL.
>
> I usually prefer to use EINVAL for illegal user input and this is a border line,
> so I prefer errors that indicate the state of the object, like EEXIST
> or ENOTEMPTY,
> but because these errors are not expected on mount, I can live with EINVAL.

Exactly.  The error should make sense in relation to a specific
argument of the syscall.  The connection with EEXIST is too deeply
embedded inside overlayfs implementation details.

> Assuming that you agree with my response to ovl_indexdir_cleanup(), let
> me know if you need me to post v3 or if you can change the choice of error
> and s/pr_warn/pr_err on commit.

I'll fix these up.

Thanks,
Miklos