[PATCH 0/4] Overlayfs index features

[PATCH 0/4] Overlayfs index features

[Amir Goldstein]

Miklos,

This series is an attempt not to repeat the mistakes of the past
w.r.t. overlayfs features.

I have given a lot of though about how to scale up with more
compat/incompat features instead of an ever growing disclaimer matrix
in overlafs/Kconfig.

My proposed solution only covers features compatibility among
sub-features of index or features which rely on index being enabled.
There is also a best effort attempt to impose features compatibility
as far as v3.18 using a trick with a dirty work dir.

Although this series is based on the stable fix patch "ovl: do not
cleanup unsupported index entries", it does NOT rely on the fix being
applied on v4.13 kernel for correct behavior on downgrade.

The first use case for incompat index features is going to be index=all
for NFS export - probably rocompat, not sure yet.

I don't think there is anything is this that should be backported
to v4.13 or rushed into v4.14.
What do you think?

Amir.

Amir Goldstein (4):
  ovl: introduce incompatible index feature
  ovl: declare index feature backward compatible
  ovl: cast a shadow of incomapt index into the past
  ovl: check incompat/rocompat index features

 fs/overlayfs/Kconfig     |  25 ++++++++---
 fs/overlayfs/dir.c       |  66 +++++++++++++++++++++++++++++
 fs/overlayfs/overlayfs.h |  20 ++++++++-
 fs/overlayfs/readdir.c   | 108 +++++++++++++++++++++++++++++++++++++++++++----
 fs/overlayfs/super.c     |  50 ++++++++++++++++++++--
 fs/overlayfs/util.c      | 105 +++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 354 insertions(+), 20 deletions(-)

-- 
2.7.4

[PATCH 1/4] ovl: introduce incompatible index feature

[Amir Goldstein]

Introduce a new config option OVERLAY_FS_INDEX_INCOMPAT.

If this config option is enabled then inodes index is declared
an incompatible feature and kernel will refuse to mount an overlay
with inodes index off when a non-empty index directory exists.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/Kconfig     |  8 ++++++++
 fs/overlayfs/dir.c       | 30 ++++++++++++++++++++++++++++++
 fs/overlayfs/overlayfs.h |  3 ++-
 fs/overlayfs/super.c     | 24 +++++++++++++++++++++++-
 4 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index cbfc196e5dc5..e5e6dec7d177 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -43,3 +43,11 @@ config OVERLAY_FS_INDEX
 	  outcomes.  However, mounting the same overlay with an old kernel
 	  read-write and then mounting it again with a new kernel, will have
 	  unexpected results.
+
+config OVERLAY_FS_INDEX_INCOMPAT
+	bool "Overlayfs: support incompatible index feature"
+	depends on OVERLAY_FS_INDEX
+	help
+	  If this config option is enabled then inodes index is declared an
+	  incompatible feature and kernel will refuse to mount an overlay with
+	  inodes index off when a non-empty index directory exists.
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index cc961a3bd3bd..7e2f748d5a7c 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/fs.h>
+#include <linux/mount.h>
 #include <linux/namei.h>
 #include <linux/xattr.h>
 #include <linux/security.h>
@@ -43,6 +44,35 @@ int ovl_cleanup(struct inode *wdir, struct dentry *wdentry)
 	return err;
 }
 
