[RFC PATCH 0/4] ovl: implement overlay feature set

[RFC PATCH 0/4] ovl: implement overlay feature set

[zhangyi (F)]

Hi All,

This is overlay feature set support patches, introduce incompatable,
read-only compatable and incompatable feature set, store on the upper
root dir via "trusted.overlay.feature_[compat|ro_compat|incompat]"
xattrs.

This patch set base on Linux 4.16-rc6.

Patch 1: Implement feature xattr and mount check, refuse mount if no
         backward incompatible feature is detected.
Patch 2: Copy up feature xattr from the lower root dirs to the upper
         root dir to prevent feature lost.
Patch 3: Add three already support feature: redirect dir, index and
         nfs_export, enable them when user give enable mount option.
patch 4: Fix redircet feature once a redirect xattr is detected for
         the case of overlay was mounted in the old kernel.
(PS: Not sure this patch is necessary, maybe just give a warning message
and handle this by fsck.overlay? If necessary, check indexdir during
mount time is also necessary too).

zhangyi (F) (4):
  ovl: add feature set support
  ovl: update features on upper root dir from lower layers
  ovl: enable features according to mount options
  ovl: fix redirect feature for backward compatibility

 fs/overlayfs/namei.c     |  12 ++
 fs/overlayfs/overlayfs.h |  90 +++++++++++
 fs/overlayfs/ovl_entry.h |  18 +++
 fs/overlayfs/super.c     | 410 ++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 525 insertions(+), 5 deletions(-)

-- 
2.13.6

[RFC PATCH 1/4] ovl: add feature set support

[zhangyi (F)]

In order to distinguish the backward compatible and backward
incompatible features in overlay filesystem, we introduce
"compat/ro_compat/incompat" three kinds of feature set and
store them on the upper root dir via xattr:
"trusted.overlay.feature_[compat|ro_compat|incompat]=xxx,xxx"

Mount operation should fail if one unknown incompatible feature is
detected on the upper root dir, and read-write mount operation should
fail if one unknown ro_compat feature is detected on the upper root
dir. At the same time, lower layers may also have some backward
incompatible features when they used to be upper root dir, so do the
same check as upper root dir.