+int ovl_cleanup_path(struct path *path, const char *name)
+{
+	struct inode *dir = path->dentry->d_inode;
+	struct dentry *dentry;
+	int err;
+
+	err = mnt_want_write(path->mnt);
+	if (err)
+		return err;
+
+	inode_lock_nested(dir, I_MUTEX_PARENT);
+
+	dentry = lookup_one_len(name, path->dentry, strlen(name));
+	if (IS_ERR(dentry)) {
+		err = PTR_ERR(dentry);
+		dentry = NULL;
+	} else if (!dentry->d_inode) {
+		err = -ENOENT;
+	} else {
+		err = ovl_cleanup(dir, dentry);
+	}
+
+	dput(dentry);
+	inode_unlock(dir);
+	mnt_drop_write(path->mnt);
+
+	return err;
+}
+
 struct dentry *ovl_lookup_temp(struct dentry *workdir)
 {
 	struct dentry *temp;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index d9a0edd4e57e..3f9a7625bded 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -300,6 +300,8 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
 
 /* dir.c */
 extern const struct inode_operations ovl_dir_inode_operations;
+int ovl_cleanup(struct inode *dir, struct dentry *dentry);
+int ovl_cleanup_path(struct path *path, const char *name);
 struct dentry *ovl_lookup_temp(struct dentry *workdir);
 struct cattr {
 	dev_t rdev;
@@ -309,7 +311,6 @@ struct cattr {
 int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 		    struct cattr *attr,
 		    struct dentry *hardlink, bool debug);
-int ovl_cleanup(struct inode *dir, struct dentry *dentry);
 
 /* copy_up.c */
 int ovl_copy_up(struct dentry *dentry);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f5738e96a052..0dc6d61f828a 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -39,6 +39,9 @@ module_param_named(index, ovl_index_def, bool, 0644);
 MODULE_PARM_DESC(ovl_index_def,
 		 "Default to on or off for the inodes index feature");
 
+static const bool ovl_incompat_index =
+	IS_ENABLED(CONFIG_OVERLAY_FS_INDEX_INCOMPAT);
+
 static void ovl_dentry_release(struct dentry *dentry)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
@@ -1060,7 +1063,15 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	else if (ufs->upper_mnt->mnt_sb != ufs->same_sb)
 		ufs->same_sb = NULL;
 
-	if (!(ovl_force_readonly(ufs)) && ufs->config.index) {
+	if (ovl_force_readonly(ufs)) {
+		/*
+		 * Cannot enable index without upperdir/workdir and cannot
+		 * test for incompat index dir, but cannot corrupt incompat
+		 * index dir either.
+		 */
+		ufs->config.index = false;
+		pr_warn("overlayfs: no upperdir/workdir, falling back to index=off.\n");
+	} else if (ufs->config.index) {
 		/* Verify lower root is upper root origin */
 		err = ovl_verify_origin(upperpath.dentry, ufs->lower_mnt[0],
 					stack[0].dentry, false, true);
@@ -1088,6 +1099,17 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 			pr_warn("overlayfs: try deleting index dir or mounting with '-o index=off' to disable inodes index.\n");
 		if (err)
 			goto out_put_indexdir;
+
+	} else if (ovl_incompat_index) {
+		/*
+		 * Try to remove empty index dir if it exists -
+		 * failure means we need to abort the mount.
+		 */
+		err = ovl_cleanup_path(&workpath, OVL_INDEXDIR_NAME);
+		if (err && err != -ENOENT) {
+			pr_err("overlayfs: incompatible index dir exists, try deleting index dir to disable inodes index.\n");
+			goto out_put_lower_mnt;
+		}
 	}
 
 	/* Show index=off/on in /proc/mounts for any of the reasons above */
-- 
2.7.4

[PATCH 2/4] ovl: declare index feature backward compatible

[Amir Goldstein]

Kconfig description on OVERLAY_FS_INDEX states that "the inodes index
feature is read-only backward compatible" and that mounting an overlay
with index enabled, disabled and then enabled again "will have
unexpected results."

Commit d1712d8fef03 ("ovl: fix EIO from lookup of non-indexed upper")
makes the results of this enable/disable/enable maneuver just as
expected and the results of a plain enable of index feature on existing
overlay - some hardlinks may have already been broken and new hardlinks
will not be broken on copy up.

Now that there is a dedicated config option to opt-in in for backward
incompatible index feature, remove the backward compatibility clause from
Kconfig description of OVERLAY_FS_INDEX.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/Kconfig | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index e5e6dec7d177..02295e10ffbe 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -37,13 +37,6 @@ config OVERLAY_FS_INDEX
 	  The inodes index feature prevents breaking of lower hardlinks on copy
 	  up.
 
-	  Note, that the inodes index feature is read-only backward compatible.
-	  That is, mounting an overlay which has an index dir on a kernel that
-	  doesn't support this feature read-only, will not have any negative
-	  outcomes.  However, mounting the same overlay with an old kernel
-	  read-write and then mounting it again with a new kernel, will have
-	  unexpected results.
-
 config OVERLAY_FS_INDEX_INCOMPAT
 	bool "Overlayfs: support incompatible index feature"
 	depends on OVERLAY_FS_INDEX
-- 
2.7.4

[PATCH 3/4] ovl: cast a shadow of incomapt index into the past

[Amir Goldstein]

Because overlayfs features were not implemented in the first version
of overlayfs merged in v3.18, we cannot enforce old kernel not to mount
overlayfs with new backward incompatible features... but we can try to
set traps for those who try to perform those mounts.

When enabling a backward incompatible feature, we create a
non-empty directory nested 2 levels deep under "work" dir, e.g.:
workdir/work/incompat_features/incomapt_index.

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_features/incompat_index' failed (-39)
  overlayfs: cleanup of 'work/incompat_features' 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 approach to complement the backward compatibility disclaimers
in Kconfig for enabling backward incompatible features (e.g.
OVERLAY_FS_INDEX_INCOMAPT).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/Kconfig     | 12 +++++++
 fs/overlayfs/dir.c       | 36 ++++++++++++++++++++
 fs/overlayfs/overlayfs.h | 15 +++++++-
 fs/overlayfs/readdir.c   | 33 +++++++++++++++---
 fs/overlayfs/super.c     | 18 +++++++++-
 fs/overlayfs/util.c      | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 196 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index 02295e10ffbe..7c97550ba39f 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -44,3 +44,15 @@ config OVERLAY_FS_INDEX_INCOMPAT
 	  If this config option is enabled then inodes index is declared an
 	  incompatible feature and kernel will refuse to mount an overlay with
 	  inodes index off when a non-empty index directory exists.
+
+	  After mounting an overlay filesystems once with incompatible index
+	  enabled, mounting the overlay with a kernel that doesn't support
+	  inodes index feature or mounting with inodes index off will either
+	  fail to mount or fallback to read-only mount.
+
+	  The inodes index dir resides under the 'workdir' path provided with
+	  overlay mount options, so it is still possible to mount the overlay
+	  on a kernel that doesn't support incompatible index feature, by
+	  cleaning the content of 'workdir' or by providing an alternative
+	  'workdir' path.  In that case, mounting the same overlay again with
+	  inodes index enabled will have unexpected results.
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 7e2f748d5a7c..63a3b848bdf0 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -159,6 +159,42 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 	return err;
 }
 
+/* Create object if it doesn't exist */
+struct dentry *ovl_test_create(struct dentry *parent, const char *name,
+			       umode_t mode)
+{
+	struct inode *dir = parent->d_inode;
+	struct dentry *dentry;
+	int err;
+
+	inode_lock_nested(dir, I_MUTEX_PARENT);
+
+	dentry = lookup_one_len(name, parent, strlen(name));
+	err = PTR_ERR(dentry);
+	if (IS_ERR(dentry))
+		return dentry;
+
+	if (!dentry->d_inode) {
+		err = ovl_create_real(dir, dentry,
+				      &(struct cattr){.mode = mode},
+				      NULL, true);
+	} else if ((mode ^ d_inode(dentry)->i_mode) & S_IFMT) {
+		/* File exists but with wrong file type? */
+		err = -EEXIST;
+	} else {
+		err = 0;
+	}
+
+	inode_unlock(dir);
+
+	if (err) {
+		dput(dentry);
+		return ERR_PTR(err);
+	}
+
+	return dentry;
+}
+
 static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
 			       int xerr)
 {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 3f9a7625bded..d1172b3f9a99 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -27,11 +27,15 @@ enum ovl_path_type {
 #define OVL_XATTR_IMPURE OVL_XATTR_PREFIX "impure"
 #define OVL_XATTR_NLINK OVL_XATTR_PREFIX "nlink"
 
+#define OVL_INCOMPAT_FEATURES_NAME "incompat_features"
+#define OVL_FEATURE_INCOMPAT_INDEX "incompat_index"
+
 enum ovl_flag {
 	OVL_IMPURE,
 	OVL_INDEX,
 };
 
+
 /*
  * The tuple (fh,uuid) is a universal unique identifier for a copy up origin,
  * where:
@@ -242,6 +246,12 @@ static inline bool ovl_is_impuredir(struct dentry *dentry)
 	return ovl_check_dir_xattr(dentry, OVL_XATTR_IMPURE);
 }
 
+bool ovl_is_features_dir(struct dentry *dentry, const char ***features);
+bool ovl_is_feature_supported(const char *name, int namelen,
+			      const char **features);
+struct ovl_fs;
+int ovl_enable_feature(struct ovl_fs *ofs, const char *dirname,
+		       const char *name);
 
 /* namei.c */
 int ovl_verify_origin(struct dentry *dentry, struct vfsmount *mnt,
@@ -261,7 +271,8 @@ void ovl_cache_free(struct list_head *list);
 void ovl_dir_cache_free(struct inode *inode);
 int ovl_check_d_type_supported(struct path *realpath);
 void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
-			 struct dentry *dentry, int level);
+			 struct dentry *dentry, int level,
+			 const char **features);
 int ovl_indexdir_cleanup(struct dentry *dentry, struct vfsmount *mnt,
 			 struct path *lowerstack, unsigned int numlower);
 
@@ -311,6 +322,8 @@ struct cattr {
 int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 		    struct cattr *attr,
 		    struct dentry *hardlink, bool debug);
+struct dentry *ovl_test_create(struct dentry *parent, const char *name,
+			       umode_t mode);
 
 /* copy_up.c */
 int ovl_copy_up(struct dentry *dentry);
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 698b74dd750e..53ee99ec5b1c 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -922,7 +922,8 @@ 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)
+static void ovl_workdir_cleanup_recurse(struct path *path, int level,
+					const char **features)
 {
 	int err;
 	struct inode *dir = path->dentry->d_inode;
@@ -950,12 +951,19 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
 				continue;
 			if (p->len == 2 && p->name[1] == '.')
 				continue;
+		} else if (features && level == 1 &&
+			   !ovl_is_feature_supported(p->name, p->len,
+						     features)) {
+			pr_warn("overlayfs: feature '%.*s' not supported\n",
+				p->len, p->name);
+			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);
+			ovl_workdir_cleanup(dir, path->mnt, dentry, level,
+					    features);
 		dput(dentry);
 	}
 	inode_unlock(dir);
@@ -964,11 +972,26 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
 }
 
 void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
-			 struct dentry *dentry, int level)
+			 struct dentry *dentry, int level,
+			 const char **features)
 {
 	int err;
 
-	if (!d_is_dir(dentry) || level > 1) {
+	/*
+	 * The features directories are treated specially:
+	 * Recursive cleanup iterates 2 levels inside features dir.
+	 * Iteration inside features dir aborts on unsupported feature name.
+	 * By aborting cleanup on unsupported feature name, remove of the
+	 * features dir will fail followed by failure to remove "work" dir
+	 * and resulting in read-only mount.
+	 * This is a hack to force read-only mount with older kernels that
+	 * do not know about features, but only iterate recursively 2 levels
+	 * inside "work" dir.
+	 */
+	if (!features && d_is_dir(dentry) && level == 1 &&
+	    ovl_is_features_dir(dentry, &features)) {
+		level = 0;
+	} else if (!d_is_dir(dentry) || level > 1) {
 		ovl_cleanup(dir, dentry);
 		return;
 	}
@@ -978,7 +1001,7 @@ void 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);
+		ovl_workdir_cleanup_recurse(&path, level + 1, features);
 		inode_lock_nested(dir, I_MUTEX_PARENT);
 		ovl_cleanup(dir, dentry);
 	}
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 0dc6d61f828a..49c6a7ad695f 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -491,7 +491,7 @@ static struct dentry *ovl_workdir_create(struct super_block *sb,
 				goto out_unlock;
 
 			retried = true;
-			ovl_workdir_cleanup(dir, mnt, work, 0);
+			ovl_workdir_cleanup(dir, mnt, work, 0, NULL);
 			dput(work);
 			goto retry;
 		}
@@ -1089,6 +1089,22 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 			if (err)
 				pr_err("overlayfs: failed to verify index dir origin\n");
 
+			/*
+			 * Prevent from mounting this overlay with index=off
+			 * or with kernel without incompat index support and
+			 * corrupting the index.
+			 * Enable incompat index feature if verified index dir
+			 * exists and regardless if ovl_indexdir_cleanup() will
+			 * fail.
+			 */
+			if (!err && ovl_incompat_index) {
+				err = ovl_enable_feature(ufs,
+						 OVL_INCOMPAT_FEATURES_NAME,
+						 OVL_FEATURE_INCOMPAT_INDEX);
+				if (err)
+					goto out_put_indexdir;
+			}
+
 			/* Cleanup bad/stale/orphan index entries */
 			if (!err)
 				err = ovl_indexdir_cleanup(ufs->indexdir,
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index b9b239fa5cfd..bef850632c80 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -579,3 +579,92 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
 	pr_err("overlayfs: failed to lock workdir+upperdir\n");
 	return -EIO;
 }