This patch doesn't add any features, so if any feature xattrs is
detected on the underlying root dir, it will be recognized as unknown
feature.

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/overlayfs/overlayfs.h |  17 +++
 fs/overlayfs/ovl_entry.h |  16 +++
 fs/overlayfs/super.c     | 302 ++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 330 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 225ff1171147..121e81cc1550 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -28,6 +28,9 @@ enum ovl_path_type {
 #define OVL_XATTR_IMPURE OVL_XATTR_PREFIX "impure"
 #define OVL_XATTR_NLINK OVL_XATTR_PREFIX "nlink"
 #define OVL_XATTR_UPPER OVL_XATTR_PREFIX "upper"
+#define OVL_XATTR_FEATURE_COMPAT OVL_XATTR_PREFIX "feature_compat"
+#define OVL_XATTR_FEATURE_RO_COMPAT OVL_XATTR_PREFIX "feature_ro_compat"
+#define OVL_XATTR_FEATURE_INCOMPAT OVL_XATTR_PREFIX "feature_incompat"
 
 enum ovl_inode_flag {
 	/* Pure upper dir that may contain non pure upper entries */
@@ -43,6 +46,20 @@ enum ovl_entry_flag {
 	OVL_E_CONNECTED,
 };
 
+/* Features */
+static inline bool ovl_has_unknown_compat_features(struct super_block *sb)
+{
+	return (OVL_FS(sb)->feature.compat_unknown != NULL);
+}
+static inline bool ovl_has_unknown_ro_compat_features(struct super_block *sb)
+{
+	return (OVL_FS(sb)->feature.ro_compat_unknown != NULL);
+}
+static inline bool ovl_has_unknown_incompat_features(struct super_block *sb)
+{
+	return (OVL_FS(sb)->feature.incompat_unknown != NULL);
+}
+
 /*
  * The tuple (fh,uuid) is a universal unique identifier for a copy up origin,
  * where:
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index bfef6edcc111..f8c5e0191a12 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -32,6 +32,15 @@ struct ovl_path {
 	struct dentry *dentry;
 };
 
+struct ovl_feature {
+	char *compat;
+	char *ro_compat;
+	char *incompat;
+	const char *compat_unknown;
+	const char *ro_compat_unknown;
+	const char *incompat_unknown;
+};
+
 /* private information held for overlayfs's superblock */
 struct ovl_fs {
 	struct vfsmount *upper_mnt;
@@ -55,6 +64,8 @@ struct ovl_fs {
 	/* Did we take the inuse lock? */
 	bool upperdir_locked;
 	bool workdir_locked;
+	/* features */
+	struct ovl_feature feature;
 };
 
 /* private information held for every overlayfs dentry */
@@ -94,6 +105,11 @@ static inline struct ovl_inode *OVL_I(struct inode *inode)
 	return container_of(inode, struct ovl_inode, vfs_inode);
 }
 
+static inline struct ovl_fs *OVL_FS(struct super_block *sb)
+{
+	return sb->s_fs_info;
+}
+
 static inline struct dentry *ovl_upperdentry_dereference(struct ovl_inode *oi)
 {
 	return READ_ONCE(oi->__upperdentry);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 7c24619ae7fc..10772ae237b4 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -184,6 +184,272 @@ static const struct dentry_operations ovl_reval_dentry_operations = {
 	.d_weak_revalidate = ovl_dentry_weak_revalidate,
 };
 
+static const char *ovl_feature_compat_supp[] = {
+	NULL
+};
+
+static const char *ovl_feature_ro_compat_supp[] = {
+	NULL
+};
+
+static const char *ovl_feature_incompat_supp[] = {
+	NULL
+};
+
+/*
+ * Get overlay feature from the real root dentry.
+ *
+ * @realroot: real underlying root dir dentry
+ * @set: feature set xattr name
+ * Return feature string if success, NULL if no feature and
+ * error number if error.
+ */
+static char *ovl_get_feature(struct dentry *realroot, const char *set)
+{
+	int res;
+	char *buf;
+
+	res = vfs_getxattr(realroot, set, NULL, 0);
+	if (res <= 0) {
+		if (res == -EOPNOTSUPP || res == -ENODATA)
+			res = 0;
+		return ERR_PTR(res);
+	}
+
+	buf = kmalloc(res + 1, GFP_KERNEL);
+	if (!buf)
+		return ERR_PTR(-ENOMEM);
+
+	res = vfs_getxattr(realroot, set, buf, res);
+	if (res <= 0) {
+		kfree(buf);
+		return ERR_PTR(res);
+	}
+
+	buf[res] = '\0';
+	return buf;
+}
+
+/*
+ * Add new feature to (or generate new) features string, set update flag
+ * if new feature added.
+ *
+ * @feature: output feature string
+ * @add: feature want to add
+ * @update: set when new feature add, no change otherwise
+ * Return 0 if success, error number otherwise.
+ */
+static int ovl_update_feature(char **feature, const char *add,
+			      bool *update)
+{
+	char *p = *feature;
+	char *tmp;
+	int size;
+
+	if (p && strstr(p, add))
+		return 0;
+
+	size = strlen(add) + 1;
+	if (p) {
+		size += strlen(p) + 1;
+		tmp = kmalloc(size, GFP_KERNEL);
+		if (!tmp)
+			return -ENOMEM;
+
+		snprintf(tmp, size, "%s,%s", p, add);
+		kfree(p);
+	} else {
+		tmp = kmalloc(size, GFP_KERNEL);
+		if (!tmp)
+			return -ENOMEM;
+
+		strncpy(tmp, add, size);
+	}
+
+	if (update)
+		*update = true;
+	*feature = tmp;
+	return 0;
+}
+
+/*
+ * Parse features and split out unknown features, set update flag
+ * if new feature added.
+ *
+ * @features: input feature string which need to parse
+ * @supp: support feature list
+ * @feature: output feature string
+ * @unknown: unknown feature string
+ * @update: set when new feature add, no change otherwise
+ * Return 0 if success, errno otherwise.
+ */
+static int ovl_parse_feature(char *features, const char *supp[],
+			     char **feature, const char **unknown,
+			     bool *update)
+{
+	const char **fs;
+	char *tmpf = NULL, *tmpu = NULL;
+	char *p;
+	int err = 0;
+
+	if (!features)
+		return 0;
+
+	while ((p = strsep(&features, ",")) != NULL) {
+		/* Add to or generate parsed feature string */
+		err = ovl_update_feature(&tmpf, p, update);
+		if (err)
+			goto err;
+
+		/* Check support or not */
+		for (fs = supp; *fs; fs++) {
+			if (strcmp(p, *fs) == 0)
+				break;
+		}
+
+		if (*fs)
+			continue;
+
+		/* Non-supported feature found, add to unknown */
+		err = ovl_update_feature(&tmpu, p, NULL);
+		if (err)
+			goto err;
+	}
+
+	*feature = tmpf;
+	*unknown = tmpu;
+	return 0;
+err:
+	kfree(tmpf);
+	kfree(tmpu);
+	return err;
+}
+
+/*
+ * Get one set of features from the underlying root dir and parse
+ * them to get the unknown features.
+ *
+ * @root: overlay root dentry
+ * @set: feature set xattr name
+ * @supp: supported feature list
+ * @feature: output features string
+ * @unknown: unknown features string
+ * Return 0 if success, errno otherwise.
+ */
+static int ovl_get_feature_set(struct dentry *root, const char *set,
+			       const char *supp[], char **feature,
+			       const char **unknown)
+{
+	struct dentry *upperroot = ovl_dentry_upper(root);
+	struct ovl_entry *oe = root->d_fsdata;
+	char *buf = NULL;
+	int i;
+	int err;
+
+	WARN_ON(*feature || *unknown);
+
+	/* Get features from the upper root dir */
+	if (upperroot) {
+		buf = ovl_get_feature(upperroot, set);
+		if (IS_ERR(buf))
+			return PTR_ERR(buf);
+		if (!buf)
+			goto get_lower;
+
+		err = ovl_parse_feature(buf, supp, feature,
+					unknown, NULL);
+		kfree(buf);
+		if (err)
+			return err;
+	}
+
+get_lower:
+	/* Get features from each lower root dir beside the bottom layer */
+	for (i = 0; i < oe->numlower-1; i++) {
+		buf = ovl_get_feature(oe->lowerstack[i].dentry, set);
+		if (IS_ERR(buf))
+			return PTR_ERR(buf);
+		if (!buf)
+			continue;
+
+		err = ovl_parse_feature(buf, supp, feature,
+					unknown, NULL);
+		kfree(buf);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+/*
+ * Get all features from the upper and lower root dir and parse them to get
+ * unknown features.
+ *
+ * @root: overlay root dir dentry
+ * @of: overlay feature set want to init
+ * Return 0 if success, errno otherwise.
+ */
+static int ovl_init_features(struct dentry *root, struct ovl_feature *of)
+{
+	int err;
+
+	err = ovl_get_feature_set(root, OVL_XATTR_FEATURE_COMPAT,
+				  ovl_feature_compat_supp, &of->compat,
+				  &of->compat_unknown);
+	if (err)
+		return err;
+
+	err = ovl_get_feature_set(root, OVL_XATTR_FEATURE_RO_COMPAT,
+				  ovl_feature_ro_compat_supp, &of->ro_compat,
+				  &of->ro_compat_unknown);
+	if (err)
+		return err;
+
+	err = ovl_get_feature_set(root, OVL_XATTR_FEATURE_INCOMPAT,
+				  ovl_feature_incompat_supp, &of->incompat,
+				  &of->incompat_unknown);
+	return err;
+}
+
+/*
+ * Check whether this filesystem can be mounted based on
+ * the features present and the read-only/read-write mount requested.
+ * Returns true if this filesystem can be mounted as requested,
+ * false if it cannot be.
+ */
+static bool ovl_check_feature_ok(struct super_block *sb, bool readonly)
+{
+	struct ovl_feature *of = &OVL_FS(sb)->feature;
+
+	if (ovl_has_unknown_incompat_features(sb)) {
+		pr_err("overlayfs: unknown optional incompat features "
+		       "enabled: %s\n", of->incompat_unknown);
+		pr_err("overlayfs: filesystem cannot not be safely mounted "
+		       "by this kernel\n");
+		return false;
+	}
+
+	if (ovl_has_unknown_ro_compat_features(sb)) {
+		pr_err("overlayfs: unknown optional ro_compat features "
+		       "enabled: %s\n", of->ro_compat_unknown);
+
+		if (readonly)
+			return true;
+
+		pr_err("overlayfs: filesystem cannot not be safely mounted "
+		       "read-write by this kernel\n");
+		return false;
+	}
+
+	if (ovl_has_unknown_compat_features(sb)) {
+		pr_warn("overlayfs: unknown optional compat features "
+		        "enabled: %s\n", of->compat_unknown);
+	}
+
+	return true;
+}
+
 static struct kmem_cache *ovl_inode_cachep;
 
 static struct inode *ovl_alloc_inode(struct super_block *sb)
@@ -242,6 +508,13 @@ static void ovl_free_fs(struct ovl_fs *ofs)
 	}
 	kfree(ofs->lower_layers);
 
+	kfree(ofs->feature.compat);
+	kfree(ofs->feature.ro_compat);
+	kfree(ofs->feature.incompat);
+	kfree(ofs->feature.compat_unknown);
+	kfree(ofs->feature.ro_compat_unknown);
+	kfree(ofs->feature.incompat_unknown);
+
 	kfree(ofs->config.lowerdir);
 	kfree(ofs->config.upperdir);
 	kfree(ofs->config.workdir);
@@ -357,7 +630,8 @@ static int ovl_remount(struct super_block *sb, int *flags, char *data)
 {
 	struct ovl_fs *ofs = sb->s_fs_info;
 
-	if (!(*flags & SB_RDONLY) && ovl_force_readonly(ofs))
+	if (!(*flags & SB_RDONLY) &&
+	     (ovl_force_readonly(ofs) || !ovl_check_feature_ok(sb, false)))
 		return -EROFS;
 
 	return 0;
@@ -902,6 +1176,7 @@ static const struct xattr_handler *ovl_xattr_handlers[] = {
 	NULL
 };
 
+
 static int ovl_get_upper(struct ovl_fs *ofs, struct path *upperpath)
 {
 	struct vfsmount *upper_mnt;
@@ -1284,11 +1559,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
 		err = ovl_get_upper(ofs, &upperpath);
 		if (err)
-			goto out_err;
+			goto out_free_upper;
 
 		err = ovl_get_workdir(ofs, &upperpath);
 		if (err)
-			goto out_err;
+			goto out_free_upper;
 
 		if (!ofs->workdir)
 			sb->s_flags |= SB_RDONLY;
@@ -1300,7 +1575,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	oe = ovl_get_lowerstack(sb, ofs);
 	err = PTR_ERR(oe);
 	if (IS_ERR(oe))
-		goto out_err;
+		goto out_free_upper;
 
 	/* If the upper fs is nonexistent, we mark overlayfs r/o too */
 	if (!ofs->upper_mnt)
@@ -1365,13 +1640,30 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
 	sb->s_root = root_dentry;
 
+	/*
+	 * Get features from the upper and lower root dir, and check
+	 * them could be mounted properly by this kernel.
+	 */
+	err = ovl_init_features(sb->s_root, &ofs->feature);
+	if (err)
+		goto out_free_root;
+
+	err = -EINVAL;
+	if (!ovl_check_feature_ok(sb, (sb_rdonly(sb))))
+		goto out_free_root;
+
 	return 0;
 
+out_free_root:
+	dput(sb->s_root);
+	sb->s_root = NULL;
+	goto out_err;
 out_free_oe:
 	ovl_entry_stack_free(oe);
 	kfree(oe);
-out_err:
+out_free_upper:
 	path_put(&upperpath);
+out_err:
 	ovl_free_fs(ofs);
 out:
 	return err;
-- 
2.13.6

[RFC PATCH 2/4] ovl: update features on upper root dir from lower layers

[zhangyi (F)]

If user mount overlay with lower dirs which used to be upper root dirs
at previous mount of overlay, maybe some features was enabled and the
the lower root dir may contain feature xattr, so features on the lower
layers maybe more than that on the upper root dir. In that case, we
should copy up (or append) all features from the lower layers to the
upper root dir during mount time.

This patch doesn't add any features. After this patch, the upper root
dir will contain all features on every lower root dir after mount.

eg:
upper: "trusted.overlay.feature_compat=a,b,c,d"
lower0: "trusted.overlay.feature_compat=a,b,c"
lower1: "trusted.overlay.feature_compat=b,c,d"

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/overlayfs/super.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 10772ae237b4..19b7d0869fbd 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -231,6 +231,31 @@ static char *ovl_get_feature(struct dentry *realroot, const char *set)
 }
 
 /*
+ * Set overlay feature on the upper root dentry.
+ *
+ * @root: overlay root dir
+ * @upper: upper root dir
+ * @set: feature set xattr name
+ * @feature: feature string
+ * Return 0 if success, errorno otherwise.
+ */
+static int ovl_set_feature(struct dentry *root, struct dentry *upper,
+			   const char *set, const char *feature)
+{
+	int err;
+
+	err = ovl_want_write(root);
+	if (err)
+		return err;
+
+	err = ovl_check_setxattr(root, upper, set, feature,
+				 strlen(feature), 0);
+
+	ovl_drop_write(root);
+	return err;
+}
+
+/*
  * Add new feature to (or generate new) features string, set update flag
  * if new feature added.
  *
@@ -344,6 +369,7 @@ static int ovl_get_feature_set(struct dentry *root, const char *set,
 	struct ovl_entry *oe = root->d_fsdata;
 	char *buf = NULL;
 	int i;
+	bool update = false;
 	int err;
 
 	WARN_ON(*feature || *unknown);
@@ -373,12 +399,23 @@ static int ovl_get_feature_set(struct dentry *root, const char *set,
 			continue;
 
 		err = ovl_parse_feature(buf, supp, feature,
-					unknown, NULL);
+					unknown, &update);
 		kfree(buf);
 		if (err)
 			return err;
 	}
 
+	/*
+	 * If features on the lower layers are more than that on the
+	 * upper dir, update them.
+	 */
+	if (upperroot && update) {
+		err = ovl_set_feature(root, ovl_dentry_upper(root),
+				      set, *feature);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
-- 
2.13.6

[RFC PATCH 3/4] ovl: enable features according to mount options

[zhangyi (F)]

Unlike common disk filesystems, overlay filesystem enable features
through mount options(e.g. -o index=on will enable index feature),
so it's a good time to save enabled features on the upper root dir
according to mount options. The given feature will be enabled when
user give option "-o xxx=on" during mount time.

This patch add three already support features: redirect_dir, index
and nfs_export, the first one belongs to the incompatable feature set,
and the other two belong to the ro_compatable feature set.

eg:
mount -t overlay -olowerdir=lower,upperdir=upper,workdir=work \
  -oredirect_dir=on,index=on,nfs_export=on overlay merge

trusted.overlay.feature_ro_compat="index,nfs_export"
trusted.overlay.feature_incompat="redirect_dir"

This patch also introduce three sets of feature check/set helper
functions ovl_has_xxx_feature() and ovl_set_xxx_feature() which
learn from ext4 filesystem driver.

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/overlayfs/overlayfs.h | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
 fs/overlayfs/ovl_entry.h |  2 ++
 fs/overlayfs/super.c     | 71 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 146 insertions(+)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 121e81cc1550..53ed4e73f889 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -47,6 +47,79 @@ enum ovl_entry_flag {
 };
 
 /* Features */
+enum ovl_feature_flag {
+	OVL_FEATURE_COMPAT,
+	OVL_FEATURE_RO_COMPAT,
+	OVL_FEATURE_INCOMPAT,
+	OVL_FEATURE_MAX,
+};
+
+#define OVL_FEATURE_RO_COMPAT_INDEX		"index"
+#define OVL_FEATURE_RO_COMPAT_NFS_EXPORT	"nfs_export"
+
+#define OVL_FEATURE_INCOMPAT_REDIRECT_DIR	"redirect_dir"
+
+int ovl_set_feature_set(struct super_block *sb,
+			enum ovl_feature_flag flag, const char *new);
+
+#define OVL_FEATURE_COMPAT_FUNCS(name, flagname) \
+static inline bool ovl_has_feature_##name(struct super_block *sb) \
+{ \
+	bool exist;\
+	down_read(&OVL_FS(sb)->feature.lock); \
+	exist = ((OVL_FS(sb)->feature.compat) && \
+		(strstr(OVL_FS(sb)->feature.compat, \
+			OVL_FEATURE_COMPAT_##flagname))); \
+	up_read(&OVL_FS(sb)->feature.lock); \
+	return exist; \
+} \
+static inline int ovl_set_feature_##name(struct super_block *sb) \
+{ \
+	return ovl_set_feature_set(sb, OVL_FEATURE_COMPAT, \
+			OVL_FEATURE_COMPAT_##flagname); \
+} \
+
+#define OVL_FEATURE_RO_COMPAT_FUNCS(name, flagname) \
+static inline bool ovl_has_feature_##name(struct super_block *sb) \
+{ \
+	bool exist;\
+	down_read(&OVL_FS(sb)->feature.lock); \
+	exist = ((OVL_FS(sb)->feature.ro_compat) && \
+		(strstr(OVL_FS(sb)->feature.ro_compat, \
+			OVL_FEATURE_RO_COMPAT_##flagname))); \
+	up_read(&OVL_FS(sb)->feature.lock); \
+	return exist; \
+} \
+static inline int ovl_set_feature_##name(struct super_block *sb) \
+{ \
+	return ovl_set_feature_set(sb, OVL_FEATURE_RO_COMPAT, \
+			OVL_FEATURE_RO_COMPAT_##flagname); \
+} \
+
+#define OVL_FEATURE_INCOMPAT_FUNCS(name, flagname) \
+static inline bool ovl_has_feature_##name(struct super_block *sb) \
+{ \
+	bool exist;\
+	down_read(&OVL_FS(sb)->feature.lock); \
+	exist = ((OVL_FS(sb)->feature.incompat) && \
+		(strstr(OVL_FS(sb)->feature.incompat, \
+			OVL_FEATURE_INCOMPAT_##flagname))); \
+	up_read(&OVL_FS(sb)->feature.lock); \
+	return exist; \
+} \
+static inline int ovl_set_feature_##name(struct super_block *sb) \
+{ \
+	return ovl_set_feature_set(sb, OVL_FEATURE_INCOMPAT, \
+			OVL_FEATURE_INCOMPAT_##flagname); \
+} \
+
+
+OVL_FEATURE_RO_COMPAT_FUNCS(index,		INDEX)
+OVL_FEATURE_RO_COMPAT_FUNCS(nfs_export,		NFS_EXPORT)
+
+OVL_FEATURE_INCOMPAT_FUNCS(redirect_dir,	REDIRECT_DIR)
+
+
 static inline bool ovl_has_unknown_compat_features(struct super_block *sb)
 {
 	return (OVL_FS(sb)->feature.compat_unknown != NULL);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index f8c5e0191a12..ff8bf783a097 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -36,6 +36,8 @@ struct ovl_feature {
 	char *compat;
 	char *ro_compat;
 	char *incompat;
+	struct rw_semaphore lock;	/* protect feature string */
+
 	const char *compat_unknown;
 	const char *ro_compat_unknown;
 	const char *incompat_unknown;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 19b7d0869fbd..4787f9356dfd 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -189,13 +189,22 @@ static const char *ovl_feature_compat_supp[] = {
 };
 
 static const char *ovl_feature_ro_compat_supp[] = {
+	OVL_FEATURE_RO_COMPAT_INDEX,
+	OVL_FEATURE_RO_COMPAT_NFS_EXPORT,
 	NULL
 };
 
 static const char *ovl_feature_incompat_supp[] = {
+	OVL_FEATURE_INCOMPAT_REDIRECT_DIR,
 	NULL
 };
 
+static const char *ovl_feature_set[] = {
+	OVL_XATTR_FEATURE_COMPAT,
+	OVL_XATTR_FEATURE_RO_COMPAT,
+	OVL_XATTR_FEATURE_INCOMPAT
+};
+
 /*
  * Get overlay feature from the real root dentry.
  *
@@ -431,6 +440,8 @@ static int ovl_init_features(struct dentry *root, struct ovl_feature *of)
 {
 	int err;
 
+	init_rwsem(&of->lock);
+
 	err = ovl_get_feature_set(root, OVL_XATTR_FEATURE_COMPAT,
 				  ovl_feature_compat_supp, &of->compat,
 				  &of->compat_unknown);
@@ -487,6 +498,49 @@ static bool ovl_check_feature_ok(struct super_block *sb, bool readonly)
 	return true;
 }
 
+/*
+ * Set one specified feature to the upper root dir
+ *
+ * @sb: overlay super block
+ * @flag: feature set flag, compat/ro compat/incompat
+ * @new: new feature want to set
+ * Return 0 if success, errno otherwise.
+ */
+int ovl_set_feature_set(struct super_block *sb,
+			enum ovl_feature_flag flag, const char *new)
+{
+	struct ovl_feature *of = &OVL_FS(sb)->feature;
+	bool update = false;
+	char **feature;
+	int err;
+
+	/* Don't set feature if no upper layer */
+	if (!OVL_FS(sb)->upper_mnt)
+		return 0;
+
+	down_write(&of->lock);
+	if (flag == OVL_FEATURE_COMPAT)
+		feature = &of->compat;
+	else if (flag == OVL_FEATURE_RO_COMPAT)
+		feature = &of->ro_compat;
+	else
+		feature = &of->incompat;
+
+	err = ovl_update_feature(feature, new, &update);
+	if (err)
+		goto out;
+	if (!update)
+		goto out;
+
+	/* Set updated compat features to upper root dir */
+	err = ovl_set_feature(sb->s_root, ovl_dentry_upper(sb->s_root),
+			      ovl_feature_set[flag],  *feature);
+out:
+	up_write(&of->lock);
+	return err;
+}
+
+
 static struct kmem_cache *ovl_inode_cachep;
 
 static struct inode *ovl_alloc_inode(struct super_block *sb)
@@ -1689,6 +1743,23 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	if (!ovl_check_feature_ok(sb, (sb_rdonly(sb))))
 		goto out_free_root;
 
+	/* Turn on features according to mount options */
+	if (ofs->config.index && !ovl_has_feature_index(sb)) {
+		err = ovl_set_feature_index(sb);
+		if (err)
+			goto out_free_root;
+	}
+	if (ofs->config.nfs_export && !ovl_has_feature_nfs_export(sb)) {
+		err = ovl_set_feature_nfs_export(sb);
+		if (err)
+			goto out_free_root;
+	}
+	if (ofs->config.redirect_dir && !ovl_has_feature_redirect_dir(sb)) {
+		err = ovl_set_feature_redirect_dir(sb);
+		if (err)
+			goto out_free_root;
+	}
+
 	return 0;
 
 out_free_root:
-- 
2.13.6

[RFC PATCH 4/4] ovl: fix redirect feature for backward compatibility

[zhangyi (F)]

For a redirect dir was created before the overlayfs support
"feature set", upper root dir does not contain the redirect dir
feature xattr. In that case, we cannot detect this feature during
mount time, so fix this feature set once a redirect xattr is detected
in ovl_lookup().

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/overlayfs/namei.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 70fcfcc684cc..9325c5a870c0 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -914,6 +914,18 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		ctr++;
 
 		/*
+		 * Detect redirect xattr but not enable this feature, this
+		 * redirect maybe set by the old overlay driver, fix this
+		 * feature for backward compatibility.
+		 */
+		if (d.redirect && !ofs->config.redirect_dir &&
+		    !ovl_has_feature_redirect_dir(dentry->d_sb)) {
+			err = ovl_set_feature_redirect_dir(dentry->d_sb);
+			if (err)
+				goto out_put;
+		}
+
+		/*
 		 * 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
-- 
2.13.6

Re: [RFC PATCH 2/4] ovl: update features on upper root dir from lower layers

[Amir Goldstein]

On Thu, Mar 22, 2018 at 1:35 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> If user mount overlay with lower dirs which used to be upper root dirs
> at previous mount of overlay, maybe some features was enabled and the
> the lower root dir may contain feature xattr, so features on the lower
> layers maybe more than that on the upper root dir. In that case, we
> should copy up (or append) all features from the lower layers to the
> upper root dir during mount time.

This logic is questionable:
- if lower has redirect feature, but overlay was never mounted with redirect=on
  then upper is free of redirects. by copying the feature to upper
layer we loose
  information
- if lower has index/nfs_export feature from the time it was upper, this feature
  flag is useless now, because where is the index dir from the time that lower
  was upper? we don't use it at all. So inheriting those features to upper is
  weird.

I see why we need to calculate composite layers features on mount time,
but don't see why we need to update the upper feature xattrs

>
> This patch doesn't add any features. After this patch, the upper root
> dir will contain all features on every lower root dir after mount.
>
> eg:
> upper: "trusted.overlay.feature_compat=a,b,c,d"
> lower0: "trusted.overlay.feature_compat=a,b,c"
> lower1: "trusted.overlay.feature_compat=b,c,d"
>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> ---
>  fs/overlayfs/super.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 10772ae237b4..19b7d0869fbd 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -231,6 +231,31 @@ static char *ovl_get_feature(struct dentry *realroot, const char *set)
>  }
>
>  /*
> + * Set overlay feature on the upper root dentry.
> + *
> + * @root: overlay root dir
> + * @upper: upper root dir
> + * @set: feature set xattr name
> + * @feature: feature string
> + * Return 0 if success, errorno otherwise.
> + */
> +static int ovl_set_feature(struct dentry *root, struct dentry *upper,
> +                          const char *set, const char *feature)
> +{
> +       int err;
> +
> +       err = ovl_want_write(root);
> +       if (err)
> +               return err;
> +
> +       err = ovl_check_setxattr(root, upper, set, feature,
> +                                strlen(feature), 0);
> +
> +       ovl_drop_write(root);
> +       return err;
> +}
> +
> +/*
>   * Add new feature to (or generate new) features string, set update flag
>   * if new feature added.
>   *
> @@ -344,6 +369,7 @@ static int ovl_get_feature_set(struct dentry *root, const char *set,
>         struct ovl_entry *oe = root->d_fsdata;
>         char *buf = NULL;
>         int i;
> +       bool update = false;
>         int err;
>
>         WARN_ON(*feature || *unknown);
> @@ -373,12 +399,23 @@ static int ovl_get_feature_set(struct dentry *root, const char *set,
>                         continue;
>
>                 err = ovl_parse_feature(buf, supp, feature,
> -                                       unknown, NULL);
> +                                       unknown, &update);
>                 kfree(buf);
>                 if (err)
>                         return err;
>         }
>
> +       /*
> +        * If features on the lower layers are more than that on the
> +        * upper dir, update them.
> +        */
> +       if (upperroot && update) {
> +               err = ovl_set_feature(root, ovl_dentry_upper(root),
> +                                     set, *feature);
> +               if (err)
> +                       return err;
> +       }
> +
>         return 0;
>  }
>
> --
> 2.13.6
>

Re: [RFC PATCH 1/4] ovl: add feature set support

[Amir Goldstein]

On Thu, Mar 22, 2018 at 1:35 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> In order to distinguish the backward compatible and backward
> incompatible features in overlay filesystem, we introduce
> "compat/ro_compat/incompat" three kinds of feature set and
> store them on the upper root dir via xattr:
> "trusted.overlay.feature_[compat|ro_compat|incompat]=xxx,xxx"
>
> Mount operation should fail if one unknown incompatible feature is
> detected on the upper root dir, and read-write mount operation should
> fail if one unknown ro_compat feature is detected on the upper root
> dir. At the same time, lower layers may also have some backward
> incompatible features when they used to be upper root dir, so do the
> same check as upper root dir.
>
> This patch doesn't add any features, so if any feature xattrs is
> detected on the underlying root dir, it will be recognized as unknown
> feature.
>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>

To be fair, I *suggested* to use Miklos' suggestion...

> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> ---
>  fs/overlayfs/overlayfs.h |  17 +++
>  fs/overlayfs/ovl_entry.h |  16 +++
>  fs/overlayfs/super.c     | 302 ++++++++++++++++++++++++++++++++++++++++++++++-

This series adds a lot of code to super.c which is big enough already
I suggest to start features.c.


>  3 files changed, 330 insertions(+), 5 deletions(-)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 225ff1171147..121e81cc1550 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -28,6 +28,9 @@ enum ovl_path_type {
>  #define OVL_XATTR_IMPURE OVL_XATTR_PREFIX "impure"
>  #define OVL_XATTR_NLINK OVL_XATTR_PREFIX "nlink"
>  #define OVL_XATTR_UPPER OVL_XATTR_PREFIX "upper"
> +#define OVL_XATTR_FEATURE_COMPAT OVL_XATTR_PREFIX "feature_compat"
> +#define OVL_XATTR_FEATURE_RO_COMPAT OVL_XATTR_PREFIX "feature_ro_compat"
> +#define OVL_XATTR_FEATURE_INCOMPAT OVL_XATTR_PREFIX "feature_incompat"
>
>  enum ovl_inode_flag {
>         /* Pure upper dir that may contain non pure upper entries */
> @@ -43,6 +46,20 @@ enum ovl_entry_flag {
>         OVL_E_CONNECTED,
>  };
>
> +/* Features */
> +static inline bool ovl_has_unknown_compat_features(struct super_block *sb)
> +{
> +       return (OVL_FS(sb)->feature.compat_unknown != NULL);
> +}
> +static inline bool ovl_has_unknown_ro_compat_features(struct super_block *sb)
> +{
> +       return (OVL_FS(sb)->feature.ro_compat_unknown != NULL);
> +}
> +static inline bool ovl_has_unknown_incompat_features(struct super_block *sb)
> +{
> +       return (OVL_FS(sb)->feature.incompat_unknown != NULL);
> +}
> +
>  /*
>   * The tuple (fh,uuid) is a universal unique identifier for a copy up origin,
>   * where:
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index bfef6edcc111..f8c5e0191a12 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -32,6 +32,15 @@ struct ovl_path {
>         struct dentry *dentry;
>  };
>
> +struct ovl_feature {
> +       char *compat;
> +       char *ro_compat;
> +       char *incompat;
> +       const char *compat_unknown;
> +       const char *ro_compat_unknown;
> +       const char *incompat_unknown;
> +};
> +

IMO those should be 64bit bit flags, like in other fs,
see more below.

>  /* private information held for overlayfs's superblock */
>  struct ovl_fs {
>         struct vfsmount *upper_mnt;
> @@ -55,6 +64,8 @@ struct ovl_fs {
>         /* Did we take the inuse lock? */
>         bool upperdir_locked;
>         bool workdir_locked;
> +       /* features */
> +       struct ovl_feature feature;
>  };
>
>  /* private information held for every overlayfs dentry */
> @@ -94,6 +105,11 @@ static inline struct ovl_inode *OVL_I(struct inode *inode)
>         return container_of(inode, struct ovl_inode, vfs_inode);
>  }
>
> +static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> +{
> +       return sb->s_fs_info;
> +}
> +
>  static inline struct dentry *ovl_upperdentry_dereference(struct ovl_inode *oi)
>  {
>         return READ_ONCE(oi->__upperdentry);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 7c24619ae7fc..10772ae237b4 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -184,6 +184,272 @@ static const struct dentry_operations ovl_reval_dentry_operations = {
>         .d_weak_revalidate = ovl_dentry_weak_revalidate,
>  };
>
> +static const char *ovl_feature_compat_supp[] = {
> +       NULL
> +};
> +
> +static const char *ovl_feature_ro_compat_supp[] = {
> +       NULL
> +};
> +
> +static const char *ovl_feature_incompat_supp[] = {
> +       NULL
> +};
> +
> +/*
> + * Get overlay feature from the real root dentry.
> + *
> + * @realroot: real underlying root dir dentry
> + * @set: feature set xattr name
> + * Return feature string if success, NULL if no feature and
> + * error number if error.
> + */
> +static char *ovl_get_feature(struct dentry *realroot, const char *set)
> +{
> +       int res;
> +       char *buf;
> +
> +       res = vfs_getxattr(realroot, set, NULL, 0);
> +       if (res <= 0) {
> +               if (res == -EOPNOTSUPP || res == -ENODATA)
> +                       res = 0;
> +               return ERR_PTR(res);
> +       }
> +
> +       buf = kmalloc(res + 1, GFP_KERNEL);
> +       if (!buf)
> +               return ERR_PTR(-ENOMEM);
> +
> +       res = vfs_getxattr(realroot, set, buf, res);
> +       if (res <= 0) {
> +               kfree(buf);
> +               return ERR_PTR(res);
> +       }
> +
> +       buf[res] = '\0';
> +       return buf;
> +}
> +
> +/*
> + * Add new feature to (or generate new) features string, set update flag
> + * if new feature added.
> + *
> + * @feature: output feature string
> + * @add: feature want to add
> + * @update: set when new feature add, no change otherwise
> + * Return 0 if success, error number otherwise.
> + */
> +static int ovl_update_feature(char **feature, const char *add,
> +                             bool *update)
> +{
> +       char *p = *feature;
> +       char *tmp;
> +       int size;
> +
> +       if (p && strstr(p, add))
> +               return 0;
> +
> +       size = strlen(add) + 1;
> +       if (p) {
> +               size += strlen(p) + 1;
> +               tmp = kmalloc(size, GFP_KERNEL);
> +               if (!tmp)
> +                       return -ENOMEM;
> +
> +               snprintf(tmp, size, "%s,%s", p, add);
> +               kfree(p);
> +       } else {
> +               tmp = kmalloc(size, GFP_KERNEL);
> +               if (!tmp)
> +                       return -ENOMEM;
> +
> +               strncpy(tmp, add, size);
> +       }
> +
> +       if (update)
> +               *update = true;
> +       *feature = tmp;
> +       return 0;
> +}
> +
> +/*
> + * Parse features and split out unknown features, set update flag
> + * if new feature added.
> + *
> + * @features: input feature string which need to parse
> + * @supp: support feature list
> + * @feature: output feature string
> + * @unknown: unknown feature string
> + * @update: set when new feature add, no change otherwise
> + * Return 0 if success, errno otherwise.
> + */
> +static int ovl_parse_feature(char *features, const char *supp[],
> +                            char **feature, const char **unknown,
> +                            bool *update)
> +{
> +       const char **fs;
> +       char *tmpf = NULL, *tmpu = NULL;
> +       char *p;
> +       int err = 0;
> +
> +       if (!features)
> +               return 0;
> +
> +       while ((p = strsep(&features, ",")) != NULL) {
> +               /* Add to or generate parsed feature string */
> +               err = ovl_update_feature(&tmpf, p, update);
> +               if (err)
> +                       goto err;
> +
> +               /* Check support or not */
> +               for (fs = supp; *fs; fs++) {
> +                       if (strcmp(p, *fs) == 0)
> +                               break;
> +               }
> +
> +               if (*fs)
> +                       continue;
> +
> +               /* Non-supported feature found, add to unknown */
> +               err = ovl_update_feature(&tmpu, p, NULL);
> +               if (err)
> +                       goto err;
> +       }
> +
> +       *feature = tmpf;
> +       *unknown = tmpu;
> +       return 0;
> +err:
> +       kfree(tmpf);
> +       kfree(tmpu);
> +       return err;
> +}
> +

The past ~200 lines are a re-implementation (probably buggy) of tokenize
and for what? saving the features as strings in xattrs is questionable -
it serves no purpose other than human convenience when reading features xattrs.
We can store a 64bit binary flags blob in big endian order, just like
any other fs,
and then we need no parsing, no string find and no string insertion helpers.

Not to mention that some fs have limited size of xattr, which is going
to limit us in
the future with the amount of features and feature name lengths.



> +/*
> + * Get one set of features from the underlying root dir and parse
> + * them to get the unknown features.
> + *
> + * @root: overlay root dentry
> + * @set: feature set xattr name
> + * @supp: supported feature list
> + * @feature: output features string
> + * @unknown: unknown features string
> + * Return 0 if success, errno otherwise.
> + */
> +static int ovl_get_feature_set(struct dentry *root, const char *set,
> +                              const char *supp[], char **feature,
> +                              const char **unknown)
> +{
> +       struct dentry *upperroot = ovl_dentry_upper(root);
> +       struct ovl_entry *oe = root->d_fsdata;
> +       char *buf = NULL;
> +       int i;
> +       int err;
> +
> +       WARN_ON(*feature || *unknown);
> +
> +       /* Get features from the upper root dir */
> +       if (upperroot) {
> +               buf = ovl_get_feature(upperroot, set);
> +               if (IS_ERR(buf))
> +                       return PTR_ERR(buf);
> +               if (!buf)
> +                       goto get_lower;
> +
> +               err = ovl_parse_feature(buf, supp, feature,
> +                                       unknown, NULL);
> +               kfree(buf);
> +               if (err)
> +                       return err;
> +       }
> +
> +get_lower:
> +       /* Get features from each lower root dir beside the bottom layer */
> +       for (i = 0; i < oe->numlower-1; i++) {
> +               buf = ovl_get_feature(oe->lowerstack[i].dentry, set);
> +               if (IS_ERR(buf))
> +                       return PTR_ERR(buf);
> +               if (!buf)
> +                       continue;
> +
> +               err = ovl_parse_feature(buf, supp, feature,
> +                                       unknown, NULL);
> +               kfree(buf);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * Get all features from the upper and lower root dir and parse them to get
> + * unknown features.
> + *
> + * @root: overlay root dir dentry
> + * @of: overlay feature set want to init
> + * Return 0 if success, errno otherwise.
> + */
> +static int ovl_init_features(struct dentry *root, struct ovl_feature *of)
> +{
> +       int err;
> +
> +       err = ovl_get_feature_set(root, OVL_XATTR_FEATURE_COMPAT,
> +                                 ovl_feature_compat_supp, &of->compat,
> +                                 &of->compat_unknown);
> +       if (err)
> +               return err;
> +
> +       err = ovl_get_feature_set(root, OVL_XATTR_FEATURE_RO_COMPAT,
> +                                 ovl_feature_ro_compat_supp, &of->ro_compat,
> +                                 &of->ro_compat_unknown);
> +       if (err)
> +               return err;
> +
> +       err = ovl_get_feature_set(root, OVL_XATTR_FEATURE_INCOMPAT,
> +                                 ovl_feature_incompat_supp, &of->incompat,
> +                                 &of->incompat_unknown);
> +       return err;
> +}
> +
> +/*
> + * Check whether this filesystem can be mounted based on
> + * the features present and the read-only/read-write mount requested.
> + * Returns true if this filesystem can be mounted as requested,
> + * false if it cannot be.
> + */
> +static bool ovl_check_feature_ok(struct super_block *sb, bool readonly)
> +{
> +       struct ovl_feature *of = &OVL_FS(sb)->feature;
> +
> +       if (ovl_has_unknown_incompat_features(sb)) {
> +               pr_err("overlayfs: unknown optional incompat features "
> +                      "enabled: %s\n", of->incompat_unknown);
> +               pr_err("overlayfs: filesystem cannot not be safely mounted "
> +                      "by this kernel\n");
> +               return false;
> +       }
> +
> +       if (ovl_has_unknown_ro_compat_features(sb)) {
> +               pr_err("overlayfs: unknown optional ro_compat features "
> +                      "enabled: %s\n", of->ro_compat_unknown);
> +
> +               if (readonly)
> +                       return true;
> +
> +               pr_err("overlayfs: filesystem cannot not be safely mounted "
> +                      "read-write by this kernel\n");
> +               return false;
> +       }
> +
> +       if (ovl_has_unknown_compat_features(sb)) {
> +               pr_warn("overlayfs: unknown optional compat features "
> +                       "enabled: %s\n", of->compat_unknown);
> +       }
> +
> +       return true;
> +}
> +
>  static struct kmem_cache *ovl_inode_cachep;
>
>  static struct inode *ovl_alloc_inode(struct super_block *sb)
> @@ -242,6 +508,13 @@ static void ovl_free_fs(struct ovl_fs *ofs)
>         }
>         kfree(ofs->lower_layers);
>
> +       kfree(ofs->feature.compat);
> +       kfree(ofs->feature.ro_compat);
> +       kfree(ofs->feature.incompat);
> +       kfree(ofs->feature.compat_unknown);
> +       kfree(ofs->feature.ro_compat_unknown);
> +       kfree(ofs->feature.incompat_unknown);
> +

If we are not getting rid of the strings, at least use a helper to
free ovl_features.

>         kfree(ofs->config.lowerdir);
>         kfree(ofs->config.upperdir);
>         kfree(ofs->config.workdir);
> @@ -357,7 +630,8 @@ static int ovl_remount(struct super_block *sb, int *flags, char *data)
>  {
>         struct ovl_fs *ofs = sb->s_fs_info;
>
> -       if (!(*flags & SB_RDONLY) && ovl_force_readonly(ofs))
> +       if (!(*flags & SB_RDONLY) &&
> +            (ovl_force_readonly(ofs) || !ovl_check_feature_ok(sb, false)))
>                 return -EROFS;
>
>         return 0;
> @@ -902,6 +1176,7 @@ static const struct xattr_handler *ovl_xattr_handlers[] = {
>         NULL
>  };
>
> +
>  static int ovl_get_upper(struct ovl_fs *ofs, struct path *upperpath)
>  {
>         struct vfsmount *upper_mnt;
> @@ -1284,11 +1559,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>
>                 err = ovl_get_upper(ofs, &upperpath);
>                 if (err)
> -                       goto out_err;
> +                       goto out_free_upper;
>
>                 err = ovl_get_workdir(ofs, &upperpath);
>                 if (err)
> -                       goto out_err;
> +                       goto out_free_upper;
>
>                 if (!ofs->workdir)
>                         sb->s_flags |= SB_RDONLY;
> @@ -1300,7 +1575,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>         oe = ovl_get_lowerstack(sb, ofs);
>         err = PTR_ERR(oe);
>         if (IS_ERR(oe))
> -               goto out_err;
> +               goto out_free_upper;
>
>         /* If the upper fs is nonexistent, we mark overlayfs r/o too */
>         if (!ofs->upper_mnt)
> @@ -1365,13 +1640,30 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>
>         sb->s_root = root_dentry;
>
> +       /*
> +        * Get features from the upper and lower root dir, and check
> +        * them could be mounted properly by this kernel.
> +        */
> +       err = ovl_init_features(sb->s_root, &ofs->feature);
> +       if (err)
> +               goto out_free_root;
> +
> +       err = -EINVAL;
> +       if (!ovl_check_feature_ok(sb, (sb_rdonly(sb))))
> +               goto out_free_root;
> +

feature check should be much sooner.
Why do it after setting up root dentry?
Miklos did a lot of cleanup on fill_super and this is messing it up.

>         return 0;
>
> +out_free_root:
> +       dput(sb->s_root);
> +       sb->s_root = NULL;
> +       goto out_err;
>  out_free_oe:
>         ovl_entry_stack_free(oe);
>         kfree(oe);
> -out_err:
> +out_free_upper:


IMO, you could and should avoid adding this goto tag.

Thanks,
Amir.

Re: [RFC PATCH 0/4] ovl: implement overlay feature set

[Amir Goldstein]

On Thu, Mar 22, 2018 at 1:35 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> Hi All,
>
> This is overlay feature set support patches, introduce incompatable,
> read-only compatable and incompatable feature set, store on the upper
> root dir via "trusted.overlay.feature_[compat|ro_compat|incompat]"
> xattrs.
>

Hi Zhangyi!

Thanks again for working on this.

For the "features" feature to be complete I think one more thing is required.

I suggest to refer to the era before "features" as overlayfs on-disk format v1
and to the era after "features" + fsck.overlayfs as on-disk format v2.
On-disk format v2 is identified by the existence of the features xattrs, even
if the are empty.

Every layer in the overlay can be of either format v1 or v2, but when you run
fsck.overlay it will scan all layers for undeclared features and
"upgrade" the layers
to v2, by explicitly stating the empty or non-empty feature sets of the layer.

So the thing that is missing IMO is -
a Kconfig option (say OVERLAY_FS_V2 ot OVERLAY_FS_FEATURES)
which defaults to n, but allows distros to opt-in for "v2 only".
With that config option set, overlay will refuse to mount v1 layers, or in other
words, fsck.overlay must be run before first time mount, or a simple
mkfs.overlay
to initialize the features xattrs

Another option is to relax the requirement for declaring lower layers features
and leave only the requirement that upper root dir is either empty or has the
features xattrs. In any case, on first mount with an empty upper dir,
features xattrs
should be initialized. Maybe that calls for yet another Kconfig option, i.e.
OVERLAY_FS_LOWER_V2.


> This patch set base on Linux 4.16-rc6.
>
> Patch 1: Implement feature xattr and mount check, refuse mount if no
>          backward incompatible feature is detected.
> Patch 2: Copy up feature xattr from the lower root dirs to the upper
>          root dir to prevent feature lost.
> Patch 3: Add three already support feature: redirect dir, index and
>          nfs_export, enable them when user give enable mount option.
> patch 4: Fix redircet feature once a redirect xattr is detected for
>          the case of overlay was mounted in the old kernel.
> (PS: Not sure this patch is necessary, maybe just give a warning message
> and handle this by fsck.overlay? If necessary, check indexdir during
> mount time is also necessary too).
>
> zhangyi (F) (4):
>   ovl: add feature set support
>   ovl: update features on upper root dir from lower layers
>   ovl: enable features according to mount options
>   ovl: fix redirect feature for backward compatibility
>
>  fs/overlayfs/namei.c     |  12 ++
>  fs/overlayfs/overlayfs.h |  90 +++++++++++
>  fs/overlayfs/ovl_entry.h |  18 +++
>  fs/overlayfs/super.c     | 410 ++++++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 525 insertions(+), 5 deletions(-)
>
> --
> 2.13.6
>

Re: [RFC PATCH 3/4] ovl: enable features according to mount options

[Amir Goldstein]

On Thu, Mar 22, 2018 at 1:35 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> Unlike common disk filesystems, overlay filesystem enable features
> through mount options(e.g. -o index=on will enable index feature),
> so it's a good time to save enabled features on the upper root dir
> according to mount options. The given feature will be enabled when
> user give option "-o xxx=on" during mount time.
>
> This patch add three already support features: redirect_dir, index
> and nfs_export, the first one belongs to the incompatable feature set,
> and the other two belong to the ro_compatable feature set.
>
> eg:
> mount -t overlay -olowerdir=lower,upperdir=upper,workdir=work \
>   -oredirect_dir=on,index=on,nfs_export=on overlay merge
>
> trusted.overlay.feature_ro_compat="index,nfs_export"
> trusted.overlay.feature_incompat="redirect_dir"
>
> This patch also introduce three sets of feature check/set helper
> functions ovl_has_xxx_feature() and ovl_set_xxx_feature() which
> learn from ext4 filesystem driver.
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> ---
>  fs/overlayfs/overlayfs.h | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/overlayfs/ovl_entry.h |  2 ++
>  fs/overlayfs/super.c     | 71 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 146 insertions(+)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 121e81cc1550..53ed4e73f889 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -47,6 +47,79 @@ enum ovl_entry_flag {
>  };
>
>  /* Features */
> +enum ovl_feature_flag {
> +       OVL_FEATURE_COMPAT,
> +       OVL_FEATURE_RO_COMPAT,
> +       OVL_FEATURE_INCOMPAT,
> +       OVL_FEATURE_MAX,
> +};
> +
> +#define OVL_FEATURE_RO_COMPAT_INDEX            "index"
> +#define OVL_FEATURE_RO_COMPAT_NFS_EXPORT       "nfs_export"
> +
> +#define OVL_FEATURE_INCOMPAT_REDIRECT_DIR      "redirect_dir"
> +
> +int ovl_set_feature_set(struct super_block *sb,
> +                       enum ovl_feature_flag flag, const char *new);
> +
> +#define OVL_FEATURE_COMPAT_FUNCS(name, flagname) \
> +static inline bool ovl_has_feature_##name(struct super_block *sb) \
> +{ \
> +       bool exist;\
> +       down_read(&OVL_FS(sb)->feature.lock); \
> +       exist = ((OVL_FS(sb)->feature.compat) && \
> +               (strstr(OVL_FS(sb)->feature.compat, \
> +                       OVL_FEATURE_COMPAT_##flagname))); \
> +       up_read(&OVL_FS(sb)->feature.lock); \
> +       return exist; \
> +} \
> +static inline int ovl_set_feature_##name(struct super_block *sb) \
> +{ \
> +       return ovl_set_feature_set(sb, OVL_FEATURE_COMPAT, \
> +                       OVL_FEATURE_COMPAT_##flagname); \
> +} \
> +
> +#define OVL_FEATURE_RO_COMPAT_FUNCS(name, flagname) \
> +static inline bool ovl_has_feature_##name(struct super_block *sb) \
> +{ \
> +       bool exist;\
> +       down_read(&OVL_FS(sb)->feature.lock); \
> +       exist = ((OVL_FS(sb)->feature.ro_compat) && \
> +               (strstr(OVL_FS(sb)->feature.ro_compat, \
> +                       OVL_FEATURE_RO_COMPAT_##flagname))); \
> +       up_read(&OVL_FS(sb)->feature.lock); \
> +       return exist; \
> +} \
> +static inline int ovl_set_feature_##name(struct super_block *sb) \
> +{ \
> +       return ovl_set_feature_set(sb, OVL_FEATURE_RO_COMPAT, \
> +                       OVL_FEATURE_RO_COMPAT_##flagname); \
> +} \
> +
> +#define OVL_FEATURE_INCOMPAT_FUNCS(name, flagname) \
> +static inline bool ovl_has_feature_##name(struct super_block *sb) \
> +{ \
> +       bool exist;\
> +       down_read(&OVL_FS(sb)->feature.lock); \
> +       exist = ((OVL_FS(sb)->feature.incompat) && \
> +               (strstr(OVL_FS(sb)->feature.incompat, \
> +                       OVL_FEATURE_INCOMPAT_##flagname))); \
> +       up_read(&OVL_FS(sb)->feature.lock); \
> +       return exist; \
> +} \
> +static inline int ovl_set_feature_##name(struct super_block *sb) \
> +{ \
> +       return ovl_set_feature_set(sb, OVL_FEATURE_INCOMPAT, \
> +                       OVL_FEATURE_INCOMPAT_##flagname); \
> +} \
> +

It is best that you copy the helpers from ext4 as is for bit flags.

> +
> +OVL_FEATURE_RO_COMPAT_FUNCS(index,             INDEX)
> +OVL_FEATURE_RO_COMPAT_FUNCS(nfs_export,                NFS_EXPORT)
> +
> +OVL_FEATURE_INCOMPAT_FUNCS(redirect_dir,       REDIRECT_DIR)
> +
> +
>  static inline bool ovl_has_unknown_compat_features(struct super_block *sb)
>  {
>         return (OVL_FS(sb)->feature.compat_unknown != NULL);
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index f8c5e0191a12..ff8bf783a097 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -36,6 +36,8 @@ struct ovl_feature {
>         char *compat;
>         char *ro_compat;
>         char *incompat;
> +       struct rw_semaphore lock;       /* protect feature string */
> +
>         const char *compat_unknown;
>         const char *ro_compat_unknown;
>         const char *incompat_unknown;
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 19b7d0869fbd..4787f9356dfd 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -189,13 +189,22 @@ static const char *ovl_feature_compat_supp[] = {
>  };
>
>  static const char *ovl_feature_ro_compat_supp[] = {
> +       OVL_FEATURE_RO_COMPAT_INDEX,
> +       OVL_FEATURE_RO_COMPAT_NFS_EXPORT,
>         NULL
>  };
>
>  static const char *ovl_feature_incompat_supp[] = {
> +       OVL_FEATURE_INCOMPAT_REDIRECT_DIR,
>         NULL
>  };
>
> +static const char *ovl_feature_set[] = {
> +       OVL_XATTR_FEATURE_COMPAT,
> +       OVL_XATTR_FEATURE_RO_COMPAT,
> +       OVL_XATTR_FEATURE_INCOMPAT
> +};
> +
>  /*
>   * Get overlay feature from the real root dentry.
>   *
> @@ -431,6 +440,8 @@ static int ovl_init_features(struct dentry *root, struct ovl_feature *of)
>  {
>         int err;
>
> +       init_rwsem(&of->lock);
> +
>         err = ovl_get_feature_set(root, OVL_XATTR_FEATURE_COMPAT,
>                                   ovl_feature_compat_supp, &of->compat,
>                                   &of->compat_unknown);
> @@ -487,6 +498,49 @@ static bool ovl_check_feature_ok(struct super_block *sb, bool readonly)
>         return true;
>  }
>
> +/*
> + * Set one specified feature to the upper root dir
> + *
> + * @sb: overlay super block
> + * @flag: feature set flag, compat/ro compat/incompat
> + * @new: new feature want to set
> + * Return 0 if success, errno otherwise.
> + */
> +int ovl_set_feature_set(struct super_block *sb,
> +                       enum ovl_feature_flag flag, const char *new)
> +{
> +       struct ovl_feature *of = &OVL_FS(sb)->feature;
> +       bool update = false;
> +       char **feature;
> +       int err;
> +
> +       /* Don't set feature if no upper layer */
> +       if (!OVL_FS(sb)->upper_mnt)
> +               return 0;
> +
> +       down_write(&of->lock);
> +       if (flag == OVL_FEATURE_COMPAT)
> +               feature = &of->compat;
> +       else if (flag == OVL_FEATURE_RO_COMPAT)
> +               feature = &of->ro_compat;
> +       else
> +               feature = &of->incompat;
> +
> +       err = ovl_update_feature(feature, new, &update);
> +       if (err)
> +               goto out;
> +       if (!update)
> +               goto out;
> +
> +       /* Set updated compat features to upper root dir */
> +       err = ovl_set_feature(sb->s_root, ovl_dentry_upper(sb->s_root),
> +                             ovl_feature_set[flag],  *feature);
> +out:
> +       up_write(&of->lock);
> +       return err;
> +}
> +
> +
>  static struct kmem_cache *ovl_inode_cachep;
>
>  static struct inode *ovl_alloc_inode(struct super_block *sb)
> @@ -1689,6 +1743,23 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>         if (!ovl_check_feature_ok(sb, (sb_rdonly(sb))))
>                 goto out_free_root;
>
> +       /* Turn on features according to mount options */
> +       if (ofs->config.index && !ovl_has_feature_index(sb)) {
> +               err = ovl_set_feature_index(sb);
> +               if (err)
> +                       goto out_free_root;
> +       }
> +       if (ofs->config.nfs_export && !ovl_has_feature_nfs_export(sb)) {
> +               err = ovl_set_feature_nfs_export(sb);
> +               if (err)
> +                       goto out_free_root;
> +       }
> +       if (ofs->config.redirect_dir && !ovl_has_feature_redirect_dir(sb)) {
> +               err = ovl_set_feature_redirect_dir(sb);
> +               if (err)
> +                       goto out_free_root;
> +       }
> +

Let's move all these to a helper.
w.r.t. redirect_dir, I am not sure it makes sense to declare an
incompat feature on mount.
It would be wiser to set the incompat feature on first set of redirect.
If we store features are bit flags, then checking if feature is
enabled before setting redirect
is not expensive.

Thanks,
Amir.

Re: [RFC PATCH 1/4] ovl: add feature set support

[zhangyi (F)]

On 2018/3/23 20:12, Amir Goldstein Wrote:
> On Thu, Mar 22, 2018 at 1:35 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> In order to distinguish the backward compatible and backward
>> incompatible features in overlay filesystem, we introduce
>> "compat/ro_compat/incompat" three kinds of feature set and
>> store them on the upper root dir via xattr:
>> "trusted.overlay.feature_[compat|ro_compat|incompat]=xxx,xxx"
>>
>> Mount operation should fail if one unknown incompatible feature is
>> detected on the upper root dir, and read-write mount operation should
>> fail if one unknown ro_compat feature is detected on the upper root
>> dir. At the same time, lower layers may also have some backward
>> incompatible features when they used to be upper root dir, so do the
>> same check as upper root dir.
>>
>> This patch doesn't add any features, so if any feature xattrs is
>> detected on the underlying root dir, it will be recognized as unknown
>> feature.
>>
>> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> 
> To be fair, I *suggested* to use Miklos' suggestion...
> 

Oh，sorry，I will add.

>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>> ---
>>   fs/overlayfs/overlayfs.h |  17 +++
>>   fs/overlayfs/ovl_entry.h |  16 +++
>>   fs/overlayfs/super.c     | 302 ++++++++++++++++++++++++++++++++++++++++++++++-
> 
> This series adds a lot of code to super.c which is big enough already
> I suggest to start features.c.
> 
> 
>>   3 files changed, 330 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>> index 225ff1171147..121e81cc1550 100644
>> --- a/fs/overlayfs/overlayfs.h
>> +++ b/fs/overlayfs/overlayfs.h
>> @@ -28,6 +28,9 @@ enum ovl_path_type {
>>   #define OVL_XATTR_IMPURE OVL_XATTR_PREFIX "impure"
>>   #define OVL_XATTR_NLINK OVL_XATTR_PREFIX "nlink"
>>   #define OVL_XATTR_UPPER OVL_XATTR_PREFIX "upper"
>> +#define OVL_XATTR_FEATURE_COMPAT OVL_XATTR_PREFIX "feature_compat"
>> +#define OVL_XATTR_FEATURE_RO_COMPAT OVL_XATTR_PREFIX "feature_ro_compat"
>> +#define OVL_XATTR_FEATURE_INCOMPAT OVL_XATTR_PREFIX "feature_incompat"
>>
>>   enum ovl_inode_flag {
>>          /* Pure upper dir that may contain non pure upper entries */
>> @@ -43,6 +46,20 @@ enum ovl_entry_flag {
>>          OVL_E_CONNECTED,
>>   };
>>
>> +/* Features */
>> +static inline bool ovl_has_unknown_compat_features(struct super_block *sb)
>> +{
>> +       return (OVL_FS(sb)->feature.compat_unknown != NULL);
>> +}
>> +static inline bool ovl_has_unknown_ro_compat_features(struct super_block *sb)
>> +{
>> +       return (OVL_FS(sb)->feature.ro_compat_unknown != NULL);
>> +}
>> +static inline bool ovl_has_unknown_incompat_features(struct super_block *sb)
>> +{
>> +       return (OVL_FS(sb)->feature.incompat_unknown != NULL);
>> +}
>> +
>>   /*
>>    * The tuple (fh,uuid) is a universal unique identifier for a copy up origin,
>>    * where:
>> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
>> index bfef6edcc111..f8c5e0191a12 100644
>> --- a/fs/overlayfs/ovl_entry.h
>> +++ b/fs/overlayfs/ovl_entry.h
>> @@ -32,6 +32,15 @@ struct ovl_path {
>>          struct dentry *dentry;
>>   };
>>
>> +struct ovl_feature {
>> +       char *compat;
>> +       char *ro_compat;
>> +       char *incompat;
>> +       const char *compat_unknown;
>> +       const char *ro_compat_unknown;
>> +       const char *incompat_unknown;
>> +};
>> +
> 
> IMO those should be 64bit bit flags, like in other fs,
> see more below.
> 
>>   /* private information held for overlayfs's superblock */
>>   struct ovl_fs {
>>          struct vfsmount *upper_mnt;
>> @@ -55,6 +64,8 @@ struct ovl_fs {
>>          /* Did we take the inuse lock? */
>>          bool upperdir_locked;
>>          bool workdir_locked;
>> +       /* features */
>> +       struct ovl_feature feature;
>>   };
>>
>>   /* private information held for every overlayfs dentry */
>> @@ -94,6 +105,11 @@ static inline struct ovl_inode *OVL_I(struct inode *inode)
>>          return container_of(inode, struct ovl_inode, vfs_inode);
>>   }
>>
>> +static inline struct ovl_fs *OVL_FS(struct super_block *sb)
>> +{
>> +       return sb->s_fs_info;
>> +}
>> +
>>   static inline struct dentry *ovl_upperdentry_dereference(struct ovl_inode *oi)
>>   {
>>          return READ_ONCE(oi->__upperdentry);
>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> index 7c24619ae7fc..10772ae237b4 100644
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>> @@ -184,6 +184,272 @@ static const struct dentry_operations ovl_reval_dentry_operations = {
>>          .d_weak_revalidate = ovl_dentry_weak_revalidate,
>>   };
>>
>> +static const char *ovl_feature_compat_supp[] = {
>> +       NULL
>> +};
>> +
>> +static const char *ovl_feature_ro_compat_supp[] = {
>> +       NULL
>> +};
>> +
>> +static const char *ovl_feature_incompat_supp[] = {
>> +       NULL
>> +};
>> +
>> +/*
>> + * Get overlay feature from the real root dentry.
>> + *
>> + * @realroot: real underlying root dir dentry
>> + * @set: feature set xattr name
>> + * Return feature string if success, NULL if no feature and
>> + * error number if error.
>> + */
>> +static char *ovl_get_feature(struct dentry *realroot, const char *set)
>> +{
>> +       int res;
>> +       char *buf;
>> +
>> +       res = vfs_getxattr(realroot, set, NULL, 0);
>> +       if (res <= 0) {
>> +               if (res == -EOPNOTSUPP || res == -ENODATA)
>> +                       res = 0;
>> +               return ERR_PTR(res);
>> +       }
>> +
>> +       buf = kmalloc(res + 1, GFP_KERNEL);
>> +       if (!buf)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       res = vfs_getxattr(realroot, set, buf, res);
>> +       if (res <= 0) {
>> +               kfree(buf);
>> +               return ERR_PTR(res);
>> +       }
>> +
>> +       buf[res] = '\0';
>> +       return buf;
>> +}
>> +
>> +/*
>> + * Add new feature to (or generate new) features string, set update flag
>> + * if new feature added.
>> + *
>> + * @feature: output feature string
>> + * @add: feature want to add
>> + * @update: set when new feature add, no change otherwise
>> + * Return 0 if success, error number otherwise.
>> + */
>> +static int ovl_update_feature(char **feature, const char *add,
>> +                             bool *update)
>> +{
>> +       char *p = *feature;
>> +       char *tmp;
>> +       int size;
>> +
>> +       if (p && strstr(p, add))
>> +               return 0;
>> +
>> +       size = strlen(add) + 1;
>> +       if (p) {
>> +               size += strlen(p) + 1;
>> +               tmp = kmalloc(size, GFP_KERNEL);
>> +               if (!tmp)
>> +                       return -ENOMEM;
>> +
>> +               snprintf(tmp, size, "%s,%s", p, add);
>> +               kfree(p);
>> +       } else {
>> +               tmp = kmalloc(size, GFP_KERNEL);
>> +               if (!tmp)
>> +                       return -ENOMEM;
>> +
>> +               strncpy(tmp, add, size);
>> +       }
>> +
>> +       if (update)
>> +               *update = true;
>> +       *feature = tmp;
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Parse features and split out unknown features, set update flag
>> + * if new feature added.
>> + *
>> + * @features: input feature string which need to parse
>> + * @supp: support feature list
>> + * @feature: output feature string
>> + * @unknown: unknown feature string
>> + * @update: set when new feature add, no change otherwise
>> + * Return 0 if success, errno otherwise.
>> + */
>> +static int ovl_parse_feature(char *features, const char *supp[],
>> +                            char **feature, const char **unknown,
>> +                            bool *update)
>> +{
>> +       const char **fs;
>> +       char *tmpf = NULL, *tmpu = NULL;
>> +       char *p;
>> +       int err = 0;
>> +
>> +       if (!features)
>> +               return 0;
>> +
>> +       while ((p = strsep(&features, ",")) != NULL) {
>> +               /* Add to or generate parsed feature string */
>> +               err = ovl_update_feature(&tmpf, p, update);
>> +               if (err)
>> +                       goto err;
>> +
>> +               /* Check support or not */
>> +               for (fs = supp; *fs; fs++) {
>> +                       if (strcmp(p, *fs) == 0)
>> +                               break;
>> +               }
>> +
>> +               if (*fs)
>> +                       continue;
>> +
>> +               /* Non-supported feature found, add to unknown */
>> +               err = ovl_update_feature(&tmpu, p, NULL);
>> +               if (err)
>> +                       goto err;
>> +       }
>> +
>> +       *feature = tmpf;
>> +       *unknown = tmpu;
>> +       return 0;
>> +err:
>> +       kfree(tmpf);
>> +       kfree(tmpu);
>> +       return err;
>> +}
>> +
> 
> The past ~200 lines are a re-implementation (probably buggy) of tokenize
> and for what? saving the features as strings in xattrs is questionable -
> it serves no purpose other than human convenience when reading features xattrs.
> We can store a 64bit binary flags blob in big endian order, just like

IIUC, I find ext4 and btrfs use little-endian but xfs use big-endian, is
little-endian better? because our cpu endian is more likely use 
little-endian by default.

> any other fs,
> and then we need no parsing, no string find and no string insertion helpers.
> 
> Not to mention that some fs have limited size of xattr, which is going
> to limit us in
> the future with the amount of features and feature name lengths.
> 

It's fine to me, I was trying to use u64 bit mask but found it's not 
readable, so I chose to use strings, but it has a lot of disadvantages 
as you said. I'd like to change to use u64 bit mask, and it means there 
is one more thing should do, we should introduce a feature translation 
tools (something like "dumpe2fs -h" or "tune2fs -l", and the feature 
xattr is more like a simple overlay super block) to show features to 
user, right?

> 
>> +/*
>> + * Get one set of features from the underlying root dir and parse
>> + * them to get the unknown features.
>> + *
>> + * @root: overlay root dentry
>> + * @set: feature set xattr name
>> + * @supp: supported feature list
>> + * @feature: output features string
>> + * @unknown: unknown features string
>> + * Return 0 if success, errno otherwise.
>> + */
>> +static int ovl_get_feature_set(struct dentry *root, const char *set,
>> +                              const char *supp[], char **feature,
>> +                              const char **unknown)
>> +{
>> +       struct dentry *upperroot = ovl_dentry_upper(root);
>> +       struct ovl_entry *oe = root->d_fsdata;
>> +       char *buf = NULL;
>> +       int i;
>> +       int err;
>> +
>> +       WARN_ON(*feature || *unknown);
>> +
>> +       /* Get features from the upper root dir */
>> +       if (upperroot) {
>> +               buf = ovl_get_feature(upperroot, set);
>> +               if (IS_ERR(buf))
>> +                       return PTR_ERR(buf);
>> +               if (!buf)
>> +                       goto get_lower;
>> +
>> +               err = ovl_parse_feature(buf, supp, feature,
>> +                                       unknown, NULL);
>> +               kfree(buf);
>> +               if (err)
>> +                       return err;
>> +       }
>> +
>> +get_lower:
>> +       /* Get features from each lower root dir beside the bottom layer */
>> +       for (i = 0; i < oe->numlower-1; i++) {
>> +               buf = ovl_get_feature(oe->lowerstack[i].dentry, set);
>> +               if (IS_ERR(buf))
>> +                       return PTR_ERR(buf);
>> +               if (!buf)
>> +                       continue;
>> +
>> +               err = ovl_parse_feature(buf, supp, feature,
>> +                                       unknown, NULL);
>> +               kfree(buf);
>> +               if (err)
>> +                       return err;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Get all features from the upper and lower root dir and parse them to get
>> + * unknown features.
>> + *
>> + * @root: overlay root dir dentry
>> + * @of: overlay feature set want to init
>> + * Return 0 if success, errno otherwise.
>> + */
>> +static int ovl_init_features(struct dentry *root, struct ovl_feature *of)
>> +{
>> +       int err;
>> +
>> +       err = ovl_get_feature_set(root, OVL_XATTR_FEATURE_COMPAT,
>> +                                 ovl_feature_compat_supp, &of->compat,
>> +                                 &of->compat_unknown);
>> +       if (err)
>> +               return err;
>> +
>> +       err = ovl_get_feature_set(root, OVL_XATTR_FEATURE_RO_COMPAT,
>> +                                 ovl_feature_ro_compat_supp, &of->ro_compat,
>> +                                 &of->ro_compat_unknown);
>> +       if (err)
>> +               return err;
>> +
>> +       err = ovl_get_feature_set(root, OVL_XATTR_FEATURE_INCOMPAT,
>> +                                 ovl_feature_incompat_supp, &of->incompat,
>> +                                 &of->incompat_unknown);
>> +       return err;
>> +}
>> +
>> +/*
>> + * Check whether this filesystem can be mounted based on
>> + * the features present and the read-only/read-write mount requested.
>> + * Returns true if this filesystem can be mounted as requested,
>> + * false if it cannot be.
>> + */
>> +static bool ovl_check_feature_ok(struct super_block *sb, bool readonly)
>> +{
>> +       struct ovl_feature *of = &OVL_FS(sb)->feature;
>> +
>> +       if (ovl_has_unknown_incompat_features(sb)) {
>> +               pr_err("overlayfs: unknown optional incompat features "
>> +                      "enabled: %s\n", of->incompat_unknown);
>> +               pr_err("overlayfs: filesystem cannot not be safely mounted "
>> +                      "by this kernel\n");
>> +               return false;
>> +       }
>> +
>> +       if (ovl_has_unknown_ro_compat_features(sb)) {
>> +               pr_err("overlayfs: unknown optional ro_compat features "
>> +                      "enabled: %s\n", of->ro_compat_unknown);
>> +
>> +               if (readonly)
>> +                       return true;
>> +
>> +               pr_err("overlayfs: filesystem cannot not be safely mounted "
>> +                      "read-write by this kernel\n");
>> +               return false;
>> +       }
>> +
>> +       if (ovl_has_unknown_compat_features(sb)) {
>> +               pr_warn("overlayfs: unknown optional compat features "
>> +                       "enabled: %s\n", of->compat_unknown);
>> +       }
>> +
>> +       return true;
>> +}
>> +
>>   static struct kmem_cache *ovl_inode_cachep;
>>
>>   static struct inode *ovl_alloc_inode(struct super_block *sb)
>> @@ -242,6 +508,13 @@ static void ovl_free_fs(struct ovl_fs *ofs)
>>          }
>>          kfree(ofs->lower_layers);
>>
>> +       kfree(ofs->feature.compat);
>> +       kfree(ofs->feature.ro_compat);
>> +       kfree(ofs->feature.incompat);
>> +       kfree(ofs->feature.compat_unknown);
>> +       kfree(ofs->feature.ro_compat_unknown);
>> +       kfree(ofs->feature.incompat_unknown);
>> +
> 
> If we are not getting rid of the strings, at least use a helper to
> free ovl_features.
> 

Yes, will do.

>>          kfree(ofs->config.lowerdir);
>>          kfree(ofs->config.upperdir);
>>          kfree(ofs->config.workdir);
>> @@ -357,7 +630,8 @@ static int ovl_remount(struct super_block *sb, int *flags, char *data)
>>   {
>>          struct ovl_fs *ofs = sb->s_fs_info;
>>
>> -       if (!(*flags & SB_RDONLY) && ovl_force_readonly(ofs))
>> +       if (!(*flags & SB_RDONLY) &&
>> +            (ovl_force_readonly(ofs) || !ovl_check_feature_ok(sb, false)))
>>                  return -EROFS;
>>
>>          return 0;
>> @@ -902,6 +1176,7 @@ static const struct xattr_handler *ovl_xattr_handlers[] = {
>>          NULL
>>   };
>>
>> +
>>   static int ovl_get_upper(struct ovl_fs *ofs, struct path *upperpath)
>>   {
>>          struct vfsmount *upper_mnt;
>> @@ -1284,11 +1559,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>>
>>                  err = ovl_get_upper(ofs, &upperpath);
>>                  if (err)
>> -                       goto out_err;
>> +                       goto out_free_upper;
>>
>>                  err = ovl_get_workdir(ofs, &upperpath);
>>                  if (err)
>> -                       goto out_err;
>> +                       goto out_free_upper;
>>
>>                  if (!ofs->workdir)
>>                          sb->s_flags |= SB_RDONLY;
>> @@ -1300,7 +1575,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>>          oe = ovl_get_lowerstack(sb, ofs);
>>          err = PTR_ERR(oe);
>>          if (IS_ERR(oe))
>> -               goto out_err;
>> +               goto out_free_upper;
>>
>>          /* If the upper fs is nonexistent, we mark overlayfs r/o too */
>>          if (!ofs->upper_mnt)
>> @@ -1365,13 +1640,30 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>>
>>          sb->s_root = root_dentry;
>>
>> +       /*
>> +        * Get features from the upper and lower root dir, and check
>> +        * them could be mounted properly by this kernel.
>> +        */
>> +       err = ovl_init_features(sb->s_root, &ofs->feature);
>> +       if (err)
>> +               goto out_free_root;
>> +
>> +       err = -EINVAL;
>> +       if (!ovl_check_feature_ok(sb, (sb_rdonly(sb))))
>> +               goto out_free_root;
>> +
> 
> feature check should be much sooner.
> Why do it after setting up root dentry?
> Miklos did a lot of cleanup on fill_super and this is messing it up.
> 

It can move earlier beside use sb as input, will do.

Thanks,
Yi.

>>          return 0;
>>
>> +out_free_root:
>> +       dput(sb->s_root);
>> +       sb->s_root = NULL;
>> +       goto out_err;
>>   out_free_oe:
>>          ovl_entry_stack_free(oe);
>>          kfree(oe);
>> -out_err:
>> +out_free_upper:
> 
> 
> IMO, you could and should avoid adding this goto tag.
> 
> Thanks,
> Amir.
> --
> 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: [RFC PATCH 0/4] ovl: implement overlay feature set

[zhangyi (F)]

Hi Amir, thanks for your suggestions.

On 2018/3/23 20:32, Amir Goldstein Wrote:
> On Thu, Mar 22, 2018 at 1:35 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> Hi All,
>>
>> This is overlay feature set support patches, introduce incompatable,
>> read-only compatable and incompatable feature set, store on the upper
>> root dir via "trusted.overlay.feature_[compat|ro_compat|incompat]"
>> xattrs.
>>
> 
> Hi Zhangyi!
> 
> Thanks again for working on this.
> 
> For the "features" feature to be complete I think one more thing is required.
> 
> I suggest to refer to the era before "features" as overlayfs on-disk format v1
> and to the era after "features" + fsck.overlayfs as on-disk format v2.
> On-disk format v2 is identified by the existence of the features xattrs, even
> if the are empty.
> 
> Every layer in the overlay can be of either format v1 or v2, but when you run
> fsck.overlay it will scan all layers for undeclared features and
> "upgrade" the layers
> to v2, by explicitly stating the empty or non-empty feature sets of the layer.
>  > So the thing that is missing IMO is -
> a Kconfig option (say OVERLAY_FS_V2 ot OVERLAY_FS_FEATURES)
> which defaults to n, but allows distros to opt-in for "v2 only".
> With that config option set, overlay will refuse to mount v1 layers, or in other
> words, fsck.overlay must be run before first time mount, or a simple
> mkfs.overlay
> to initialize the features xattrs
> 
Let me see, if "OVERLAY_FS_V2=y",
- overlay will refuse to mount overlayfs which was mounted on old 
kernel, user should run fsck.overlay to check features in each layer
and set feature xattr on the layer's root dir. So the feature xattr on 
one root dir only contain this layer's features.
- overlay will refuse to mount overlayfs whose upper is empty(no feature 
xattr), maybe new created image and never mounted before,
uesr should run a simple mkfs.overlay to init the feature xattr for all 
layers (maybe accept -o options to enable features).

and if "OVERLAY_FS_V2=n",
This is what we do now in kernel, features xattr will set in the upper 
root dir according to mount options (or maybe where we set xattrs used 
by features, like redirect). And one more thing should do is init an 
empty feature xattr if user don't give any feature mount options.

Right?

> Another option is to relax the requirement for declaring lower layers features
> and leave only the requirement that upper root dir is either empty or has the
> features xattrs. In any case, on first mount with an empty upper dir,
> features xattrs
> should be initialized. Maybe that calls for yet another Kconfig option, i.e.
> OVERLAY_FS_LOWER_V2.
> 
Sorry, I don't follow this option.
Do you mean this option is a sub-option of OVERLAY_FS_V2 ?
If "OVERLAY_FS_LOWER_V2=y", refuse to mount just if there is no feature 
xattrs on the upper root dir (empty or non-empty will pass). Introduce 
this option because overlayfs only set feature xattrs on the upper 
layer, so there is no feature xattrs on the lower root dir, right or 
some other reasons ?

Now, the quesion is:
The feature xattrs on the upper root dir should contain all features in 
the overlayfs(1), or features just in the upper layer(2), not sure which 
one is better ?

If (1), say "this overlayfs image have index feature, not this layer", 
it's easy to do but we will meet the indexdir problem you mentioned in 2/4.
if (2), the auto fix (e.g. fix redirect feature if we detect redirect 
xattr) becomes useless becase we cannot set feature xattr to the lower 
root dir, so need to remove this implement or give a warn to ask 
fsck.overlay to handle this. But we can distinguish features in the 
upper layer and lowers layer, maybe useful in the future.

Thanks,
Yi

> 
>> This patch set base on Linux 4.16-rc6.
>>
>> Patch 1: Implement feature xattr and mount check, refuse mount if no
>>           backward incompatible feature is detected.
>> Patch 2: Copy up feature xattr from the lower root dirs to the upper
>>           root dir to prevent feature lost.
>> Patch 3: Add three already support feature: redirect dir, index and
>>           nfs_export, enable them when user give enable mount option.
>> patch 4: Fix redircet feature once a redirect xattr is detected for
>>           the case of overlay was mounted in the old kernel.
>> (PS: Not sure this patch is necessary, maybe just give a warning message
>> and handle this by fsck.overlay? If necessary, check indexdir during
>> mount time is also necessary too).
>>
>> zhangyi (F) (4):
>>    ovl: add feature set support
>>    ovl: update features on upper root dir from lower layers
>>    ovl: enable features according to mount options
>>    ovl: fix redirect feature for backward compatibility
>>
>>   fs/overlayfs/namei.c     |  12 ++
>>   fs/overlayfs/overlayfs.h |  90 +++++++++++
>>   fs/overlayfs/ovl_entry.h |  18 +++
>>   fs/overlayfs/super.c     | 410 ++++++++++++++++++++++++++++++++++++++++++++++-
>>   4 files changed, 525 insertions(+), 5 deletions(-)
>>
>> --
>> 2.13.6
>>
> --
> 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: [RFC PATCH 1/4] ovl: add feature set support

[Amir Goldstein]

On Sat, Mar 24, 2018 at 3:55 PM, zhangyi (F) <yizhang089@gmail.com> wrote:
> On 2018/3/23 20:12, Amir Goldstein Wrote:
>>
>> On Thu, Mar 22, 2018 at 1:35 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>
>>> In order to distinguish the backward compatible and backward
>>> incompatible features in overlay filesystem, we introduce
>>> "compat/ro_compat/incompat" three kinds of feature set and
>>> store them on the upper root dir via xattr:
>>> "trusted.overlay.feature_[compat|ro_compat|incompat]=xxx,xxx"
>>>
>>> Mount operation should fail if one unknown incompatible feature is
>>> detected on the upper root dir, and read-write mount operation should
>>> fail if one unknown ro_compat feature is detected on the upper root
>>> dir. At the same time, lower layers may also have some backward
>>> incompatible features when they used to be upper root dir, so do the
>>> same check as upper root dir.
>>>
>>> This patch doesn't add any features, so if any feature xattrs is
>>> detected on the underlying root dir, it will be recognized as unknown
>>> feature.
>>>
>>> Suggested-by: Amir Goldstein <amir73il@gmail.com>
>>
>>
>> To be fair, I *suggested* to use Miklos' suggestion...
>>
>
> Oh，sorry，I will add.
>
>
>>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>>> ---
>>>   fs/overlayfs/overlayfs.h |  17 +++
>>>   fs/overlayfs/ovl_entry.h |  16 +++
>>>   fs/overlayfs/super.c     | 302
>>> ++++++++++++++++++++++++++++++++++++++++++++++-
>>
>>
>> This series adds a lot of code to super.c which is big enough already
>> I suggest to start features.c.
>>
>>
>>>   3 files changed, 330 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>>> index 225ff1171147..121e81cc1550 100644
>>> --- a/fs/overlayfs/overlayfs.h
>>> +++ b/fs/overlayfs/overlayfs.h
>>> @@ -28,6 +28,9 @@ enum ovl_path_type {
>>>   #define OVL_XATTR_IMPURE OVL_XATTR_PREFIX "impure"
>>>   #define OVL_XATTR_NLINK OVL_XATTR_PREFIX "nlink"
>>>   #define OVL_XATTR_UPPER OVL_XATTR_PREFIX "upper"
>>> +#define OVL_XATTR_FEATURE_COMPAT OVL_XATTR_PREFIX "feature_compat"
>>> +#define OVL_XATTR_FEATURE_RO_COMPAT OVL_XATTR_PREFIX "feature_ro_compat"
>>> +#define OVL_XATTR_FEATURE_INCOMPAT OVL_XATTR_PREFIX "feature_incompat"
>>>
>>>   enum ovl_inode_flag {
>>>          /* Pure upper dir that may contain non pure upper entries */
>>> @@ -43,6 +46,20 @@ enum ovl_entry_flag {
>>>          OVL_E_CONNECTED,
>>>   };
>>>
>>> +/* Features */
>>> +static inline bool ovl_has_unknown_compat_features(struct super_block
>>> *sb)
>>> +{
>>> +       return (OVL_FS(sb)->feature.compat_unknown != NULL);
>>> +}
>>> +static inline bool ovl_has_unknown_ro_compat_features(struct super_block
>>> *sb)
>>> +{
>>> +       return (OVL_FS(sb)->feature.ro_compat_unknown != NULL);
>>> +}
>>> +static inline bool ovl_has_unknown_incompat_features(struct super_block
>>> *sb)
>>> +{
>>> +       return (OVL_FS(sb)->feature.incompat_unknown != NULL);
>>> +}
>>> +
>>>   /*
>>>    * The tuple (fh,uuid) is a universal unique identifier for a copy up
>>> origin,
>>>    * where:
>>> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
>>> index bfef6edcc111..f8c5e0191a12 100644
>>> --- a/fs/overlayfs/ovl_entry.h
>>> +++ b/fs/overlayfs/ovl_entry.h
>>> @@ -32,6 +32,15 @@ struct ovl_path {
>>>          struct dentry *dentry;
>>>   };
>>>
>>> +struct ovl_feature {
>>> +       char *compat;
>>> +       char *ro_compat;
>>> +       char *incompat;
>>> +       const char *compat_unknown;
>>> +       const char *ro_compat_unknown;
>>> +       const char *incompat_unknown;
>>> +};
>>> +
>>
>>
>> IMO those should be 64bit bit flags, like in other fs,
>> see more below.
>>
>>>   /* private information held for overlayfs's superblock */
>>>   struct ovl_fs {
>>>          struct vfsmount *upper_mnt;
>>> @@ -55,6 +64,8 @@ struct ovl_fs {
>>>          /* Did we take the inuse lock? */
>>>          bool upperdir_locked;
>>>          bool workdir_locked;
>>> +       /* features */
>>> +       struct ovl_feature feature;
>>>   };
>>>
>>>   /* private information held for every overlayfs dentry */
>>> @@ -94,6 +105,11 @@ static inline struct ovl_inode *OVL_I(struct inode
>>> *inode)
>>>          return container_of(inode, struct ovl_inode, vfs_inode);
>>>   }
>>>
>>> +static inline struct ovl_fs *OVL_FS(struct super_block *sb)
>>> +{
>>> +       return sb->s_fs_info;
>>> +}
>>> +
>>>   static inline struct dentry *ovl_upperdentry_dereference(struct
>>> ovl_inode *oi)
>>>   {
>>>          return READ_ONCE(oi->__upperdentry);
>>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>>> index 7c24619ae7fc..10772ae237b4 100644
>>> --- a/fs/overlayfs/super.c
>>> +++ b/fs/overlayfs/super.c
>>> @@ -184,6 +184,272 @@ static const struct dentry_operations
>>> ovl_reval_dentry_operations = {
>>>          .d_weak_revalidate = ovl_dentry_weak_revalidate,
>>>   };
>>>
>>> +static const char *ovl_feature_compat_supp[] = {
>>> +       NULL
>>> +};
>>> +
>>> +static const char *ovl_feature_ro_compat_supp[] = {
>>> +       NULL
>>> +};
>>> +
>>> +static const char *ovl_feature_incompat_supp[] = {
>>> +       NULL
>>> +};
>>> +
>>> +/*
>>> + * Get overlay feature from the real root dentry.
>>> + *
>>> + * @realroot: real underlying root dir dentry
>>> + * @set: feature set xattr name
>>> + * Return feature string if success, NULL if no feature and
>>> + * error number if error.
>>> + */
>>> +static char *ovl_get_feature(struct dentry *realroot, const char *set)
>>> +{
>>> +       int res;
>>> +       char *buf;
>>> +
>>> +       res = vfs_getxattr(realroot, set, NULL, 0);
>>> +       if (res <= 0) {
>>> +               if (res == -EOPNOTSUPP || res == -ENODATA)
>>> +                       res = 0;
>>> +               return ERR_PTR(res);
>>> +       }
>>> +
>>> +       buf = kmalloc(res + 1, GFP_KERNEL);
>>> +       if (!buf)
>>> +               return ERR_PTR(-ENOMEM);
>>> +
>>> +       res = vfs_getxattr(realroot, set, buf, res);
>>> +       if (res <= 0) {
>>> +               kfree(buf);
>>> +               return ERR_PTR(res);
>>> +       }
>>> +
>>> +       buf[res] = '\0';
>>> +       return buf;
>>> +}
>>> +
>>> +/*
>>> + * Add new feature to (or generate new) features string, set update flag
>>> + * if new feature added.
>>> + *
>>> + * @feature: output feature string
>>> + * @add: feature want to add
>>> + * @update: set when new feature add, no change otherwise
>>> + * Return 0 if success, error number otherwise.
>>> + */
>>> +static int ovl_update_feature(char **feature, const char *add,
>>> +                             bool *update)
>>> +{
>>> +       char *p = *feature;
>>> +       char *tmp;
>>> +       int size;
>>> +
>>> +       if (p && strstr(p, add))
>>> +               return 0;
>>> +
>>> +       size = strlen(add) + 1;
>>> +       if (p) {
>>> +               size += strlen(p) + 1;
>>> +               tmp = kmalloc(size, GFP_KERNEL);
>>> +               if (!tmp)
>>> +                       return -ENOMEM;
>>> +
>>> +               snprintf(tmp, size, "%s,%s", p, add);
>>> +               kfree(p);
>>> +       } else {
>>> +               tmp = kmalloc(size, GFP_KERNEL);
>>> +               if (!tmp)
>>> +                       return -ENOMEM;
>>> +
>>> +               strncpy(tmp, add, size);
>>> +       }
>>> +
>>> +       if (update)
>>> +               *update = true;
>>> +       *feature = tmp;
>>> +       return 0;
>>> +}
>>> +
>>> +/*
>>> + * Parse features and split out unknown features, set update flag
>>> + * if new feature added.
>>> + *
>>> + * @features: input feature string which need to parse
>>> + * @supp: support feature list
>>> + * @feature: output feature string
>>> + * @unknown: unknown feature string
>>> + * @update: set when new feature add, no change otherwise
>>> + * Return 0 if success, errno otherwise.
>>> + */
>>> +static int ovl_parse_feature(char *features, const char *supp[],
>>> +                            char **feature, const char **unknown,
>>> +                            bool *update)
>>> +{
>>> +       const char **fs;
>>> +       char *tmpf = NULL, *tmpu = NULL;
>>> +       char *p;
>>> +       int err = 0;
>>> +
>>> +       if (!features)
>>> +               return 0;
>>> +
>>> +       while ((p = strsep(&features, ",")) != NULL) {
>>> +               /* Add to or generate parsed feature string */
>>> +               err = ovl_update_feature(&tmpf, p, update);
>>> +               if (err)
>>> +                       goto err;
>>> +
>>> +               /* Check support or not */
>>> +               for (fs = supp; *fs; fs++) {
>>> +                       if (strcmp(p, *fs) == 0)
>>> +                               break;
>>> +               }
>>> +
>>> +               if (*fs)
>>> +                       continue;
>>> +
>>> +               /* Non-supported feature found, add to unknown */
>>> +               err = ovl_update_feature(&tmpu, p, NULL);
>>> +               if (err)
>>> +                       goto err;
>>> +       }
>>> +
>>> +       *feature = tmpf;
>>> +       *unknown = tmpu;
>>> +       return 0;
>>> +err:
>>> +       kfree(tmpf);
>>> +       kfree(tmpu);
>>> +       return err;
>>> +}
>>> +
>>
>>
>> The past ~200 lines are a re-implementation (probably buggy) of tokenize
>> and for what? saving the features as strings in xattrs is questionable -
>> it serves no purpose other than human convenience when reading features
>> xattrs.
>> We can store a 64bit binary flags blob in big endian order, just like
>
>
> IIUC, I find ext4 and btrfs use little-endian but xfs use big-endian, is
> little-endian better? because our cpu endian is more likely use
> little-endian by default.

I have no strong feeling. I was under the impression that file systems
ext4 included usually stores on-disk data as big-endiad.
The only slight advantage of big-endian for humans in this context
is that getfattr -e hex displays a bitmask that is easier to read
at least when you look at the #define values of the flags.

>
>> any other fs,
>> and then we need no parsing, no string find and no string insertion
>> helpers.
>>
>> Not to mention that some fs have limited size of xattr, which is going
>> to limit us in
>> the future with the amount of features and feature name lengths.
>>
>
> It's fine to me, I was trying to use u64 bit mask but found it's not
> readable, so I chose to use strings, but it has a lot of disadvantages as
> you said. I'd like to change to use u64 bit mask, and it means there is one
> more thing should do, we should introduce a feature translation tools
> (something like "dumpe2fs -h" or "tune2fs -l", and the feature xattr is more
> like a simple overlay super block) to show features to user, right?
>

It's nice to have and optional IMO, but not a must.
Since fsck.overlay is going to play a big part in the transition from
v1 to v2, detecting and setting features, I think it could also be the tool
to report existing features (-h flag?).

Thanks,
Amir.

Re: [RFC PATCH 0/4] ovl: implement overlay feature set

[Amir Goldstein]

On Sat, Mar 24, 2018 at 3:57 PM, zhangyi (F) <yizhang089@gmail.com> wrote:
> Hi Amir, thanks for your suggestions.
>
>
> On 2018/3/23 20:32, Amir Goldstein Wrote:
>>
>> On Thu, Mar 22, 2018 at 1:35 PM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>
>>> Hi All,
>>>
>>> This is overlay feature set support patches, introduce incompatable,
>>> read-only compatable and incompatable feature set, store on the upper
>>> root dir via "trusted.overlay.feature_[compat|ro_compat|incompat]"
>>> xattrs.
>>>
>>
>> Hi Zhangyi!
>>
>> Thanks again for working on this.
>>
>> For the "features" feature to be complete I think one more thing is
>> required.
>>
>> I suggest to refer to the era before "features" as overlayfs on-disk
>> format v1
>> and to the era after "features" + fsck.overlayfs as on-disk format v2.
>> On-disk format v2 is identified by the existence of the features xattrs,
>> even
>> if the are empty.
>>
>> Every layer in the overlay can be of either format v1 or v2, but when you
>> run
>> fsck.overlay it will scan all layers for undeclared features and
>> "upgrade" the layers
>> to v2, by explicitly stating the empty or non-empty feature sets of the
>> layer.
>>  > So the thing that is missing IMO is -
>> a Kconfig option (say OVERLAY_FS_V2 ot OVERLAY_FS_FEATURES)
>> which defaults to n, but allows distros to opt-in for "v2 only".
>> With that config option set, overlay will refuse to mount v1 layers, or in
>> other
>> words, fsck.overlay must be run before first time mount, or a simple
>> mkfs.overlay
>> to initialize the features xattrs
>>
> Let me see, if "OVERLAY_FS_V2=y",
> - overlay will refuse to mount overlayfs which was mounted on old kernel,
> user should run fsck.overlay to check features in each layer
> and set feature xattr on the layer's root dir. So the feature xattr on one
> root dir only contain this layer's features.
> - overlay will refuse to mount overlayfs whose upper is empty(no feature
> xattr), maybe new created image and never mounted before,
> uesr should run a simple mkfs.overlay to init the feature xattr for all
> layers (maybe accept -o options to enable features).

Yes.

>
> and if "OVERLAY_FS_V2=n",
> This is what we do now in kernel, features xattr will set in the upper root
> dir according to mount options (or maybe where we set xattrs used by
> features, like redirect). And one more thing should do is init an empty
> feature xattr if user don't give any feature mount options.
>
> Right?

Yes.

>
>> Another option is to relax the requirement for declaring lower layers
>> features
>> and leave only the requirement that upper root dir is either empty or has
>> the
>> features xattrs. In any case, on first mount with an empty upper dir,
>> features xattrs
>> should be initialized. Maybe that calls for yet another Kconfig option,
>> i.e.
>> OVERLAY_FS_LOWER_V2.
>>
> Sorry, I don't follow this option.
> Do you mean this option is a sub-option of OVERLAY_FS_V2 ?
> If "OVERLAY_FS_LOWER_V2=y", refuse to mount just if there is no feature
> xattrs on the upper root dir (empty or non-empty will pass). Introduce this
> option because overlayfs only set feature xattrs on the upper layer, so
> there is no feature xattrs on the lower root dir, right or some other
> reasons ?

Right. There are use cases, like lower is read-only media where setting
feature xattr on lower is not an option.

We must support this use case.
So either we relax the requirement for feature xattr to exist in lower
and assume that no features xattr is same as empty features xattr
or we introduce a config option OVERLAY_FS_LOWER_V2 to
allow distro to make this decision.

>
> Now, the quesion is:
> The feature xattrs on the upper root dir should contain all features in the
> overlayfs(1), or features just in the upper layer(2), not sure which one is
> better ?
>
> If (1), say "this overlayfs image have index feature, not this layer", it's
> easy to do but we will meet the indexdir problem you mentioned in 2/4.
> if (2), the auto fix (e.g. fix redirect feature if we detect redirect xattr)
> becomes useless becase we cannot set feature xattr to the lower root dir, so
> need to remove this implement or give a warn to ask fsck.overlay to handle
> this. But we can distinguish features in the upper layer and lowers layer,
> maybe useful in the future.
>

Right. if detecting redirect in lower layers is important we can introduce a
different set of features (i.e. trusted.overlay.merge_XXX_features) so we
do not polute the layer specific detected features.
I think a warning and advice to run fsck is good enough for now.
patch 4/4 is still relevant for redirect that is detected in upper layer.

This is in line with the pylosophy that lower layers are allowed to be
"bare" v1 with no features, but lower layers that were once an upper
layer are expected to be converted to v2. If someone has an old overlay
with lower layers that were once v1 upper layers, then this should trigger
a warning and running fsck is the recommended remedy.

Thanks,
Amir.