+
+static const char * const ovl_incompat_features[] = {
+#ifdef CONFIG_OVERLAY_FS_INDEX_INCOMPAT
+	OVL_FEATURE_INCOMPAT_INDEX,
+#endif
+	NULL,
+};
+
+bool ovl_is_features_dir(struct dentry *dentry, const char ***features)
+{
+	if (!dentry || !d_is_dir(dentry))
+		return false;
+
+	if (!strcmp(dentry->d_name.name, OVL_INCOMPAT_FEATURES_NAME)) {
+		*features = (const char **)ovl_incompat_features;
+		return true;
+	}
+
+	return false;
+}
+
+bool ovl_is_feature_supported(const char *name, int namelen,
+			      const char **features)
+{
+	const char **p;
+
+	for (p = features; *p; p++) {
+		if (!p[namelen] && !strncmp(name, *p, namelen))
+			return true;
+	}
+	return false;
+}
+
+/*
+ * Create a non-empty directory under features dir, e.g.:
+ * work/incompat_features/incompat_index
+ */
+static int ovl_create_feature_dir(struct dentry *dentry,  struct vfsmount *mnt,
+				  const char *dirname, const char *name)
+{
+	struct dentry *features, *feature, *enabled;
+	int err;
+
+	features = ovl_test_create(dentry, dirname, S_IFDIR | 0);
+	err = PTR_ERR(features);
+	if (IS_ERR(features))
+		return err;
+
+	feature = ovl_test_create(features, name, S_IFDIR | 0);
+	dput(features);
+	err = PTR_ERR(feature);
+	if (IS_ERR(feature))
+		return err;
+
+	enabled = ovl_test_create(feature, "enabled", S_IFREG | 0);
+	dput(feature);
+	err = PTR_ERR(enabled);
+	if (IS_ERR(enabled))
+		return err;
+
+	dput(enabled);
+
+	return 0;
+}
+
+/*
+ * Prevent kernel with no support for the enabled feature from mounting
+ * this overlay read-write and corrupting the index by creating a
+ * non-empty nested dir entires in workdir, that old kernels
+ * do not know how to clean on mount.
+ */
+int ovl_enable_feature(struct ovl_fs *ofs, const char *dirname,
+		       const char *name)
+{
+	struct vfsmount *mnt = ofs->upper_mnt;
+	int err;
+
+	if (!mnt || !ofs->workdir || !ofs->indexdir)
+		return -EROFS;
+
+	err = mnt_want_write(mnt);
+	if (err)
+		return err;
+
+	err = ovl_create_feature_dir(ofs->workdir, mnt, dirname, name);
+
+	mnt_drop_write(mnt);
+	return err;
+}
-- 
2.7.4

[PATCH 4/4] ovl: check incompat/rocompat index features

[Amir Goldstein]

When enabling a backward incompatible feature, we create a
non-empty directory nested 2 levels deep under "index" dir, e.g.:
workdir/index/incompat_features/incomapt_index.

Kernels with inodes index support, but with no dir index entries support
(i.e. v4.13) will fail to verify the index dir entry "incompat_features"
and fail the mount.

User mounting with those kernels will see warnings like these in dmesg:
  overlayfs: failed to verify index (index/incompat_features,
             ftype=4000, err=-30)
  overlayfs: failed index dir cleanup (-30)
  overlayfs: try deleting index dir or mounting with '-o index=off' to
             disable inodes index.

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 "index" directory

New kernels will recognize the special features directories and will
only fail to mount if any of the names inside the features dir is not a
supported feature.
This will allow us to maintain feature compatibility for index features
going forward.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/overlayfs.h |  4 ++-
 fs/overlayfs/readdir.c   | 75 +++++++++++++++++++++++++++++++++++++++++++++---
 fs/overlayfs/super.c     |  8 +++++-
 fs/overlayfs/util.c      | 26 +++++++++++++----
 4 files changed, 102 insertions(+), 11 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index d1172b3f9a99..1470713cb05e 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -30,6 +30,8 @@ enum ovl_path_type {
 #define OVL_INCOMPAT_FEATURES_NAME "incompat_features"
 #define OVL_FEATURE_INCOMPAT_INDEX "incompat_index"
 
+#define OVL_ROCOMPAT_FEATURES_NAME "rocompat_features"
+
 enum ovl_flag {
 	OVL_IMPURE,
 	OVL_INDEX,
@@ -246,7 +248,7 @@ static inline bool ovl_is_impuredir(struct dentry *dentry)
 	return ovl_check_dir_xattr(dentry, OVL_XATTR_IMPURE);
 }
 
-bool ovl_is_features_dir(struct dentry *dentry, const char ***features);
+int ovl_is_features_dir(struct dentry *dentry, const char ***features);
 bool ovl_is_feature_supported(const char *name, int namelen,
 			      const char **features);
 struct ovl_fs;
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 53ee99ec5b1c..44ef65ce03bf 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -48,7 +48,8 @@ struct ovl_readdir_data {
 	int count;
 	int err;
 	bool is_upper;
-	bool d_type_supported;
+	const char **match;
+	bool found;
 };
 
 struct ovl_dir_file {
@@ -898,7 +899,7 @@ static int ovl_check_d_type(struct dir_context *ctx, const char *name,
 		return 0;
 
 	if (d_type != DT_UNKNOWN)
-		rdd->d_type_supported = true;
+		rdd->found = true;
 
 	return 0;
 }
@@ -912,14 +913,14 @@ int ovl_check_d_type_supported(struct path *realpath)
 	int err;
 	struct ovl_readdir_data rdd = {
 		.ctx.actor = ovl_check_d_type,
-		.d_type_supported = false,
+		.found = false,
 	};
 
 	err = ovl_dir_read(realpath, &rdd);
 	if (err)
 		return err;
 
-	return rdd.d_type_supported;
+	return rdd.found;
 }
 
 static void ovl_workdir_cleanup_recurse(struct path *path, int level,
@@ -1007,6 +1008,48 @@ void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
 	}
 }
 
+static int ovl_check_feature_name(struct dir_context *ctx, const char *name,
+				  int namelen, loff_t offset, u64 ino,
+				  unsigned int d_type)
+{
+	struct ovl_readdir_data *rdd =
+		container_of(ctx, struct ovl_readdir_data, ctx);
+
+	if (name[0] == '.')
+		return 0;
+
+	if (!ovl_is_feature_supported(name, namelen, rdd->match)) {
+		pr_warn("overlayfs: feature '%.*s' not supported\n",
+			namelen, name);
+		rdd->found = true;
+	}
+
+	return 0;
+}
+
+/*
+ * Returns 0 if all feature names are supported.
+ * Returns 1 if an unsupported feature name was found.
+ * Negative values if other error is encountered.
+ */
+int ovl_check_unsupported_feature(struct dentry *dentry, struct vfsmount *mnt,
+				  const char **features)
+{
+	int err;
+	struct path path = { .mnt = mnt, .dentry = dentry };
+	struct ovl_readdir_data rdd = {
+		.ctx.actor = ovl_check_feature_name,
+		.match = features,
+		.found = false,
+	};
+
+	err = ovl_dir_read(&path, &rdd);
+	if (err)
+		return err;
+
+	return rdd.found;
+}
+
 int ovl_indexdir_cleanup(struct dentry *dentry, struct vfsmount *mnt,
 			 struct path *lowerstack, unsigned int numlower)
 {
@@ -1031,6 +1074,9 @@ int ovl_indexdir_cleanup(struct dentry *dentry, struct vfsmount *mnt,
 
 	inode_lock_nested(dir, I_MUTEX_PARENT);
 	list_for_each_entry(p, &list, l_node) {
+		const char **features;
+		int ret = 0;
+
 		if (p->name[0] == '.') {
 			if (p->len == 1)
 				continue;
@@ -1043,6 +1089,26 @@ int ovl_indexdir_cleanup(struct dentry *dentry, struct vfsmount *mnt,
 			index = NULL;
 			break;
 		}
+
+		if (d_is_dir(index))
+			ret = ovl_is_features_dir(index, &features);
+		if (ret) {
+			/*
+			 * ret is -EINVAL for incompat features dir and
+			 * -EROFS for rocompat features dir.
+			 */
+			inode_unlock(dir);
+			err = ovl_check_unsupported_feature(index, mnt,
+							    features);
+			if (err > 0)
+				err = ret;
+			inode_lock_nested(dir, I_MUTEX_PARENT);
+			if (err)
+				break;
+
+			goto next;
+		}
+
 		err = ovl_verify_index(index, lowerstack, numlower);
 		/* Cleanup stale and orphan index entries */
 		if (err && (err == -ESTALE || err == -ENOENT))
@@ -1050,6 +1116,7 @@ int ovl_indexdir_cleanup(struct dentry *dentry, struct vfsmount *mnt,
 		if (err)
 			break;
 
+next:
 		dput(index);
 		index = NULL;
 	}
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 49c6a7ad695f..2d37afc6bf42 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1111,8 +1111,14 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 							   ufs->upper_mnt,
 							   stack, numlower);
 		}
-		if (err || !ufs->indexdir)
+		if (err || !ufs->indexdir) {
+			if (err == -EROFS) {
+				pr_warn("overlayfs: unsupported index features; mounting read-only to avoid corrupting inodes index.\n");
+				sb->s_flags |= MS_RDONLY;
+				err = 0;
+			}
 			pr_warn("overlayfs: try deleting index dir or mounting with '-o index=off' to disable inodes index.\n");
+		}
 		if (err)
 			goto out_put_indexdir;
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index bef850632c80..feb4aae736a6 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -587,17 +587,31 @@ static const char * const ovl_incompat_features[] = {
 	NULL,
 };
 
-bool ovl_is_features_dir(struct dentry *dentry, const char ***features)
+static const char * const ovl_rocompat_features[] = {
+	NULL,
+};
+
+/*
+ * Returns 0 if not a features dir name.
+ * Returns error value to be returned from mount if an unsuppoted
+ * feature name is found in this features dir.
+ */
+int ovl_is_features_dir(struct dentry *dentry, const char ***features)
 {
 	if (!dentry || !d_is_dir(dentry))
-		return false;
+		return 0;
 
 	if (!strcmp(dentry->d_name.name, OVL_INCOMPAT_FEATURES_NAME)) {
 		*features = (const char **)ovl_incompat_features;
-		return true;
+		return -EINVAL;
 	}
 
-	return false;
+	if (!strcmp(dentry->d_name.name, OVL_ROCOMPAT_FEATURES_NAME)) {
+		*features = (const char **)ovl_rocompat_features;
+		return -EROFS;
+	}
+
+	return 0;
 }
 
 bool ovl_is_feature_supported(const char *name, int namelen,
@@ -647,7 +661,7 @@ static int ovl_create_feature_dir(struct dentry *dentry,  struct vfsmount *mnt,
 /*
  * Prevent kernel with no support for the enabled feature from mounting
  * this overlay read-write and corrupting the index by creating a
- * non-empty nested dir entires in workdir, that old kernels
+ * non-empty nested dir entires in workdir and in indexdir, that old kernels
  * do not know how to clean on mount.
  */
 int ovl_enable_feature(struct ovl_fs *ofs, const char *dirname,
@@ -664,6 +678,8 @@ int ovl_enable_feature(struct ovl_fs *ofs, const char *dirname,
 		return err;
 
 	err = ovl_create_feature_dir(ofs->workdir, mnt, dirname, name);
+	if (!err)
+		err = ovl_create_feature_dir(ofs->indexdir, mnt, dirname, name);
 
 	mnt_drop_write(mnt);
 	return err;
-- 
2.7.4

Re: [PATCH 0/4] Overlayfs index features

[Amir Goldstein]

On Tue, Oct 24, 2017 at 1:02 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Miklos,
>
> This series is an attempt not to repeat the mistakes of the past
> w.r.t. overlayfs features.
>
> I have given a lot of though about how to scale up with more
> compat/incompat features instead of an ever growing disclaimer matrix
> in overlafs/Kconfig.
>
> My proposed solution only covers features compatibility among
> sub-features of index or features which rely on index being enabled.
> There is also a best effort attempt to impose features compatibility
> as far as v3.18 using a trick with a dirty work dir.
>
> Although this series is based on the stable fix patch "ovl: do not
> cleanup unsupported index entries", it does NOT rely on the fix being
> applied on v4.13 kernel for correct behavior on downgrade.
>
> The first use case for incompat index features is going to be index=all
> for NFS export - probably rocompat, not sure yet.

Pushed example of that use to:
https://github.com/amir73il/linux/commits/ovl-index-all
(commit "ovl: add support for mount option index=all")

It required some changes to last patch of this series
in order to check not only that feature is "supported" by
kernel, but also that feature is "enabled", i.e. kernel
refuses to let user mount without index=all if index_all
feature has already been enabled by a previous mount.
Pushed changes to:
https://github.com/amir73il/linux/commits/ovl-features

>
> I don't think there is anything is this that should be backported
> to v4.13 or rushed into v4.14.
> What do you think?
>
> Amir.
>
> Amir Goldstein (4):
>   ovl: introduce incompatible index feature
>   ovl: declare index feature backward compatible
>   ovl: cast a shadow of incomapt index into the past
>   ovl: check incompat/rocompat index features
>
>  fs/overlayfs/Kconfig     |  25 ++++++++---
>  fs/overlayfs/dir.c       |  66 +++++++++++++++++++++++++++++
>  fs/overlayfs/overlayfs.h |  20 ++++++++-
>  fs/overlayfs/readdir.c   | 108 +++++++++++++++++++++++++++++++++++++++++++----
>  fs/overlayfs/super.c     |  50 ++++++++++++++++++++--
>  fs/overlayfs/util.c      | 105 +++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 354 insertions(+), 20 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCH 1/4] ovl: introduce incompatible index feature

[Vivek Goyal]

On Tue, Oct 24, 2017 at 01:02:58PM +0300, Amir Goldstein wrote:
> Introduce a new config option OVERLAY_FS_INDEX_INCOMPAT.
> 
> If this config option is enabled then inodes index is declared
> an incompatible feature and kernel will refuse to mount an overlay
> with inodes index off when a non-empty index directory exists.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/Kconfig     |  8 ++++++++
>  fs/overlayfs/dir.c       | 30 ++++++++++++++++++++++++++++++
>  fs/overlayfs/overlayfs.h |  3 ++-
>  fs/overlayfs/super.c     | 24 +++++++++++++++++++++++-
>  4 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index cbfc196e5dc5..e5e6dec7d177 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -43,3 +43,11 @@ config OVERLAY_FS_INDEX
>  	  outcomes.  However, mounting the same overlay with an old kernel
>  	  read-write and then mounting it again with a new kernel, will have
>  	  unexpected results.
> +
> +config OVERLAY_FS_INDEX_INCOMPAT
> +	bool "Overlayfs: support incompatible index feature"
> +	depends on OVERLAY_FS_INDEX
> +	help
> +	  If this config option is enabled then inodes index is declared an
> +	  incompatible feature and kernel will refuse to mount an overlay with
> +	  inodes index off when a non-empty index directory exists.

Hi Amir,

I don't know much about the issues you have faced. So I have few very
basic questions.

So the problem you are trying to fix is that if somebody mounted overlay
with index=on and later they try to mount it with index=off. 

What problems happen if we allow that? If its a problem, why not always
disallow that instead of making it an option. IOW, is there a use case
where we will still let user mount later with index=off.

Vivek
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index cc961a3bd3bd..7e2f748d5a7c 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -8,6 +8,7 @@
>   */
>  
>  #include <linux/fs.h>
> +#include <linux/mount.h>
>  #include <linux/namei.h>
>  #include <linux/xattr.h>
>  #include <linux/security.h>
> @@ -43,6 +44,35 @@ int ovl_cleanup(struct inode *wdir, struct dentry *wdentry)
>  	return err;
>  }
>  
> +int ovl_cleanup_path(struct path *path, const char *name)
> +{
> +	struct inode *dir = path->dentry->d_inode;
> +	struct dentry *dentry;
> +	int err;
> +
> +	err = mnt_want_write(path->mnt);
> +	if (err)
> +		return err;
> +
> +	inode_lock_nested(dir, I_MUTEX_PARENT);
> +
> +	dentry = lookup_one_len(name, path->dentry, strlen(name));
> +	if (IS_ERR(dentry)) {
> +		err = PTR_ERR(dentry);
> +		dentry = NULL;
> +	} else if (!dentry->d_inode) {
> +		err = -ENOENT;
> +	} else {
> +		err = ovl_cleanup(dir, dentry);
> +	}
> +
> +	dput(dentry);
> +	inode_unlock(dir);
> +	mnt_drop_write(path->mnt);
> +
> +	return err;
> +}
> +
>  struct dentry *ovl_lookup_temp(struct dentry *workdir)
>  {
>  	struct dentry *temp;
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index d9a0edd4e57e..3f9a7625bded 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -300,6 +300,8 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
>  
>  /* dir.c */
>  extern const struct inode_operations ovl_dir_inode_operations;
> +int ovl_cleanup(struct inode *dir, struct dentry *dentry);
> +int ovl_cleanup_path(struct path *path, const char *name);
>  struct dentry *ovl_lookup_temp(struct dentry *workdir);
>  struct cattr {
>  	dev_t rdev;
> @@ -309,7 +311,6 @@ struct cattr {
>  int ovl_create_real(struct inode *dir, struct dentry *newdentry,
>  		    struct cattr *attr,
>  		    struct dentry *hardlink, bool debug);
> -int ovl_cleanup(struct inode *dir, struct dentry *dentry);
>  
>  /* copy_up.c */
>  int ovl_copy_up(struct dentry *dentry);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index f5738e96a052..0dc6d61f828a 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -39,6 +39,9 @@ module_param_named(index, ovl_index_def, bool, 0644);
>  MODULE_PARM_DESC(ovl_index_def,
>  		 "Default to on or off for the inodes index feature");
>  
> +static const bool ovl_incompat_index =
> +	IS_ENABLED(CONFIG_OVERLAY_FS_INDEX_INCOMPAT);
> +
>  static void ovl_dentry_release(struct dentry *dentry)
>  {
>  	struct ovl_entry *oe = dentry->d_fsdata;
> @@ -1060,7 +1063,15 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>  	else if (ufs->upper_mnt->mnt_sb != ufs->same_sb)
>  		ufs->same_sb = NULL;
>  
> -	if (!(ovl_force_readonly(ufs)) && ufs->config.index) {
> +	if (ovl_force_readonly(ufs)) {
> +		/*
> +		 * Cannot enable index without upperdir/workdir and cannot
> +		 * test for incompat index dir, but cannot corrupt incompat
> +		 * index dir either.
> +		 */
> +		ufs->config.index = false;
> +		pr_warn("overlayfs: no upperdir/workdir, falling back to index=off.\n");
> +	} else if (ufs->config.index) {
>  		/* Verify lower root is upper root origin */
>  		err = ovl_verify_origin(upperpath.dentry, ufs->lower_mnt[0],
>  					stack[0].dentry, false, true);
> @@ -1088,6 +1099,17 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>  			pr_warn("overlayfs: try deleting index dir or mounting with '-o index=off' to disable inodes index.\n");
>  		if (err)
>  			goto out_put_indexdir;
> +
> +	} else if (ovl_incompat_index) {
> +		/*
> +		 * Try to remove empty index dir if it exists -
> +		 * failure means we need to abort the mount.
> +		 */
> +		err = ovl_cleanup_path(&workpath, OVL_INDEXDIR_NAME);
> +		if (err && err != -ENOENT) {
> +			pr_err("overlayfs: incompatible index dir exists, try deleting index dir to disable inodes index.\n");
> +			goto out_put_lower_mnt;
> +		}
>  	}
>  
>  	/* Show index=off/on in /proc/mounts for any of the reasons above */
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] ovl: declare index feature backward compatible

[Vivek Goyal]

On Tue, Oct 24, 2017 at 01:02:59PM +0300, Amir Goldstein wrote:
> Kconfig description on OVERLAY_FS_INDEX states that "the inodes index
> feature is read-only backward compatible" and that mounting an overlay
> with index enabled, disabled and then enabled again "will have
> unexpected results."
> 
> Commit d1712d8fef03 ("ovl: fix EIO from lookup of non-indexed upper")
> makes the results of this enable/disable/enable maneuver just as
> expected and the results of a plain enable of index feature on existing
> overlay - some hardlinks may have already been broken and new hardlinks
> will not be broken on copy up.
> 
> Now that there is a dedicated config option to opt-in in for backward
> incompatible index feature, remove the backward compatibility clause from
> Kconfig description of OVERLAY_FS_INDEX.

What if user does OVERLAY_FS_INDEX_INCOMPAT=n, then this text is still
relevant?

Vivek
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/Kconfig | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index e5e6dec7d177..02295e10ffbe 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -37,13 +37,6 @@ config OVERLAY_FS_INDEX
>  	  The inodes index feature prevents breaking of lower hardlinks on copy
>  	  up.
>  
> -	  Note, that the inodes index feature is read-only backward compatible.
> -	  That is, mounting an overlay which has an index dir on a kernel that
> -	  doesn't support this feature read-only, will not have any negative
> -	  outcomes.  However, mounting the same overlay with an old kernel
> -	  read-write and then mounting it again with a new kernel, will have
> -	  unexpected results.
> -
>  config OVERLAY_FS_INDEX_INCOMPAT
>  	bool "Overlayfs: support incompatible index feature"
>  	depends on OVERLAY_FS_INDEX
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] ovl: declare index feature backward compatible

[Amir Goldstein]

On Fri, Nov 10, 2017 at 4:21 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Oct 24, 2017 at 01:02:59PM +0300, Amir Goldstein wrote:
>> Kconfig description on OVERLAY_FS_INDEX states that "the inodes index
>> feature is read-only backward compatible" and that mounting an overlay
>> with index enabled, disabled and then enabled again "will have
>> unexpected results."
>>
>> Commit d1712d8fef03 ("ovl: fix EIO from lookup of non-indexed upper")
>> makes the results of this enable/disable/enable maneuver just as
>> expected and the results of a plain enable of index feature on existing
>> overlay - some hardlinks may have already been broken and new hardlinks
>> will not be broken on copy up.
>>
>> Now that there is a dedicated config option to opt-in in for backward
>> incompatible index feature, remove the backward compatibility clause from
>> Kconfig description of OVERLAY_FS_INDEX.
>
> What if user does OVERLAY_FS_INDEX_INCOMPAT=n, then this text is still
> relevant?
>

In theory Yes.
Because at the moment, after fixing the EIO behavior,
I am not aware of anything making  OVERLAY_FS_INDEX non backward compat.

You upgrade to index=on
copy up doesn't break hardlinks
downgrade to index=off
copy up breaks hardlinks
upgrade to index=on
copy up joins indexed hardlinks
...
and so on.

Unless there is something else I didn't think of.

The idea with introducing the explicit option OVERLAY_FS_INDEX_INCOMPAT
is that any new index feature (e.g. full index for NFS export) can require
user to commit to not going backwards (i.e. mount -o index=off will fail).

Perhaps we need to work out the details of the allowed transistions
better then I defined them.

Amir.

Re: [PATCH 1/4] ovl: introduce incompatible index feature

[Amir Goldstein]

On Fri, Nov 10, 2017 at 3:57 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Oct 24, 2017 at 01:02:58PM +0300, Amir Goldstein wrote:
>> Introduce a new config option OVERLAY_FS_INDEX_INCOMPAT.
>>
>> If this config option is enabled then inodes index is declared
>> an incompatible feature and kernel will refuse to mount an overlay
>> with inodes index off when a non-empty index directory exists.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/Kconfig     |  8 ++++++++
>>  fs/overlayfs/dir.c       | 30 ++++++++++++++++++++++++++++++
>>  fs/overlayfs/overlayfs.h |  3 ++-
>>  fs/overlayfs/super.c     | 24 +++++++++++++++++++++++-
>>  4 files changed, 63 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
>> index cbfc196e5dc5..e5e6dec7d177 100644
>> --- a/fs/overlayfs/Kconfig
>> +++ b/fs/overlayfs/Kconfig
>> @@ -43,3 +43,11 @@ config OVERLAY_FS_INDEX
>>         outcomes.  However, mounting the same overlay with an old kernel
>>         read-write and then mounting it again with a new kernel, will have
>>         unexpected results.
>> +
>> +config OVERLAY_FS_INDEX_INCOMPAT
>> +     bool "Overlayfs: support incompatible index feature"
>> +     depends on OVERLAY_FS_INDEX
>> +     help
>> +       If this config option is enabled then inodes index is declared an
>> +       incompatible feature and kernel will refuse to mount an overlay with
>> +       inodes index off when a non-empty index directory exists.
>
> Hi Amir,
>
> I don't know much about the issues you have faced. So I have few very
> basic questions.
>
> So the problem you are trying to fix is that if somebody mounted overlay
> with index=on and later they try to mount it with index=off.
>
> What problems happen if we allow that? If its a problem, why not always
> disallow that instead of making it an option. IOW, is there a use case
> where we will still let user mount later with index=off.
>

So I have no current issue with going back from index=on, but I would like
to start adding incompatible features (like index=all), but in order to add
the concept of incompatible feature, first user will need to opt-in in compile
time to "overlayfs v2 format" which supports the feature checks.
opting-in to format v2 means that user knows he cannot (*) go backwards
to kernel that doesn't support v2 format and mount v2 format overlayfs.

Actually, the user can go backwards, but he will need to manually remove
workdir and indexdir in order to "degrade" overlayfs to v1 before mount.

I guess config option could have been
OVERLAY_FS_INCOMAT_V2

and it doesn't even need to make index=on an incompat feature as this
patch does, but it was easier for me to implement it this way.
OTOH, if users cannot enable index=on on existing systems because
of the exclusive lock thing, it is better if users go directly to
OVERLAY_FS_INDEX_INCOMPAT on a flag day when thier system
has adapted to all the new behaviors of index=on and there is not
really any good reason to mount with index=off.

If for example, user migrates layers and mount fails file handle verifications
then user should either fix file handles with fsck.overlayfs or kill the index
dir (not migrate it in the first place) and then user can mount with index=off.

Hope this brings more clarity to the purpose of this series.
Comments and thought are welcome.

Thanks,
Amir.

Re: [PATCH 3/4] ovl: cast a shadow of incomapt index into the past

[Vivek Goyal]

On Tue, Oct 24, 2017 at 01:03:00PM +0300, Amir Goldstein wrote:
> Because overlayfs features were not implemented in the first version
> of overlayfs merged in v3.18, we cannot enforce old kernel not to mount
> overlayfs with new backward incompatible features... but we can try to
> set traps for those who try to perform those mounts.
> 
> When enabling a backward incompatible feature, we create a
> non-empty directory nested 2 levels deep under "work" dir, e.g.:
> workdir/work/incompat_features/incomapt_index.
> 
> 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_features/incompat_index' failed (-39)
>   overlayfs: cleanup of 'work/incompat_features' failed (-39)
>   overlayfs: cleanup of 'ovl-work/work' failed (-39)
>   overlayfs: failed to create directory /vdf/ovl-work/work (errno: 17);
>              mounting read-only

This sounds interesting. I am scratching my head and understand what is
backward compatibility in this case. 

So basically somebody booted a new kernel and enabled a new feature. And
now downgraded to old kernel and tried to mount again. And old kernel is
working with metadata of new kernel. To me this if forward compatibility
and not backward compatibility. (Something like ext3 being able to mount
ext4).

If yes, failing to mount using old kernel for metadata created by
newer kernel (incomatible metadata), sounds reasonable to me.

> 
> These warnings should give the hint to the user that:
> 1. mount failure is caused by backward incompatible features

Should we call this forward incomatible feature? Older kernels can't work
with metadata of index feature.

Vivek

> 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 approach to complement the backward compatibility disclaimers
> in Kconfig for enabling backward incompatible features (e.g.
> OVERLAY_FS_INDEX_INCOMAPT).
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/Kconfig     | 12 +++++++
>  fs/overlayfs/dir.c       | 36 ++++++++++++++++++++
>  fs/overlayfs/overlayfs.h | 15 +++++++-
>  fs/overlayfs/readdir.c   | 33 +++++++++++++++---
>  fs/overlayfs/super.c     | 18 +++++++++-
>  fs/overlayfs/util.c      | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 196 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index 02295e10ffbe..7c97550ba39f 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -44,3 +44,15 @@ config OVERLAY_FS_INDEX_INCOMPAT
>  	  If this config option is enabled then inodes index is declared an
>  	  incompatible feature and kernel will refuse to mount an overlay with
>  	  inodes index off when a non-empty index directory exists.
> +
> +	  After mounting an overlay filesystems once with incompatible index
> +	  enabled, mounting the overlay with a kernel that doesn't support
> +	  inodes index feature or mounting with inodes index off will either
> +	  fail to mount or fallback to read-only mount.
> +
> +	  The inodes index dir resides under the 'workdir' path provided with
> +	  overlay mount options, so it is still possible to mount the overlay
> +	  on a kernel that doesn't support incompatible index feature, by
> +	  cleaning the content of 'workdir' or by providing an alternative
> +	  'workdir' path.  In that case, mounting the same overlay again with
> +	  inodes index enabled will have unexpected results.
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 7e2f748d5a7c..63a3b848bdf0 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -159,6 +159,42 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
>  	return err;
>  }
>  
> +/* Create object if it doesn't exist */
> +struct dentry *ovl_test_create(struct dentry *parent, const char *name,
> +			       umode_t mode)
> +{
> +	struct inode *dir = parent->d_inode;
> +	struct dentry *dentry;
> +	int err;
> +
> +	inode_lock_nested(dir, I_MUTEX_PARENT);
> +
> +	dentry = lookup_one_len(name, parent, strlen(name));
> +	err = PTR_ERR(dentry);
> +	if (IS_ERR(dentry))
> +		return dentry;
> +
> +	if (!dentry->d_inode) {
> +		err = ovl_create_real(dir, dentry,
> +				      &(struct cattr){.mode = mode},
> +				      NULL, true);
> +	} else if ((mode ^ d_inode(dentry)->i_mode) & S_IFMT) {
> +		/* File exists but with wrong file type? */
> +		err = -EEXIST;
> +	} else {
> +		err = 0;
> +	}
> +
> +	inode_unlock(dir);
> +
> +	if (err) {
> +		dput(dentry);
> +		return ERR_PTR(err);
> +	}
> +
> +	return dentry;
> +}
> +
>  static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
>  			       int xerr)
>  {
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 3f9a7625bded..d1172b3f9a99 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -27,11 +27,15 @@ enum ovl_path_type {
>  #define OVL_XATTR_IMPURE OVL_XATTR_PREFIX "impure"
>  #define OVL_XATTR_NLINK OVL_XATTR_PREFIX "nlink"
>  
> +#define OVL_INCOMPAT_FEATURES_NAME "incompat_features"
> +#define OVL_FEATURE_INCOMPAT_INDEX "incompat_index"
> +
>  enum ovl_flag {
>  	OVL_IMPURE,
>  	OVL_INDEX,
>  };
>  
> +
>  /*
>   * The tuple (fh,uuid) is a universal unique identifier for a copy up origin,
>   * where:
> @@ -242,6 +246,12 @@ static inline bool ovl_is_impuredir(struct dentry *dentry)
>  	return ovl_check_dir_xattr(dentry, OVL_XATTR_IMPURE);
>  }
>  
> +bool ovl_is_features_dir(struct dentry *dentry, const char ***features);
> +bool ovl_is_feature_supported(const char *name, int namelen,
> +			      const char **features);
> +struct ovl_fs;
> +int ovl_enable_feature(struct ovl_fs *ofs, const char *dirname,
> +		       const char *name);
>  
>  /* namei.c */
>  int ovl_verify_origin(struct dentry *dentry, struct vfsmount *mnt,
> @@ -261,7 +271,8 @@ void ovl_cache_free(struct list_head *list);
>  void ovl_dir_cache_free(struct inode *inode);
>  int ovl_check_d_type_supported(struct path *realpath);
>  void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
> -			 struct dentry *dentry, int level);
> +			 struct dentry *dentry, int level,
> +			 const char **features);
>  int ovl_indexdir_cleanup(struct dentry *dentry, struct vfsmount *mnt,
>  			 struct path *lowerstack, unsigned int numlower);
>  
> @@ -311,6 +322,8 @@ struct cattr {
>  int ovl_create_real(struct inode *dir, struct dentry *newdentry,
>  		    struct cattr *attr,
>  		    struct dentry *hardlink, bool debug);
> +struct dentry *ovl_test_create(struct dentry *parent, const char *name,
> +			       umode_t mode);
>  
>  /* copy_up.c */
>  int ovl_copy_up(struct dentry *dentry);
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 698b74dd750e..53ee99ec5b1c 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -922,7 +922,8 @@ 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)
> +static void ovl_workdir_cleanup_recurse(struct path *path, int level,
> +					const char **features)
>  {
>  	int err;
>  	struct inode *dir = path->dentry->d_inode;
> @@ -950,12 +951,19 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
>  				continue;
>  			if (p->len == 2 && p->name[1] == '.')
>  				continue;
> +		} else if (features && level == 1 &&
> +			   !ovl_is_feature_supported(p->name, p->len,
> +						     features)) {
> +			pr_warn("overlayfs: feature '%.*s' not supported\n",
> +				p->len, p->name);
> +			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);
> +			ovl_workdir_cleanup(dir, path->mnt, dentry, level,
> +					    features);
>  		dput(dentry);
>  	}
>  	inode_unlock(dir);
> @@ -964,11 +972,26 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
>  }
>  
>  void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
> -			 struct dentry *dentry, int level)
> +			 struct dentry *dentry, int level,
> +			 const char **features)
>  {
>  	int err;
>  
> -	if (!d_is_dir(dentry) || level > 1) {
> +	/*
> +	 * The features directories are treated specially:
> +	 * Recursive cleanup iterates 2 levels inside features dir.
> +	 * Iteration inside features dir aborts on unsupported feature name.
> +	 * By aborting cleanup on unsupported feature name, remove of the
> +	 * features dir will fail followed by failure to remove "work" dir
> +	 * and resulting in read-only mount.
> +	 * This is a hack to force read-only mount with older kernels that
> +	 * do not know about features, but only iterate recursively 2 levels
> +	 * inside "work" dir.
> +	 */
> +	if (!features && d_is_dir(dentry) && level == 1 &&
> +	    ovl_is_features_dir(dentry, &features)) {
> +		level = 0;
> +	} else if (!d_is_dir(dentry) || level > 1) {
>  		ovl_cleanup(dir, dentry);
>  		return;
>  	}
> @@ -978,7 +1001,7 @@ void 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);
> +		ovl_workdir_cleanup_recurse(&path, level + 1, features);
>  		inode_lock_nested(dir, I_MUTEX_PARENT);
>  		ovl_cleanup(dir, dentry);
>  	}
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 0dc6d61f828a..49c6a7ad695f 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -491,7 +491,7 @@ static struct dentry *ovl_workdir_create(struct super_block *sb,
>  				goto out_unlock;
>  
>  			retried = true;
> -			ovl_workdir_cleanup(dir, mnt, work, 0);
> +			ovl_workdir_cleanup(dir, mnt, work, 0, NULL);
>  			dput(work);
>  			goto retry;
>  		}
> @@ -1089,6 +1089,22 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>  			if (err)
>  				pr_err("overlayfs: failed to verify index dir origin\n");
>  
> +			/*
> +			 * Prevent from mounting this overlay with index=off
> +			 * or with kernel without incompat index support and
> +			 * corrupting the index.
> +			 * Enable incompat index feature if verified index dir
> +			 * exists and regardless if ovl_indexdir_cleanup() will
> +			 * fail.
> +			 */
> +			if (!err && ovl_incompat_index) {
> +				err = ovl_enable_feature(ufs,
> +						 OVL_INCOMPAT_FEATURES_NAME,
> +						 OVL_FEATURE_INCOMPAT_INDEX);
> +				if (err)
> +					goto out_put_indexdir;
> +			}
> +
>  			/* Cleanup bad/stale/orphan index entries */
>  			if (!err)
>  				err = ovl_indexdir_cleanup(ufs->indexdir,
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index b9b239fa5cfd..bef850632c80 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -579,3 +579,92 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
>  	pr_err("overlayfs: failed to lock workdir+upperdir\n");
>  	return -EIO;
>  }
> +
> +static const char * const ovl_incompat_features[] = {
> +#ifdef CONFIG_OVERLAY_FS_INDEX_INCOMPAT
> +	OVL_FEATURE_INCOMPAT_INDEX,
> +#endif
> +	NULL,
> +};
> +
> +bool ovl_is_features_dir(struct dentry *dentry, const char ***features)
> +{
> +	if (!dentry || !d_is_dir(dentry))
> +		return false;
> +
> +	if (!strcmp(dentry->d_name.name, OVL_INCOMPAT_FEATURES_NAME)) {
> +		*features = (const char **)ovl_incompat_features;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +bool ovl_is_feature_supported(const char *name, int namelen,
> +			      const char **features)
> +{
> +	const char **p;
> +
> +	for (p = features; *p; p++) {
> +		if (!p[namelen] && !strncmp(name, *p, namelen))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +/*
> + * Create a non-empty directory under features dir, e.g.:
> + * work/incompat_features/incompat_index
> + */
> +static int ovl_create_feature_dir(struct dentry *dentry,  struct vfsmount *mnt,
> +				  const char *dirname, const char *name)
> +{
> +	struct dentry *features, *feature, *enabled;
> +	int err;
> +
> +	features = ovl_test_create(dentry, dirname, S_IFDIR | 0);
> +	err = PTR_ERR(features);
> +	if (IS_ERR(features))
> +		return err;
> +
> +	feature = ovl_test_create(features, name, S_IFDIR | 0);
> +	dput(features);
> +	err = PTR_ERR(feature);
> +	if (IS_ERR(feature))
> +		return err;
> +
> +	enabled = ovl_test_create(feature, "enabled", S_IFREG | 0);
> +	dput(feature);
> +	err = PTR_ERR(enabled);
> +	if (IS_ERR(enabled))
> +		return err;
> +
> +	dput(enabled);
> +
> +	return 0;
> +}
> +
> +/*
> + * Prevent kernel with no support for the enabled feature from mounting
> + * this overlay read-write and corrupting the index by creating a
> + * non-empty nested dir entires in workdir, that old kernels
> + * do not know how to clean on mount.
> + */
> +int ovl_enable_feature(struct ovl_fs *ofs, const char *dirname,
> +		       const char *name)
> +{
> +	struct vfsmount *mnt = ofs->upper_mnt;
> +	int err;
> +
> +	if (!mnt || !ofs->workdir || !ofs->indexdir)
> +		return -EROFS;
> +
> +	err = mnt_want_write(mnt);
> +	if (err)
> +		return err;
> +
> +	err = ovl_create_feature_dir(ofs->workdir, mnt, dirname, name);
> +
> +	mnt_drop_write(mnt);
> +	return err;
> +}
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] ovl: cast a shadow of incomapt index into the past

[Amir Goldstein]

On Fri, Nov 10, 2017 at 4:53 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Oct 24, 2017 at 01:03:00PM +0300, Amir Goldstein wrote:
>> Because overlayfs features were not implemented in the first version
>> of overlayfs merged in v3.18, we cannot enforce old kernel not to mount
>> overlayfs with new backward incompatible features... but we can try to
>> set traps for those who try to perform those mounts.
>>
>> When enabling a backward incompatible feature, we create a
>> non-empty directory nested 2 levels deep under "work" dir, e.g.:
>> workdir/work/incompat_features/incomapt_index.
>>
>> 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_features/incompat_index' failed (-39)
>>   overlayfs: cleanup of 'work/incompat_features' failed (-39)
>>   overlayfs: cleanup of 'ovl-work/work' failed (-39)
>>   overlayfs: failed to create directory /vdf/ovl-work/work (errno: 17);
>>              mounting read-only
>
> This sounds interesting. I am scratching my head and understand what is
> backward compatibility in this case.
>
> So basically somebody booted a new kernel and enabled a new feature. And
> now downgraded to old kernel and tried to mount again. And old kernel is
> working with metadata of new kernel. To me this if forward compatibility
> and not backward compatibility. (Something like ext3 being able to mount
> ext4).
>
> If yes, failing to mount using old kernel for metadata created by
> newer kernel (incomatible metadata), sounds reasonable to me.
>
>>
>> These warnings should give the hint to the user that:
>> 1. mount failure is caused by backward incompatible features
>
> Should we call this forward incomatible feature? Older kernels can't work
> with metadata of index feature.
>

To avoid confusion let's just call them incomap/rocompat features without
referring to forward/backward, because the terminology is not what matters.

When you are confused go to documentation of ext4/xfs and see what that
means. Its the exact same concept:
- old kernel cannot mount an fs with unknown incomapt features
- old kernel can mount only read-only, as fs with unknown rocompat features

ext4 also has "compat" features, meaning that old kernel can mount them,
but user programs can check if feature exist for whatever reasons.
e2fsck will refuse to modify fs even if it has unknown "compat" features.
xfs_repair will just warn on unknown "compat" features but will modify fs.
I did not see a reason to introduce "compat" features for overlayfs just yet,
because there are no userspace tools/lib that can check them.

Amir.

Re: [PATCH 1/4] ovl: introduce incompatible index feature

[Vivek Goyal]

On Fri, Nov 10, 2017 at 04:46:53PM +0200, Amir Goldstein wrote:
> On Fri, Nov 10, 2017 at 3:57 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Tue, Oct 24, 2017 at 01:02:58PM +0300, Amir Goldstein wrote:
> >> Introduce a new config option OVERLAY_FS_INDEX_INCOMPAT.
> >>
> >> If this config option is enabled then inodes index is declared
> >> an incompatible feature and kernel will refuse to mount an overlay
> >> with inodes index off when a non-empty index directory exists.
> >>
> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >> ---
> >>  fs/overlayfs/Kconfig     |  8 ++++++++
> >>  fs/overlayfs/dir.c       | 30 ++++++++++++++++++++++++++++++
> >>  fs/overlayfs/overlayfs.h |  3 ++-
> >>  fs/overlayfs/super.c     | 24 +++++++++++++++++++++++-
> >>  4 files changed, 63 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> >> index cbfc196e5dc5..e5e6dec7d177 100644
> >> --- a/fs/overlayfs/Kconfig
> >> +++ b/fs/overlayfs/Kconfig
> >> @@ -43,3 +43,11 @@ config OVERLAY_FS_INDEX
> >>         outcomes.  However, mounting the same overlay with an old kernel
> >>         read-write and then mounting it again with a new kernel, will have
> >>         unexpected results.
> >> +
> >> +config OVERLAY_FS_INDEX_INCOMPAT
> >> +     bool "Overlayfs: support incompatible index feature"
> >> +     depends on OVERLAY_FS_INDEX
> >> +     help
> >> +       If this config option is enabled then inodes index is declared an
> >> +       incompatible feature and kernel will refuse to mount an overlay with
> >> +       inodes index off when a non-empty index directory exists.
> >
> > Hi Amir,
> >
> > I don't know much about the issues you have faced. So I have few very
> > basic questions.
> >
> > So the problem you are trying to fix is that if somebody mounted overlay
> > with index=on and later they try to mount it with index=off.
> >
> > What problems happen if we allow that? If its a problem, why not always
> > disallow that instead of making it an option. IOW, is there a use case
> > where we will still let user mount later with index=off.
> >
> 
> So I have no current issue with going back from index=on, but I would like
> to start adding incompatible features (like index=all), but in order to add
> the concept of incompatible feature, first user will need to opt-in in compile
> time to "overlayfs v2 format" which supports the feature checks.
> opting-in to format v2 means that user knows he cannot (*) go backwards
> to kernel that doesn't support v2 format and mount v2 format overlayfs.
> 
> Actually, the user can go backwards, but he will need to manually remove
> workdir and indexdir in order to "degrade" overlayfs to v1 before mount.
> 
> I guess config option could have been
> OVERLAY_FS_INCOMAT_V2

So this does not protect against user downgrading to older kernel and
then mount with index=off.

Vivek

Re: [PATCH 1/4] ovl: introduce incompatible index feature

[Amir Goldstein]

On Wed, Nov 15, 2017 at 4:34 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Nov 10, 2017 at 04:46:53PM +0200, Amir Goldstein wrote:
>> On Fri, Nov 10, 2017 at 3:57 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Tue, Oct 24, 2017 at 01:02:58PM +0300, Amir Goldstein wrote:
>> >> Introduce a new config option OVERLAY_FS_INDEX_INCOMPAT.
>> >>
>> >> If this config option is enabled then inodes index is declared
>> >> an incompatible feature and kernel will refuse to mount an overlay
>> >> with inodes index off when a non-empty index directory exists.
>> >>
>> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> >> ---
>> >>  fs/overlayfs/Kconfig     |  8 ++++++++
>> >>  fs/overlayfs/dir.c       | 30 ++++++++++++++++++++++++++++++
>> >>  fs/overlayfs/overlayfs.h |  3 ++-
>> >>  fs/overlayfs/super.c     | 24 +++++++++++++++++++++++-
>> >>  4 files changed, 63 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
>> >> index cbfc196e5dc5..e5e6dec7d177 100644
>> >> --- a/fs/overlayfs/Kconfig
>> >> +++ b/fs/overlayfs/Kconfig
>> >> @@ -43,3 +43,11 @@ config OVERLAY_FS_INDEX
>> >>         outcomes.  However, mounting the same overlay with an old kernel
>> >>         read-write and then mounting it again with a new kernel, will have
>> >>         unexpected results.
>> >> +
>> >> +config OVERLAY_FS_INDEX_INCOMPAT
>> >> +     bool "Overlayfs: support incompatible index feature"
>> >> +     depends on OVERLAY_FS_INDEX
>> >> +     help
>> >> +       If this config option is enabled then inodes index is declared an
>> >> +       incompatible feature and kernel will refuse to mount an overlay with
>> >> +       inodes index off when a non-empty index directory exists.
>> >
>> > Hi Amir,
>> >
>> > I don't know much about the issues you have faced. So I have few very
>> > basic questions.
>> >
>> > So the problem you are trying to fix is that if somebody mounted overlay
>> > with index=on and later they try to mount it with index=off.
>> >
>> > What problems happen if we allow that? If its a problem, why not always
>> > disallow that instead of making it an option. IOW, is there a use case
>> > where we will still let user mount later with index=off.
>> >
>>
>> So I have no current issue with going back from index=on, but I would like
>> to start adding incompatible features (like index=all), but in order to add
>> the concept of incompatible feature, first user will need to opt-in in compile
>> time to "overlayfs v2 format" which supports the feature checks.
>> opting-in to format v2 means that user knows he cannot (*) go backwards
>> to kernel that doesn't support v2 format and mount v2 format overlayfs.
>>
>> Actually, the user can go backwards, but he will need to manually remove
>> workdir and indexdir in order to "degrade" overlayfs to v1 before mount.
>>
>> I guess config option could have been
>> OVERLAY_FS_INCOMAT_V2
>
> So this does not protect against user downgrading to older kernel and
> then mount with index=off.
>

In principal no, it does not, although the patch
"ovl: cast a shadow of incomapt index into the past"
does try to make it easier for admin to understand an unsupported downgrade
may be involved.

What I wanted to achieve with this config option is instead of letting
the admin take the blame for every new OVERLAY config option
and having to understand what she is signing up for with every
permutation of incompatible config options - let admin sign up
once for V2 format and understand that this format is incompatible
with V1 format (i.e. downgrade is not allowed).

Then, at flag day, kernel is upgraded to support overlay format V2,
all old overlays can still be mounted,
depending on mount options and possibly on operations (e.g. dir
rename, hardlink copy up), V2 features will be enabled (i.e. redirect_dir,
index, metacopy).

After an incompat feature is enabled on disk, that overlay can no longer
be mounted by a kernel with V2 format but without the specific feature
support. Mounting that overlay with a really old kernel (no V2 support)
may yield ro-mount if workdir was not removed.

What complicates things a bit, because we are planing feature support
this late is that we already deployed some incompatible features and
you probably want users to be able to deploy metacopy before V2
"flag day".
That is still possible to do, without the protection that incompat_V2
feature provide.
In the future, when kernel is upgraded to support V2, the first mount
with metacopy enabled, or the first lookup to notice a metacopy xattr
will mark the metacopy feature enabled and going back to
metacopy=off will no longer be possible.

Any of this make sense to you?

Amir.

P.S. for the record, w.r.t. the discussion about backward/forward
compatible terminology,  file systems terminology usually refers
to backward/forward compatible of the "on-disk format changes".
It is almost never allowed to make on-disk format changes that
are not "forward compatible", meaning that a new kernel cannot
read the old file system from disk.
Is it allowed to make "backward incompatible on-disk changes",
so old kernel cannot mount new file system, but users need to
somehow opt-in for changing the format of an existing fs, so they
know they can no longer downgrade their kernel.

When Miklos introduced redirect_dir, it was going to be an automatic
on-disk format change on kernel upgrade, but Linus did not agree
to this. Miklos also included a proposal for future proof feature set
after this first "silent format upgrade":
https://marc.info/?l=linux-unionfs&m=147739471816673&w=2
which was the inspiration to my patches