[PATCH v2 00/23] Overlayfs consistency verification with full index

[PATCH v2 00/23] Overlayfs consistency verification with full index

[Amir Goldstein]

Miklos,

This is the 2nd revision of the prep patch series for NFS export.
I am posing the prep patches only to overlayfs list and will post
NFS export implementation to fsdevel.

In the 1st revision the mount option 'index=all' was used to request
a full index. This revision re-brands the feature as 'verify=on',
including a config/module option and documentation that explains the
benefits of full index (i.e. avoid multiple redirect).

The prep series goes on a bit beyond implementation of 'verify=on'.
Patches 17-23 create the whiteout index entries and make some more
changes that are prerequisites for NFS export implementation.

To sanity test full index, I have implemented _overlay_check_fs()
helper for xfstests [3], which is a very basic "fsck" of the index dir.
This helper is called at the end of each overlay xfstest to verify the
sanity of the index entries. There is also a new test to verify multiple
redirect dir is detected on lookup.

The series is based on top of my ovl-fixes [1] branch, which includes
some patches I had already posted. The complete NFS export work is
available here [2].

Amir.

[1] https://github.com/amir73il/linux/commits/ovl-fixes
[2] https://github.com/amir73il/linux/commits/ovl-nfs-export-v2
[3] https://github.com/amir73il/xfstests/commits/ovl-index-all

Amir Goldstein (23):
  ovl: disable index when no xattr support
  ovl: ignore index mount option when no upper layer
  ovl: store layer index in ovl_layer
  ovl: factor out ovl_check_origin_fh()
  ovl: pass ovl_layer array to ovl_check_origin_fh()
  ovl: add support for "verify" feature
  ovl: verify stored origin fh matches lower dir
  ovl: unbless lower st_ino of files under unverified redirected dir
  ovl: lookup index for directories
  ovl: verify whiteout index entries on mount
  ovl: verify directory index entries on mount
  ovl: cleanup temp index entries
  ovl: create ovl_need_index() helper
  ovl: index all files on copy up with 'verify=on'
  ovl: index directories on copy up with 'verify=on'
  ovl: cleanup dir index when dir nlink drops to zero
  ovl: whiteout index when union nlink drops to zero
  ovl: whiteout orphan index entries on mount
  ovl: factor out ovl_get_index_fh() helper
  ovl: do not pass overlay dentry to ovl_get_inode()
  ovl: grab i_count reference of lower inode
  ovl: use d_splice_alias() in place of d_add() in lookup
  ovl: copy up of disconnected dentries

 Documentation/filesystems/overlayfs.txt |  16 ++
 fs/dcache.c                             |   1 +
 fs/overlayfs/Kconfig                    |  18 ++
 fs/overlayfs/copy_up.c                  | 179 ++++++++++---
 fs/overlayfs/dir.c                      |  58 +++--
 fs/overlayfs/inode.c                    |  38 ++-
 fs/overlayfs/namei.c                    | 442 +++++++++++++++++++++++---------
 fs/overlayfs/overlayfs.h                |  29 ++-
 fs/overlayfs/ovl_entry.h                |   3 +
 fs/overlayfs/readdir.c                  |  37 ++-
 fs/overlayfs/super.c                    |  74 ++++--
 fs/overlayfs/util.c                     |  94 +++++--
 12 files changed, 752 insertions(+), 237 deletions(-)

-- 
2.7.4

[PATCH v2 01/23] ovl: disable index when no xattr support

[Amir Goldstein]

Overlayfs falls back to index=off if lower/upper fs does not support
file handles. Do the same if upper fs does not support xattr.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 5f6d2385c0b3..5d5e2b39ba69 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -967,7 +967,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 	err = ovl_do_setxattr(ofs->workdir, OVL_XATTR_OPAQUE, "0", 1, 0);
 	if (err) {
 		ofs->noxattr = true;
-		pr_warn("overlayfs: upper fs does not support xattr.\n");
+		ofs->config.index = false;
+		pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off.\n");
 		err = 0;
 	} else {
 		vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
-- 
2.7.4

[PATCH v2 02/23] ovl: ignore index mount option when no upper layer

[Amir Goldstein]

For non-upper overlay mount, the value of index mount option is moot.
When mounter tries to set this mount options, emit a warning and leave
the value at the default so it won't show up in /proc/mounts.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 5d5e2b39ba69..7e33c83c5a07 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -496,12 +496,18 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		}
 	}
 
-	/* Workdir is useless in non-upper mount */
-	if (!config->upperdir && config->workdir) {
-		pr_info("overlayfs: option \"workdir=%s\" is useless in a non-upper mount, ignore\n",
-			config->workdir);
-		kfree(config->workdir);
-		config->workdir = NULL;
+	if (!config->upperdir) {
+		/* Workdir is useless in non-upper mount */
+		if (config->workdir) {
+			pr_info("overlayfs: option \"workdir=%s\" is useless in a non-upper mount, ignore\n",
+				config->workdir);
+			kfree(config->workdir);
+			config->workdir = NULL;
+		}
+		if (config->index != ovl_index_def) {
+			pr_info("overlayfs: option \"index\" is useless in a non-upper mount, ignore\n");
+			config->index = ovl_index_def;
+		}
 	}
 
 	return ovl_parse_redirect_mode(config, config->redirect_mode);
@@ -698,7 +704,8 @@ static int ovl_lower_dir(const char *name, struct path *path,
 	 * The inodes index feature needs to encode and decode file
 	 * handles, so it requires that all layers support them.
 	 */
-	if (ofs->config.index && !ovl_can_decode_fh(path->dentry->d_sb)) {
+	if (ofs->config.upperdir && ofs->config.index &&
+	    !ovl_can_decode_fh(path->dentry->d_sb)) {
 		ofs->config.index = false;
 		pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off.\n", name);
 	}
-- 
2.7.4

[PATCH v2 03/23] ovl: store layer index in ovl_layer

[Amir Goldstein]

Store the fs root layer index inside ovl_layer struct, so we can
get the root fs layer index from merge dir lower layer instead of
find it with ovl_find_layer() helper.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c     | 17 +----------------
 fs/overlayfs/ovl_entry.h |  2 ++
 fs/overlayfs/super.c     |  1 +
 3 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 71db9a966d88..a48ee02c4524 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -572,18 +572,6 @@ int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
 	return (idx < oe->numlower) ? idx + 1 : -1;
 }
 
-static int ovl_find_layer(struct ovl_fs *ofs, struct ovl_path *path)
-{
-	int i;
-
-	for (i = 0; i < ofs->numlower; i++) {
-		if (ofs->lower_layers[i].mnt == path->layer->mnt)
-			break;
-	}
-
-	return i;
-}
-
 /* Fix missing 'origin' xattr */
 static int ovl_fix_origin(struct dentry *dentry, struct dentry *lower,
 			  struct dentry *upper)
@@ -733,11 +721,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 		if (d.redirect && d.redirect[0] == '/' && poe != roe) {
 			poe = roe;
-
 			/* Find the current layer on the root dentry */
-			i = ovl_find_layer(ofs, &lower);
-			if (WARN_ON(i == ofs->numlower))
-				break;
+			i = lower.layer->idx - 1;
 		}
 	}
 
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 9d0bc03bf6e4..608e48755070 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -22,6 +22,8 @@ struct ovl_config {
 struct ovl_layer {
 	struct vfsmount *mnt;
 	dev_t pseudo_dev;
+	/* Index of this layer in fs root (upper == 0) */
+	int idx;
 };
 
 struct ovl_path {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 7e33c83c5a07..2b58620eedf0 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1111,6 +1111,7 @@ static int ovl_get_lower_layers(struct ovl_fs *ofs, struct path *stack,
 
 		ofs->lower_layers[ofs->numlower].mnt = mnt;
 		ofs->lower_layers[ofs->numlower].pseudo_dev = dev;
+		ofs->lower_layers[ofs->numlower].idx = i + 1;
 		ofs->numlower++;
 
 		/* Check if all lower layers are on same sb */
-- 
2.7.4

[PATCH v2 04/23] ovl: factor out ovl_check_origin_fh()

[Amir Goldstein]

Re-factor ovl_check_origin() and ovl_get_origin(), so origin fh xattr is
read from upper inode only once during lookup with multiple lower layers
and only once when verifying index entry origin.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c | 142 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 92 insertions(+), 50 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index a48ee02c4524..4cc8fb64c879 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -87,9 +87,36 @@ static int ovl_acceptable(void *ctx, struct dentry *dentry)
 	return 1;
 }
 
+/*
+ * Check validity of an overlay file handle buffer.
+ *
+ * Return 0 for a valid file handle.
+ * Return -ENODATA for "origin unknown".
+ * Return <0 for an invalid file handle.
+ */
+static int ovl_check_fh_len(struct ovl_fh *fh, int fh_len)
+{
+	if (fh_len < sizeof(struct ovl_fh) || fh_len < fh->len)
+		return -EINVAL;
+
+	if (fh->magic != OVL_FH_MAGIC)
+		return -EINVAL;
+
+	/* Treat larger version and unknown flags as "origin unknown" */
+	if (fh->version > OVL_FH_VERSION || fh->flags & ~OVL_FH_FLAG_ALL)
+		return -ENODATA;
+
+	/* Treat endianness mismatch as "origin unknown" */
+	if (!(fh->flags & OVL_FH_FLAG_ANY_ENDIAN) &&
+	    (fh->flags & OVL_FH_FLAG_BIG_ENDIAN) != OVL_FH_FLAG_CPU_ENDIAN)
+		return -ENODATA;
+
+	return 0;
+}
+
 static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
 {
-	int res;
+	int res, err;
 	struct ovl_fh *fh = NULL;
 
 	res = vfs_getxattr(dentry, OVL_XATTR_ORIGIN, NULL, 0);
@@ -102,7 +129,7 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
 	if (res == 0)
 		return NULL;
 
-	fh  = kzalloc(res, GFP_KERNEL);
+	fh = kzalloc(res, GFP_KERNEL);
 	if (!fh)
 		return ERR_PTR(-ENOMEM);
 
@@ -110,20 +137,12 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
 	if (res < 0)
 		goto fail;
 
-	if (res < sizeof(struct ovl_fh) || res < fh->len)
-		goto invalid;
-
-	if (fh->magic != OVL_FH_MAGIC)
+	err = ovl_check_fh_len(fh, res);
+	if (err < 0) {
+		if (err == -ENODATA)
+			goto out;
 		goto invalid;
-
-	/* Treat larger version and unknown flags as "origin unknown" */
-	if (fh->version > OVL_FH_VERSION || fh->flags & ~OVL_FH_FLAG_ALL)
-		goto out;
-
-	/* Treat endianness mismatch as "origin unknown" */
-	if (!(fh->flags & OVL_FH_FLAG_ANY_ENDIAN) &&
-	    (fh->flags & OVL_FH_FLAG_BIG_ENDIAN) != OVL_FH_FLAG_CPU_ENDIAN)
-		goto out;
+	}
 
 	return fh;
 
@@ -139,22 +158,17 @@ static struct ovl_fh *ovl_get_origin_fh(struct dentry *dentry)
 	goto out;
 }
 
-static struct dentry *ovl_get_origin(struct dentry *dentry,
-				     struct vfsmount *mnt)
+static struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt)
 {
-	struct dentry *origin = NULL;
-	struct ovl_fh *fh = ovl_get_origin_fh(dentry);
+	struct dentry *origin;
 	int bytes;
 
-	if (IS_ERR_OR_NULL(fh))
-		return (struct dentry *)fh;
-
 	/*
 	 * Make sure that the stored uuid matches the uuid of the lower
 	 * layer where file handle will be decoded.
 	 */
 	if (!uuid_equal(&fh->uuid, &mnt->mnt_sb->s_uuid))
-		goto out;
+		return NULL;
 
 	bytes = (fh->len - offsetof(struct ovl_fh, fid));
 	origin = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
@@ -164,22 +178,15 @@ static struct dentry *ovl_get_origin(struct dentry *dentry,
 		/* Treat stale file handle as "origin unknown" */
 		if (origin == ERR_PTR(-ESTALE))
 			origin = NULL;
-		goto out;
+		return origin;
 	}
 
-	if (ovl_dentry_weird(origin) ||
-	    ((d_inode(origin)->i_mode ^ d_inode(dentry)->i_mode) & S_IFMT))
-		goto invalid;
+	if (ovl_dentry_weird(origin)) {
+		dput(origin);
+		return NULL;
+	}
 
-out:
-	kfree(fh);
 	return origin;
-
-invalid:
-	pr_warn_ratelimited("overlayfs: invalid origin (%pd2)\n", origin);
-	dput(origin);
-	origin = NULL;
-	goto out;
 }
 
 static bool ovl_is_opaquedir(struct dentry *dentry)
@@ -284,9 +291,9 @@ static int ovl_lookup_layer(struct dentry *base, struct ovl_lookup_data *d,
 }
 
 
-static int ovl_check_origin(struct dentry *upperdentry,
-			    struct ovl_path *lower, unsigned int numlower,
-			    struct ovl_path **stackp, unsigned int *ctrp)
+static int ovl_check_origin_fh(struct ovl_fh *fh, struct dentry *upperdentry,
+			       struct ovl_path *lower, unsigned int numlower,
+			       struct ovl_path **stackp)
 {
 	struct vfsmount *mnt;
 	struct dentry *origin = NULL;
@@ -294,18 +301,20 @@ static int ovl_check_origin(struct dentry *upperdentry,
 
 	for (i = 0; i < numlower; i++) {
 		mnt = lower[i].layer->mnt;
-		origin = ovl_get_origin(upperdentry, mnt);
-		if (IS_ERR(origin))
-			return PTR_ERR(origin);
-
+		origin = ovl_decode_fh(fh, mnt);
 		if (origin)
 			break;
 	}
 
 	if (!origin)
-		return 0;
+		return -ESTALE;
+	else if (IS_ERR(origin))
+		return PTR_ERR(origin);
+
+	if (!ovl_is_whiteout(upperdentry) &&
+	    ((d_inode(origin)->i_mode ^ d_inode(upperdentry)->i_mode) & S_IFMT))
+		goto invalid;
 
-	BUG_ON(*ctrp);
 	if (!*stackp)
 		*stackp = kmalloc(sizeof(struct ovl_path), GFP_KERNEL);
 	if (!*stackp) {
@@ -313,9 +322,41 @@ static int ovl_check_origin(struct dentry *upperdentry,
 		return -ENOMEM;
 	}
 	**stackp = (struct ovl_path){.dentry = origin, .layer = lower[i].layer};
-	*ctrp = 1;
 
 	return 0;
+
+invalid:
+	pr_warn_ratelimited("overlayfs: invalid origin (%pd2, ftype=%x, origin ftype=%x).\n",
+			    upperdentry, d_inode(upperdentry)->i_mode & S_IFMT,
+			    d_inode(origin)->i_mode & S_IFMT);
+	dput(origin);
+	return -EIO;
+}
+
+static int ovl_check_origin(struct dentry *upperdentry,
+			    struct ovl_path *lower, unsigned int numlower,
+			    struct ovl_path **stackp, unsigned int *ctrp)
+{
+	struct ovl_fh *fh = ovl_get_origin_fh(upperdentry);
+	int err;
+
+	if (IS_ERR_OR_NULL(fh))
+		return PTR_ERR(fh);
+
+	err = ovl_check_origin_fh(fh, upperdentry, lower, numlower, stackp);
+	kfree(fh);
+
+	if (err) {
+		if (err == -ESTALE)
+			return 0;
+		return err;
+	}
+
+	if (WARN_ON(*ctrp))
+		return -EIO;
+
+	*ctrp = 1;
+	return 0;
 }
 
 /*
@@ -389,7 +430,6 @@ int ovl_verify_index(struct dentry *index, struct ovl_path *lower,
 	size_t len;
 	struct ovl_path origin = { };
 	struct ovl_path *stack = &origin;
-	unsigned int ctr = 0;
 	int err;
 
 	if (!d_inode(index))
@@ -420,16 +460,18 @@ int ovl_verify_index(struct dentry *index, struct ovl_path *lower,
 		goto fail;
 
 	err = -EINVAL;
-	if (hex2bin((u8 *)fh, index->d_name.name, len) || len != fh->len)
+	if (hex2bin((u8 *)fh, index->d_name.name, len))
+		goto fail;
+
+	err = ovl_check_fh_len(fh, len);
+	if (err)
 		goto fail;
 
 	err = ovl_verify_origin_fh(index, fh);
 	if (err)
 		goto fail;
 
-	err = ovl_check_origin(index, lower, numlower, &stack, &ctr);
-	if (!err && !ctr)
-		err = -ESTALE;
+	err = ovl_check_origin_fh(fh, index, lower, numlower, &stack);
 	if (err)
 		goto fail;
 
-- 
2.7.4

[PATCH v2 05/23] ovl: pass ovl_layer array to ovl_check_origin_fh()

[Amir Goldstein]

Pass the fs instance lower_layer array instead of lowerstack array to
ovl_check_origin_fh(), because the dentry member of ovl_path plays no
part in this helper.

This change simplifies the argument list of ovl_check_origin(),
ovl_cleanup_index() and ovl_verify_index().

We pass lower_layer array and numlower and not struct ovl_fs to
ovl_check_origin_fh(), because we are going to use this helper for
decoding upper layer file handles for NFS export support.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c     | 25 +++++++++++--------------
 fs/overlayfs/overlayfs.h |  9 ++++-----
 fs/overlayfs/readdir.c   | 12 ++++++------
 fs/overlayfs/super.c     |  5 +----
 4 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 4cc8fb64c879..46a3e31b0225 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -292,16 +292,14 @@ static int ovl_lookup_layer(struct dentry *base, struct ovl_lookup_data *d,
 
 
 static int ovl_check_origin_fh(struct ovl_fh *fh, struct dentry *upperdentry,
-			       struct ovl_path *lower, unsigned int numlower,
+			       struct ovl_layer *layers, unsigned int numlayers,
 			       struct ovl_path **stackp)
 {
-	struct vfsmount *mnt;
 	struct dentry *origin = NULL;
 	int i;
 
-	for (i = 0; i < numlower; i++) {
-		mnt = lower[i].layer->mnt;
-		origin = ovl_decode_fh(fh, mnt);
+	for (i = 0; i < numlayers; i++) {
+		origin = ovl_decode_fh(fh, layers[i].mnt);
 		if (origin)
 			break;
 	}
@@ -321,7 +319,7 @@ static int ovl_check_origin_fh(struct ovl_fh *fh, struct dentry *upperdentry,
 		dput(origin);
 		return -ENOMEM;
 	}
-	**stackp = (struct ovl_path){.dentry = origin, .layer = lower[i].layer};
+	**stackp = (struct ovl_path){.dentry = origin, .layer = &layers[i]};
 
 	return 0;
 
@@ -333,8 +331,7 @@ static int ovl_check_origin_fh(struct ovl_fh *fh, struct dentry *upperdentry,
 	return -EIO;
 }
 
-static int ovl_check_origin(struct dentry *upperdentry,
-			    struct ovl_path *lower, unsigned int numlower,
+static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
 			    struct ovl_path **stackp, unsigned int *ctrp)
 {
 	struct ovl_fh *fh = ovl_get_origin_fh(upperdentry);
@@ -343,7 +340,8 @@ static int ovl_check_origin(struct dentry *upperdentry,
 	if (IS_ERR_OR_NULL(fh))
 		return PTR_ERR(fh);
 
-	err = ovl_check_origin_fh(fh, upperdentry, lower, numlower, stackp);
+	err = ovl_check_origin_fh(fh, upperdentry, ofs->lower_layers,
+				  ofs->numlower, stackp);
 	kfree(fh);
 
 	if (err) {
@@ -423,8 +421,7 @@ int ovl_verify_origin(struct dentry *dentry, struct dentry *origin,
  * OVL_XATTR_ORIGIN and that origin file handle can be decoded to lower path.
  * Return 0 on match, -ESTALE on mismatch or stale origin, < 0 on error.
  */
-int ovl_verify_index(struct dentry *index, struct ovl_path *lower,
-		     unsigned int numlower)
+int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 {
 	struct ovl_fh *fh = NULL;
 	size_t len;
@@ -471,7 +468,8 @@ int ovl_verify_index(struct dentry *index, struct ovl_path *lower,
 	if (err)
 		goto fail;
 
-	err = ovl_check_origin_fh(fh, index, lower, numlower, &stack);
+	err = ovl_check_origin_fh(fh, index, ofs->lower_layers,
+				  ofs->numlower, &stack);
 	if (err)
 		goto fail;
 
@@ -689,8 +687,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			 * number - it's the same as if we held a reference
 			 * to a dentry in lower layer that was moved under us.
 			 */
-			err = ovl_check_origin(upperdentry, roe->lowerstack,
-					       roe->numlower, &stack, &ctr);
+			err = ovl_check_origin(ofs, upperdentry, &stack, &ctr);
 			if (err)
 				goto out_put_upper;
 		}
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index d1cfa69c98b5..d55afb6646b0 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -251,11 +251,11 @@ static inline bool ovl_is_impuredir(struct dentry *dentry)
 /* namei.c */
 int ovl_verify_origin(struct dentry *dentry, struct dentry *origin,
 		      bool is_upper, bool set);
-int ovl_verify_index(struct dentry *index, struct ovl_path *lower,
-		     unsigned int numlower);
+int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index);
 int ovl_get_index_name(struct dentry *origin, struct qstr *name);
 int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
-struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags);
+struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
+			  unsigned int flags);
 bool ovl_lower_positive(struct dentry *dentry);
 
 /* readdir.c */
@@ -267,8 +267,7 @@ void ovl_dir_cache_free(struct inode *inode);
 int ovl_check_d_type_supported(struct path *realpath);
 void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
 			 struct dentry *dentry, int level);
-int ovl_indexdir_cleanup(struct dentry *dentry, struct vfsmount *mnt,
-			 struct ovl_path *lower, unsigned int numlower);
+int ovl_indexdir_cleanup(struct ovl_fs *ofs);
 
 /* inode.c */
 int ovl_set_nlink_upper(struct dentry *dentry);
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 001c79297bc5..0cdd1ad0b4f6 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1023,13 +1023,13 @@ void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
 	}
 }
 
-int ovl_indexdir_cleanup(struct dentry *dentry, struct vfsmount *mnt,
-			 struct ovl_path *lower, unsigned int numlower)
+int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 {
 	int err;
+	struct dentry *indexdir = ofs->indexdir;
 	struct dentry *index = NULL;
-	struct inode *dir = dentry->d_inode;
-	struct path path = { .mnt = mnt, .dentry = dentry };
+	struct inode *dir = indexdir->d_inode;
+	struct path path = { .mnt = ofs->upper_mnt, .dentry = indexdir };
 	LIST_HEAD(list);
 	struct rb_root root = RB_ROOT;
 	struct ovl_cache_entry *p;
@@ -1053,13 +1053,13 @@ int ovl_indexdir_cleanup(struct dentry *dentry, struct vfsmount *mnt,
 			if (p->len == 2 && p->name[1] == '.')
 				continue;
 		}
-		index = lookup_one_len(p->name, dentry, p->len);
+		index = lookup_one_len(p->name, indexdir, p->len);
 		if (IS_ERR(index)) {
 			err = PTR_ERR(index);
 			index = NULL;
 			break;
 		}
-		err = ovl_verify_index(index, lower, numlower);
+		err = ovl_verify_index(ofs, index);
 		/* Cleanup stale and orphan index entries */
 		if (err && (err == -ESTALE || err == -ENOENT))
 			err = ovl_cleanup(dir, index);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 2b58620eedf0..994d35628acf 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1062,10 +1062,7 @@ static int ovl_get_indexdir(struct ovl_fs *ofs, struct ovl_entry *oe,
 
 		/* Cleanup bad/stale/orphan index entries */
 		if (!err)
-			err = ovl_indexdir_cleanup(ofs->indexdir,
-						   ofs->upper_mnt,
-						   oe->lowerstack,
-						   oe->numlower);
+			err = ovl_indexdir_cleanup(ofs);
 	}
 	if (err || !ofs->indexdir)
 		pr_warn("overlayfs: try deleting index dir or mounting with '-o index=off' to disable inodes index.\n");
-- 
2.7.4

[PATCH v2 06/23] ovl: add support for "verify" feature

[Amir Goldstein]

Introduce the "verify" config, module and mount options.

If the "verify" feature is enabled then overlay filesystems will use
the inodes index dir to verify layers consistency.

It is possible to enable consistency verification by default during
build time with the OVERLAY_FS_VERIFY config option, during runtime
with the "verify=on" module option or on a filesystem instance basis
with the "verify=on" mount option.

The "verify" feature will prevent multiple redirects to the same lower
dir and will prevent broken hardlinks from using the same inode number.

On an overlay filesystem instance with multiple lower layers, "verify"
feature can only use the index to prevent multiple redirects from upper
dirs and not from middle layer dirs. Emit a warning about this on mount
with multiple lower layers and "verify=on".

This is going to be used for NFS export.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/Kconfig     | 18 ++++++++++++++++++
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     | 49 +++++++++++++++++++++++++++++++++++++++++-------
 fs/overlayfs/util.c      |  7 +++++++
 5 files changed, 69 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index 5ac415466861..0e4764ed4e23 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -53,3 +53,21 @@ config OVERLAY_FS_INDEX
 	  outcomes.  However, mounting the same overlay with an old kernel
 	  read-write and then mounting it again with a new kernel, will have
 	  unexpected results.
+
+config OVERLAY_FS_VERIFY
+	bool "Overlayfs: turn on verify feature by default"
+	depends on OVERLAY_FS
+	depends on OVERLAY_FS_INDEX
+	help
+	  If this config option is enabled then overlay filesystems will use
+	  the inodes index dir to verify layers consistency by default.
+	  In this case, it is still possible to turn off consistency
+	  verification globally with the "verify=off" module option or on a
+	  filesystem instance basis with the "verify=off" mount option.
+
+	  The verify feature prevents multiple redirects to the same lower dir
+	  and prevents broken hardlinks from using the same inode number.
+
+	  Note, that verify feature is not backward compatible.  That is,
+	  mounting an overlay with verification index entries on a kernel
+	  that doesn't support this feature will have unexpected results.
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index d55afb6646b0..379bb349acc4 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -194,6 +194,7 @@ const struct cred *ovl_override_creds(struct super_block *sb);
 struct super_block *ovl_same_sb(struct super_block *sb);
 bool ovl_can_decode_fh(struct super_block *sb);
 struct dentry *ovl_indexdir(struct super_block *sb);
+bool ovl_verify(struct super_block *sb);
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
 bool ovl_dentry_remote(struct dentry *dentry);
 bool ovl_dentry_weird(struct dentry *dentry);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 608e48755070..c886ed743a8a 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -17,6 +17,7 @@ struct ovl_config {
 	bool redirect_follow;
 	const char *redirect_mode;
 	bool index;
+	bool verify;
 };
 
 struct ovl_layer {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 994d35628acf..96136528861f 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -45,6 +45,11 @@ module_param_named(index, ovl_index_def, bool, 0644);
 MODULE_PARM_DESC(ovl_index_def,
 		 "Default to on or off for the inodes index feature");
 
+static bool ovl_verify_def = IS_ENABLED(CONFIG_OVERLAY_FS_VERIFY);
+module_param_named(verify, ovl_verify_def, bool, 0644);
+MODULE_PARM_DESC(ovl_verify_def,
+		 "Default to on or off for the verify feature");
+
 static void ovl_entry_stack_free(struct ovl_entry *oe)
 {
 	unsigned int i;
@@ -341,6 +346,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 		seq_printf(m, ",redirect_dir=%s", ofs->config.redirect_mode);
 	if (ofs->config.index != ovl_index_def)
 		seq_printf(m, ",index=%s", ofs->config.index ? "on" : "off");
+	if (ofs->config.verify != ovl_verify_def)
+		seq_printf(m, ",verify=%s", ofs->config.verify ? "on" : "off");
 	return 0;
 }
 
@@ -373,6 +380,8 @@ enum {
 	OPT_REDIRECT_DIR,
 	OPT_INDEX_ON,
 	OPT_INDEX_OFF,
+	OPT_VERIFY_ON,
+	OPT_VERIFY_OFF,
 	OPT_ERR,
 };
 
@@ -384,6 +393,8 @@ static const match_table_t ovl_tokens = {
 	{OPT_REDIRECT_DIR,		"redirect_dir=%s"},
 	{OPT_INDEX_ON,			"index=on"},
 	{OPT_INDEX_OFF,			"index=off"},
+	{OPT_VERIFY_ON,			"verify=on"},
+	{OPT_VERIFY_OFF,		"verify=off"},
 	{OPT_ERR,			NULL}
 };
 
@@ -487,7 +498,19 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			break;
 
 		case OPT_INDEX_OFF:
+			/* verify depends on index */
 			config->index = false;
+			config->verify = false;
+			break;
+
+		case OPT_VERIFY_ON:
+			/* verify depends on index */
+			config->index = true;
+			config->verify = true;
+			break;
+
+		case OPT_VERIFY_OFF:
+			config->verify = false;
 			break;
 
 		default:
@@ -504,9 +527,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			kfree(config->workdir);
 			config->workdir = NULL;
 		}
-		if (config->index != ovl_index_def) {
-			pr_info("overlayfs: option \"index\" is useless in a non-upper mount, ignore\n");
+		if (config->index != ovl_index_def ||
+		    config->verify != ovl_verify_def) {
+			pr_info("overlayfs: options \"index\" and \"verify\" are useless in a non-upper mount, ignore\n");
 			config->index = ovl_index_def;
+			config->verify = ovl_verify_def;
 		}
 	}
 
@@ -707,7 +732,9 @@ static int ovl_lower_dir(const char *name, struct path *path,
 	if (ofs->config.upperdir && ofs->config.index &&
 	    !ovl_can_decode_fh(path->dentry->d_sb)) {
 		ofs->config.index = false;
-		pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off.\n", name);
+		ofs->config.verify = false;
+		pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off,verify=off.\n",
+			name);
 	}
 
 	return 0;
@@ -975,7 +1002,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 	if (err) {
 		ofs->noxattr = true;
 		ofs->config.index = false;
-		pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off.\n");
+		ofs->config.verify = false;
+		pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off,verify=off.\n");
 		err = 0;
 	} else {
 		vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
@@ -985,7 +1013,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 	if (ofs->config.index &&
 	    !ovl_can_decode_fh(ofs->workdir->d_sb)) {
 		ofs->config.index = false;
-		pr_warn("overlayfs: upper fs does not support file handles, falling back to index=off.\n");
+		ofs->config.verify = false;
+		pr_warn("overlayfs: upper fs does not support file handles, falling back to index=off,verify=off.\n");
 	}
 
 out:
@@ -1146,6 +1175,8 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	} else if (!ofs->config.upperdir && stacklen == 1) {
 		pr_err("overlayfs: at least 2 lowerdir are needed while upperdir nonexistent\n");
 		goto out_err;
+	} else if (ofs->config.verify && ofs->config.upperdir && stacklen > 1) {
+		pr_warn("overlayfs: option 'verify=on' cannot verify redirects from middle layer dirs\n");
 	}
 
 	err = -ENOMEM;
@@ -1222,6 +1253,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		goto out_err;
 
 	ofs->config.index = ovl_index_def;
+	/* verify depends on index */
+	ofs->config.verify = ovl_verify_def && ovl_index_def;
 	err = ovl_parse_opt((char *) data, &ofs->config);
 	if (err)
 		goto out_err;
@@ -1276,9 +1309,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 			sb->s_flags |= SB_RDONLY;
 	}
 
-	/* Show index=off/on in /proc/mounts for any of the reasons above */
-	if (!ofs->indexdir)
+	/* Show index=off,verify=off in /proc/mounts if no indexdir */
+	if (!ofs->indexdir) {
 		ofs->config.index = false;
+		ofs->config.verify = false;
+	}
 
 	/* Never override disk quota limits or use reserved space */
 	cap_lower(cred->cap_effective, CAP_SYS_RESOURCE);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index d6bb1c9f5e7a..fb0b561b0568 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -63,6 +63,13 @@ struct dentry *ovl_indexdir(struct super_block *sb)
 	return ofs->indexdir;
 }
 
+bool ovl_verify(struct super_block *sb)
+{
+	struct ovl_fs *ofs = sb->s_fs_info;
+
+	return ofs->config.verify && ofs->config.index;
+}
+
 struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
 {
 	size_t size = offsetof(struct ovl_entry, lowerstack[numlower]);
-- 
2.7.4

[PATCH v2 07/23] ovl: verify stored origin fh matches lower dir

[Amir Goldstein]

When the "verify" feature is enabled, a directory inode found in lower
layer by name or by redirect_dir is verified against the file handle of
the copy up origin that is stored in the upper layer.

This introduces a change of behavior for the case of lower layer
modification while overlay is offline. A lower directory created or
moved offline under an exisitng upper directory, will not be merged with
that upper directory.

The "verify" feature should not be used after copying layers,
because the new lower directory inodes would fail verification.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 Documentation/filesystems/overlayfs.txt | 16 ++++++++++++++++
 fs/overlayfs/namei.c                    | 13 +++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
index e6a5f4912b6d..00e0595f3d7e 100644
--- a/Documentation/filesystems/overlayfs.txt
+++ b/Documentation/filesystems/overlayfs.txt
@@ -299,6 +299,22 @@ filesystem are not allowed.  If the underlying filesystem is changed,
 the behavior of the overlay is undefined, though it will not result in
 a crash or deadlock.
 
+When the underlying filesystems supports NFS export, overlay mount can be
+made more resilient to offline and online changes of the underlying lower
+layer by enabling the "verify" feature.
+
+On every copy_up, an NFS file handle of the lower inode, along with the
+UUID of the lower filesystem, are encoded and stored in an extended
+attribute "trusted.overlay.origin" on the upper inode.
+
+When the "verify" feature is enabled, a lookup of a merged directory, that
+found a lower directory at the lookup path or at the path pointed to by
+the "trusted.overlay.redirect" extended attribute, will verify that the
+found lower directory file handle and lower filesystem UUID match the
+origin file handle that was stored at copy_up time.  If a found lower
+directory does not match the stored origin, that directory will not be
+merged with the upper directory.
+
 Testsuite
 ---------
 
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 46a3e31b0225..56deb2785af7 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -734,6 +734,19 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			}
 		}
 
+		/*
+		 * When "verify" feature is enabled, do not merge with a lower
+		 * dir that does not match a stored origin xattr.
+		 */
+		if (upperdentry && !ctr && ovl_verify(dentry->d_sb)) {
+			err = ovl_verify_origin(upperdentry, this, false,
+						false);
+			if (err) {
+				dput(this);
+				break;
+			}
+		}
+
 		stack[ctr].dentry = this;
 		stack[ctr].layer = lower.layer;
 		ctr++;
-- 
2.7.4

[PATCH v2 08/23] ovl: unbless lower st_ino of files under unverified redirected dir

[Amir Goldstein]

On a malformed overlay, several redirected dirs can point to the same
dir on a lower layer. This presents a similar challenge as broken
hardlinks, because different objects in the overlay can return the same
st_ino/st_dev pair from stat(2).

For broken hardlinks, we do not provide constant st_ino on copy up to
avoid this inconsistency. When "verify" feature is enabled, apply
the same logic to files nested under unverified redirected dirs.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/inode.c     | 18 ++++++++++++++----
 fs/overlayfs/namei.c     |  4 ++++
 fs/overlayfs/overlayfs.h |  4 ++++
 fs/overlayfs/util.c      | 27 +++++++++++++++++++++++++++
 4 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 00b6b294272a..1b53755bd86c 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -105,12 +105,22 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 			 * Lower hardlinks may be broken on copy up to different
 			 * upper files, so we cannot use the lower origin st_ino
 			 * for those different files, even for the same fs case.
+			 *
+			 * Similarly, several redirected dirs can point to the
+			 * same dir on a lower layer. With the "verify" feature,
+			 * we do not use the lower origin st_ino, if any parent
+			 * is redirected and we haven't verified that this
+			 * redirect is unique.
+			 *
 			 * With inodes index enabled, it is safe to use st_ino
-			 * of an indexed hardlinked origin. The index validates
-			 * that the upper hardlink is not broken.
+			 * of an indexed origin. The index validates that the
+			 * upper hardlink is not broken and that a redirected
+			 * dir is the only redirect to that origin.
 			 */
-			if (is_dir || lowerstat.nlink == 1 ||
-			    ovl_test_flag(OVL_INDEX, d_inode(dentry)))
+			if (ovl_test_flag(OVL_INDEX, d_inode(dentry)) ||
+			    ((is_dir || lowerstat.nlink == 1) &&
+			     (!ovl_verify(dentry->d_sb) ||
+			      !ovl_dentry_is_renamed(dentry))))
 				stat->ino = lowerstat.ino;
 
 			if (samefs)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 56deb2785af7..8aa4724d4fb0 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -811,6 +811,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			goto out_free_oe;
 
 		OVL_I(inode)->redirect = upperredirect;
+		/* OVL_RENAMED indicates rename in any layer/path component */
+		if (d.redirect ||
+		    ovl_test_flag(OVL_RENAMED, d_inode(dentry->d_parent)))
+			ovl_set_flag(OVL_RENAMED, inode);
 		if (index)
 			ovl_set_flag(OVL_INDEX, inode);
 	}
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 379bb349acc4..46f9f097e822 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -33,7 +33,10 @@ enum ovl_flag {
 	OVL_IMPURE,
 	/* Non-merge dir that may contain whiteout entries */
 	OVL_WHITEOUTS,
+	/* Found index entry for origin inode */
 	OVL_INDEX,
+	/* Merge dir in path was redirected */
+	OVL_RENAMED,
 };
 
 /*
@@ -219,6 +222,7 @@ void ovl_dentry_set_upper_alias(struct dentry *dentry);
 bool ovl_redirect_dir(struct super_block *sb);
 const char *ovl_dentry_get_redirect(struct dentry *dentry);
 void ovl_dentry_set_redirect(struct dentry *dentry, const char *redirect);
+bool ovl_dentry_is_renamed(struct dentry *dentry);
 void ovl_inode_init(struct inode *inode, struct dentry *upperdentry,
 		    struct dentry *lowerdentry);
 void ovl_inode_update(struct inode *inode, struct dentry *upperdentry);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index fb0b561b0568..42809eae0992 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -256,6 +256,33 @@ void ovl_dentry_set_redirect(struct dentry *dentry, const char *redirect)
 
 	kfree(oi->redirect);
 	oi->redirect = redirect;
+	/* Descendant are marked RENAMED lazily by ovl_dentry_is_renamed() */
+	ovl_set_flag(OVL_RENAMED, d_inode(dentry));
+}
+
+/*
+ * Check if any component in path has been renamed.
+ */
+bool ovl_dentry_is_renamed(struct dentry *dentry)
+{
+	struct dentry *d, *tmp;
+	bool renamed;
+
+	for (d = dget(dentry); !IS_ROOT(d);) {
+		if (ovl_test_flag(OVL_RENAMED, d_inode(d)))
+			break;
+
+		tmp = dget_parent(d);
+		dput(d);
+		d = tmp;
+	}
+
+	renamed = !IS_ROOT(d);
+	dput(d);
+
+	if (renamed)
+		ovl_set_flag(OVL_RENAMED, d_inode(dentry));
+	return renamed;
 }
 
 void ovl_inode_init(struct inode *inode, struct dentry *upperdentry,
-- 
2.7.4

[PATCH v2 09/23] ovl: lookup index for directories

[Amir Goldstein]

A directory index is a directory type entry in index dir with a
"trusted.overlay.origin" xattr containing an encoded ovl_fh of the merge
directory upper dir inode.

On lookup of non-dir files, lower file is followed by origin file handle.
On lookup of dir entries, lower dir is found by name and then compared
to origin file handle. We only trust dir index if we verified that lower
dir matches origin file handle, otherwise index may be inconsistent and
we ignore it.

If we find an indexed non-upper dir or an indexed merged dir, whose
index 'origin' points to a different upper dir, that means that the lower
directory may be also referenced by another upper dir via redirect, so we
fail the lookup on inconsistency error.

Directory index entries are going to be used for NFS export.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c | 42 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 8aa4724d4fb0..6e556d2e2365 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -535,6 +535,7 @@ static struct dentry *ovl_lookup_index(struct dentry *dentry,
 	struct dentry *index;
 	struct inode *inode;
 	struct qstr name;
+	bool is_dir = d_is_dir(origin);
 	int err;
 
 	err = ovl_get_index_name(origin, &name);
@@ -558,8 +559,6 @@ static struct dentry *ovl_lookup_index(struct dentry *dentry,
 	inode = d_inode(index);
 	if (d_is_negative(index)) {
 		goto out_dput;
-	} else if (upper && d_inode(upper) != inode) {
-		goto out_dput;
 	} else if (ovl_dentry_weird(index) || ovl_is_whiteout(index) ||
 		   ((inode->i_mode ^ d_inode(origin)->i_mode) & S_IFMT)) {
 		/*
@@ -573,8 +572,25 @@ static struct dentry *ovl_lookup_index(struct dentry *dentry,
 				    index, d_inode(index)->i_mode & S_IFMT,
 				    d_inode(origin)->i_mode & S_IFMT);
 		goto fail;
-	}
+	} else if (is_dir) {
+		if (!upper) {
+			pr_warn_ratelimited("overlayfs: suspected uncovered redirected dir found (origin=%pd2, index=%pd2).\n",
+					    origin, index);
+			goto fail;
+		}
 
+		/* Verify that dir index origin points to upper dir */
+		err = ovl_verify_origin(index, upper, true, false);
+		if (err) {
+			if (err == -ESTALE) {
+				pr_warn_ratelimited("overlayfs: suspected multiply redirected dir found (upper=%pd2, origin=%pd2, index=%pd2).\n",
+						    upper, origin, index);
+			}
+			goto fail;
+		}
+	} else if (upper && d_inode(upper) != inode) {
+		goto out_dput;
+	}
 out:
 	kfree(name.name);
 	return index;
@@ -643,6 +659,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
 	struct ovl_path *stack = NULL;
 	struct dentry *upperdir, *upperdentry = NULL;
+	struct dentry *origin = NULL;
 	struct dentry *index = NULL;
 	unsigned int ctr = 0;
 	struct inode *inode = NULL;
@@ -736,7 +753,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 		/*
 		 * When "verify" feature is enabled, do not merge with a lower
-		 * dir that does not match a stored origin xattr.
+		 * dir that does not match a stored origin xattr. In any case,
+		 * only verified origin is used for index lookup.
 		 */
 		if (upperdentry && !ctr && ovl_verify(dentry->d_sb)) {
 			err = ovl_verify_origin(upperdentry, this, false,
@@ -745,6 +763,9 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 				dput(this);
 				break;
 			}
+
+			/* Bless lower dir as verified origin */
+			origin = this;
 		}
 
 		stack[ctr].dentry = this;
@@ -778,10 +799,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 	}
 
-	/* Lookup index by lower inode and verify it matches upper inode */
-	if (ctr && !d.is_dir && ovl_indexdir(dentry->d_sb)) {
-		struct dentry *origin = stack[0].dentry;
+	/*
+	 * Lookup index by lower inode and verify it matches upper inode.
+	 * We only trust dir index if we verified that lower dir matches
+	 * origin, otherwise dir index entries may be inconsistent and we
+	 * ignore them. Always lookup index of non-dir and non-upper.
+	 */
+	if (ctr && (!upperdentry || !d.is_dir))
+		origin = stack[0].dentry;
 
+	if (origin && ovl_indexdir(dentry->d_sb) &&
+	    (!d.is_dir || ovl_verify(dentry->d_sb))) {
 		index = ovl_lookup_index(dentry, upperdentry, origin);
 		if (IS_ERR(index)) {
 			err = PTR_ERR(index);
-- 
2.7.4

[PATCH v2 10/23] ovl: verify whiteout index entries on mount

[Amir Goldstein]

Whiteout index entries are used as an indication that an exported
overlay file handle should be treated as stale (i.e. after unlink
of the overlay inode).

Check on mount that whiteout index entries have a name that looks like
a valid file handle and cleanup invalid index entries.

For whiteout index entries, do not check that they also have valid
origin fh and nlink xattr, because those xattr do not exist for a
whiteout index entry.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 6e556d2e2365..b481bc4e79d9 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -435,16 +435,13 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 	/*
 	 * Directory index entries are going to be used for looking up
 	 * redirected upper dirs by lower dir fh when decoding an overlay
-	 * file handle of a merge dir. Whiteout index entries are going to be
-	 * used as an indication that an exported overlay file handle should
-	 * be treated as stale (i.e. after unlink of the overlay inode).
-	 * We don't know the verification rules for directory and whiteout
-	 * index entries, because they have not been implemented yet, so return
-	 * EINVAL if those entries are found to abort the mount to avoid
-	 * corrupting an index that was created by a newer kernel.
+	 * file handle of a merge dir.  We don't know the verification rules
+	 * for directory index entries, because they have not been implemented
+	 * yet, so return EINVAL if those entries are found to abort the mount
+	 * and to avoid corrupting an index that was created by a newer kernel.
 	 */
 	err = -EINVAL;
-	if (d_is_dir(index) || ovl_is_whiteout(index))
+	if (d_is_dir(index))
 		goto fail;
 
 	if (index->d_name.len < sizeof(struct ovl_fh)*2)
@@ -464,6 +461,14 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 	if (err)
 		goto fail;
 
+	/*
+	 * Whiteout index entries are used as an indication that an exported
+	 * overlay file handle should be treated as stale (i.e. after unlink
+	 * of the overlay inode). These entries contain no origin xattr.
+	 */
+	if (ovl_is_whiteout(index))
+		goto out;
+
 	err = ovl_verify_origin_fh(index, fh);
 	if (err)
 		goto fail;
-- 
2.7.4

[PATCH v2 11/23] ovl: verify directory index entries on mount

[Amir Goldstein]

Directory index entries should have origin xattr pointing to the real
upper dir. Verifying that the upper dir file handle is not stale is
expensive, so only verify stale directory index entries on mount if
the "verify" feature is enabled.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/dcache.c          |  1 +
 fs/overlayfs/namei.c | 95 +++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 73 insertions(+), 23 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 5c7df1df81ff..b5d5ea984ac4 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -3527,6 +3527,7 @@ bool is_subdir(struct dentry *new_dentry, struct dentry *old_dentry)
 
 	return result;
 }
+EXPORT_SYMBOL(is_subdir);
 
 static enum d_walk_ret d_genocide_kill(void *data, struct dentry *dentry)
 {
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index b481bc4e79d9..e53c54ecb7cc 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -84,7 +84,19 @@ static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
 
 static int ovl_acceptable(void *ctx, struct dentry *dentry)
 {
-	return 1;
+	/*
+	 * A non-dir origin may be disconnected, which is fine, because
+	 * we only need it for its unique inode number.
+	 */
+	if (!d_is_dir(dentry))
+		return 1;
+
+	/* Don't decode a deleted empty directory */
+	if (d_unhashed(dentry))
+		return 0;
+
+	/* Check if directory belongs to the layer we are decoding from */
+	return is_subdir(dentry, ((struct vfsmount *)ctx)->mnt_root);
 }
 
 /*
@@ -173,7 +185,7 @@ static struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt)
 	bytes = (fh->len - offsetof(struct ovl_fh, fid));
 	origin = exportfs_decode_fh(mnt, (struct fid *)fh->fid,
 				    bytes >> 2, (int)fh->type,
-				    ovl_acceptable, NULL);
+				    ovl_acceptable, mnt);
 	if (IS_ERR(origin)) {
 		/* Treat stale file handle as "origin unknown" */
 		if (origin == ERR_PTR(-ESTALE))
@@ -416,6 +428,30 @@ int ovl_verify_origin(struct dentry *dentry, struct dentry *origin,
 	goto out;
 }
 
+/* Get upper dentry from index */
+static struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
+{
+	struct ovl_fh *fh;
+	struct ovl_layer layer = { .mnt = ofs->upper_mnt };
+	struct ovl_path upper = { .layer = &layer };
+	struct ovl_path *stack = &upper;
+	int err;
+
+	if (!d_is_dir(index))
+		return dget(index);
+
+	fh = ovl_get_origin_fh(index);
+	if (IS_ERR_OR_NULL(fh))
+		return ERR_CAST(fh);
+
+	err = ovl_check_origin_fh(fh, index, &layer, 1, &stack);
+	kfree(fh);
+	if (err)
+		return ERR_PTR(err);
+
+	return upper.dentry;
+}
+
 /*
  * Verify that an index entry name matches the origin file handle stored in
  * OVL_XATTR_ORIGIN and that origin file handle can be decoded to lower path.
@@ -427,23 +463,13 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 	size_t len;
 	struct ovl_path origin = { };
 	struct ovl_path *stack = &origin;
+	struct dentry *upper = NULL;
 	int err;
 
 	if (!d_inode(index))
 		return 0;
 
-	/*
-	 * Directory index entries are going to be used for looking up
-	 * redirected upper dirs by lower dir fh when decoding an overlay
-	 * file handle of a merge dir.  We don't know the verification rules
-	 * for directory index entries, because they have not been implemented
-	 * yet, so return EINVAL if those entries are found to abort the mount
-	 * and to avoid corrupting an index that was created by a newer kernel.
-	 */
 	err = -EINVAL;
-	if (d_is_dir(index))
-		goto fail;
-
 	if (index->d_name.len < sizeof(struct ovl_fh)*2)
 		goto fail;
 
@@ -469,22 +495,45 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 	if (ovl_is_whiteout(index))
 		goto out;
 
-	err = ovl_verify_origin_fh(index, fh);
-	if (err)
-		goto fail;
+	/*
+	 * verifying directory index entries are not stale is expensive, so
+	 * only verify stale dir index if 'verify' feature is enabled.
+	 */
+	if (d_is_dir(index) && !ofs->config.verify)
+		goto out;
 
-	err = ovl_check_origin_fh(fh, index, ofs->lower_layers,
-				  ofs->numlower, &stack);
+	/*
+	 * Directory index entries should have origin xattr pointing to the
+	 * real upper dir. Non-dir index entries are hardlinks to the upper
+	 * real inode. For non-dir index, we can read the copy up origin xattr
+	 * directly from the index dentry, but for dir index we first need to
+	 * decode the upper directory.
+	 */
+	upper = ovl_index_upper(ofs, index);
+	if (IS_ERR(upper)) {
+		err = PTR_ERR(upper);
+		if (err)
+			goto fail;
+	}
+
+	err = ovl_verify_origin_fh(upper, fh);
+	dput(upper);
 	if (err)
 		goto fail;
 
-	/* Check if index is orphan and don't warn before cleaning it */
-	if (d_inode(index)->i_nlink == 1 &&
-	    ovl_get_nlink(origin.dentry, index, 0) == 0)
-		err = -ENOENT;
+	/* Check if non-dir index is orphan and don't warn before cleaning it */
+	if (!d_is_dir(index) && d_inode(index)->i_nlink == 1) {
+		err = ovl_check_origin_fh(fh, index, ofs->lower_layers,
+					  ofs->numlower, &stack);
+		if (err)
+			goto fail;
+
+		if (ovl_get_nlink(origin.dentry, index, 0) == 0)
+			err = -ENOENT;
+	}
 
-	dput(origin.dentry);
 out:
+	dput(origin.dentry);
 	kfree(fh);
 	return err;
 
-- 
2.7.4

[PATCH v2 12/23] ovl: cleanup temp index entries

[Amir Goldstein]

A previous failed attempt to create or whiteout a directory index may leave
index entries named '#%x' in the index dir.
Cleanup those temp entries on mount instead of failing the mount.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index e53c54ecb7cc..4987d7df1581 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -9,6 +9,7 @@
 
 #include <linux/fs.h>
 #include <linux/cred.h>
+#include <linux/ctype.h>
 #include <linux/namei.h>
 #include <linux/xattr.h>
 #include <linux/ratelimit.h>
@@ -452,6 +453,24 @@ static struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
 	return upper.dentry;
 }
 
+/* Is this a leftover from create/whiteout of directory index entry? */
+static bool ovl_is_temp_index(struct dentry *index)
+{
+	const char *p = index->d_name.name;
+	int len = index->d_name.len;
+
+	if (!d_is_dir(index) && !ovl_is_whiteout(index))
+		return false;
+
+	if (*p++ != '#' || len < 2)
+		return false;
+
+	while (--len > 0 && isdigit(*p))
+		p++;
+
+	return !len;
+}
+
 /*
  * Verify that an index entry name matches the origin file handle stored in
  * OVL_XATTR_ORIGIN and that origin file handle can be decoded to lower path.
@@ -469,6 +488,11 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 	if (!d_inode(index))
 		return 0;
 
+	/* Cleanup leftover from index create/cleanup attempt */
+	err = -ESTALE;
+	if (ovl_is_temp_index(index))
+		goto fail;
+
 	err = -EINVAL;
 	if (index->d_name.len < sizeof(struct ovl_fh)*2)
 		goto fail;
-- 
2.7.4

[PATCH v2 13/23] ovl: create ovl_need_index() helper

[Amir Goldstein]

The helper determines which lower file needs to be indexed
on copy up and before nlink changes.

For index=on, the helper evaluates to true for lower hardlinks.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   |  6 +-----
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 24 ++++++++++++++++++++----
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 206ececd5ae7..27856663c8c7 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -536,11 +536,7 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 {
 	int err;
 	struct ovl_fs *ofs = c->dentry->d_sb->s_fs_info;
-	bool indexed = false;
-
-	if (ovl_indexdir(c->dentry->d_sb) && !S_ISDIR(c->stat.mode) &&
-	    c->stat.nlink > 1)
-		indexed = true;
+	bool indexed = ovl_need_index(c->dentry);
 
 	if (S_ISDIR(c->stat.mode) || c->stat.nlink == 1 || indexed)
 		c->origin = true;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 46f9f097e822..86767b71e485 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -243,6 +243,7 @@ void ovl_clear_flag(unsigned long flag, struct inode *inode);
 bool ovl_test_flag(unsigned long flag, struct inode *inode);
 bool ovl_inuse_trylock(struct dentry *dentry);
 void ovl_inuse_unlock(struct dentry *dentry);
+bool ovl_need_index(struct dentry *dentry);
 int ovl_nlink_start(struct dentry *dentry, bool *locked);
 void ovl_nlink_end(struct dentry *dentry, bool locked);
 int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 42809eae0992..0254d7f0d401 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -481,6 +481,23 @@ void ovl_inuse_unlock(struct dentry *dentry)
 	}
 }
 
+/*
+ * Does this overlay dentry need to be indexed on copy up?
+ */
+bool ovl_need_index(struct dentry *dentry)
+{
+	struct dentry *lower = ovl_dentry_lower(dentry);
+
+	if (!lower || !ovl_indexdir(dentry->d_sb))
+		return false;
+
+	/* Index only lower hardlinks on copy up */
+	if (!d_is_dir(lower) && d_inode(lower)->i_nlink > 1)
+		return true;
+
+	return false;
+}
+
 /* Caller must hold OVL_I(inode)->lock */
 static void ovl_cleanup_index(struct dentry *dentry)
 {
@@ -551,11 +568,11 @@ int ovl_nlink_start(struct dentry *dentry, bool *locked)
 
 	/*
 	 * With inodes index is enabled, we store the union overlay nlink
-	 * in an xattr on the index inode. When whiting out lower hardlinks
+	 * in an xattr on the index inode. When whiting out an indexed lower,
 	 * we need to decrement the overlay persistent nlink, but before the
 	 * first copy up, we have no upper index inode to store the xattr.
 	 *
-	 * As a workaround, before whiteout/rename over of a lower hardlink,
+	 * As a workaround, before whiteout/rename over an indexed lower,
 	 * copy up to create the upper index. Creating the upper index will
 	 * initialize the overlay nlink, so it could be dropped if unlink
 	 * or rename succeeds.
@@ -563,8 +580,7 @@ int ovl_nlink_start(struct dentry *dentry, bool *locked)
 	 * TODO: implement metadata only index copy up when called with
 	 *       ovl_copy_up_flags(dentry, O_PATH).
 	 */
-	if (ovl_indexdir(dentry->d_sb) && !ovl_dentry_has_upper_alias(dentry) &&
-	    d_inode(ovl_dentry_lower(dentry))->i_nlink > 1) {
+	if (ovl_need_index(dentry) && !ovl_dentry_has_upper_alias(dentry)) {
 		err = ovl_copy_up(dentry);
 		if (err)
 			return err;
-- 
2.7.4

[PATCH v2 14/23] ovl: index all files on copy up with 'verify=on'

[Amir Goldstein]

With the 'verify' feature enabled, all files are indexed on copy up.
The copy up origin inode of an indexed non-dir can be used as a unique
identifier of the overlay object.

This is going to be used for NFS export.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/util.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 0254d7f0d401..f40351dcea7b 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -491,6 +491,10 @@ bool ovl_need_index(struct dentry *dentry)
 	if (!lower || !ovl_indexdir(dentry->d_sb))
 		return false;
 
+	/* Index all files for unique origin verification */
+	if (!d_is_dir(lower) && ovl_verify(dentry->d_sb))
+		return true;
+
 	/* Index only lower hardlinks on copy up */
 	if (!d_is_dir(lower) && d_inode(lower)->i_nlink > 1)
 		return true;
-- 
2.7.4

[PATCH v2 15/23] ovl: index directories on copy up with 'verify=on'

[Amir Goldstein]

With the 'verify' feature enabled, all dirs are indexed on copy up.
Non-dir files are copied up directly to indexdir and then hardlinked
to upper dir.

Directories are copied up to indexdir, then an index entry is created
in indexdir with origin xattr pointing to the copied up dir and then
the copied up dir is moved to upper dir.

This is going to be used for NFS export.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   | 123 +++++++++++++++++++++++++++++++++++++++++------
 fs/overlayfs/namei.c     |   2 +-
 fs/overlayfs/overlayfs.h |   6 +--
 fs/overlayfs/util.c      |   4 +-
 4 files changed, 115 insertions(+), 20 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 27856663c8c7..275a08ab0dd3 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -232,13 +232,13 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
 	return err;
 }
 
-struct ovl_fh *ovl_encode_fh(struct dentry *lower, bool is_upper)
+struct ovl_fh *ovl_encode_fh(struct dentry *origin, bool is_upper)
 {
 	struct ovl_fh *fh;
 	int fh_type, fh_len, dwords;
 	void *buf;
 	int buflen = MAX_HANDLE_SZ;
-	uuid_t *uuid = &lower->d_sb->s_uuid;
+	uuid_t *uuid = &origin->d_sb->s_uuid;
 
 	buf = kmalloc(buflen, GFP_KERNEL);
 	if (!buf)
@@ -250,7 +250,7 @@ struct ovl_fh *ovl_encode_fh(struct dentry *lower, bool is_upper)
 	 * the price or reconnecting the dentry.
 	 */
 	dwords = buflen >> 2;
-	fh_type = exportfs_encode_fh(lower, buf, &dwords, 0);
+	fh_type = exportfs_encode_fh(origin, buf, &dwords, 0);
 	buflen = (dwords << 2);
 
 	fh = ERR_PTR(-EIO);
@@ -288,8 +288,8 @@ struct ovl_fh *ovl_encode_fh(struct dentry *lower, bool is_upper)
 	return fh;
 }
 
-int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
-		   struct dentry *upper)
+int ovl_set_origin(struct dentry *dentry, struct dentry *origin,
+		   struct dentry *upper, bool is_upper)
 {
 	const struct ovl_fh *fh = NULL;
 	int err;
@@ -299,8 +299,8 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
 	 * so we can use the overlay.origin xattr to distignuish between a copy
 	 * up and a pure upper inode.
 	 */
-	if (ovl_can_decode_fh(lower->d_sb)) {
-		fh = ovl_encode_fh(lower, false);
+	if (ovl_can_decode_fh(origin->d_sb)) {
+		fh = ovl_encode_fh(origin, is_upper);
 		if (IS_ERR(fh))
 			return PTR_ERR(fh);
 	}
@@ -315,6 +315,78 @@ int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
 	return err;
 }
 
+/*
+ * Create and install index entry.
+ *
+ * Caller must hold i_mutex on indexdir.
+ */
+static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
+			    struct dentry *upper)
+{
+	struct dentry *indexdir = ovl_indexdir(dentry->d_sb);
+	struct inode *dir = d_inode(indexdir);
+	struct dentry *index = NULL;
+	struct dentry *temp = NULL;
+	struct qstr name = { };
+	int err;
+
+	/*
+	 * For now this is only used for creating index entry for directories,
+	 * because non-dir are copied up directly to index and then hardlinked
+	 * to upper dir.
+	 *
+	 * TODO: implement create index for non-dir, so we can call it when
+	 * encoding file handle for non-dir in case index does not exist.
+	 */
+	if (WARN_ON(!d_is_dir(dentry)))
+		return -EIO;
+
+	/* Directory not expected to be indexed before copy up */
+	if (WARN_ON(ovl_test_flag(OVL_INDEX, d_inode(dentry))))
+		return -EIO;
+
+	err = ovl_get_index_name(origin, &name);
+	if (err)
+		return err;
+
+	temp = ovl_lookup_temp(indexdir);
+	if (IS_ERR(temp))
+		goto temp_err;
+
+	err = ovl_do_mkdir(dir, temp, S_IFDIR, true);
+	if (err)
+		goto out;
+
+	err = ovl_set_origin(dentry, upper, temp, true);
+	if (err)
+		goto out_cleanup;
+
+	index = lookup_one_len(name.name, indexdir, name.len);
+	if (IS_ERR(index)) {
+		err = PTR_ERR(index);
+	} else {
+		err = ovl_do_rename(dir, temp, dir, index, 0);
+		dput(index);
+	}
+
+	if (err)
+		goto out_cleanup;
+
+out:
+	dput(temp);
+	kfree(name.name);
+	return err;
+
+temp_err:
+	err = PTR_ERR(temp);
+	temp = NULL;
+	goto out;
+
+out_cleanup:
+	ovl_cleanup(dir, temp);
+	goto out;
+}
+
 struct ovl_copy_up_ctx {
 	struct dentry *parent;
 	struct dentry *dentry;
@@ -327,6 +399,7 @@ struct ovl_copy_up_ctx {
 	struct dentry *workdir;
 	bool tmpfile;
 	bool origin;
+	bool indexed;
 };
 
 static int ovl_link_up(struct ovl_copy_up_ctx *c)
@@ -474,7 +547,8 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 	 * hard link.
 	 */
 	if (c->origin) {
-		err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
+		err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp,
+				     false);
 		if (err)
 			return err;
 	}
@@ -498,6 +572,12 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
 	if (err)
 		goto out_cleanup;
 
+	if (S_ISDIR(c->stat.mode) && c->indexed) {
+		err = ovl_create_index(c->dentry, c->lowerpath.dentry, temp);
+		if (err)
+			goto out_cleanup;
+	}
+
 	if (c->tmpfile) {
 		inode_lock_nested(udir, I_MUTEX_PARENT);
 		err = ovl_install_temp(c, temp, &newdentry);
@@ -536,12 +616,26 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 {
 	int err;
 	struct ovl_fs *ofs = c->dentry->d_sb->s_fs_info;
-	bool indexed = ovl_need_index(c->dentry);
+	bool to_index = false;
+
+	/*
+	 * Indexed non-dir is copied up directly to the index entry and then
+	 * hardlinked to upper dir. Indexed dir is copied up to indexdir,
+	 * then index entry is created and then copied up dir installed.
+	 * Copying dir up to indexdir instead of workdir simplifies locking.
+	 */
+	if (ovl_need_index(c->dentry)) {
+		c->indexed = true;
+		if (S_ISDIR(c->stat.mode))
+			c->workdir = ovl_indexdir(c->dentry->d_sb);
+		else
+			to_index = true;
+	}
 
-	if (S_ISDIR(c->stat.mode) || c->stat.nlink == 1 || indexed)
+	if (S_ISDIR(c->stat.mode) || c->stat.nlink == 1 || to_index)
 		c->origin = true;
 
-	if (indexed) {
+	if (to_index) {
 		c->destdir = ovl_indexdir(c->dentry->d_sb);
 		err = ovl_get_index_name(c->lowerpath.dentry, &c->destname);
 		if (err)
@@ -568,9 +662,10 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 		}
 	}
 
-	if (indexed) {
-		if (!err)
-			ovl_set_flag(OVL_INDEX, d_inode(c->dentry));
+	if (!err && c->indexed)
+		ovl_set_flag(OVL_INDEX, d_inode(c->dentry));
+
+	if (to_index) {
 		kfree(c->destname.name);
 	} else if (!err) {
 		struct inode *udir = d_inode(c->destdir);
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 4987d7df1581..7cab6071c916 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -719,7 +719,7 @@ static int ovl_fix_origin(struct dentry *dentry, struct dentry *lower,
 	if (err)
 		return err;
 
-	err = ovl_set_origin(dentry, lower, upper);
+	err = ovl_set_origin(dentry, lower, upper, false);
 	if (!err)
 		err = ovl_set_impure(dentry->d_parent, upper->d_parent);
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 86767b71e485..759cb3a465b4 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -326,6 +326,6 @@ int ovl_copy_up(struct dentry *dentry);
 int ovl_copy_up_flags(struct dentry *dentry, int flags);
 int ovl_copy_xattr(struct dentry *old, struct dentry *new);
 int ovl_set_attr(struct dentry *upper, struct kstat *stat);
-struct ovl_fh *ovl_encode_fh(struct dentry *lower, bool is_upper);
-int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
-		   struct dentry *upper);
+struct ovl_fh *ovl_encode_fh(struct dentry *origin, bool is_upper);
+int ovl_set_origin(struct dentry *dentry, struct dentry *origin,
+		   struct dentry *upper, bool is_upper);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index f40351dcea7b..35a49a1fe3e7 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -491,8 +491,8 @@ bool ovl_need_index(struct dentry *dentry)
 	if (!lower || !ovl_indexdir(dentry->d_sb))
 		return false;
 
-	/* Index all files for unique origin verification */
-	if (!d_is_dir(lower) && ovl_verify(dentry->d_sb))
+	/* Index all files and dirs for unique origin verification */
+	if (ovl_verify(dentry->d_sb))
 		return true;
 
 	/* Index only lower hardlinks on copy up */
-- 
2.7.4

[PATCH v2 16/23] ovl: cleanup dir index when dir nlink drops to zero

[Amir Goldstein]

When non-dir index union nlink drops to zero the non-dir index
is cleaned. Do the same for directory type index entries when
union directory is removed.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/util.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 35a49a1fe3e7..7b54b4ecab3b 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -518,7 +518,7 @@ static void ovl_cleanup_index(struct dentry *dentry)
 		goto fail;
 
 	inode = d_inode(upperdentry);
-	if (inode->i_nlink != 1) {
+	if (!S_ISDIR(inode->i_mode) && inode->i_nlink != 1) {
 		pr_warn_ratelimited("overlayfs: cleanup linked index (%pd2, ino=%lu, nlink=%u)\n",
 				    upperdentry, inode->i_ino, inode->i_nlink);
 		/*
@@ -567,7 +567,7 @@ int ovl_nlink_start(struct dentry *dentry, bool *locked)
 	const struct cred *old_cred;
 	int err;
 
-	if (!d_inode(dentry) || d_is_dir(dentry))
+	if (!d_inode(dentry))
 		return 0;
 
 	/*
@@ -594,7 +594,7 @@ int ovl_nlink_start(struct dentry *dentry, bool *locked)
 	if (err)
 		return err;
 
-	if (!ovl_test_flag(OVL_INDEX, d_inode(dentry)))
+	if (d_is_dir(dentry) || !ovl_test_flag(OVL_INDEX, d_inode(dentry)))
 		goto out;
 
 	old_cred = ovl_override_creds(dentry->d_sb);
-- 
2.7.4

[PATCH v2 17/23] ovl: whiteout index when union nlink drops to zero

[Amir Goldstein]

With the 'verify' feature enabled, when overlay inode nlink drops to
zero, instead of removing the index entry, replace it with a whiteout
index entry.

This is needed for NFS export in order to prevent future open by handle
from opening the lower file directly.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/dir.c       | 58 +++++++++++++++++++++++++++++-------------------
 fs/overlayfs/overlayfs.h |  2 ++
 fs/overlayfs/util.c      | 17 +++++++++-----
 3 files changed, 48 insertions(+), 29 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index f9788bc116a8..f1104e56a29e 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -63,8 +63,7 @@ struct dentry *ovl_lookup_temp(struct dentry *workdir)
 }
 
 /* caller holds i_mutex on workdir */
-static struct dentry *ovl_whiteout(struct dentry *workdir,
-				   struct dentry *dentry)
+static struct dentry *ovl_whiteout(struct dentry *workdir)
 {
 	int err;
 	struct dentry *whiteout;
@@ -83,6 +82,38 @@ static struct dentry *ovl_whiteout(struct dentry *workdir,
 	return whiteout;
 }
 
+/* Caller must hold i_mutex on both workdir and dir */
+int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
+			     struct dentry *dentry)
+{
+	struct inode *wdir = workdir->d_inode;
+	struct dentry *whiteout;
+	int err;
+	int flags = 0;
+
+	whiteout = ovl_whiteout(workdir);
+	err = PTR_ERR(whiteout);
+	if (IS_ERR(whiteout))
+		return err;
+
+	if (d_is_dir(dentry))
+		flags = RENAME_EXCHANGE;
+
+	err = ovl_do_rename(wdir, whiteout, dir, dentry, flags);
+	if (err)
+		goto kill_whiteout;
+	if (flags)
+		ovl_cleanup(wdir, dentry);
+
+out:
+	dput(whiteout);
+	return err;
+
+kill_whiteout:
+	ovl_cleanup(wdir, whiteout);
+	goto out;
+}
+
 int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 		    struct cattr *attr, struct dentry *hardlink, bool debug)
 {
@@ -626,14 +657,10 @@ static bool ovl_matches_upper(struct dentry *dentry, struct dentry *upper)
 static int ovl_remove_and_whiteout(struct dentry *dentry, bool is_dir)
 {
 	struct dentry *workdir = ovl_workdir(dentry);
-	struct inode *wdir = workdir->d_inode;
 	struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
-	struct inode *udir = upperdir->d_inode;
-	struct dentry *whiteout;
 	struct dentry *upper;
 	struct dentry *opaquedir = NULL;
 	int err;
-	int flags = 0;
 
 	if (WARN_ON(!workdir))
 		return -EROFS;
@@ -662,24 +689,13 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, bool is_dir)
 		goto out_dput_upper;
 	}
 
-	whiteout = ovl_whiteout(workdir, dentry);
-	err = PTR_ERR(whiteout);
-	if (IS_ERR(whiteout))
-		goto out_dput_upper;
-
-	if (d_is_dir(upper))
-		flags = RENAME_EXCHANGE;
-
-	err = ovl_do_rename(wdir, whiteout, udir, upper, flags);
+	err = ovl_cleanup_and_whiteout(workdir, d_inode(upperdir), upper);
 	if (err)
-		goto kill_whiteout;
-	if (flags)
-		ovl_cleanup(wdir, upper);
+		goto out_d_drop;
 
 	ovl_dentry_version_inc(dentry->d_parent, true);
 out_d_drop:
 	d_drop(dentry);
-	dput(whiteout);
 out_dput_upper:
 	dput(upper);
 out_unlock:
@@ -688,10 +704,6 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, bool is_dir)
 	dput(opaquedir);
 out:
 	return err;
-
-kill_whiteout:
-	ovl_cleanup(wdir, whiteout);
-	goto out_d_drop;
 }
 
 static int ovl_remove_upper(struct dentry *dentry, bool is_dir)
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 759cb3a465b4..9d0dd8b70e03 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -311,6 +311,8 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
 /* dir.c */
 extern const struct inode_operations ovl_dir_inode_operations;
 struct dentry *ovl_lookup_temp(struct dentry *workdir);
+int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
+			     struct dentry *dentry);
 struct cattr {
 	dev_t rdev;
 	umode_t mode;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 7b54b4ecab3b..ce4ac3846b14 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -505,7 +505,8 @@ bool ovl_need_index(struct dentry *dentry)
 /* Caller must hold OVL_I(inode)->lock */
 static void ovl_cleanup_index(struct dentry *dentry)
 {
-	struct inode *dir = ovl_indexdir(dentry->d_sb)->d_inode;
+	struct dentry *indexdir = ovl_indexdir(dentry->d_sb);
+	struct inode *dir = indexdir->d_inode;
 	struct dentry *lowerdentry = ovl_dentry_lower(dentry);
 	struct dentry *upperdentry = ovl_dentry_upper(dentry);
 	struct dentry *index = NULL;
@@ -536,13 +537,17 @@ static void ovl_cleanup_index(struct dentry *dentry)
 	}
 
 	inode_lock_nested(dir, I_MUTEX_PARENT);
-	/* TODO: whiteout instead of cleanup to block future open by handle */
-	index = lookup_one_len(name.name, ovl_indexdir(dentry->d_sb), name.len);
+	index = lookup_one_len(name.name, indexdir, name.len);
 	err = PTR_ERR(index);
-	if (!IS_ERR(index))
-		err = ovl_cleanup(dir, index);
-	else
+	if (IS_ERR(index)) {
 		index = NULL;
+	} else if (ovl_verify(dentry->d_sb)) {
+		/* Whiteout orphan index to block future open by handle */
+		err = ovl_cleanup_and_whiteout(indexdir, dir, index);
+	} else {
+		/* Cleanup orphan index entries */
+		err = ovl_cleanup(dir, index);
+	}
 
 	inode_unlock(dir);
 	if (err)
-- 
2.7.4

[PATCH v2 18/23] ovl: whiteout orphan index entries on mount

[Amir Goldstein]

Orphan index entries are non-dir index entries whose union nlink count
dropped to zero. With index=on, orphan index entries are removed on
mount. With verify=on, orphan index entries are replaced with white
out index entries to block future open by handle from opening the lower
file.

When dir index has a stale origin fh to upper dir, we assume that the
upper dir was removed and we treat the dir index as orphan entry that
needs to be whited out or removed.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c   | 27 ++++++++++++++++++++++++---
 fs/overlayfs/readdir.c | 25 +++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 7cab6071c916..1a052f821fff 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -188,8 +188,14 @@ static struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt)
 				    bytes >> 2, (int)fh->type,
 				    ovl_acceptable, mnt);
 	if (IS_ERR(origin)) {
-		/* Treat stale file handle as "origin unknown" */
-		if (origin == ERR_PTR(-ESTALE))
+		/*
+		 * Treat stale file handle to lower file as "origin unknown".
+		 * upper file handle could become stale when upper file is
+		 * unlinked and this information is needed to handle stale
+		 * index entries correctly.
+		 */
+		if (origin == ERR_PTR(-ESTALE) &&
+		    !(fh->flags & OVL_FH_FLAG_PATH_UPPER))
 			origin = NULL;
 		return origin;
 	}
@@ -536,6 +542,14 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 	upper = ovl_index_upper(ofs, index);
 	if (IS_ERR(upper)) {
 		err = PTR_ERR(upper);
+		/*
+		 * Invalid index entries and stale non-dir index entries need
+		 * to be removed.  When dir index has a stale origin fh to upper
+		 * dir, we assume that upper dir was removed and we treat the
+		 * dir index as orphan entry that needs to be whited out.
+		 */
+		if (err == -ESTALE)
+			goto orphan;
 		if (err)
 			goto fail;
 	}
@@ -553,7 +567,7 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 			goto fail;
 
 		if (ovl_get_nlink(origin.dentry, index, 0) == 0)
-			err = -ENOENT;
+			goto orphan;
 	}
 
 out:
@@ -565,6 +579,13 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 	pr_warn_ratelimited("overlayfs: failed to verify index (%pd2, ftype=%x, err=%i)\n",
 			    index, d_inode(index)->i_mode & S_IFMT, err);
 	goto out;
+
+orphan:
+	pr_warn_ratelimited("overlayfs: orphan index entry (%pd2, ftype=%x, nlink=%u)\n",
+			    index, d_inode(index)->i_mode & S_IFMT,
+			    d_inode(index)->i_nlink);
+	err = -ENOENT;
+	goto out;
 }
 
 /*
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 0cdd1ad0b4f6..a5e4182149dc 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1060,12 +1060,33 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 			break;
 		}
 		err = ovl_verify_index(ofs, index);
-		/* Cleanup stale and orphan index entries */
-		if (err && (err == -ESTALE || err == -ENOENT))
+		if (!err) {
+			goto next;
+		} else if (err == -ESTALE) {
+			/* Cleanup stale index entries */
 			err = ovl_cleanup(dir, index);
+		} else if (err != -ENOENT) {
+			/*
+			 * Abort mount to avoid corrupting the index if
+			 * an incompatible index entry was found or on out
+			 * of memory.
+			 */
+			break;
+		} else if (ofs->config.verify) {
+			/*
+			 * Whiteout orphan index to block future open by
+			 * handle after overlay nlink dropped to zero.
+			 */
+			err = ovl_cleanup_and_whiteout(indexdir, dir, index);
+		} else {
+			/* Cleanup orphan index entries */
+			err = ovl_cleanup(dir, index);
+		}
+
 		if (err)
 			break;
 
+next:
 		dput(index);
 		index = NULL;
 	}
-- 
2.7.4

[PATCH v2 19/23] ovl: factor out ovl_get_index_fh() helper

[Amir Goldstein]

The helper is needed to lookup an index by file handle for NFS export.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c     | 59 ++++++++++++++++++++++++++++++++++++++++--------
 fs/overlayfs/overlayfs.h |  1 +
 2 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 1a052f821fff..ac902fa7c5e2 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -588,6 +588,21 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
 	goto out;
 }
 
+static int ovl_get_index_name_fh(struct ovl_fh *fh, struct qstr *name)
+{
+	char *n, *s;
+
+	n = kzalloc(fh->len * 2, GFP_KERNEL);
+	if (!n)
+		return -ENOMEM;
+
+	s  = bin2hex(n, fh, fh->len);
+	*name = (struct qstr) QSTR_INIT(n, s - n);
+
+	return 0;
+
+}
+
 /*
  * Lookup in indexdir for the index entry of a lower real inode or a copy up
  * origin inode. The index entry name is the hex representation of the lower
@@ -605,25 +620,49 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
  */
 int ovl_get_index_name(struct dentry *origin, struct qstr *name)
 {
-	int err;
 	struct ovl_fh *fh;
-	char *n, *s;
+	int err;
 
 	fh = ovl_encode_fh(origin, false);
 	if (IS_ERR(fh))
 		return PTR_ERR(fh);
 
-	err = -ENOMEM;
-	n = kzalloc(fh->len * 2, GFP_KERNEL);
-	if (n) {
-		s  = bin2hex(n, fh, fh->len);
-		*name = (struct qstr) QSTR_INIT(n, s - n);
-		err = 0;
-	}
-	kfree(fh);
+	err = ovl_get_index_name_fh(fh, name);
 
+	kfree(fh);
 	return err;
+}
+
+/* Lookup index by file handle for NFS export */
+struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh)
+{
+	struct dentry *index;
+	struct qstr name;
+	int err;
+
+	err = ovl_get_index_name_fh(fh, &name);
+	if (err)
+		return ERR_PTR(err);
+
+	index = lookup_one_len_unlocked(name.name, ofs->indexdir, name.len);
+	kfree(name.name);
+	if (IS_ERR(index)) {
+		if (PTR_ERR(index) == -ENOENT)
+			index = NULL;
+		return index;
+	}
 
+	if (d_is_negative(index))
+		err = 0;
+	else if (ovl_is_whiteout(index))
+		err = -ESTALE;
+	else if (ovl_dentry_weird(index))
+		err = -EIO;
+	else
+		return index;
+
+	dput(index);
+	return ERR_PTR(err);
 }
 
 static struct dentry *ovl_lookup_index(struct dentry *dentry,
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 9d0dd8b70e03..d9467eab8514 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -259,6 +259,7 @@ int ovl_verify_origin(struct dentry *dentry, struct dentry *origin,
 		      bool is_upper, bool set);
 int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index);
 int ovl_get_index_name(struct dentry *origin, struct qstr *name);
+struct dentry *ovl_get_index_fh(struct ovl_fs *ofs, struct ovl_fh *fh);
 int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
 struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			  unsigned int flags);
-- 
2.7.4

[PATCH v2 20/23] ovl: do not pass overlay dentry to ovl_get_inode()

[Amir Goldstein]

This is needed for using ovl_get_inode() for decoding file handles
for NFS export.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/inode.c     | 16 +++++++---------
 fs/overlayfs/namei.c     |  3 ++-
 fs/overlayfs/overlayfs.h |  5 +++--
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 1b53755bd86c..fb436d66da7c 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -635,14 +635,14 @@ static bool ovl_verify_inode(struct inode *inode, struct dentry *lowerdentry,
 	return true;
 }
 
-struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry,
-			    struct dentry *index)
+struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
+			    struct dentry *lowerdentry, struct dentry *index,
+			    unsigned int numlower)
 {
-	struct dentry *lowerdentry = ovl_dentry_lower(dentry);
 	struct inode *realinode = upperdentry ? d_inode(upperdentry) : NULL;
 	struct inode *inode;
 	/* Already indexed or could be indexed on copy up? */
-	bool indexed = (index || (ovl_indexdir(dentry->d_sb) && !upperdentry));
+	bool indexed = (index || (ovl_indexdir(sb) && !upperdentry));
 
 	if (WARN_ON(upperdentry && indexed && !lowerdentry))
 		return ERR_PTR(-EIO);
@@ -661,7 +661,7 @@ struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry,
 						      upperdentry);
 		unsigned int nlink;
 
-		inode = iget5_locked(dentry->d_sb, (unsigned long) key,
+		inode = iget5_locked(sb, (unsigned long) key,
 				     ovl_inode_test, ovl_inode_set, key);
 		if (!inode)
 			goto out_nomem;
@@ -684,7 +684,7 @@ struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry,
 				      realinode->i_nlink);
 		set_nlink(inode, nlink);
 	} else {
-		inode = new_inode(dentry->d_sb);
+		inode = new_inode(sb);
 		if (!inode)
 			goto out_nomem;
 	}
@@ -696,9 +696,7 @@ struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry,
 
 	/* Check for non-merge dir that may have whiteouts */
 	if (S_ISDIR(realinode->i_mode)) {
-		struct ovl_entry *oe = dentry->d_fsdata;
-
-		if (((upperdentry && lowerdentry) || oe->numlower > 1) ||
+		if (((upperdentry && lowerdentry) || numlower > 1) ||
 		    ovl_check_origin_xattr(upperdentry ?: lowerdentry)) {
 			ovl_set_flag(OVL_WHITEOUTS, inode);
 		}
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index ac902fa7c5e2..80233d5587b7 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -971,7 +971,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		upperdentry = dget(index);
 
 	if (upperdentry || ctr) {
-		inode = ovl_get_inode(dentry, upperdentry, index);
+		inode = ovl_get_inode(dentry->d_sb, upperdentry, origin, index,
+				      ctr);
 		err = PTR_ERR(inode);
 		if (IS_ERR(inode))
 			goto out_free_oe;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index d9467eab8514..f4b064023826 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -297,8 +297,9 @@ int ovl_update_time(struct inode *inode, struct timespec *ts, int flags);
 bool ovl_is_private_xattr(const char *name);
 
 struct inode *ovl_new_inode(struct super_block *sb, umode_t mode, dev_t rdev);
-struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry,
-			    struct dentry *index);
+struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
+			    struct dentry *lowerdentry, struct dentry *index,
+			    unsigned int numlower);
 static inline void ovl_copyattr(struct inode *from, struct inode *to)
 {
 	to->i_uid = from->i_uid;
-- 
2.7.4

[PATCH v2 21/23] ovl: grab i_count reference of lower inode

[Amir Goldstein]

An overlay inode with elevated i_count may outlive its overlay dentry.
Since the only reference to OVL_I(inode)->lower was held by the lower
dentry object in the overlay dentry (oe->lowerstack[0]), releasing the
overlay dentry could leave a live inode with reference to freed lower
inode.

This may materialize to a bug when a disconnected dentry is obtained
from decoding an NFS file handle and a new allocated real inode has
the same address as the freed inode in OVL_I(inode)->lower, by which
the overlay inode is hashed.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 1 +
 fs/overlayfs/util.c  | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 96136528861f..f152b817e4d0 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -216,6 +216,7 @@ static void ovl_destroy_inode(struct inode *inode)
 	struct ovl_inode *oi = OVL_I(inode);
 
 	dput(oi->__upperdentry);
+	iput(oi->lower);
 	kfree(oi->redirect);
 	ovl_dir_cache_free(inode);
 	mutex_destroy(&oi->lock);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index ce4ac3846b14..d6fe01ece7e8 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -291,7 +291,7 @@ void ovl_inode_init(struct inode *inode, struct dentry *upperdentry,
 	if (upperdentry)
 		OVL_I(inode)->__upperdentry = upperdentry;
 	if (lowerdentry)
-		OVL_I(inode)->lower = d_inode(lowerdentry);
+		OVL_I(inode)->lower = igrab(d_inode(lowerdentry));
 
 	ovl_copyattr(d_inode(upperdentry ?: lowerdentry), inode);
 }
-- 
2.7.4

[PATCH v2 22/23] ovl: use d_splice_alias() in place of d_add() in lookup

[Amir Goldstein]

This is required for NFS export.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 80233d5587b7..a69cedf06000 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -990,9 +990,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	dput(index);
 	kfree(stack);
 	kfree(d.redirect);
-	d_add(dentry, inode);
-
-	return NULL;
+	return d_splice_alias(inode, dentry);
 
 out_free_oe:
 	dentry->d_fsdata = NULL;
-- 
2.7.4

[PATCH v2 23/23] ovl: copy up of disconnected dentries

[Amir Goldstein]

With NFS export, some operations on decoded file handles (e.g. open,
link, setattr, xattr_set) may call copy up with a disconnected non-dir.
In this case, we will copy up lower inode to index dir without
linking it to upper dir.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c | 56 ++++++++++++++++++++++++++++++++++++--------------
 fs/overlayfs/inode.c   |  4 +++-
 fs/overlayfs/util.c    |  7 ++++---
 3 files changed, 48 insertions(+), 19 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 275a08ab0dd3..384a3b37eeea 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -434,7 +434,10 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
 		}
 	}
 	inode_unlock(udir);
-	ovl_set_nlink_upper(c->dentry);
+	if (err)
+		return err;
+
+	err = ovl_set_nlink_upper(c->dentry);
 
 	return err;
 }
@@ -640,6 +643,9 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 		err = ovl_get_index_name(c->lowerpath.dentry, &c->destname);
 		if (err)
 			return err;
+	} else if (WARN_ON(!c->parent)) {
+		/* Disconnected dentry must be copied up to index dir */
+		return -EIO;
 	} else {
 		/*
 		 * Mark parent "impure" because it may now contain non-pure
@@ -662,12 +668,17 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 		}
 	}
 
-	if (!err && c->indexed)
+
+	if (err)
+		goto out;
+
+	if (c->indexed)
 		ovl_set_flag(OVL_INDEX, d_inode(c->dentry));
 
 	if (to_index) {
-		kfree(c->destname.name);
-	} else if (!err) {
+		/* Initialize nlink for copy up of disconnected dentry */
+		err = ovl_set_nlink_upper(c->dentry);
+	} else {
 		struct inode *udir = d_inode(c->destdir);
 
 		/* Restore timestamps on parent (best effort) */
@@ -678,6 +689,9 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 		ovl_dentry_set_upper_alias(c->dentry);
 	}
 
+out:
+	if (to_index)
+		kfree(c->destname.name);
 	return err;
 }
 
@@ -702,14 +716,17 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	if (err)
 		return err;
 
-	ovl_path_upper(parent, &parentpath);
-	ctx.destdir = parentpath.dentry;
-	ctx.destname = dentry->d_name;
+	if (parent) {
+		ovl_path_upper(parent, &parentpath);
+		ctx.destdir = parentpath.dentry;
+		ctx.destname = dentry->d_name;
 
-	err = vfs_getattr(&parentpath, &ctx.pstat,
-			  STATX_ATIME | STATX_MTIME, AT_STATX_SYNC_AS_STAT);
-	if (err)
-		return err;
+		err = vfs_getattr(&parentpath, &ctx.pstat,
+				  STATX_ATIME | STATX_MTIME,
+				  AT_STATX_SYNC_AS_STAT);
+		if (err)
+			return err;
+	}
 
 	/* maybe truncate regular file. this has no effect on dirs */
 	if (flags & O_TRUNC)
@@ -730,7 +747,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	} else {
 		if (!ovl_dentry_upper(dentry))
 			err = ovl_do_copy_up(&ctx);
-		if (!err && !ovl_dentry_has_upper_alias(dentry))
+		if (!err && parent && !ovl_dentry_has_upper_alias(dentry))
 			err = ovl_link_up(&ctx);
 		ovl_copy_up_end(dentry);
 	}
@@ -743,10 +760,19 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 {
 	int err = 0;
 	const struct cred *old_cred = ovl_override_creds(dentry->d_sb);
+	bool disconnected = (dentry->d_flags & DCACHE_DISCONNECTED);
+
+	/*
+	 * With NFS export, copy up can get called for a disconnected non-dir.
+	 * In this case, we will copy up lower inode to index dir without
+	 * linking it to upper dir.
+	 */
+	if (WARN_ON(disconnected && d_is_dir(dentry)))
+		return -EIO;
 
 	while (!err) {
 		struct dentry *next;
-		struct dentry *parent;
+		struct dentry *parent = NULL;
 
 		/*
 		 * Check if copy-up has happened as well as for upper alias (in
@@ -762,12 +788,12 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		 *      with rename.
 		 */
 		if (ovl_dentry_upper(dentry) &&
-		    ovl_dentry_has_upper_alias(dentry))
+		    (ovl_dentry_has_upper_alias(dentry) || disconnected))
 			break;
 
 		next = dget(dentry);
 		/* find the topmost dentry not yet copied up */
-		for (;;) {
+		for (; !disconnected;) {
 			parent = dget_parent(next);
 
 			if (ovl_dentry_upper(parent))
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index fb436d66da7c..bb742d195f12 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -353,8 +353,10 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
 
 static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
 {
+	/* Copy up of disconnected dentry does not set upper alias */
 	if (ovl_dentry_upper(dentry) &&
-	    ovl_dentry_has_upper_alias(dentry))
+	    (ovl_dentry_has_upper_alias(dentry) ||
+	     (dentry->d_flags & DCACHE_DISCONNECTED)))
 		return false;
 
 	if (special_file(d_inode(dentry)->i_mode))
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index d6fe01ece7e8..1b0dc903cf6d 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -220,9 +220,10 @@ void ovl_dentry_set_opaque(struct dentry *dentry)
 }
 
 /*
- * For hard links it's possible for ovl_dentry_upper() to return positive, while
- * there's no actual upper alias for the inode.  Copy up code needs to know
- * about the existence of the upper alias, so it can't use ovl_dentry_upper().
+ * For hard links and decoded file handles, it's possible for ovl_dentry_upper()
+ * to return positive, while there's no actual upper alias for the inode.
+ * Copy up code needs to know about the existence of the upper alias, so it
+ * can't use ovl_dentry_upper().
  */
 bool ovl_dentry_has_upper_alias(struct dentry *dentry)
 {
-- 
2.7.4

Re: [PATCH v2 02/23] ovl: ignore index mount option when no upper layer

[Vivek Goyal]

On Thu, Jan 04, 2018 at 06:39:57PM +0200, Amir Goldstein wrote:
> For non-upper overlay mount, the value of index mount option is moot.
> When mounter tries to set this mount options, emit a warning and leave
> the value at the default so it won't show up in /proc/mounts.

I am curious, what should show in /proc/mounts. Current code seems to
print this only if it is different from default.

So if detfault is index=on, and user specifies index=off (with no upper),
it should still show in /proc/mounts?

Thanks
Vivek

> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/super.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 5d5e2b39ba69..7e33c83c5a07 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -496,12 +496,18 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  		}
>  	}
>  
> -	/* Workdir is useless in non-upper mount */
> -	if (!config->upperdir && config->workdir) {
> -		pr_info("overlayfs: option \"workdir=%s\" is useless in a non-upper mount, ignore\n",
> -			config->workdir);
> -		kfree(config->workdir);
> -		config->workdir = NULL;
> +	if (!config->upperdir) {
> +		/* Workdir is useless in non-upper mount */
> +		if (config->workdir) {
> +			pr_info("overlayfs: option \"workdir=%s\" is useless in a non-upper mount, ignore\n",
> +				config->workdir);
> +			kfree(config->workdir);
> +			config->workdir = NULL;
> +		}
> +		if (config->index != ovl_index_def) {
> +			pr_info("overlayfs: option \"index\" is useless in a non-upper mount, ignore\n");
> +			config->index = ovl_index_def;
> +		}
>  	}
>  
>  	return ovl_parse_redirect_mode(config, config->redirect_mode);
> @@ -698,7 +704,8 @@ static int ovl_lower_dir(const char *name, struct path *path,
>  	 * The inodes index feature needs to encode and decode file
>  	 * handles, so it requires that all layers support them.
>  	 */
> -	if (ofs->config.index && !ovl_can_decode_fh(path->dentry->d_sb)) {
> +	if (ofs->config.upperdir && ofs->config.index &&
> +	    !ovl_can_decode_fh(path->dentry->d_sb)) {
>  		ofs->config.index = false;
>  		pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off.\n", name);
>  	}
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/23] ovl: ignore index mount option when no upper layer

[Amir Goldstein]

On Thu, Jan 4, 2018 at 8:42 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Jan 04, 2018 at 06:39:57PM +0200, Amir Goldstein wrote:
>> For non-upper overlay mount, the value of index mount option is moot.
>> When mounter tries to set this mount options, emit a warning and leave
>> the value at the default so it won't show up in /proc/mounts.
>
> I am curious, what should show in /proc/mounts. Current code seems to
> print this only if it is different from default.
>

I guess that is a common pattern for filesystems only show options
that are different from default.

I think the logic is that one would try to reconstruct mounts from a
snapshot of /proc/mounts, there is no reason to specify values that
are default, because resulting mount will be the same.

> So if detfault is index=on, and user specifies index=off (with no upper),
> it should still show in /proc/mounts?
>

What this patch says is that index is not a relevant option with no upper,
so basically specifying either index=on/off is wrong, but so does specifying
workdir and the action that overlay takes is ignoring the wrong option.
I realize the setting index to default (on) when there is no upper is
a bit hacky, but I think we can live with that in order to keep the parsing
code simple and storing only a boolean single state.

Amir.

Re: [PATCH v2 02/23] ovl: ignore index mount option when no upper layer

[Vivek Goyal]

On Thu, Jan 04, 2018 at 09:39:48PM +0200, Amir Goldstein wrote:
> On Thu, Jan 4, 2018 at 8:42 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Thu, Jan 04, 2018 at 06:39:57PM +0200, Amir Goldstein wrote:
> >> For non-upper overlay mount, the value of index mount option is moot.
> >> When mounter tries to set this mount options, emit a warning and leave
> >> the value at the default so it won't show up in /proc/mounts.
> >
> > I am curious, what should show in /proc/mounts. Current code seems to
> > print this only if it is different from default.
> >
> 
> I guess that is a common pattern for filesystems only show options
> that are different from default.
> 
> I think the logic is that one would try to reconstruct mounts from a
> snapshot of /proc/mounts, there is no reason to specify values that
> are default, because resulting mount will be the same.
> 
> > So if detfault is index=on, and user specifies index=off (with no upper),
> > it should still show in /proc/mounts?
> >
> 
> What this patch says is that index is not a relevant option with no upper,
> so basically specifying either index=on/off is wrong, but so does specifying
> workdir and the action that overlay takes is ignoring the wrong option.
> I realize the setting index to default (on) when there is no upper is
> a bit hacky, but I think we can live with that in order to keep the parsing
> code simple and storing only a boolean single state.

Ok, fair enough. Thanks

Vivek

Re: [PATCH v2 03/23] ovl: store layer index in ovl_layer

[Vivek Goyal]

On Thu, Jan 04, 2018 at 06:39:58PM +0200, Amir Goldstein wrote:
> Store the fs root layer index inside ovl_layer struct, so we can
> get the root fs layer index from merge dir lower layer instead of
> find it with ovl_find_layer() helper.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/namei.c     | 17 +----------------
>  fs/overlayfs/ovl_entry.h |  2 ++
>  fs/overlayfs/super.c     |  1 +
>  3 files changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 71db9a966d88..a48ee02c4524 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -572,18 +572,6 @@ int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
>  	return (idx < oe->numlower) ? idx + 1 : -1;
>  }
>  
> -static int ovl_find_layer(struct ovl_fs *ofs, struct ovl_path *path)
> -{
> -	int i;
> -
> -	for (i = 0; i < ofs->numlower; i++) {
> -		if (ofs->lower_layers[i].mnt == path->layer->mnt)
> -			break;
> -	}
> -
> -	return i;
> -}
> -
>  /* Fix missing 'origin' xattr */
>  static int ovl_fix_origin(struct dentry *dentry, struct dentry *lower,
>  			  struct dentry *upper)
> @@ -733,11 +721,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>  
>  		if (d.redirect && d.redirect[0] == '/' && poe != roe) {
>  			poe = roe;
> -
>  			/* Find the current layer on the root dentry */
> -			i = ovl_find_layer(ofs, &lower);
> -			if (WARN_ON(i == ofs->numlower))
> -				break;
> +			i = lower.layer->idx - 1;
>  		}
>  	}
>  
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 9d0bc03bf6e4..608e48755070 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -22,6 +22,8 @@ struct ovl_config {
>  struct ovl_layer {
>  	struct vfsmount *mnt;
>  	dev_t pseudo_dev;
> +	/* Index of this layer in fs root (upper == 0) */
> +	int idx;

Just curious. If we are stroing idx, then do we have to store ovl_layer
and every ovl_entry. Just idx can give us the position and we should
be able to then access mnt and pseudo_dev directly from
ofs->lower_layers[idx]. If yes, we can avoid storing vfsmount and 
pseudo_dev per dentry.

Vivek

>  };
>  
>  struct ovl_path {
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 7e33c83c5a07..2b58620eedf0 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1111,6 +1111,7 @@ static int ovl_get_lower_layers(struct ovl_fs *ofs, struct path *stack,
>  
>  		ofs->lower_layers[ofs->numlower].mnt = mnt;
>  		ofs->lower_layers[ofs->numlower].pseudo_dev = dev;
> +		ofs->lower_layers[ofs->numlower].idx = i + 1;
>  		ofs->numlower++;
>  
>  		/* Check if all lower layers are on same sb */
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 05/23] ovl: pass ovl_layer array to ovl_check_origin_fh()

[Vivek Goyal]

On Thu, Jan 04, 2018 at 06:40:00PM +0200, Amir Goldstein wrote:
> Pass the fs instance lower_layer array instead of lowerstack array to
> ovl_check_origin_fh(), because the dentry member of ovl_path plays no
> part in this helper.
> 
> This change simplifies the argument list of ovl_check_origin(),
> ovl_cleanup_index() and ovl_verify_index().
> 
> We pass lower_layer array and numlower and not struct ovl_fs to
> ovl_check_origin_fh(), because we are going to use this helper for
> decoding upper layer file handles for NFS export support.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/namei.c     | 25 +++++++++++--------------
>  fs/overlayfs/overlayfs.h |  9 ++++-----
>  fs/overlayfs/readdir.c   | 12 ++++++------
>  fs/overlayfs/super.c     |  5 +----
>  4 files changed, 22 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 4cc8fb64c879..46a3e31b0225 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -292,16 +292,14 @@ static int ovl_lookup_layer(struct dentry *base, struct ovl_lookup_data *d,
>  
>  
>  static int ovl_check_origin_fh(struct ovl_fh *fh, struct dentry *upperdentry,
> -			       struct ovl_path *lower, unsigned int numlower,
> +			       struct ovl_layer *layers, unsigned int numlayers,
>  			       struct ovl_path **stackp)

I was thinking if this will lead to decoding in all lower layers (as opposed
to only some depending on how many lower layers ovl_entry has stored). But
looks like we already do decoding in all the lower layers of fs (both for
origin and index). So this is not a concern. 

Vivek

>  {
> -	struct vfsmount *mnt;
>  	struct dentry *origin = NULL;
>  	int i;
>  
> -	for (i = 0; i < numlower; i++) {
> -		mnt = lower[i].layer->mnt;
> -		origin = ovl_decode_fh(fh, mnt);
> +	for (i = 0; i < numlayers; i++) {
> +		origin = ovl_decode_fh(fh, layers[i].mnt);
>  		if (origin)
>  			break;
>  	}
> @@ -321,7 +319,7 @@ static int ovl_check_origin_fh(struct ovl_fh *fh, struct dentry *upperdentry,
>  		dput(origin);
>  		return -ENOMEM;
>  	}
> -	**stackp = (struct ovl_path){.dentry = origin, .layer = lower[i].layer};
> +	**stackp = (struct ovl_path){.dentry = origin, .layer = &layers[i]};
>  
>  	return 0;
>  
> @@ -333,8 +331,7 @@ static int ovl_check_origin_fh(struct ovl_fh *fh, struct dentry *upperdentry,
>  	return -EIO;
>  }
>  
> -static int ovl_check_origin(struct dentry *upperdentry,
> -			    struct ovl_path *lower, unsigned int numlower,
> +static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
>  			    struct ovl_path **stackp, unsigned int *ctrp)
>  {
>  	struct ovl_fh *fh = ovl_get_origin_fh(upperdentry);
> @@ -343,7 +340,8 @@ static int ovl_check_origin(struct dentry *upperdentry,
>  	if (IS_ERR_OR_NULL(fh))
>  		return PTR_ERR(fh);
>  
> -	err = ovl_check_origin_fh(fh, upperdentry, lower, numlower, stackp);
> +	err = ovl_check_origin_fh(fh, upperdentry, ofs->lower_layers,
> +				  ofs->numlower, stackp);
>  	kfree(fh);
>  
>  	if (err) {
> @@ -423,8 +421,7 @@ int ovl_verify_origin(struct dentry *dentry, struct dentry *origin,
>   * OVL_XATTR_ORIGIN and that origin file handle can be decoded to lower path.
>   * Return 0 on match, -ESTALE on mismatch or stale origin, < 0 on error.
>   */
> -int ovl_verify_index(struct dentry *index, struct ovl_path *lower,
> -		     unsigned int numlower)
> +int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
>  {
>  	struct ovl_fh *fh = NULL;
>  	size_t len;
> @@ -471,7 +468,8 @@ int ovl_verify_index(struct dentry *index, struct ovl_path *lower,
>  	if (err)
>  		goto fail;
>  
> -	err = ovl_check_origin_fh(fh, index, lower, numlower, &stack);
> +	err = ovl_check_origin_fh(fh, index, ofs->lower_layers,
> +				  ofs->numlower, &stack);
>  	if (err)
>  		goto fail;
>  
> @@ -689,8 +687,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>  			 * number - it's the same as if we held a reference
>  			 * to a dentry in lower layer that was moved under us.
>  			 */
> -			err = ovl_check_origin(upperdentry, roe->lowerstack,
> -					       roe->numlower, &stack, &ctr);
> +			err = ovl_check_origin(ofs, upperdentry, &stack, &ctr);
>  			if (err)
>  				goto out_put_upper;
>  		}
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index d1cfa69c98b5..d55afb6646b0 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -251,11 +251,11 @@ static inline bool ovl_is_impuredir(struct dentry *dentry)
>  /* namei.c */
>  int ovl_verify_origin(struct dentry *dentry, struct dentry *origin,
>  		      bool is_upper, bool set);
> -int ovl_verify_index(struct dentry *index, struct ovl_path *lower,
> -		     unsigned int numlower);
> +int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index);
>  int ovl_get_index_name(struct dentry *origin, struct qstr *name);
>  int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
> -struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags);
> +struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> +			  unsigned int flags);
>  bool ovl_lower_positive(struct dentry *dentry);
>  
>  /* readdir.c */
> @@ -267,8 +267,7 @@ void ovl_dir_cache_free(struct inode *inode);
>  int ovl_check_d_type_supported(struct path *realpath);
>  void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
>  			 struct dentry *dentry, int level);
> -int ovl_indexdir_cleanup(struct dentry *dentry, struct vfsmount *mnt,
> -			 struct ovl_path *lower, unsigned int numlower);
> +int ovl_indexdir_cleanup(struct ovl_fs *ofs);
>  
>  /* inode.c */
>  int ovl_set_nlink_upper(struct dentry *dentry);
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 001c79297bc5..0cdd1ad0b4f6 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1023,13 +1023,13 @@ void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
>  	}
>  }
>  
> -int ovl_indexdir_cleanup(struct dentry *dentry, struct vfsmount *mnt,
> -			 struct ovl_path *lower, unsigned int numlower)
> +int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>  {
>  	int err;
> +	struct dentry *indexdir = ofs->indexdir;
>  	struct dentry *index = NULL;
> -	struct inode *dir = dentry->d_inode;
> -	struct path path = { .mnt = mnt, .dentry = dentry };
> +	struct inode *dir = indexdir->d_inode;
> +	struct path path = { .mnt = ofs->upper_mnt, .dentry = indexdir };
>  	LIST_HEAD(list);
>  	struct rb_root root = RB_ROOT;
>  	struct ovl_cache_entry *p;
> @@ -1053,13 +1053,13 @@ int ovl_indexdir_cleanup(struct dentry *dentry, struct vfsmount *mnt,
>  			if (p->len == 2 && p->name[1] == '.')
>  				continue;
>  		}
> -		index = lookup_one_len(p->name, dentry, p->len);
> +		index = lookup_one_len(p->name, indexdir, p->len);
>  		if (IS_ERR(index)) {
>  			err = PTR_ERR(index);
>  			index = NULL;
>  			break;
>  		}
> -		err = ovl_verify_index(index, lower, numlower);
> +		err = ovl_verify_index(ofs, index);
>  		/* Cleanup stale and orphan index entries */
>  		if (err && (err == -ESTALE || err == -ENOENT))
>  			err = ovl_cleanup(dir, index);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 2b58620eedf0..994d35628acf 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1062,10 +1062,7 @@ static int ovl_get_indexdir(struct ovl_fs *ofs, struct ovl_entry *oe,
>  
>  		/* Cleanup bad/stale/orphan index entries */
>  		if (!err)
> -			err = ovl_indexdir_cleanup(ofs->indexdir,
> -						   ofs->upper_mnt,
> -						   oe->lowerstack,
> -						   oe->numlower);
> +			err = ovl_indexdir_cleanup(ofs);
>  	}
>  	if (err || !ofs->indexdir)
>  		pr_warn("overlayfs: try deleting index dir or mounting with '-o index=off' to disable inodes index.\n");
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 03/23] ovl: store layer index in ovl_layer

[Amir Goldstein]

On Thu, Jan 4, 2018 at 11:00 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Jan 04, 2018 at 06:39:58PM +0200, Amir Goldstein wrote:
>> Store the fs root layer index inside ovl_layer struct, so we can
>> get the root fs layer index from merge dir lower layer instead of
>> find it with ovl_find_layer() helper.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/namei.c     | 17 +----------------
>>  fs/overlayfs/ovl_entry.h |  2 ++
>>  fs/overlayfs/super.c     |  1 +
>>  3 files changed, 4 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> index 71db9a966d88..a48ee02c4524 100644
>> --- a/fs/overlayfs/namei.c
>> +++ b/fs/overlayfs/namei.c
>> @@ -572,18 +572,6 @@ int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
>>       return (idx < oe->numlower) ? idx + 1 : -1;
>>  }
>>
>> -static int ovl_find_layer(struct ovl_fs *ofs, struct ovl_path *path)
>> -{
>> -     int i;
>> -
>> -     for (i = 0; i < ofs->numlower; i++) {
>> -             if (ofs->lower_layers[i].mnt == path->layer->mnt)
>> -                     break;
>> -     }
>> -
>> -     return i;
>> -}
>> -
>>  /* Fix missing 'origin' xattr */
>>  static int ovl_fix_origin(struct dentry *dentry, struct dentry *lower,
>>                         struct dentry *upper)
>> @@ -733,11 +721,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>>
>>               if (d.redirect && d.redirect[0] == '/' && poe != roe) {
>>                       poe = roe;
>> -
>>                       /* Find the current layer on the root dentry */
>> -                     i = ovl_find_layer(ofs, &lower);
>> -                     if (WARN_ON(i == ofs->numlower))
>> -                             break;
>> +                     i = lower.layer->idx - 1;
>>               }
>>       }
>>
>> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
>> index 9d0bc03bf6e4..608e48755070 100644
>> --- a/fs/overlayfs/ovl_entry.h
>> +++ b/fs/overlayfs/ovl_entry.h
>> @@ -22,6 +22,8 @@ struct ovl_config {
>>  struct ovl_layer {
>>       struct vfsmount *mnt;
>>       dev_t pseudo_dev;
>> +     /* Index of this layer in fs root (upper == 0) */
>> +     int idx;
>
> Just curious. If we are stroing idx, then do we have to store ovl_layer
> and every ovl_entry. Just idx can give us the position and we should
> be able to then access mnt and pseudo_dev directly from
> ofs->lower_layers[idx]. If yes, we can avoid storing vfsmount and
> pseudo_dev per dentry.
>

But we are not storing vfsmount and pseudo_dev per dentry
We are storing &ofs->lower_layers[idx] per dentry.
Do we gain anything from storing the idx instead?
Most of the times code needs to access ofs->lower_layers[idx]
fields (mnt mostly).
The cases where idx itself matter are rare, for example:
if dentry has numlower == 1 and idx > 1, this is a quick indication
that a parent of lower dir may be copied up but not be indexed
(because a dir on top of it in layer 1 is indexed).

The current series does not make use of the example above,
but it can be used to relax copy up of dir on encode when
lower idx == 1.

Amir.

Re: [PATCH v2 05/23] ovl: pass ovl_layer array to ovl_check_origin_fh()

[Amir Goldstein]

On Fri, Jan 5, 2018 at 12:35 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Jan 04, 2018 at 06:40:00PM +0200, Amir Goldstein wrote:
>> Pass the fs instance lower_layer array instead of lowerstack array to
>> ovl_check_origin_fh(), because the dentry member of ovl_path plays no
>> part in this helper.
>>
>> This change simplifies the argument list of ovl_check_origin(),
>> ovl_cleanup_index() and ovl_verify_index().
>>
>> We pass lower_layer array and numlower and not struct ovl_fs to
>> ovl_check_origin_fh(), because we are going to use this helper for
>> decoding upper layer file handles for NFS export support.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/namei.c     | 25 +++++++++++--------------
>>  fs/overlayfs/overlayfs.h |  9 ++++-----
>>  fs/overlayfs/readdir.c   | 12 ++++++------
>>  fs/overlayfs/super.c     |  5 +----
>>  4 files changed, 22 insertions(+), 29 deletions(-)
>>
>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> index 4cc8fb64c879..46a3e31b0225 100644
>> --- a/fs/overlayfs/namei.c
>> +++ b/fs/overlayfs/namei.c
>> @@ -292,16 +292,14 @@ static int ovl_lookup_layer(struct dentry *base, struct ovl_lookup_data *d,
>>
>>
>>  static int ovl_check_origin_fh(struct ovl_fh *fh, struct dentry *upperdentry,
>> -                            struct ovl_path *lower, unsigned int numlower,
>> +                            struct ovl_layer *layers, unsigned int numlayers,
>>                              struct ovl_path **stackp)
>
> I was thinking if this will lead to decoding in all lower layers (as opposed
> to only some depending on how many lower layers ovl_entry has stored). But
> looks like we already do decoding in all the lower layers of fs (both for
> origin and index). So this is not a concern.
>

So what happens today is that we are only decoding non-dir disconnected
origin in ovl_lookup().
If numlower > 1 and all of the lower layers are on same fs, we will decode
origin from the first lower layer and get an unreliable idx, because non-dir
origin could be on any of the lower layers.
But we don't really care on which layer non-dir origin is, we just need to
know its inode number (or copy its data).

But when decoding an NFS dir file handle it really does matters to get a
reliable layer idx.

Amir.

Re: [PATCH v2 03/23] ovl: store layer index in ovl_layer

[Amir Goldstein]

On Fri, Jan 5, 2018 at 7:05 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Jan 4, 2018 at 11:00 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> On Thu, Jan 04, 2018 at 06:39:58PM +0200, Amir Goldstein wrote:
>>> Store the fs root layer index inside ovl_layer struct, so we can
>>> get the root fs layer index from merge dir lower layer instead of
>>> find it with ovl_find_layer() helper.
>>>
>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>> ---
>>>  fs/overlayfs/namei.c     | 17 +----------------
>>>  fs/overlayfs/ovl_entry.h |  2 ++
>>>  fs/overlayfs/super.c     |  1 +
>>>  3 files changed, 4 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>>> index 71db9a966d88..a48ee02c4524 100644
>>> --- a/fs/overlayfs/namei.c
>>> +++ b/fs/overlayfs/namei.c
>>> @@ -572,18 +572,6 @@ int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
>>>       return (idx < oe->numlower) ? idx + 1 : -1;
>>>  }
>>>
>>> -static int ovl_find_layer(struct ovl_fs *ofs, struct ovl_path *path)
>>> -{
>>> -     int i;
>>> -
>>> -     for (i = 0; i < ofs->numlower; i++) {
>>> -             if (ofs->lower_layers[i].mnt == path->layer->mnt)
>>> -                     break;
>>> -     }
>>> -
>>> -     return i;
>>> -}
>>> -
>>>  /* Fix missing 'origin' xattr */
>>>  static int ovl_fix_origin(struct dentry *dentry, struct dentry *lower,
>>>                         struct dentry *upper)
>>> @@ -733,11 +721,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>>>
>>>               if (d.redirect && d.redirect[0] == '/' && poe != roe) {
>>>                       poe = roe;
>>> -
>>>                       /* Find the current layer on the root dentry */
>>> -                     i = ovl_find_layer(ofs, &lower);
>>> -                     if (WARN_ON(i == ofs->numlower))
>>> -                             break;
>>> +                     i = lower.layer->idx - 1;
>>>               }
>>>       }
>>>
>>> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
>>> index 9d0bc03bf6e4..608e48755070 100644
>>> --- a/fs/overlayfs/ovl_entry.h
>>> +++ b/fs/overlayfs/ovl_entry.h
>>> @@ -22,6 +22,8 @@ struct ovl_config {
>>>  struct ovl_layer {
>>>       struct vfsmount *mnt;
>>>       dev_t pseudo_dev;
>>> +     /* Index of this layer in fs root (upper == 0) */
>>> +     int idx;
>>
>> Just curious. If we are stroing idx, then do we have to store ovl_layer
>> and every ovl_entry. Just idx can give us the position and we should
>> be able to then accessovl_find_layer mnt and pseudo_dev directly from
>> ofs->lower_layers[idx]. If yes, we can avoid storing vfsmount and
>> pseudo_dev per dentry.
>>
>
> But we are not storing vfsmount and pseudo_dev per dentry
> We are storing &ofs->lower_layers[idx] per dentry.
> Do we gain anything from storing the idx instead?
> Most of the times code needs to access ofs->lower_layers[idx]
> fields (mnt mostly).
> The cases where idx itself matter are rare, for example:
> if dentry has numlower == 1 and idx > 1, this is a quick indication
> that a parent of lower dir may be copied up but not be indexed
> (because a dir on top of it in layer 1 is indexed).
>
> The current series does not make use of the example above,
> but it can be used to relax copy up of dir on encode when
> lower idx == 1.
>

Evidently, not only that the series does not use layer idx info
for decoding lower dir file handle, it does not use layer idx at all,
except for the ovl_lookup() change in this patch.
The patch is in the series because of an early attempt on mine
to implement connectable file handles, which I abandoned for now.
However:
1. I think that getting rid of ovl_find_layer() alone is a good enough reason
    to store layer idx
2. layer idx turned out to be useful in several cases I worked on, like ovl-xino
    and ovl-redirect-origin, so it probably doesn't hurt to store it
for future sake
3. Based on the current code, I can post a trivial patch to relax copy up of dir
   on encode when ofs->nulower > 1 in case lower dir layer->idx == 1.
   Just wasn't sure if that case is worth optimizing, so decided to wait for
   comments on the ofs->nulower > 1 behavior.

Cheers,
Amir.

Re: [PATCH v2 03/23] ovl: store layer index in ovl_layer

[Vivek Goyal]

On Fri, Jan 05, 2018 at 07:05:45AM +0200, Amir Goldstein wrote:
> On Thu, Jan 4, 2018 at 11:00 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Thu, Jan 04, 2018 at 06:39:58PM +0200, Amir Goldstein wrote:
> >> Store the fs root layer index inside ovl_layer struct, so we can
> >> get the root fs layer index from merge dir lower layer instead of
> >> find it with ovl_find_layer() helper.
> >>
> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >> ---
> >>  fs/overlayfs/namei.c     | 17 +----------------
> >>  fs/overlayfs/ovl_entry.h |  2 ++
> >>  fs/overlayfs/super.c     |  1 +
> >>  3 files changed, 4 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> >> index 71db9a966d88..a48ee02c4524 100644
> >> --- a/fs/overlayfs/namei.c
> >> +++ b/fs/overlayfs/namei.c
> >> @@ -572,18 +572,6 @@ int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
> >>       return (idx < oe->numlower) ? idx + 1 : -1;
> >>  }
> >>
> >> -static int ovl_find_layer(struct ovl_fs *ofs, struct ovl_path *path)
> >> -{
> >> -     int i;
> >> -
> >> -     for (i = 0; i < ofs->numlower; i++) {
> >> -             if (ofs->lower_layers[i].mnt == path->layer->mnt)
> >> -                     break;
> >> -     }
> >> -
> >> -     return i;
> >> -}
> >> -
> >>  /* Fix missing 'origin' xattr */
> >>  static int ovl_fix_origin(struct dentry *dentry, struct dentry *lower,
> >>                         struct dentry *upper)
> >> @@ -733,11 +721,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >>
> >>               if (d.redirect && d.redirect[0] == '/' && poe != roe) {
> >>                       poe = roe;
> >> -
> >>                       /* Find the current layer on the root dentry */
> >> -                     i = ovl_find_layer(ofs, &lower);
> >> -                     if (WARN_ON(i == ofs->numlower))
> >> -                             break;
> >> +                     i = lower.layer->idx - 1;
> >>               }
> >>       }
> >>
> >> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> >> index 9d0bc03bf6e4..608e48755070 100644
> >> --- a/fs/overlayfs/ovl_entry.h
> >> +++ b/fs/overlayfs/ovl_entry.h
> >> @@ -22,6 +22,8 @@ struct ovl_config {
> >>  struct ovl_layer {
> >>       struct vfsmount *mnt;
> >>       dev_t pseudo_dev;
> >> +     /* Index of this layer in fs root (upper == 0) */
> >> +     int idx;
> >
> > Just curious. If we are stroing idx, then do we have to store ovl_layer
> > and every ovl_entry. Just idx can give us the position and we should
> > be able to then access mnt and pseudo_dev directly from
> > ofs->lower_layers[idx]. If yes, we can avoid storing vfsmount and
> > pseudo_dev per dentry.
> >
> 
> But we are not storing vfsmount and pseudo_dev per dentry
> We are storing &ofs->lower_layers[idx] per dentry.

Aha, I misread the code. So we are storing layer pointer per dentry.

> Do we gain anything from storing the idx instead?
> Most of the times code needs to access ofs->lower_layers[idx]
> fields (mnt mostly).
> The cases where idx itself matter are rare, for example:
> if dentry has numlower == 1 and idx > 1, this is a quick indication
> that a parent of lower dir may be copied up but not be indexed
> (because a dir on top of it in layer 1 is indexed).

I am lost now. Can you elaborate a bit more. What is indexed dir?

Only call to ovl_find_layer() now seems to be in ovl_lookup() when we
find a redirect entry starting with "/". In that case we want to 
start looking from next lower layer in root dentry. So finding current
index of lower layer is important (So that one can continue looking 
into next lower layer of root dentry) and hence storing idx makes sense.

Vivek

> 
> The current series does not make use of the example above,
> but it can be used to relax copy up of dir on encode when
> lower idx == 1.
> 
> 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: [PATCH v2 03/23] ovl: store layer index in ovl_layer

[Vivek Goyal]

On Fri, Jan 05, 2018 at 01:22:34PM +0200, Amir Goldstein wrote:
> On Fri, Jan 5, 2018 at 7:05 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Thu, Jan 4, 2018 at 11:00 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> On Thu, Jan 04, 2018 at 06:39:58PM +0200, Amir Goldstein wrote:
> >>> Store the fs root layer index inside ovl_layer struct, so we can
> >>> get the root fs layer index from merge dir lower layer instead of
> >>> find it with ovl_find_layer() helper.
> >>>
> >>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >>> ---
> >>>  fs/overlayfs/namei.c     | 17 +----------------
> >>>  fs/overlayfs/ovl_entry.h |  2 ++
> >>>  fs/overlayfs/super.c     |  1 +
> >>>  3 files changed, 4 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> >>> index 71db9a966d88..a48ee02c4524 100644
> >>> --- a/fs/overlayfs/namei.c
> >>> +++ b/fs/overlayfs/namei.c
> >>> @@ -572,18 +572,6 @@ int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
> >>>       return (idx < oe->numlower) ? idx + 1 : -1;
> >>>  }
> >>>
> >>> -static int ovl_find_layer(struct ovl_fs *ofs, struct ovl_path *path)
> >>> -{
> >>> -     int i;
> >>> -
> >>> -     for (i = 0; i < ofs->numlower; i++) {
> >>> -             if (ofs->lower_layers[i].mnt == path->layer->mnt)
> >>> -                     break;
> >>> -     }
> >>> -
> >>> -     return i;
> >>> -}
> >>> -
> >>>  /* Fix missing 'origin' xattr */
> >>>  static int ovl_fix_origin(struct dentry *dentry, struct dentry *lower,
> >>>                         struct dentry *upper)
> >>> @@ -733,11 +721,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >>>
> >>>               if (d.redirect && d.redirect[0] == '/' && poe != roe) {
> >>>                       poe = roe;
> >>> -
> >>>                       /* Find the current layer on the root dentry */
> >>> -                     i = ovl_find_layer(ofs, &lower);
> >>> -                     if (WARN_ON(i == ofs->numlower))
> >>> -                             break;
> >>> +                     i = lower.layer->idx - 1;
> >>>               }
> >>>       }
> >>>
> >>> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> >>> index 9d0bc03bf6e4..608e48755070 100644
> >>> --- a/fs/overlayfs/ovl_entry.h
> >>> +++ b/fs/overlayfs/ovl_entry.h
> >>> @@ -22,6 +22,8 @@ struct ovl_config {
> >>>  struct ovl_layer {
> >>>       struct vfsmount *mnt;
> >>>       dev_t pseudo_dev;
> >>> +     /* Index of this layer in fs root (upper == 0) */
> >>> +     int idx;
> >>
> >> Just curious. If we are stroing idx, then do we have to store ovl_layer
> >> and every ovl_entry. Just idx can give us the position and we should
> >> be able to then accessovl_find_layer mnt and pseudo_dev directly from
> >> ofs->lower_layers[idx]. If yes, we can avoid storing vfsmount and
> >> pseudo_dev per dentry.
> >>
> >
> > But we are not storing vfsmount and pseudo_dev per dentry
> > We are storing &ofs->lower_layers[idx] per dentry.
> > Do we gain anything from storing the idx instead?
> > Most of the times code needs to access ofs->lower_layers[idx]
> > fields (mnt mostly).
> > The cases where idx itself matter are rare, for example:
> > if dentry has numlower == 1 and idx > 1, this is a quick indication
> > that a parent of lower dir may be copied up but not be indexed
> > (because a dir on top of it in layer 1 is indexed).
> >
> > The current series does not make use of the example above,
> > but it can be used to relax copy up of dir on encode when
> > lower idx == 1.
> >
> 
> Evidently, not only that the series does not use layer idx info
> for decoding lower dir file handle, it does not use layer idx at all,
> except for the ovl_lookup() change in this patch.
> The patch is in the series because of an early attempt on mine
> to implement connectable file handles, which I abandoned for now.
> However:
> 1. I think that getting rid of ovl_find_layer() alone is a good enough reason
>     to store layer idx
> 2. layer idx turned out to be useful in several cases I worked on, like ovl-xino
>     and ovl-redirect-origin, so it probably doesn't hurt to store it
> for future sake

Agreed. Just for the case of ovl_lookup(), storing idx in layer probably
is a good idea.

> 3. Based on the current code, I can post a trivial patch to relax copy up of dir
>    on encode when ofs->nulower > 1 in case lower dir layer->idx == 1.
>    Just wasn't sure if that case is worth optimizing, so decided to wait for
>    comments on the ofs->nulower > 1 behavior.

I must admit, I can't understand this. Please elaborate a bit more.

Vivek
> 
> Cheers,
> 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: [PATCH v2 06/23] ovl: add support for "verify" feature

[Vivek Goyal]

On Thu, Jan 04, 2018 at 06:40:01PM +0200, Amir Goldstein wrote:
> Introduce the "verify" config, module and mount options.
> 
> If the "verify" feature is enabled then overlay filesystems will use
> the inodes index dir to verify layers consistency.
> 
> It is possible to enable consistency verification by default during
> build time with the OVERLAY_FS_VERIFY config option, during runtime
> with the "verify=on" module option or on a filesystem instance basis
> with the "verify=on" mount option.
> 
> The "verify" feature will prevent multiple redirects to the same lower
> dir and will prevent broken hardlinks from using the same inode number.

So how does one end up with multiple redirects to same lower dir. 

Also what's the issue with broken hardlinks using same inode number. You
mean lower and upper are on different fs and upper can use same inode
number? With index=on, broken hardlink issue is taken care of so. So
why would somebody use verify=on, which looks like will create index
for all kind of files/dir.

I have not followed this development and there is not much background
in patch descrition or header patch, so asking all these basic questions.

Vivek

> 
> On an overlay filesystem instance with multiple lower layers, "verify"
> feature can only use the index to prevent multiple redirects from upper
> dirs and not from middle layer dirs. Emit a warning about this on mount
> with multiple lower layers and "verify=on".
> 
> This is going to be used for NFS export.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/Kconfig     | 18 ++++++++++++++++++
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/ovl_entry.h |  1 +
>  fs/overlayfs/super.c     | 49 +++++++++++++++++++++++++++++++++++++++++-------
>  fs/overlayfs/util.c      |  7 +++++++
>  5 files changed, 69 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index 5ac415466861..0e4764ed4e23 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -53,3 +53,21 @@ config OVERLAY_FS_INDEX
>  	  outcomes.  However, mounting the same overlay with an old kernel
>  	  read-write and then mounting it again with a new kernel, will have
>  	  unexpected results.
> +
> +config OVERLAY_FS_VERIFY
> +	bool "Overlayfs: turn on verify feature by default"
> +	depends on OVERLAY_FS
> +	depends on OVERLAY_FS_INDEX
> +	help
> +	  If this config option is enabled then overlay filesystems will use
> +	  the inodes index dir to verify layers consistency by default.
> +	  In this case, it is still possible to turn off consistency
> +	  verification globally with the "verify=off" module option or on a
> +	  filesystem instance basis with the "verify=off" mount option.
> +
> +	  The verify feature prevents multiple redirects to the same lower dir
> +	  and prevents broken hardlinks from using the same inode number.
> +
> +	  Note, that verify feature is not backward compatible.  That is,
> +	  mounting an overlay with verification index entries on a kernel
> +	  that doesn't support this feature will have unexpected results.
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index d55afb6646b0..379bb349acc4 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -194,6 +194,7 @@ const struct cred *ovl_override_creds(struct super_block *sb);
>  struct super_block *ovl_same_sb(struct super_block *sb);
>  bool ovl_can_decode_fh(struct super_block *sb);
>  struct dentry *ovl_indexdir(struct super_block *sb);
> +bool ovl_verify(struct super_block *sb);
>  struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
>  bool ovl_dentry_remote(struct dentry *dentry);
>  bool ovl_dentry_weird(struct dentry *dentry);
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 608e48755070..c886ed743a8a 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -17,6 +17,7 @@ struct ovl_config {
>  	bool redirect_follow;
>  	const char *redirect_mode;
>  	bool index;
> +	bool verify;
>  };
>  
>  struct ovl_layer {
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 994d35628acf..96136528861f 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -45,6 +45,11 @@ module_param_named(index, ovl_index_def, bool, 0644);
>  MODULE_PARM_DESC(ovl_index_def,
>  		 "Default to on or off for the inodes index feature");
>  
> +static bool ovl_verify_def = IS_ENABLED(CONFIG_OVERLAY_FS_VERIFY);
> +module_param_named(verify, ovl_verify_def, bool, 0644);
> +MODULE_PARM_DESC(ovl_verify_def,
> +		 "Default to on or off for the verify feature");
> +
>  static void ovl_entry_stack_free(struct ovl_entry *oe)
>  {
>  	unsigned int i;
> @@ -341,6 +346,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>  		seq_printf(m, ",redirect_dir=%s", ofs->config.redirect_mode);
>  	if (ofs->config.index != ovl_index_def)
>  		seq_printf(m, ",index=%s", ofs->config.index ? "on" : "off");
> +	if (ofs->config.verify != ovl_verify_def)
> +		seq_printf(m, ",verify=%s", ofs->config.verify ? "on" : "off");
>  	return 0;
>  }
>  
> @@ -373,6 +380,8 @@ enum {
>  	OPT_REDIRECT_DIR,
>  	OPT_INDEX_ON,
>  	OPT_INDEX_OFF,
> +	OPT_VERIFY_ON,
> +	OPT_VERIFY_OFF,
>  	OPT_ERR,
>  };
>  
> @@ -384,6 +393,8 @@ static const match_table_t ovl_tokens = {
>  	{OPT_REDIRECT_DIR,		"redirect_dir=%s"},
>  	{OPT_INDEX_ON,			"index=on"},
>  	{OPT_INDEX_OFF,			"index=off"},
> +	{OPT_VERIFY_ON,			"verify=on"},
> +	{OPT_VERIFY_OFF,		"verify=off"},
>  	{OPT_ERR,			NULL}
>  };
>  
> @@ -487,7 +498,19 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  			break;
>  
>  		case OPT_INDEX_OFF:
> +			/* verify depends on index */
>  			config->index = false;
> +			config->verify = false;
> +			break;
> +
> +		case OPT_VERIFY_ON:
> +			/* verify depends on index */
> +			config->index = true;
> +			config->verify = true;
> +			break;
> +
> +		case OPT_VERIFY_OFF:
> +			config->verify = false;
>  			break;
>  
>  		default:
> @@ -504,9 +527,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  			kfree(config->workdir);
>  			config->workdir = NULL;
>  		}
> -		if (config->index != ovl_index_def) {
> -			pr_info("overlayfs: option \"index\" is useless in a non-upper mount, ignore\n");
> +		if (config->index != ovl_index_def ||
> +		    config->verify != ovl_verify_def) {
> +			pr_info("overlayfs: options \"index\" and \"verify\" are useless in a non-upper mount, ignore\n");
>  			config->index = ovl_index_def;
> +			config->verify = ovl_verify_def;
>  		}
>  	}
>  
> @@ -707,7 +732,9 @@ static int ovl_lower_dir(const char *name, struct path *path,
>  	if (ofs->config.upperdir && ofs->config.index &&
>  	    !ovl_can_decode_fh(path->dentry->d_sb)) {
>  		ofs->config.index = false;
> -		pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off.\n", name);
> +		ofs->config.verify = false;
> +		pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off,verify=off.\n",
> +			name);
>  	}
>  
>  	return 0;
> @@ -975,7 +1002,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
>  	if (err) {
>  		ofs->noxattr = true;
>  		ofs->config.index = false;
> -		pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off.\n");
> +		ofs->config.verify = false;
> +		pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off,verify=off.\n");
>  		err = 0;
>  	} else {
>  		vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
> @@ -985,7 +1013,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
>  	if (ofs->config.index &&
>  	    !ovl_can_decode_fh(ofs->workdir->d_sb)) {
>  		ofs->config.index = false;
> -		pr_warn("overlayfs: upper fs does not support file handles, falling back to index=off.\n");
> +		ofs->config.verify = false;
> +		pr_warn("overlayfs: upper fs does not support file handles, falling back to index=off,verify=off.\n");
>  	}
>  
>  out:
> @@ -1146,6 +1175,8 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>  	} else if (!ofs->config.upperdir && stacklen == 1) {
>  		pr_err("overlayfs: at least 2 lowerdir are needed while upperdir nonexistent\n");
>  		goto out_err;
> +	} else if (ofs->config.verify && ofs->config.upperdir && stacklen > 1) {
> +		pr_warn("overlayfs: option 'verify=on' cannot verify redirects from middle layer dirs\n");
>  	}
>  
>  	err = -ENOMEM;
> @@ -1222,6 +1253,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>  		goto out_err;
>  
>  	ofs->config.index = ovl_index_def;
> +	/* verify depends on index */
> +	ofs->config.verify = ovl_verify_def && ovl_index_def;
>  	err = ovl_parse_opt((char *) data, &ofs->config);
>  	if (err)
>  		goto out_err;
> @@ -1276,9 +1309,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>  			sb->s_flags |= SB_RDONLY;
>  	}
>  
> -	/* Show index=off/on in /proc/mounts for any of the reasons above */
> -	if (!ofs->indexdir)
> +	/* Show index=off,verify=off in /proc/mounts if no indexdir */
> +	if (!ofs->indexdir) {
>  		ofs->config.index = false;
> +		ofs->config.verify = false;
> +	}
>  
>  	/* Never override disk quota limits or use reserved space */
>  	cap_lower(cred->cap_effective, CAP_SYS_RESOURCE);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index d6bb1c9f5e7a..fb0b561b0568 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -63,6 +63,13 @@ struct dentry *ovl_indexdir(struct super_block *sb)
>  	return ofs->indexdir;
>  }
>  
> +bool ovl_verify(struct super_block *sb)
> +{
> +	struct ovl_fs *ofs = sb->s_fs_info;
> +
> +	return ofs->config.verify && ofs->config.index;
> +}
> +
>  struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
>  {
>  	size_t size = offsetof(struct ovl_entry, lowerstack[numlower]);
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/23] ovl: add support for "verify" feature

[Amir Goldstein]

On Fri, Jan 5, 2018 at 5:43 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Jan 04, 2018 at 06:40:01PM +0200, Amir Goldstein wrote:
>> Introduce the "verify" config, module and mount options.
>>
>> If the "verify" feature is enabled then overlay filesystems will use
>> the inodes index dir to verify layers consistency.
>>
>> It is possible to enable consistency verification by default during
>> build time with the OVERLAY_FS_VERIFY config option, during runtime
>> with the "verify=on" module option or on a filesystem instance basis
>> with the "verify=on" mount option.
>>
>> The "verify" feature will prevent multiple redirects to the same lower
>> dir and will prevent broken hardlinks from using the same inode number.
>
> So how does one end up with multiple redirects to same lower dir.
>
> Also what's the issue with broken hardlinks using same inode number. You
> mean lower and upper are on different fs and upper can use same inode
> number? With index=on, broken hardlink issue is taken care of so. So
> why would somebody use verify=on, which looks like will create index
> for all kind of files/dir.
>
> I have not followed this development and there is not much background
> in patch descrition or header patch, so asking all these basic questions.
>

I have a ready answer for you :)
This is how it can happen and this is what can go wrong:
https://github.com/amir73il/xfstests/commit/f3c18125539660329195f142973ea620d6899418

The test duplicates a redirect dir offline with cp -a, but the
same thing can be done with a dir with origin.
In both cases, 'diff' will wrongly report that the files/dir are the same
even though they may differ (after being copied).

Amir.

Re: [PATCH v2 06/23] ovl: add support for "verify" feature

[Amir Goldstein]

On Fri, Jan 5, 2018 at 5:47 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Jan 5, 2018 at 5:43 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> On Thu, Jan 04, 2018 at 06:40:01PM +0200, Amir Goldstein wrote:
>>> Introduce the "verify" config, module and mount options.
>>>
>>> If the "verify" feature is enabled then overlay filesystems will use
>>> the inodes index dir to verify layers consistency.
>>>
>>> It is possible to enable consistency verification by default during
>>> build time with the OVERLAY_FS_VERIFY config option, during runtime
>>> with the "verify=on" module option or on a filesystem instance basis
>>> with the "verify=on" mount option.
>>>
>>> The "verify" feature will prevent multiple redirects to the same lower
>>> dir and will prevent broken hardlinks from using the same inode number.
>>
>> So how does one end up with multiple redirects to same lower dir.
>>
>> Also what's the issue with broken hardlinks using same inode number. You
>> mean lower and upper are on different fs and upper can use same inode
>> number? With index=on, broken hardlink issue is taken care of so. So
>> why would somebody use verify=on, which looks like will create index
>> for all kind of files/dir.
>>
>> I have not followed this development and there is not much background
>> in patch descrition or header patch, so asking all these basic questions.
>>
>
> I have a ready answer for you :)
> This is how it can happen and this is what can go wrong:
> https://github.com/amir73il/xfstests/commit/f3c18125539660329195f142973ea620d6899418
>
> The test duplicates a redirect dir offline with cp -a, but the
> same thing can be done with a dir with origin.
typo: with a non-dir with origin.
> In both cases, 'diff' will wrongly report that the files/dir are the same
> even though they may differ (after being copied).
>
> Amir.

Re: [PATCH v2 07/23] ovl: verify stored origin fh matches lower dir

[Vivek Goyal]

On Thu, Jan 04, 2018 at 06:40:02PM +0200, Amir Goldstein wrote:
> When the "verify" feature is enabled, a directory inode found in lower
> layer by name or by redirect_dir is verified against the file handle of
> the copy up origin that is stored in the upper layer.
> 
> This introduces a change of behavior for the case of lower layer
> modification while overlay is offline. A lower directory created or
> moved offline under an exisitng upper directory, will not be merged with
> that upper directory.
> 
> The "verify" feature should not be used after copying layers,
> because the new lower directory inodes would fail verification.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  Documentation/filesystems/overlayfs.txt | 16 ++++++++++++++++
>  fs/overlayfs/namei.c                    | 13 +++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> index e6a5f4912b6d..00e0595f3d7e 100644
> --- a/Documentation/filesystems/overlayfs.txt
> +++ b/Documentation/filesystems/overlayfs.txt
> @@ -299,6 +299,22 @@ filesystem are not allowed.  If the underlying filesystem is changed,
>  the behavior of the overlay is undefined, though it will not result in
>  a crash or deadlock.
>  
> +When the underlying filesystems supports NFS export, overlay mount can be
> +made more resilient to offline and online changes of the underlying lower
> +layer by enabling the "verify" feature.
> +
> +On every copy_up, an NFS file handle of the lower inode, along with the
> +UUID of the lower filesystem, are encoded and stored in an extended
> +attribute "trusted.overlay.origin" on the upper inode.
> +
> +When the "verify" feature is enabled, a lookup of a merged directory, that
> +found a lower directory at the lookup path or at the path pointed to by
> +the "trusted.overlay.redirect" extended attribute, will verify that the
> +found lower directory file handle and lower filesystem UUID match the
> +origin file handle that was stored at copy_up time.  If a found lower
> +directory does not match the stored origin, that directory will not be
> +merged with the upper directory.
> +
>  Testsuite
>  ---------
>  
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 46a3e31b0225..56deb2785af7 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -734,6 +734,19 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>  			}
>  		}
>  
> +		/*
> +		 * When "verify" feature is enabled, do not merge with a lower
> +		 * dir that does not match a stored origin xattr.
> +		 */
> +		if (upperdentry && !ctr && ovl_verify(dentry->d_sb)) {
> +			err = ovl_verify_origin(upperdentry, this, false,
> +						false);
> +			if (err) {
> +				dput(this);
> +				break;
> +			}
> +		}
> +

So this will verify directory origin only for top level lower dir. Rest
of the lowers can still be modified offline without this code noticing it?

Vivek
>  		stack[ctr].dentry = this;
>  		stack[ctr].layer = lower.layer;
>  		ctr++;
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 07/23] ovl: verify stored origin fh matches lower dir

[Amir Goldstein]

On Fri, Jan 5, 2018 at 6:04 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Jan 04, 2018 at 06:40:02PM +0200, Amir Goldstein wrote:
>> When the "verify" feature is enabled, a directory inode found in lower
>> layer by name or by redirect_dir is verified against the file handle of
>> the copy up origin that is stored in the upper layer.
>>
>> This introduces a change of behavior for the case of lower layer
>> modification while overlay is offline. A lower directory created or
>> moved offline under an exisitng upper directory, will not be merged with
>> that upper directory.
>>
>> The "verify" feature should not be used after copying layers,
>> because the new lower directory inodes would fail verification.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  Documentation/filesystems/overlayfs.txt | 16 ++++++++++++++++
>>  fs/overlayfs/namei.c                    | 13 +++++++++++++
>>  2 files changed, 29 insertions(+)
>>
>> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
>> index e6a5f4912b6d..00e0595f3d7e 100644
>> --- a/Documentation/filesystems/overlayfs.txt
>> +++ b/Documentation/filesystems/overlayfs.txt
>> @@ -299,6 +299,22 @@ filesystem are not allowed.  If the underlying filesystem is changed,
>>  the behavior of the overlay is undefined, though it will not result in
>>  a crash or deadlock.
>>
>> +When the underlying filesystems supports NFS export, overlay mount can be
>> +made more resilient to offline and online changes of the underlying lower
>> +layer by enabling the "verify" feature.
>> +
>> +On every copy_up, an NFS file handle of the lower inode, along with the
>> +UUID of the lower filesystem, are encoded and stored in an extended
>> +attribute "trusted.overlay.origin" on the upper inode.
>> +
>> +When the "verify" feature is enabled, a lookup of a merged directory, that
>> +found a lower directory at the lookup path or at the path pointed to by
>> +the "trusted.overlay.redirect" extended attribute, will verify that the
>> +found lower directory file handle and lower filesystem UUID match the
>> +origin file handle that was stored at copy_up time.  If a found lower
>> +directory does not match the stored origin, that directory will not be
>> +merged with the upper directory.
>> +
>>  Testsuite
>>  ---------
>>
>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> index 46a3e31b0225..56deb2785af7 100644
>> --- a/fs/overlayfs/namei.c
>> +++ b/fs/overlayfs/namei.c
>> @@ -734,6 +734,19 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>>                       }
>>               }
>>
>> +             /*
>> +              * When "verify" feature is enabled, do not merge with a lower
>> +              * dir that does not match a stored origin xattr.
>> +              */
>> +             if (upperdentry && !ctr && ovl_verify(dentry->d_sb)) {
>> +                     err = ovl_verify_origin(upperdentry, this, false,
>> +                                             false);
>> +                     if (err) {
>> +                             dput(this);
>> +                             break;
>> +                     }
>> +             }
>> +
>
> So this will verify directory origin only for top level lower dir. Rest
> of the lowers can still be modified offline without this code noticing it?
>

Correct, from patch 6/23:

+       } else if (ofs->config.verify && ofs->config.upperdir && stacklen > 1) {
+               pr_warn("overlayfs: option 'verify=on' cannot verify
redirects from middle layer dirs\n");

Amir.

Re: [PATCH v2 07/23] ovl: verify stored origin fh matches lower dir

[Vivek Goyal]

On Fri, Jan 05, 2018 at 06:26:55PM +0200, Amir Goldstein wrote:
> On Fri, Jan 5, 2018 at 6:04 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Thu, Jan 04, 2018 at 06:40:02PM +0200, Amir Goldstein wrote:
> >> When the "verify" feature is enabled, a directory inode found in lower
> >> layer by name or by redirect_dir is verified against the file handle of
> >> the copy up origin that is stored in the upper layer.
> >>
> >> This introduces a change of behavior for the case of lower layer
> >> modification while overlay is offline. A lower directory created or
> >> moved offline under an exisitng upper directory, will not be merged with
> >> that upper directory.
> >>
> >> The "verify" feature should not be used after copying layers,
> >> because the new lower directory inodes would fail verification.
> >>
> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >> ---
> >>  Documentation/filesystems/overlayfs.txt | 16 ++++++++++++++++
> >>  fs/overlayfs/namei.c                    | 13 +++++++++++++
> >>  2 files changed, 29 insertions(+)
> >>
> >> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> >> index e6a5f4912b6d..00e0595f3d7e 100644
> >> --- a/Documentation/filesystems/overlayfs.txt
> >> +++ b/Documentation/filesystems/overlayfs.txt
> >> @@ -299,6 +299,22 @@ filesystem are not allowed.  If the underlying filesystem is changed,
> >>  the behavior of the overlay is undefined, though it will not result in
> >>  a crash or deadlock.
> >>
> >> +When the underlying filesystems supports NFS export, overlay mount can be
> >> +made more resilient to offline and online changes of the underlying lower
> >> +layer by enabling the "verify" feature.
> >> +
> >> +On every copy_up, an NFS file handle of the lower inode, along with the
> >> +UUID of the lower filesystem, are encoded and stored in an extended
> >> +attribute "trusted.overlay.origin" on the upper inode.
> >> +
> >> +When the "verify" feature is enabled, a lookup of a merged directory, that
> >> +found a lower directory at the lookup path or at the path pointed to by
> >> +the "trusted.overlay.redirect" extended attribute, will verify that the
> >> +found lower directory file handle and lower filesystem UUID match the
> >> +origin file handle that was stored at copy_up time.  If a found lower
> >> +directory does not match the stored origin, that directory will not be
> >> +merged with the upper directory.
> >> +
> >>  Testsuite
> >>  ---------
> >>
> >> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> >> index 46a3e31b0225..56deb2785af7 100644
> >> --- a/fs/overlayfs/namei.c
> >> +++ b/fs/overlayfs/namei.c
> >> @@ -734,6 +734,19 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >>                       }
> >>               }
> >>
> >> +             /*
> >> +              * When "verify" feature is enabled, do not merge with a lower
> >> +              * dir that does not match a stored origin xattr.
> >> +              */
> >> +             if (upperdentry && !ctr && ovl_verify(dentry->d_sb)) {
> >> +                     err = ovl_verify_origin(upperdentry, this, false,
> >> +                                             false);
> >> +                     if (err) {
> >> +                             dput(this);
> >> +                             break;
> >> +                     }
> >> +             }
> >> +
> >
> > So this will verify directory origin only for top level lower dir. Rest
> > of the lowers can still be modified offline without this code noticing it?
> >
> 
> Correct, from patch 6/23:
> 
> +       } else if (ofs->config.verify && ofs->config.upperdir && stacklen > 1) {
> +               pr_warn("overlayfs: option 'verify=on' cannot verify
> redirects from middle layer dirs\n");

So for non-dir case, we check origin by default and error out if decoding
of fh fails.  Why not do the same thing for directories as well. I mean
why directory origin check should be hidden behind a mount option
(verfiy=).

Vivek

> 
> 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: [PATCH v2 06/23] ovl: add support for "verify" feature

[Vivek Goyal]

On Fri, Jan 05, 2018 at 05:47:36PM +0200, Amir Goldstein wrote:
> On Fri, Jan 5, 2018 at 5:43 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Thu, Jan 04, 2018 at 06:40:01PM +0200, Amir Goldstein wrote:
> >> Introduce the "verify" config, module and mount options.
> >>
> >> If the "verify" feature is enabled then overlay filesystems will use
> >> the inodes index dir to verify layers consistency.
> >>
> >> It is possible to enable consistency verification by default during
> >> build time with the OVERLAY_FS_VERIFY config option, during runtime
> >> with the "verify=on" module option or on a filesystem instance basis
> >> with the "verify=on" mount option.
> >>
> >> The "verify" feature will prevent multiple redirects to the same lower
> >> dir and will prevent broken hardlinks from using the same inode number.
> >
> > So how does one end up with multiple redirects to same lower dir.
> >
> > Also what's the issue with broken hardlinks using same inode number. You
> > mean lower and upper are on different fs and upper can use same inode
> > number? With index=on, broken hardlink issue is taken care of so. So
> > why would somebody use verify=on, which looks like will create index
> > for all kind of files/dir.
> >
> > I have not followed this development and there is not much background
> > in patch descrition or header patch, so asking all these basic questions.
> >
> 
> I have a ready answer for you :)
> This is how it can happen and this is what can go wrong:
> https://github.com/amir73il/xfstests/commit/f3c18125539660329195f142973ea620d6899418
> 
> The test duplicates a redirect dir offline with cp -a, but the
> same thing can be done with a dir with origin.
> In both cases, 'diff' will wrongly report that the files/dir are the same
> even though they may differ (after being copied).

So there are so many things a user can do with overlayfs offline. Should
we need to put runtime checks for all these cases. I mean will
fsck.overlay be a better place to catch these kind of anomalies.

Secondly, verify=foo feels like a very generic option. Its not clear
what all will it verify and will we continue to pile up more checks
down the line under it.

As a user, when should one use verify=on? Its not clear to me.

Vivek

> 
> 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: [PATCH v2 07/23] ovl: verify stored origin fh matches lower dir

[Amir Goldstein]

On Fri, Jan 5, 2018 at 6:35 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Jan 05, 2018 at 06:26:55PM +0200, Amir Goldstein wrote:
>> On Fri, Jan 5, 2018 at 6:04 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Thu, Jan 04, 2018 at 06:40:02PM +0200, Amir Goldstein wrote:
>> >> When the "verify" feature is enabled, a directory inode found in lower
>> >> layer by name or by redirect_dir is verified against the file handle of
>> >> the copy up origin that is stored in the upper layer.
>> >>
>> >> This introduces a change of behavior for the case of lower layer
>> >> modification while overlay is offline. A lower directory created or
>> >> moved offline under an exisitng upper directory, will not be merged with
>> >> that upper directory.
>> >>
>> >> The "verify" feature should not be used after copying layers,
>> >> because the new lower directory inodes would fail verification.
>> >>
>> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> >> ---
>> >>  Documentation/filesystems/overlayfs.txt | 16 ++++++++++++++++
>> >>  fs/overlayfs/namei.c                    | 13 +++++++++++++
>> >>  2 files changed, 29 insertions(+)
>> >>
>> >> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
>> >> index e6a5f4912b6d..00e0595f3d7e 100644
>> >> --- a/Documentation/filesystems/overlayfs.txt
>> >> +++ b/Documentation/filesystems/overlayfs.txt
>> >> @@ -299,6 +299,22 @@ filesystem are not allowed.  If the underlying filesystem is changed,
>> >>  the behavior of the overlay is undefined, though it will not result in
>> >>  a crash or deadlock.
>> >>
>> >> +When the underlying filesystems supports NFS export, overlay mount can be
>> >> +made more resilient to offline and online changes of the underlying lower
>> >> +layer by enabling the "verify" feature.
>> >> +
>> >> +On every copy_up, an NFS file handle of the lower inode, along with the
>> >> +UUID of the lower filesystem, are encoded and stored in an extended
>> >> +attribute "trusted.overlay.origin" on the upper inode.
>> >> +
>> >> +When the "verify" feature is enabled, a lookup of a merged directory, that
>> >> +found a lower directory at the lookup path or at the path pointed to by
>> >> +the "trusted.overlay.redirect" extended attribute, will verify that the
>> >> +found lower directory file handle and lower filesystem UUID match the
>> >> +origin file handle that was stored at copy_up time.  If a found lower
>> >> +directory does not match the stored origin, that directory will not be
>> >> +merged with the upper directory.
>> >> +
>> >>  Testsuite
>> >>  ---------
>> >>
>> >> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> >> index 46a3e31b0225..56deb2785af7 100644
>> >> --- a/fs/overlayfs/namei.c
>> >> +++ b/fs/overlayfs/namei.c
>> >> @@ -734,6 +734,19 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>> >>                       }
>> >>               }
>> >>
>> >> +             /*
>> >> +              * When "verify" feature is enabled, do not merge with a lower
>> >> +              * dir that does not match a stored origin xattr.
>> >> +              */
>> >> +             if (upperdentry && !ctr && ovl_verify(dentry->d_sb)) {
>> >> +                     err = ovl_verify_origin(upperdentry, this, false,
>> >> +                                             false);
>> >> +                     if (err) {
>> >> +                             dput(this);
>> >> +                             break;
>> >> +                     }
>> >> +             }
>> >> +
>> >
>> > So this will verify directory origin only for top level lower dir. Rest
>> > of the lowers can still be modified offline without this code noticing it?
>> >
>>
>> Correct, from patch 6/23:
>>
>> +       } else if (ofs->config.verify && ofs->config.upperdir && stacklen > 1) {
>> +               pr_warn("overlayfs: option 'verify=on' cannot verify
>> redirects from middle layer dirs\n");
>
> So for non-dir case, we check origin by default and error out if decoding
> of fh fails.  Why not do the same thing for directories as well. I mean
> why directory origin check should be hidden behind a mount option
> (verfiy=).
>

Because it is a change of behavior so we need users to opt-in for it.
When copying layers, file handles become stale.
As a result, inode numbers of non-dir are taken from upper, but after
copying inode numbers change anyway, so the implication is moot.
You cannot say the same for copied merge dirs with origin that becomes
stale. The implication of not following lower dir after copying are significant.

Amir.

Re: [PATCH v2 06/23] ovl: add support for "verify" feature

[Amir Goldstein]

On Fri, Jan 5, 2018 at 6:39 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Jan 05, 2018 at 05:47:36PM +0200, Amir Goldstein wrote:
>> On Fri, Jan 5, 2018 at 5:43 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Thu, Jan 04, 2018 at 06:40:01PM +0200, Amir Goldstein wrote:
>> >> Introduce the "verify" config, module and mount options.
>> >>
>> >> If the "verify" feature is enabled then overlay filesystems will use
>> >> the inodes index dir to verify layers consistency.
>> >>
>> >> It is possible to enable consistency verification by default during
>> >> build time with the OVERLAY_FS_VERIFY config option, during runtime
>> >> with the "verify=on" module option or on a filesystem instance basis
>> >> with the "verify=on" mount option.
>> >>
>> >> The "verify" feature will prevent multiple redirects to the same lower
>> >> dir and will prevent broken hardlinks from using the same inode number.
>> >
>> > So how does one end up with multiple redirects to same lower dir.
>> >
>> > Also what's the issue with broken hardlinks using same inode number. You
>> > mean lower and upper are on different fs and upper can use same inode
>> > number? With index=on, broken hardlink issue is taken care of so. So
>> > why would somebody use verify=on, which looks like will create index
>> > for all kind of files/dir.
>> >
>> > I have not followed this development and there is not much background
>> > in patch descrition or header patch, so asking all these basic questions.
>> >
>>
>> I have a ready answer for you :)
>> This is how it can happen and this is what can go wrong:
>> https://github.com/amir73il/xfstests/commit/f3c18125539660329195f142973ea620d6899418
>>
>> The test duplicates a redirect dir offline with cp -a, but the
>> same thing can be done with a dir with origin.
>> In both cases, 'diff' will wrongly report that the files/dir are the same
>> even though they may differ (after being copied).
>
> So there are so many things a user can do with overlayfs offline. Should

Like what things for example?
I admit the test case is a gray area, which is why I asked Eryu to hold
back on merging the test until Miklos weights in his opinion.
But I argued before that the offline changes done by the test could in fact
be made by an innocent user.
For example, by unpacking an upper image tarball over an existing
upper layer offline.
Before redirect_dir, this might have worked fine.
After redirect_dir, this could result in multiple redirect if any of the upper
dirs were renamed.


> we need to put runtime checks for all these cases. I mean will
> fsck.overlay be a better place to catch these kind of anomalies.
>

I will answer the same thing I answered to Zhangyi.
My opinion is that like ext4/xfs/etc, the jurisdiction of the file
system is to detect
inconsistencies at runtime as much as possible and return an error to user and
suggest the user to run fsck to fix the problem.
And short of naming fsck.overlay, this is exactly what verify=on does.
If it detects a multiple redirect or redirect without whiteout covering
origin, it errors on lookup and prints a message to log.

> Secondly, verify=foo feels like a very generic option. Its not clear
> what all will it verify and will we continue to pile up more checks
> down the line under it.
>
> As a user, when should one use verify=on? Its not clear to me.
>

Those are good questions to address to Miklos.
In V1, the mount option was called 'index=all' for indexing every file/dir
on copy up and the verify origin functionality was implemented with
a different option (verify_dir).
Then I realized that full indexing could actually be a feature on its own,
so I named this feature and documented what it does in Kconfig.

Another option name I was contemplating is 'redirect_dir=verify'
this name makes a lot more sense for the part of detecting multiple
redirects.
The reason I went for the more generic name 'verify=on' is because the
feature also detects multiple files with same origin (which are not hardlinks).

I have no strong feelings about the verify=on feature name.
BTW, I also have a follow up work, related to snapshots that uses the
mount option 'redirect_dir=origin', which means, if lower does not match
origin, try to decode origin from file handle and fix redirect.
So personally, I am quite fond of replacing 'verify=on' with
'redirect_dir=verify' even though it also affects non-dir.

Thanks for your inputs!

Amir.

Re: [PATCH v2 03/23] ovl: store layer index in ovl_layer

[Amir Goldstein]

On Fri, Jan 5, 2018 at 5:00 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Jan 05, 2018 at 01:22:34PM +0200, Amir Goldstein wrote:
[...]
>
>> 3. Based on the current code, I can post a trivial patch to relax copy up of dir
>>    on encode when ofs->nulower > 1 in case lower dir layer->idx == 1.
>>    Just wasn't sure if that case is worth optimizing, so decided to wait for
>>    comments on the ofs->nulower > 1 behavior.
>
> I must admit, I can't understand this. Please elaborate a bit more.
>

I admit this needs elaborating...

Because middle layers redirects are not "indexed", decoding an overlay
dir file handle that was encoded from a dir in layer > 1 (before copy up)
is complicated and was not implemented in my implementation of NFS export.
The complication is due to the fact that lower dir can have an ancestor in
middle layer above (say in layer 1) that is redirected and we cannot find that
redirect in index.

In V1, NFS export was not supported for ofs->numlower > 1 (lame).
In V2, all dirs are copied up on encode and the upper dir encoded,
so any lower redirect becomes irrelevant when decoding.

Copying up all dirs for ->numlower > 1 to work around middle layer redirects
is a bit of an overkill.
Lower dirs on layer 1 can be exempted from copy up, because they should (*)
not have an ancestor in layer above (upper layer) that is redirected and not
indexed.

So it is easy to apply this relaxing rule if the lower layer index
info is available
in the overlay dir dentry. I did not make that optimization, because I
can't tell
if that will matter to anyone.

(*) NFS export is expected to work well when starting with a clean upper layer
and verify=on, so all copy ups dir and non-dir are indexed. When mounting an
overlay with verify=on, where old non-indexed merge dirs exist, NFS export
will be "best effort", meaning:
- encoding/decoding a non-indexed merge dir works fine whether or not this dir
  is redirected (because decoded upper dir provides the overlay path).
- encoding/decoding a lower dir, whose ancestors is a non-indexed merge dir,
  but NOT redirected works fine (because decoding lower dir yields the
correct path).
- encoding/decoding a lower dir, whose ancestors is a non-indexed merge dir,
  AND redirected will result in -ESTALE

Should probably add a line to documentation about "undefined behavior" when
exporting overlayfs that was not mounted with 'verify=on' to begin with.... and
in the future suggest to run fsck.overlay to index all redirects.

Was I able to clarify more than confusing you??

Amir.

Re: [PATCH v2 07/23] ovl: verify stored origin fh matches lower dir

[Vivek Goyal]

On Fri, Jan 05, 2018 at 06:42:09PM +0200, Amir Goldstein wrote:
> On Fri, Jan 5, 2018 at 6:35 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Fri, Jan 05, 2018 at 06:26:55PM +0200, Amir Goldstein wrote:
> >> On Fri, Jan 5, 2018 at 6:04 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > On Thu, Jan 04, 2018 at 06:40:02PM +0200, Amir Goldstein wrote:
> >> >> When the "verify" feature is enabled, a directory inode found in lower
> >> >> layer by name or by redirect_dir is verified against the file handle of
> >> >> the copy up origin that is stored in the upper layer.
> >> >>
> >> >> This introduces a change of behavior for the case of lower layer
> >> >> modification while overlay is offline. A lower directory created or
> >> >> moved offline under an exisitng upper directory, will not be merged with
> >> >> that upper directory.
> >> >>
> >> >> The "verify" feature should not be used after copying layers,
> >> >> because the new lower directory inodes would fail verification.
> >> >>
> >> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >> >> ---
> >> >>  Documentation/filesystems/overlayfs.txt | 16 ++++++++++++++++
> >> >>  fs/overlayfs/namei.c                    | 13 +++++++++++++
> >> >>  2 files changed, 29 insertions(+)
> >> >>
> >> >> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> >> >> index e6a5f4912b6d..00e0595f3d7e 100644
> >> >> --- a/Documentation/filesystems/overlayfs.txt
> >> >> +++ b/Documentation/filesystems/overlayfs.txt
> >> >> @@ -299,6 +299,22 @@ filesystem are not allowed.  If the underlying filesystem is changed,
> >> >>  the behavior of the overlay is undefined, though it will not result in
> >> >>  a crash or deadlock.
> >> >>
> >> >> +When the underlying filesystems supports NFS export, overlay mount can be
> >> >> +made more resilient to offline and online changes of the underlying lower
> >> >> +layer by enabling the "verify" feature.
> >> >> +
> >> >> +On every copy_up, an NFS file handle of the lower inode, along with the
> >> >> +UUID of the lower filesystem, are encoded and stored in an extended
> >> >> +attribute "trusted.overlay.origin" on the upper inode.
> >> >> +
> >> >> +When the "verify" feature is enabled, a lookup of a merged directory, that
> >> >> +found a lower directory at the lookup path or at the path pointed to by
> >> >> +the "trusted.overlay.redirect" extended attribute, will verify that the
> >> >> +found lower directory file handle and lower filesystem UUID match the
> >> >> +origin file handle that was stored at copy_up time.  If a found lower
> >> >> +directory does not match the stored origin, that directory will not be
> >> >> +merged with the upper directory.
> >> >> +
> >> >>  Testsuite
> >> >>  ---------
> >> >>
> >> >> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> >> >> index 46a3e31b0225..56deb2785af7 100644
> >> >> --- a/fs/overlayfs/namei.c
> >> >> +++ b/fs/overlayfs/namei.c
> >> >> @@ -734,6 +734,19 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >> >>                       }
> >> >>               }
> >> >>
> >> >> +             /*
> >> >> +              * When "verify" feature is enabled, do not merge with a lower
> >> >> +              * dir that does not match a stored origin xattr.
> >> >> +              */
> >> >> +             if (upperdentry && !ctr && ovl_verify(dentry->d_sb)) {
> >> >> +                     err = ovl_verify_origin(upperdentry, this, false,
> >> >> +                                             false);
> >> >> +                     if (err) {
> >> >> +                             dput(this);
> >> >> +                             break;
> >> >> +                     }
> >> >> +             }
> >> >> +
> >> >
> >> > So this will verify directory origin only for top level lower dir. Rest
> >> > of the lowers can still be modified offline without this code noticing it?
> >> >
> >>
> >> Correct, from patch 6/23:
> >>
> >> +       } else if (ofs->config.verify && ofs->config.upperdir && stacklen > 1) {
> >> +               pr_warn("overlayfs: option 'verify=on' cannot verify
> >> redirects from middle layer dirs\n");
> >
> > So for non-dir case, we check origin by default and error out if decoding
> > of fh fails.  Why not do the same thing for directories as well. I mean
> > why directory origin check should be hidden behind a mount option
> > (verfiy=).
> >
> 
> Because it is a change of behavior so we need users to opt-in for it.
> When copying layers, file handles become stale.

But do we support copying layers? Because in that case non-dir
fh will become stale. So that suggests that we don't support it
already.

> As a result, inode numbers of non-dir are taken from upper, but after
> copying inode numbers change anyway, so the implication is moot.

With metadata copy, implications are very significant again.

> You cannot say the same for copied merge dirs with origin that becomes
> stale. The implication of not following lower dir after copying are significant.

It really feels odd that we will have different behavior for dir and
non-dir objects when it comes to origin verification. Will a new
mount option say verify_origin=on/off make sense. That way this is verify
specific and verifies origin (both for dir and non-dir objects) and user
will opt in to maintain backward compatibility.

Vivek

Re: [PATCH v2 07/23] ovl: verify stored origin fh matches lower dir

[Amir Goldstein]

On Fri, Jan 5, 2018 at 8:33 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Jan 05, 2018 at 06:42:09PM +0200, Amir Goldstein wrote:
[...]
>> >
>> > So for non-dir case, we check origin by default and error out if decoding
>> > of fh fails.  Why not do the same thing for directories as well. I mean
>> > why directory origin check should be hidden behind a mount option
>> > (verfiy=).
>> >
>>
>> Because it is a change of behavior so we need users to opt-in for it.
>> When copying layers, file handles become stale.
>
> But do we support copying layers? Because in that case non-dir
> fh will become stale. So that suggests that we don't support it
> already.

Its perfectly fine if cannot decode origin for non-dir. it is not an error.
just won't use lower inode number.

>
>> As a result, inode numbers of non-dir are taken from upper, but after
>> copying inode numbers change anyway, so the implication is moot.
>
> With metadata copy, implications are very significant again.

Right, but with metadata copy you cannot copy layers.

>
>> You cannot say the same for copied merge dirs with origin that becomes
>> stale. The implication of not following lower dir after copying are significant.
>
> It really feels odd that we will have different behavior for dir and
> non-dir objects when it comes to origin verification. Will a new
> mount option say verify_origin=on/off make sense. That way this is verify
> specific and verifies origin (both for dir and non-dir objects) and user
> will opt in to maintain backward compatibility.
>

I can go with renaming verify to verify_origin.
Let's see what Miklos has to say.

Thanks,
Amir.

Re: [PATCH v2 06/23] ovl: add support for "verify" feature

[Vivek Goyal]

On Fri, Jan 05, 2018 at 07:07:47PM +0200, Amir Goldstein wrote:
> On Fri, Jan 5, 2018 at 6:39 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Fri, Jan 05, 2018 at 05:47:36PM +0200, Amir Goldstein wrote:
> >> On Fri, Jan 5, 2018 at 5:43 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > On Thu, Jan 04, 2018 at 06:40:01PM +0200, Amir Goldstein wrote:
> >> >> Introduce the "verify" config, module and mount options.
> >> >>
> >> >> If the "verify" feature is enabled then overlay filesystems will use
> >> >> the inodes index dir to verify layers consistency.
> >> >>
> >> >> It is possible to enable consistency verification by default during
> >> >> build time with the OVERLAY_FS_VERIFY config option, during runtime
> >> >> with the "verify=on" module option or on a filesystem instance basis
> >> >> with the "verify=on" mount option.
> >> >>
> >> >> The "verify" feature will prevent multiple redirects to the same lower
> >> >> dir and will prevent broken hardlinks from using the same inode number.
> >> >
> >> > So how does one end up with multiple redirects to same lower dir.
> >> >
> >> > Also what's the issue with broken hardlinks using same inode number. You
> >> > mean lower and upper are on different fs and upper can use same inode
> >> > number? With index=on, broken hardlink issue is taken care of so. So
> >> > why would somebody use verify=on, which looks like will create index
> >> > for all kind of files/dir.
> >> >
> >> > I have not followed this development and there is not much background
> >> > in patch descrition or header patch, so asking all these basic questions.
> >> >
> >>
> >> I have a ready answer for you :)
> >> This is how it can happen and this is what can go wrong:
> >> https://github.com/amir73il/xfstests/commit/f3c18125539660329195f142973ea620d6899418
> >>
> >> The test duplicates a redirect dir offline with cp -a, but the
> >> same thing can be done with a dir with origin.
> >> In both cases, 'diff' will wrongly report that the files/dir are the same
> >> even though they may differ (after being copied).
> >
> > So there are so many things a user can do with overlayfs offline. Should
> 
> Like what things for example?

I am referring to all the offline modifications user can do to lower
and upper. Remove some overlay xattrs we maintain, or add some or play
with link counts on lower. It could be anything.

> I admit the test case is a gray area, which is why I asked Eryu to hold
> back on merging the test until Miklos weights in his opinion.
> But I argued before that the offline changes done by the test could in fact
> be made by an innocent user.
> For example, by unpacking an upper image tarball over an existing
> upper layer offline.

What is "upper image tarabll"? You mean a tarball of a directory which
is being used as upper somewhere else. If somebody does that and adds
contents to upper dir, then we will have unpredictable consequences.
My understanding is that we are not supposed to write to lower or upper
dir once overlay has been created.

> Before redirect_dir, this might have worked fine.
> After redirect_dir, this could result in multiple redirect if any of the upper
> dirs were renamed.
> 
> 
> > we need to put runtime checks for all these cases. I mean will
> > fsck.overlay be a better place to catch these kind of anomalies.
> >
> 
> I will answer the same thing I answered to Zhangyi.
> My opinion is that like ext4/xfs/etc, the jurisdiction of the file
> system is to detect
> inconsistencies at runtime as much as possible and return an error to user and
> suggest the user to run fsck to fix the problem.

Sounds reasonable. So with every inconsistency check, did they introduce
new mount option to let user opt-in. Looks like that's what we will have
to do if we decide to not break backward comatibility.

> And short of naming fsck.overlay, this is exactly what verify=on does.
> If it detects a multiple redirect or redirect without whiteout covering
> origin, it errors on lookup and prints a message to log.
> 
> > Secondly, verify=foo feels like a very generic option. Its not clear
> > what all will it verify and will we continue to pile up more checks
> > down the line under it.
> >
> > As a user, when should one use verify=on? Its not clear to me.
> >
> 
> Those are good questions to address to Miklos.
> In V1, the mount option was called 'index=all' for indexing every file/dir
> on copy up and the verify origin functionality was implemented with
> a different option (verify_dir).
> Then I realized that full indexing could actually be a feature on its own,
> so I named this feature and documented what it does in Kconfig.

index=all was much more intuitive to me. I could understand that it will
create index for all kind of objects (and not just hardlinks).

> 
> Another option name I was contemplating is 'redirect_dir=verify'
> this name makes a lot more sense for the part of detecting multiple
> redirects.

Or verify_redirect_dir=on/off. Advantage of fine grained options is
that user can opt-in. But at the same time too many options will be
confusing. 

> The reason I went for the more generic name 'verify=on' is because the
> feature also detects multiple files with same origin (which are not hardlinks).

So one problem with verify=on is that say it checks for 3 inconsistencies
today. What will happen when we check for 4th inconsistency tomorrow. Say
we want to enforce ORIGIN validity on non-dir objects. If we combine it
with verify=on, then it becomes a backward copatibility issue. If we
come up with a separate option, then we kind of split it badly. That is
non-dir origin verification has separate mount option while dir origin
verification is part of verify=on.

Atleast a normal user will find it very hard to figure it out.

> I have no strong feelings about the verify=on feature name.

I don't have strong feelings either. I am just trying to think from a
user's point of view and also thinking what will happen when more
inconsistency checks are added in future. Will these be part of verify=on
or we will create new options due to backward compatibility concerns.

Vivek

> BTW, I also have a follow up work, related to snapshots that uses the
> mount option 'redirect_dir=origin', which means, if lower does not match
> origin, try to decode origin from file handle and fix redirect.
> So personally, I am quite fond of replacing 'verify=on' with
> 'redirect_dir=verify' even though it also affects non-dir.
> 
> Thanks for your inputs!
> 
> Amir.

Re: [PATCH v2 06/23] ovl: add support for "verify" feature

[Amir Goldstein]

On Fri, Jan 5, 2018 at 9:07 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Jan 05, 2018 at 07:07:47PM +0200, Amir Goldstein wrote:
>> On Fri, Jan 5, 2018 at 6:39 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Fri, Jan 05, 2018 at 05:47:36PM +0200, Amir Goldstein wrote:
>> >> On Fri, Jan 5, 2018 at 5:43 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> > On Thu, Jan 04, 2018 at 06:40:01PM +0200, Amir Goldstein wrote:
>> >> >> Introduce the "verify" config, module and mount options.
>> >> >>
>> >> >> If the "verify" feature is enabled then overlay filesystems will use
>> >> >> the inodes index dir to verify layers consistency.
>> >> >>
>> >> >> It is possible to enable consistency verification by default during
>> >> >> build time with the OVERLAY_FS_VERIFY config option, during runtime
>> >> >> with the "verify=on" module option or on a filesystem instance basis
>> >> >> with the "verify=on" mount option.
>> >> >>
>> >> >> The "verify" feature will prevent multiple redirects to the same lower
>> >> >> dir and will prevent broken hardlinks from using the same inode number.
>> >> >
>> >> > So how does one end up with multiple redirects to same lower dir.
>> >> >
>> >> > Also what's the issue with broken hardlinks using same inode number. You
>> >> > mean lower and upper are on different fs and upper can use same inode
>> >> > number? With index=on, broken hardlink issue is taken care of so. So
>> >> > why would somebody use verify=on, which looks like will create index
>> >> > for all kind of files/dir.
>> >> >
>> >> > I have not followed this development and there is not much background
>> >> > in patch descrition or header patch, so asking all these basic questions.
>> >> >
>> >>
>> >> I have a ready answer for you :)
>> >> This is how it can happen and this is what can go wrong:
>> >> https://github.com/amir73il/xfstests/commit/f3c18125539660329195f142973ea620d6899418
>> >>
>> >> The test duplicates a redirect dir offline with cp -a, but the
>> >> same thing can be done with a dir with origin.
>> >> In both cases, 'diff' will wrongly report that the files/dir are the same
>> >> even though they may differ (after being copied).
>> >
>> > So there are so many things a user can do with overlayfs offline. Should
>>
>> Like what things for example?
>
> I am referring to all the offline modifications user can do to lower
> and upper. Remove some overlay xattrs we maintain, or add some or play
> with link counts on lower. It could be anything.
>
>> I admit the test case is a gray area, which is why I asked Eryu to hold
>> back on merging the test until Miklos weights in his opinion.
>> But I argued before that the offline changes done by the test could in fact
>> be made by an innocent user.
>> For example, by unpacking an upper image tarball over an existing
>> upper layer offline.
>
> What is "upper image tarabll"? You mean a tarball of a directory which
> is being used as upper somewhere else. If somebody does that and adds
> contents to upper dir, then we will have unpredictable consequences.
> My understanding is that we are not supposed to write to lower or upper
> dir once overlay has been created.
>

There is a notorious clause in overlayfs.txt documentation that says
that offline modifications to layers are allowed. This clause says nothing
about unpredictable results.

Miklos already said that we should add exceptions to this clause, but
that's not really the point. The point is how people use overlayfs out there
and what is their perception of allowed modifications.

I don't think that anyone sane would think of intentionally mangeling
with trusted overlay xattr and believe that is an acceptable modification,
so we can rule these out.
But people do think it is sane to manually add files and remove files from
layers offline and for that matter, why would they think it is wrong to
copy (-a) files inside upper layer?
So an innocent user copied a file in upper layer (that was copied up)
and unintentionally copies the origin xattr with it, causing a real problem -
from now on those files may have different data, but the same st_dev;st_ino
so diff thinks they are the same.

>> Before redirect_dir, this might have worked fine.
>> After redirect_dir, this could result in multiple redirect if any of the upper
>> dirs were renamed.
>>
>>
>> > we need to put runtime checks for all these cases. I mean will
>> > fsck.overlay be a better place to catch these kind of anomalies.
>> >
>>
>> I will answer the same thing I answered to Zhangyi.
>> My opinion is that like ext4/xfs/etc, the jurisdiction of the file
>> system is to detect
>> inconsistencies at runtime as much as possible and return an error to user and
>> suggest the user to run fsck to fix the problem.
>
> Sounds reasonable. So with every inconsistency check, did they introduce
> new mount option to let user opt-in. Looks like that's what we will have
> to do if we decide to not break backward comatibility.
>

No they don't need to add an opt-in feature when adding consistency checks,
but those fs already have a well established on-disk format and an fsck tool
to verify it.
Whenever we change the on-disk format we need an opt-in feature.
verify_origin=on (I'm sizing up new new name ;) is not changing on-disk format
per-se, but it is re-interpreting  on-disk format. With
verify_origin=off the origin
xattr on dirs does not really mean much (except for OVL_WHITEOUT flag).
the origin xattr for merge dirs was added for future feature, such as
verify_origin.

Users today are allowed to remove and re-create lower dir offline and expect
new lower dir to be merged. So in order to have a stronger interpretation of
origin xattr on merge dir, we need an opt-in feature. and we may also need
some options for the feature to choose between different behaviors:
- skip unverified lower (verify_origin=on | redirect_dir=verify ?)
- fix redirect from origin for unverified lower
(verify_origin=redirect | redirect_dir=origin ?)
- fix origin from redirect for unverified lower (verify_origin=fix |
redirect_dir=fix ?)

>> And short of naming fsck.overlay, this is exactly what verify=on does.
>> If it detects a multiple redirect or redirect without whiteout covering
>> origin, it errors on lookup and prints a message to log.
>>
>> > Secondly, verify=foo feels like a very generic option. Its not clear
>> > what all will it verify and will we continue to pile up more checks
>> > down the line under it.
>> >
>> > As a user, when should one use verify=on? Its not clear to me.
>> >
>>
>> Those are good questions to address to Miklos.
>> In V1, the mount option was called 'index=all' for indexing every file/dir
>> on copy up and the verify origin functionality was implemented with
>> a different option (verify_dir).
>> Then I realized that full indexing could actually be a feature on its own,
>> so I named this feature and documented what it does in Kconfig.
>
> index=all was much more intuitive to me. I could understand that it will
> create index for all kind of objects (and not just hardlinks).
>

Yes, it was straight forward to understand technically, but what does it
say to users? absolutely nothing. OTOH redirect_dir is not called
dir_rename, so yeh, index=all can work.
Whatever Miklos prefers is fine by me.

>>
>> Another option name I was contemplating is 'redirect_dir=verify'
>> this name makes a lot more sense for the part of detecting multiple
>> redirects.
>
> Or verify_redirect_dir=on/off. Advantage of fine grained options is
> that user can opt-in. But at the same time too many options will be
> confusing.

The reason I like 'redirect_dir=verify' is because redirect_dir already has
modes now and those modes are not going to be a complex matrix of
options, just a linear scale of options: off,follow,on,verify
which are relatively easy to document.

So maybe the most logical thing to do is to add redirect_dir=verify to
control the new behavior of verifying merge dir and detecting multiple redirect
and add an option index=all to control full indexing and detecting multiple
origin of non-dir. And either let redirect_dir=verify imply and turn on
index=all or let redirect_dir=verify require index=all and fail mount.

How do you feel about this option?

>
>> The reason I went for the more generic name 'verify=on' is because the
>> feature also detects multiple files with same origin (which are not hardlinks).
>
> So one problem with verify=on is that say it checks for 3 inconsistencies
> today. What will happen when we check for 4th inconsistency tomorrow. Say
> we want to enforce ORIGIN validity on non-dir objects. If we combine it
> with verify=on, then it becomes a backward copatibility issue. If we
> come up with a separate option, then we kind of split it badly. That is
> non-dir origin verification has separate mount option while dir origin
> verification is part of verify=on.
>
> Atleast a normal user will find it very hard to figure it out.
>
>> I have no strong feelings about the verify=on feature name.
>
> I don't have strong feelings either. I am just trying to think from a
> user's point of view and also thinking what will happen when more
> inconsistency checks are added in future. Will these be part of verify=on
> or we will create new options due to backward compatibility concerns.
>

I understand your concern.
I agree that verify=on sounds a bit too broad and inviting trouble ;-)

Amir.

Re: [PATCH v2 06/23] ovl: add support for "verify" feature

[Amir Goldstein]

On Fri, Jan 5, 2018 at 10:20 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Jan 5, 2018 at 9:07 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
[...]
>>> Those are good questions to address to Miklos.
>>> In V1, the mount option was called 'index=all' for indexing every file/dir
>>> on copy up and the verify origin functionality was implemented with
>>> a different option (verify_dir).
>>> Then I realized that full indexing could actually be a feature on its own,
>>> so I named this feature and documented what it does in Kconfig.
>>
>> index=all was much more intuitive to me. I could understand that it will
>> create index for all kind of objects (and not just hardlinks).
>>
>
> Yes, it was straight forward to understand technically, but what does it
> say to users? absolutely nothing. OTOH redirect_dir is not called
> dir_rename, so yeh, index=all can work.
> Whatever Miklos prefers is fine by me.
>
>>>
>>> Another option name I was contemplating is 'redirect_dir=verify'
>>> this name makes a lot more sense for the part of detecting multiple
>>> redirects.
>>
>> Or verify_redirect_dir=on/off. Advantage of fine grained options is
>> that user can opt-in. But at the same time too many options will be
>> confusing.
>
> The reason I like 'redirect_dir=verify' is because redirect_dir already has
> modes now and those modes are not going to be a complex matrix of
> options, just a linear scale of options: off,follow,on,verify
> which are relatively easy to document.
>
> So maybe the most logical thing to do is to add redirect_dir=verify to
> control the new behavior of verifying merge dir and detecting multiple redirect
> and add an option index=all to control full indexing and detecting multiple
> origin of non-dir. And either let redirect_dir=verify imply and turn on
> index=all or let redirect_dir=verify require index=all and fail mount.
>
> How do you feel about this option?
>
>>
>>> The reason I went for the more generic name 'verify=on' is because the
>>> feature also detects multiple files with same origin (which are not hardlinks).
>>
>> So one problem with verify=on is that say it checks for 3 inconsistencies
>> today. What will happen when we check for 4th inconsistency tomorrow. Say
>> we want to enforce ORIGIN validity on non-dir objects. If we combine it
>> with verify=on, then it becomes a backward copatibility issue. If we
>> come up with a separate option, then we kind of split it badly. That is
>> non-dir origin verification has separate mount option while dir origin
>> verification is part of verify=on.
>>
>> Atleast a normal user will find it very hard to figure it out.
>>
>>> I have no strong feelings about the verify=on feature name.
>>
>> I don't have strong feelings either. I am just trying to think from a
>> user's point of view and also thinking what will happen when more
>> inconsistency checks are added in future. Will these be part of verify=on
>> or we will create new options due to backward compatibility concerns.
>>
>
> I understand your concern.
> I agree that verify=on sounds a bit too broad and inviting trouble ;-)
>

But I just remembered why I switched from 2 opt-in mount options in V1
to a single config/module option in V2. consistency verification as well
NFS export don't play well with overlays that are mounted without full
index, so I wanted to have a config option that would make NFS export
"just work" with overlayfs if default mount options are used.

I guess I could have OVERLAY_FS_INDEX_ALL as a config option
that depends on OVERLAY_FS_INDEX and redirect_dir=verify as
a mount option.

Or, I could follow you advise and just rename OVERLAY_FS_VERIFY
to OVERLAY_FS_VERIFY_ORIGIN, which implies both full index
and verification of lower dir.

That last option is my favorite at the moment.

How about it?

Amir.

Re: [PATCH v2 03/23] ovl: store layer index in ovl_layer

[Miklos Szeredi]

On Fri, Jan 5, 2018 at 12:22 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Jan 5, 2018 at 7:05 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Thu, Jan 4, 2018 at 11:00 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>> On Thu, Jan 04, 2018 at 06:39:58PM +0200, Amir Goldstein wrote:
>>>> Store the fs root layer index inside ovl_layer struct, so we can
>>>> get the root fs layer index from merge dir lower layer instead of
>>>> find it with ovl_find_layer() helper.
>>>>
>>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>>> ---
>>>>  fs/overlayfs/namei.c     | 17 +----------------
>>>>  fs/overlayfs/ovl_entry.h |  2 ++
>>>>  fs/overlayfs/super.c     |  1 +
>>>>  3 files changed, 4 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>>>> index 71db9a966d88..a48ee02c4524 100644
>>>> --- a/fs/overlayfs/namei.c
>>>> +++ b/fs/overlayfs/namei.c
>>>> @@ -572,18 +572,6 @@ int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
>>>>       return (idx < oe->numlower) ? idx + 1 : -1;
>>>>  }
>>>>
>>>> -static int ovl_find_layer(struct ovl_fs *ofs, struct ovl_path *path)
>>>> -{
>>>> -     int i;
>>>> -
>>>> -     for (i = 0; i < ofs->numlower; i++) {
>>>> -             if (ofs->lower_layers[i].mnt == path->layer->mnt)
>>>> -                     break;
>>>> -     }
>>>> -
>>>> -     return i;
>>>> -}
>>>> -
>>>>  /* Fix missing 'origin' xattr */
>>>>  static int ovl_fix_origin(struct dentry *dentry, struct dentry *lower,
>>>>                         struct dentry *upper)
>>>> @@ -733,11 +721,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>>>>
>>>>               if (d.redirect && d.redirect[0] == '/' && poe != roe) {
>>>>                       poe = roe;
>>>> -
>>>>                       /* Find the current layer on the root dentry */
>>>> -                     i = ovl_find_layer(ofs, &lower);
>>>> -                     if (WARN_ON(i == ofs->numlower))
>>>> -                             break;
>>>> +                     i = lower.layer->idx - 1;
>>>>               }
>>>>       }
>>>>
>>>> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
>>>> index 9d0bc03bf6e4..608e48755070 100644
>>>> --- a/fs/overlayfs/ovl_entry.h
>>>> +++ b/fs/overlayfs/ovl_entry.h
>>>> @@ -22,6 +22,8 @@ struct ovl_config {
>>>>  struct ovl_layer {
>>>>       struct vfsmount *mnt;
>>>>       dev_t pseudo_dev;
>>>> +     /* Index of this layer in fs root (upper == 0) */
>>>> +     int idx;
>>>
>>> Just curious. If we are stroing idx, then do we have to store ovl_layer
>>> and every ovl_entry. Just idx can give us the position and we should
>>> be able to then accessovl_find_layer mnt and pseudo_dev directly from
>>> ofs->lower_layers[idx]. If yes, we can avoid storing vfsmount and
>>> pseudo_dev per dentry.
>>>
>>
>> But we are not storing vfsmount and pseudo_dev per dentry
>> We are storing &ofs->lower_layers[idx] per dentry.
>> Do we gain anything from storing the idx instead?

We could go to 16bit index and decrease memory requirement of
ofs->lower_layers by 75% on 64bit archs.

Probably not worth it, though, since very few entries will have a
large enough layers to matter in most situations, I guess.

Also, someone might want >64k layers, although that really stretches
the limits of current implementation.

Thanks,
Miklos

Re: [PATCH v2 06/23] ovl: add support for "verify" feature

[Miklos Szeredi]

On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Introduce the "verify" config, module and mount options.
>
> If the "verify" feature is enabled then overlay filesystems will use
> the inodes index dir to verify layers consistency.
>
> It is possible to enable consistency verification by default during
> build time with the OVERLAY_FS_VERIFY config option, during runtime
> with the "verify=on" module option or on a filesystem instance basis
> with the "verify=on" mount option.
>
> The "verify" feature will prevent multiple redirects to the same lower
> dir and will prevent broken hardlinks from using the same inode number.
>
> On an overlay filesystem instance with multiple lower layers, "verify"
> feature can only use the index to prevent multiple redirects from upper
> dirs and not from middle layer dirs. Emit a warning about this on mount
> with multiple lower layers and "verify=on".

Could the above go into Documentation/filesystems/overlayfs.txt as well?

Also, the verify feature does have some overhead, which should also be
noted in the documentation.  Do you have some measurements about how
much impact it has on performance?

Thanks,
Miklos


>
> This is going to be used for NFS export.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/Kconfig     | 18 ++++++++++++++++++
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/ovl_entry.h |  1 +
>  fs/overlayfs/super.c     | 49 +++++++++++++++++++++++++++++++++++++++++-------
>  fs/overlayfs/util.c      |  7 +++++++
>  5 files changed, 69 insertions(+), 7 deletions(-)
>
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index 5ac415466861..0e4764ed4e23 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -53,3 +53,21 @@ config OVERLAY_FS_INDEX
>           outcomes.  However, mounting the same overlay with an old kernel
>           read-write and then mounting it again with a new kernel, will have
>           unexpected results.
> +
> +config OVERLAY_FS_VERIFY
> +       bool "Overlayfs: turn on verify feature by default"
> +       depends on OVERLAY_FS
> +       depends on OVERLAY_FS_INDEX
> +       help
> +         If this config option is enabled then overlay filesystems will use
> +         the inodes index dir to verify layers consistency by default.
> +         In this case, it is still possible to turn off consistency
> +         verification globally with the "verify=off" module option or on a
> +         filesystem instance basis with the "verify=off" mount option.
> +
> +         The verify feature prevents multiple redirects to the same lower dir
> +         and prevents broken hardlinks from using the same inode number.
> +
> +         Note, that verify feature is not backward compatible.  That is,
> +         mounting an overlay with verification index entries on a kernel
> +         that doesn't support this feature will have unexpected results.
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index d55afb6646b0..379bb349acc4 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -194,6 +194,7 @@ const struct cred *ovl_override_creds(struct super_block *sb);
>  struct super_block *ovl_same_sb(struct super_block *sb);
>  bool ovl_can_decode_fh(struct super_block *sb);
>  struct dentry *ovl_indexdir(struct super_block *sb);
> +bool ovl_verify(struct super_block *sb);
>  struct ovl_entry *ovl_alloc_entry(unsigned int numlower);
>  bool ovl_dentry_remote(struct dentry *dentry);
>  bool ovl_dentry_weird(struct dentry *dentry);
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 608e48755070..c886ed743a8a 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -17,6 +17,7 @@ struct ovl_config {
>         bool redirect_follow;
>         const char *redirect_mode;
>         bool index;
> +       bool verify;
>  };
>
>  struct ovl_layer {
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 994d35628acf..96136528861f 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -45,6 +45,11 @@ module_param_named(index, ovl_index_def, bool, 0644);
>  MODULE_PARM_DESC(ovl_index_def,
>                  "Default to on or off for the inodes index feature");
>
> +static bool ovl_verify_def = IS_ENABLED(CONFIG_OVERLAY_FS_VERIFY);
> +module_param_named(verify, ovl_verify_def, bool, 0644);
> +MODULE_PARM_DESC(ovl_verify_def,
> +                "Default to on or off for the verify feature");
> +
>  static void ovl_entry_stack_free(struct ovl_entry *oe)
>  {
>         unsigned int i;
> @@ -341,6 +346,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>                 seq_printf(m, ",redirect_dir=%s", ofs->config.redirect_mode);
>         if (ofs->config.index != ovl_index_def)
>                 seq_printf(m, ",index=%s", ofs->config.index ? "on" : "off");
> +       if (ofs->config.verify != ovl_verify_def)
> +               seq_printf(m, ",verify=%s", ofs->config.verify ? "on" : "off");
>         return 0;
>  }
>
> @@ -373,6 +380,8 @@ enum {
>         OPT_REDIRECT_DIR,
>         OPT_INDEX_ON,
>         OPT_INDEX_OFF,
> +       OPT_VERIFY_ON,
> +       OPT_VERIFY_OFF,
>         OPT_ERR,
>  };
>
> @@ -384,6 +393,8 @@ static const match_table_t ovl_tokens = {
>         {OPT_REDIRECT_DIR,              "redirect_dir=%s"},
>         {OPT_INDEX_ON,                  "index=on"},
>         {OPT_INDEX_OFF,                 "index=off"},
> +       {OPT_VERIFY_ON,                 "verify=on"},
> +       {OPT_VERIFY_OFF,                "verify=off"},
>         {OPT_ERR,                       NULL}
>  };
>
> @@ -487,7 +498,19 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>                         break;
>
>                 case OPT_INDEX_OFF:
> +                       /* verify depends on index */
>                         config->index = false;
> +                       config->verify = false;
> +                       break;
> +
> +               case OPT_VERIFY_ON:
> +                       /* verify depends on index */
> +                       config->index = true;
> +                       config->verify = true;
> +                       break;
> +
> +               case OPT_VERIFY_OFF:
> +                       config->verify = false;
>                         break;
>
>                 default:
> @@ -504,9 +527,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>                         kfree(config->workdir);
>                         config->workdir = NULL;
>                 }
> -               if (config->index != ovl_index_def) {
> -                       pr_info("overlayfs: option \"index\" is useless in a non-upper mount, ignore\n");
> +               if (config->index != ovl_index_def ||
> +                   config->verify != ovl_verify_def) {
> +                       pr_info("overlayfs: options \"index\" and \"verify\" are useless in a non-upper mount, ignore\n");
>                         config->index = ovl_index_def;
> +                       config->verify = ovl_verify_def;
>                 }
>         }
>
> @@ -707,7 +732,9 @@ static int ovl_lower_dir(const char *name, struct path *path,
>         if (ofs->config.upperdir && ofs->config.index &&
>             !ovl_can_decode_fh(path->dentry->d_sb)) {
>                 ofs->config.index = false;
> -               pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off.\n", name);
> +               ofs->config.verify = false;
> +               pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off,verify=off.\n",
> +                       name);
>         }
>
>         return 0;
> @@ -975,7 +1002,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
>         if (err) {
>                 ofs->noxattr = true;
>                 ofs->config.index = false;
> -               pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off.\n");
> +               ofs->config.verify = false;
> +               pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off,verify=off.\n");
>                 err = 0;
>         } else {
>                 vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
> @@ -985,7 +1013,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
>         if (ofs->config.index &&
>             !ovl_can_decode_fh(ofs->workdir->d_sb)) {
>                 ofs->config.index = false;
> -               pr_warn("overlayfs: upper fs does not support file handles, falling back to index=off.\n");
> +               ofs->config.verify = false;
> +               pr_warn("overlayfs: upper fs does not support file handles, falling back to index=off,verify=off.\n");
>         }
>
>  out:
> @@ -1146,6 +1175,8 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>         } else if (!ofs->config.upperdir && stacklen == 1) {
>                 pr_err("overlayfs: at least 2 lowerdir are needed while upperdir nonexistent\n");
>                 goto out_err;
> +       } else if (ofs->config.verify && ofs->config.upperdir && stacklen > 1) {
> +               pr_warn("overlayfs: option 'verify=on' cannot verify redirects from middle layer dirs\n");
>         }
>
>         err = -ENOMEM;
> @@ -1222,6 +1253,8 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>                 goto out_err;
>
>         ofs->config.index = ovl_index_def;
> +       /* verify depends on index */
> +       ofs->config.verify = ovl_verify_def && ovl_index_def;
>         err = ovl_parse_opt((char *) data, &ofs->config);
>         if (err)
>                 goto out_err;
> @@ -1276,9 +1309,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>                         sb->s_flags |= SB_RDONLY;
>         }
>
> -       /* Show index=off/on in /proc/mounts for any of the reasons above */
> -       if (!ofs->indexdir)
> +       /* Show index=off,verify=off in /proc/mounts if no indexdir */
> +       if (!ofs->indexdir) {
>                 ofs->config.index = false;
> +               ofs->config.verify = false;
> +       }
>
>         /* Never override disk quota limits or use reserved space */
>         cap_lower(cred->cap_effective, CAP_SYS_RESOURCE);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index d6bb1c9f5e7a..fb0b561b0568 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -63,6 +63,13 @@ struct dentry *ovl_indexdir(struct super_block *sb)
>         return ofs->indexdir;
>  }
>
> +bool ovl_verify(struct super_block *sb)
> +{
> +       struct ovl_fs *ofs = sb->s_fs_info;
> +
> +       return ofs->config.verify && ofs->config.index;
> +}
> +
>  struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
>  {
>         size_t size = offsetof(struct ovl_entry, lowerstack[numlower]);
> --
> 2.7.4
>

Re: [PATCH v2 07/23] ovl: verify stored origin fh matches lower dir

[Miklos Szeredi]

On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> When the "verify" feature is enabled, a directory inode found in lower
> layer by name or by redirect_dir is verified against the file handle of
> the copy up origin that is stored in the upper layer.
>
> This introduces a change of behavior for the case of lower layer
> modification while overlay is offline. A lower directory created or
> moved offline under an exisitng upper directory, will not be merged with
> that upper directory.
>
> The "verify" feature should not be used after copying layers,
> because the new lower directory inodes would fail verification.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  Documentation/filesystems/overlayfs.txt | 16 ++++++++++++++++
>  fs/overlayfs/namei.c                    | 13 +++++++++++++
>  2 files changed, 29 insertions(+)
>
> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> index e6a5f4912b6d..00e0595f3d7e 100644
> --- a/Documentation/filesystems/overlayfs.txt
> +++ b/Documentation/filesystems/overlayfs.txt
> @@ -299,6 +299,22 @@ filesystem are not allowed.  If the underlying filesystem is changed,
>  the behavior of the overlay is undefined, though it will not result in
>  a crash or deadlock.
>
> +When the underlying filesystems supports NFS export, overlay mount can be
> +made more resilient to offline and online changes of the underlying lower
> +layer by enabling the "verify" feature.
> +
> +On every copy_up, an NFS file handle of the lower inode, along with the
> +UUID of the lower filesystem, are encoded and stored in an extended
> +attribute "trusted.overlay.origin" on the upper inode.
> +
> +When the "verify" feature is enabled, a lookup of a merged directory, that
> +found a lower directory at the lookup path or at the path pointed to by
> +the "trusted.overlay.redirect" extended attribute, will verify that the
> +found lower directory file handle and lower filesystem UUID match the
> +origin file handle that was stored at copy_up time.  If a found lower
> +directory does not match the stored origin, that directory will not be
> +merged with the upper directory.

Ah, here's the doc.

Thinking a bit about this...  I think this is a slightly different
category than "redirect_dir=on".  There we were expanding the
functionality, to allow directory renaming.  With "verify=on" we are
restricting the functionality to not allow some offline changes.
Which is really what we want for NFS.

But the doc should come with more info about when it makes sense to
enable it for non-NFS case and when it does not.  I think enabling
directory origin verification by default (e.g. with config option or
module option) is probably something most users or distros do not want
to do, and the documentation should reflect that.

As for the name, whatever describes the function best but is as short
as possible (e.g. "verify_dir=on/off" should be okay, I think).

The issue here is that NFS export will need lots of options, and that
may be confusing.  How about an "nfs_export=on" option that acts as a
shorthand for all options needed for NFS export?

Thanks,
Miklos

Re: [PATCH v2 06/23] ovl: add support for "verify" feature

[Amir Goldstein]

On Tue, Jan 9, 2018 at 11:16 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Introduce the "verify" config, module and mount options.
>>
>> If the "verify" feature is enabled then overlay filesystems will use
>> the inodes index dir to verify layers consistency.
>>
>> It is possible to enable consistency verification by default during
>> build time with the OVERLAY_FS_VERIFY config option, during runtime
>> with the "verify=on" module option or on a filesystem instance basis
>> with the "verify=on" mount option.
>>
>> The "verify" feature will prevent multiple redirects to the same lower
>> dir and will prevent broken hardlinks from using the same inode number.
>>
>> On an overlay filesystem instance with multiple lower layers, "verify"
>> feature can only use the index to prevent multiple redirects from upper
>> dirs and not from middle layer dirs. Emit a warning about this on mount
>> with multiple lower layers and "verify=on".
>
> Could the above go into Documentation/filesystems/overlayfs.txt as well?

Will do.
BTW, are you ok with the feature name "verify" or do you prefer that
I rename it to "verify_origin" to be more descriptive as Vivek suggested?

>
> Also, the verify feature does have some overhead, which should also be
> noted in the documentation.  Do you have some measurements about how
> much impact it has on performance?
>

I do not. It's on my TODO list.

I'm curious though, where do you think it should performance?
I would expect that encoding a file handle and index lookup would not be
that expensive, so I don't expect much lookup overhead. No?
For copy up, I doubt that creating 2 dirs instead of one, or 2 hardlinks
instead of one file would make a big difference?
I suppose that a highly concurrent copy workload up will become a little
contended of the index dir lock, but still nowhere near the fully sequential
nature of dir copy up.

"verify" feature will surely have an impact of index directory size.
I do have a concern about the impact of a very large index directory on
index lookup and creation times and I intend to test that.

A very just concern that Vivek raised is performance of overlay mount time.
Apparently, there are some workloads that are sensitive to mount/umount
time in the containers world.
For those workloads, ovl_indexdir_cleanup() could be a problem.
For directory index entries, ovl_verify_index() is decoding the upper dir file
handle and that can be expensive.
The cleanup on mount is not strictly needed for correctness of "verify" feature,
nor for NFS export, so we may want to consider a mount option for not
running ovl_indexdir_cleanup() or for running it in a non expensive mode
that skips the file handle verification?

Thanks,
Amir.

Re: [PATCH v2 07/23] ovl: verify stored origin fh matches lower dir

[Amir Goldstein]

On Tue, Jan 9, 2018 at 11:52 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> When the "verify" feature is enabled, a directory inode found in lower
>> layer by name or by redirect_dir is verified against the file handle of
>> the copy up origin that is stored in the upper layer.
>>
>> This introduces a change of behavior for the case of lower layer
>> modification while overlay is offline. A lower directory created or
>> moved offline under an exisitng upper directory, will not be merged with
>> that upper directory.
>>
>> The "verify" feature should not be used after copying layers,
>> because the new lower directory inodes would fail verification.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  Documentation/filesystems/overlayfs.txt | 16 ++++++++++++++++
>>  fs/overlayfs/namei.c                    | 13 +++++++++++++
>>  2 files changed, 29 insertions(+)
>>
>> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
>> index e6a5f4912b6d..00e0595f3d7e 100644
>> --- a/Documentation/filesystems/overlayfs.txt
>> +++ b/Documentation/filesystems/overlayfs.txt
>> @@ -299,6 +299,22 @@ filesystem are not allowed.  If the underlying filesystem is changed,
>>  the behavior of the overlay is undefined, though it will not result in
>>  a crash or deadlock.
>>
>> +When the underlying filesystems supports NFS export, overlay mount can be
>> +made more resilient to offline and online changes of the underlying lower
>> +layer by enabling the "verify" feature.
>> +
>> +On every copy_up, an NFS file handle of the lower inode, along with the
>> +UUID of the lower filesystem, are encoded and stored in an extended
>> +attribute "trusted.overlay.origin" on the upper inode.
>> +
>> +When the "verify" feature is enabled, a lookup of a merged directory, that
>> +found a lower directory at the lookup path or at the path pointed to by
>> +the "trusted.overlay.redirect" extended attribute, will verify that the
>> +found lower directory file handle and lower filesystem UUID match the
>> +origin file handle that was stored at copy_up time.  If a found lower
>> +directory does not match the stored origin, that directory will not be
>> +merged with the upper directory.
>
> Ah, here's the doc.
>
> Thinking a bit about this...  I think this is a slightly different
> category than "redirect_dir=on".  There we were expanding the
> functionality, to allow directory renaming.  With "verify=on" we are
> restricting the functionality to not allow some offline changes.
> Which is really what we want for NFS.
>
> But the doc should come with more info about when it makes sense to
> enable it for non-NFS case and when it does not.  I think enabling
> directory origin verification by default (e.g. with config option or
> module option) is probably something most users or distros do not want
> to do, and the documentation should reflect that.
>
> As for the name, whatever describes the function best but is as short
> as possible (e.g. "verify_dir=on/off" should be okay, I think).
>
> The issue here is that NFS export will need lots of options, and that
> may be confusing.  How about an "nfs_export=on" option that acts as a
> shorthand for all options needed for NFS export?
>

Yeh, I like that option much better. Especially, I like that it saves me the
trouble of documenting when it makes sense to turn on "verify" for
non-NFS case, because I don't know when it makes sense.

I will try to adjust documentation to explain that with NFS export enabled
lookup of merge dir becomes more "file handle oriented" and is therefore
less sensitive to some offlline changes (rename) and more sensitive to
other offline changes (remove+create).

Thanks,
Amir.

Re: [PATCH v2 06/23] ovl: add support for "verify" feature

[Miklos Szeredi]

On Tue, Jan 9, 2018 at 10:54 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Jan 9, 2018 at 11:16 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> Introduce the "verify" config, module and mount options.
>>>
>>> If the "verify" feature is enabled then overlay filesystems will use
>>> the inodes index dir to verify layers consistency.
>>>
>>> It is possible to enable consistency verification by default during
>>> build time with the OVERLAY_FS_VERIFY config option, during runtime
>>> with the "verify=on" module option or on a filesystem instance basis
>>> with the "verify=on" mount option.
>>>
>>> The "verify" feature will prevent multiple redirects to the same lower
>>> dir and will prevent broken hardlinks from using the same inode number.
>>>
>>> On an overlay filesystem instance with multiple lower layers, "verify"
>>> feature can only use the index to prevent multiple redirects from upper
>>> dirs and not from middle layer dirs. Emit a warning about this on mount
>>> with multiple lower layers and "verify=on".
>>
>> Could the above go into Documentation/filesystems/overlayfs.txt as well?
>
> Will do.
> BTW, are you ok with the feature name "verify" or do you prefer that
> I rename it to "verify_origin" to be more descriptive as Vivek suggested?
>
>>
>> Also, the verify feature does have some overhead, which should also be
>> noted in the documentation.  Do you have some measurements about how
>> much impact it has on performance?
>>
>
> I do not. It's on my TODO list.
>
> I'm curious though, where do you think it should performance?
> I would expect that encoding a file handle and index lookup would not be
> that expensive, so I don't expect much lookup overhead. No?

I wasn't thinking about anything specific, but now you ask:
directories always need to be connected, and that results in multiple
readdirs, etc, that is probably noticeable under some conditions.

> For copy up, I doubt that creating 2 dirs instead of one, or 2 hardlinks
> instead of one file would make a big difference?
> I suppose that a highly concurrent copy workload up will become a little
> contended of the index dir lock, but still nowhere near the fully sequential
> nature of dir copy up.
>
> "verify" feature will surely have an impact of index directory size.
> I do have a concern about the impact of a very large index directory on
> index lookup and creation times and I intend to test that.
>
> A very just concern that Vivek raised is performance of overlay mount time.
> Apparently, there are some workloads that are sensitive to mount/umount
> time in the containers world.
> For those workloads, ovl_indexdir_cleanup() could be a problem.
> For directory index entries, ovl_verify_index() is decoding the upper dir file
> handle and that can be expensive.
> The cleanup on mount is not strictly needed for correctness of "verify" feature,
> nor for NFS export, so we may want to consider a mount option for not
> running ovl_indexdir_cleanup() or for running it in a non expensive mode
> that skips the file handle verification?

Right.

Thanks,
Miklos

Re: [PATCH v2 07/23] ovl: verify stored origin fh matches lower dir

[Miklos Szeredi]

On Tue, Jan 9, 2018 at 11:04 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Jan 9, 2018 at 11:52 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:

> Yeh, I like that option much better. Especially, I like that it saves me the
> trouble of documenting when it makes sense to turn on "verify" for
> non-NFS case, because I don't know when it makes sense.
>
> I will try to adjust documentation to explain that with NFS export enabled
> lookup of merge dir becomes more "file handle oriented" and is therefore
> less sensitive to some offlline changes (rename) and more sensitive to
> other offline changes (remove+create).

Okay.  So lets start with a broad "nfs_export=on/off" option, and if
later it becomes necessary add more fine grained options controlling
parts of what encompassed by this option.

Thanks,
Miklos

Re: [PATCH v2 06/23] ovl: add support for "verify" feature

[Amir Goldstein]

On Tue, Jan 9, 2018 at 12:07 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Jan 9, 2018 at 10:54 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Tue, Jan 9, 2018 at 11:16 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> Introduce the "verify" config, module and mount options.
>>>>
>>>> If the "verify" feature is enabled then overlay filesystems will use
>>>> the inodes index dir to verify layers consistency.
>>>>
>>>> It is possible to enable consistency verification by default during
>>>> build time with the OVERLAY_FS_VERIFY config option, during runtime
>>>> with the "verify=on" module option or on a filesystem instance basis
>>>> with the "verify=on" mount option.
>>>>
>>>> The "verify" feature will prevent multiple redirects to the same lower
>>>> dir and will prevent broken hardlinks from using the same inode number.
>>>>
>>>> On an overlay filesystem instance with multiple lower layers, "verify"
>>>> feature can only use the index to prevent multiple redirects from upper
>>>> dirs and not from middle layer dirs. Emit a warning about this on mount
>>>> with multiple lower layers and "verify=on".
>>>
>>> Could the above go into Documentation/filesystems/overlayfs.txt as well?
>>
>> Will do.
>> BTW, are you ok with the feature name "verify" or do you prefer that
>> I rename it to "verify_origin" to be more descriptive as Vivek suggested?
>>
>>>
>>> Also, the verify feature does have some overhead, which should also be
>>> noted in the documentation.  Do you have some measurements about how
>>> much impact it has on performance?
>>>
>>
>> I do not. It's on my TODO list.
>>
>> I'm curious though, where do you think it should performance?
>> I would expect that encoding a file handle and index lookup would not be
>> that expensive, so I don't expect much lookup overhead. No?
>
> I wasn't thinking about anything specific, but now you ask:
> directories always need to be connected, and that results in multiple
> readdirs, etc, that is probably noticeable under some conditions.
>

Right. For the record, there is no decoding of directory file handles
with verify=on expect for ovl_indexdir_cleanup() that could become
optional and expect for decoding an overlay dir file handle for nfsd
of course.

The feature that does decoding of file handles on lookup for snapshots
was separated (per your request) to a different opt-in mount option:
https://github.com/amir73il/linux/commits/ovl-redirect-origin
and is not included in this posting.

Thanks,
Amir.

Re: [PATCH v2 06/23] ovl: add support for "verify" feature

[Miklos Szeredi]

On Tue, Jan 9, 2018 at 11:44 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Jan 9, 2018 at 12:07 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Tue, Jan 9, 2018 at 10:54 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On Tue, Jan 9, 2018 at 11:16 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>>> Introduce the "verify" config, module and mount options.
>>>>>
>>>>> If the "verify" feature is enabled then overlay filesystems will use
>>>>> the inodes index dir to verify layers consistency.
>>>>>
>>>>> It is possible to enable consistency verification by default during
>>>>> build time with the OVERLAY_FS_VERIFY config option, during runtime
>>>>> with the "verify=on" module option or on a filesystem instance basis
>>>>> with the "verify=on" mount option.
>>>>>
>>>>> The "verify" feature will prevent multiple redirects to the same lower
>>>>> dir and will prevent broken hardlinks from using the same inode number.
>>>>>
>>>>> On an overlay filesystem instance with multiple lower layers, "verify"
>>>>> feature can only use the index to prevent multiple redirects from upper
>>>>> dirs and not from middle layer dirs. Emit a warning about this on mount
>>>>> with multiple lower layers and "verify=on".
>>>>
>>>> Could the above go into Documentation/filesystems/overlayfs.txt as well?
>>>
>>> Will do.
>>> BTW, are you ok with the feature name "verify" or do you prefer that
>>> I rename it to "verify_origin" to be more descriptive as Vivek suggested?
>>>
>>>>
>>>> Also, the verify feature does have some overhead, which should also be
>>>> noted in the documentation.  Do you have some measurements about how
>>>> much impact it has on performance?
>>>>
>>>
>>> I do not. It's on my TODO list.
>>>
>>> I'm curious though, where do you think it should performance?
>>> I would expect that encoding a file handle and index lookup would not be
>>> that expensive, so I don't expect much lookup overhead. No?
>>
>> I wasn't thinking about anything specific, but now you ask:
>> directories always need to be connected, and that results in multiple
>> readdirs, etc, that is probably noticeable under some conditions.
>>
>
> Right. For the record, there is no decoding of directory file handles
> with verify=on expect for ovl_indexdir_cleanup() that could become
> optional and expect for decoding an overlay dir file handle for nfsd
> of course.

Ah, okay.

>
> The feature that does decoding of file handles on lookup for snapshots
> was separated (per your request) to a different opt-in mount option:
> https://github.com/amir73il/linux/commits/ovl-redirect-origin
> and is not included in this posting.

Yes, I remember now.

Thanks,
Miklos

Re: [PATCH v2 08/23] ovl: unbless lower st_ino of files under unverified redirected dir

[Miklos Szeredi]

On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On a malformed overlay, several redirected dirs can point to the same
> dir on a lower layer. This presents a similar challenge as broken
> hardlinks, because different objects in the overlay can return the same
> st_ino/st_dev pair from stat(2).
>
> For broken hardlinks, we do not provide constant st_ino on copy up to
> avoid this inconsistency. When "verify" feature is enabled, apply
> the same logic to files nested under unverified redirected dirs.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/inode.c     | 18 ++++++++++++++----
>  fs/overlayfs/namei.c     |  4 ++++
>  fs/overlayfs/overlayfs.h |  4 ++++
>  fs/overlayfs/util.c      | 27 +++++++++++++++++++++++++++
>  4 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 00b6b294272a..1b53755bd86c 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -105,12 +105,22 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>                          * Lower hardlinks may be broken on copy up to different
>                          * upper files, so we cannot use the lower origin st_ino
>                          * for those different files, even for the same fs case.
> +                        *
> +                        * Similarly, several redirected dirs can point to the
> +                        * same dir on a lower layer. With the "verify" feature,
> +                        * we do not use the lower origin st_ino, if any parent
> +                        * is redirected and we haven't verified that this
> +                        * redirect is unique.
> +                        *
>                          * With inodes index enabled, it is safe to use st_ino
> -                        * of an indexed hardlinked origin. The index validates
> -                        * that the upper hardlink is not broken.
> +                        * of an indexed origin. The index validates that the
> +                        * upper hardlink is not broken and that a redirected
> +                        * dir is the only redirect to that origin.
>                          */
> -                       if (is_dir || lowerstat.nlink == 1 ||
> -                           ovl_test_flag(OVL_INDEX, d_inode(dentry)))
> +                       if (ovl_test_flag(OVL_INDEX, d_inode(dentry)) ||
> +                           ((is_dir || lowerstat.nlink == 1) &&
> +                            (!ovl_verify(dentry->d_sb) ||
> +                             !ovl_dentry_is_renamed(dentry))))

Won't this make st_ino change when an ancestor directory is renamed?
Even if there is no "double redirect" that we are fearing?  If so,
than I think the cure is worse than the disease.

Thanks,
Miklos

Re: [PATCH v2 08/23] ovl: unbless lower st_ino of files under unverified redirected dir

[Amir Goldstein]

On Tue, Jan 9, 2018 at 3:31 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On a malformed overlay, several redirected dirs can point to the same
>> dir on a lower layer. This presents a similar challenge as broken
>> hardlinks, because different objects in the overlay can return the same
>> st_ino/st_dev pair from stat(2).
>>
>> For broken hardlinks, we do not provide constant st_ino on copy up to
>> avoid this inconsistency. When "verify" feature is enabled, apply
>> the same logic to files nested under unverified redirected dirs.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/inode.c     | 18 ++++++++++++++----
>>  fs/overlayfs/namei.c     |  4 ++++
>>  fs/overlayfs/overlayfs.h |  4 ++++
>>  fs/overlayfs/util.c      | 27 +++++++++++++++++++++++++++
>>  4 files changed, 49 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
>> index 00b6b294272a..1b53755bd86c 100644
>> --- a/fs/overlayfs/inode.c
>> +++ b/fs/overlayfs/inode.c
>> @@ -105,12 +105,22 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>>                          * Lower hardlinks may be broken on copy up to different
>>                          * upper files, so we cannot use the lower origin st_ino
>>                          * for those different files, even for the same fs case.
>> +                        *
>> +                        * Similarly, several redirected dirs can point to the
>> +                        * same dir on a lower layer. With the "verify" feature,
>> +                        * we do not use the lower origin st_ino, if any parent
>> +                        * is redirected and we haven't verified that this
>> +                        * redirect is unique.
>> +                        *
>>                          * With inodes index enabled, it is safe to use st_ino
>> -                        * of an indexed hardlinked origin. The index validates
>> -                        * that the upper hardlink is not broken.
>> +                        * of an indexed origin. The index validates that the
>> +                        * upper hardlink is not broken and that a redirected
>> +                        * dir is the only redirect to that origin.
>>                          */
>> -                       if (is_dir || lowerstat.nlink == 1 ||
>> -                           ovl_test_flag(OVL_INDEX, d_inode(dentry)))
>> +                       if (ovl_test_flag(OVL_INDEX, d_inode(dentry)) ||
>> +                           ((is_dir || lowerstat.nlink == 1) &&
>> +                            (!ovl_verify(dentry->d_sb) ||
>> +                             !ovl_dentry_is_renamed(dentry))))
>
> Won't this make st_ino change when an ancestor directory is renamed?
> Even if there is no "double redirect" that we are fearing?  If so,
> than I think the cure is worse than the disease.
>

Right. I should drop ovl_dentry_is_renamed(). It was ill conceived.

For verify=off, we trust lower st_ino for non-indexed dir and non-hardlink.
For verify=on, we only trust lower st_ino for indexed dir and non-dir.
Using the upper st_ino for non-indexed upper with verify=on is
consistent with the fact that we also encode an upper file handle
for non-indexed upper.

That means that st_ino remains constant on copy up for both
verify=on/off, but when same overlay is mounted verify=off and then
verify=on, st_ino of an already copied up (and non-indexed) file st_ino
will change.

I don't think that behavior is a problem, considering the fact that merge
dir without verified origin will also not behave the same on verify=on/off
cases.

Thanks,
Amir.

Re: [PATCH v2 08/23] ovl: unbless lower st_ino of files under unverified redirected dir

[Miklos Szeredi]

On Tue, Jan 9, 2018 at 3:24 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Jan 9, 2018 at 3:31 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On a malformed overlay, several redirected dirs can point to the same
>>> dir on a lower layer. This presents a similar challenge as broken
>>> hardlinks, because different objects in the overlay can return the same
>>> st_ino/st_dev pair from stat(2).
>>>
>>> For broken hardlinks, we do not provide constant st_ino on copy up to
>>> avoid this inconsistency. When "verify" feature is enabled, apply
>>> the same logic to files nested under unverified redirected dirs.
>>>
>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>> ---
>>>  fs/overlayfs/inode.c     | 18 ++++++++++++++----
>>>  fs/overlayfs/namei.c     |  4 ++++
>>>  fs/overlayfs/overlayfs.h |  4 ++++
>>>  fs/overlayfs/util.c      | 27 +++++++++++++++++++++++++++
>>>  4 files changed, 49 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
>>> index 00b6b294272a..1b53755bd86c 100644
>>> --- a/fs/overlayfs/inode.c
>>> +++ b/fs/overlayfs/inode.c
>>> @@ -105,12 +105,22 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>>>                          * Lower hardlinks may be broken on copy up to different
>>>                          * upper files, so we cannot use the lower origin st_ino
>>>                          * for those different files, even for the same fs case.
>>> +                        *
>>> +                        * Similarly, several redirected dirs can point to the
>>> +                        * same dir on a lower layer. With the "verify" feature,
>>> +                        * we do not use the lower origin st_ino, if any parent
>>> +                        * is redirected and we haven't verified that this
>>> +                        * redirect is unique.
>>> +                        *
>>>                          * With inodes index enabled, it is safe to use st_ino
>>> -                        * of an indexed hardlinked origin. The index validates
>>> -                        * that the upper hardlink is not broken.
>>> +                        * of an indexed origin. The index validates that the
>>> +                        * upper hardlink is not broken and that a redirected
>>> +                        * dir is the only redirect to that origin.
>>>                          */
>>> -                       if (is_dir || lowerstat.nlink == 1 ||
>>> -                           ovl_test_flag(OVL_INDEX, d_inode(dentry)))
>>> +                       if (ovl_test_flag(OVL_INDEX, d_inode(dentry)) ||
>>> +                           ((is_dir || lowerstat.nlink == 1) &&
>>> +                            (!ovl_verify(dentry->d_sb) ||
>>> +                             !ovl_dentry_is_renamed(dentry))))
>>
>> Won't this make st_ino change when an ancestor directory is renamed?
>> Even if there is no "double redirect" that we are fearing?  If so,
>> than I think the cure is worse than the disease.
>>
>
> Right. I should drop ovl_dentry_is_renamed(). It was ill conceived.
>
> For verify=off, we trust lower st_ino for non-indexed dir and non-hardlink.
> For verify=on, we only trust lower st_ino for indexed dir and non-dir.
> Using the upper st_ino for non-indexed upper with verify=on is
> consistent with the fact that we also encode an upper file handle
> for non-indexed upper.
>
> That means that st_ino remains constant on copy up for both
> verify=on/off, but when same overlay is mounted verify=off and then
> verify=on, st_ino of an already copied up (and non-indexed) file st_ino
> will change.
>
> I don't think that behavior is a problem, considering the fact that merge
> dir without verified origin will also not behave the same on verify=on/off
> cases.

Yes, fine.

Thanks,
Miklos

Re: [PATCH v2 09/23] ovl: lookup index for directories

[Miklos Szeredi]

On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> A directory index is a directory type entry in index dir with a
> "trusted.overlay.origin" xattr containing an encoded ovl_fh of the merge
> directory upper dir inode.

While it's convenient to call this ".origin", it's not really that,
since now we are pointing to the latest incarnation of the file, which
is the opposite of origin.

Could we call it something more appropriate?

Thanks,
Miklos

Re: [PATCH v2 12/23] ovl: cleanup temp index entries

[Miklos Szeredi]

On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> A previous failed attempt to create or whiteout a directory index may leave
> index entries named '#%x' in the index dir.
> Cleanup those temp entries on mount instead of failing the mount.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/namei.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index e53c54ecb7cc..4987d7df1581 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -9,6 +9,7 @@
>
>  #include <linux/fs.h>
>  #include <linux/cred.h>
> +#include <linux/ctype.h>
>  #include <linux/namei.h>
>  #include <linux/xattr.h>
>  #include <linux/ratelimit.h>
> @@ -452,6 +453,24 @@ static struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
>         return upper.dentry;
>  }
>
> +/* Is this a leftover from create/whiteout of directory index entry? */
> +static bool ovl_is_temp_index(struct dentry *index)
> +{
> +       const char *p = index->d_name.name;
> +       int len = index->d_name.len;
> +
> +       if (!d_is_dir(index) && !ovl_is_whiteout(index))
> +               return false;
> +
> +       if (*p++ != '#' || len < 2)
> +               return false;

I don't see a point in extensive verification that it's a well formed
temp file name.   Just checking that the first character is "#" should
be enough, no?

Thanks,
Miklos

Re: [PATCH v2 09/23] ovl: lookup index for directories

[Amir Goldstein]

On Tue, Jan 9, 2018 at 4:49 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> A directory index is a directory type entry in index dir with a
>> "trusted.overlay.origin" xattr containing an encoded ovl_fh of the merge
>> directory upper dir inode.
>
> While it's convenient to call this ".origin", it's not really that,
> since now we are pointing to the latest incarnation of the file, which
> is the opposite of origin.
>
> Could we call it something more appropriate?
>

I can't come up with anything more creative than ".upper"...
I guess I can try to rename ovl_{check|verify}_origin_*
helpers to a more common name or and have
ovl_{check|verify}_{origin|upper}_* wrappers.

Thanks,
Amir.

Re: [PATCH v2 15/23] ovl: index directories on copy up with 'verify=on'

[Miklos Szeredi]

On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> With the 'verify' feature enabled, all dirs are indexed on copy up.
> Non-dir files are copied up directly to indexdir and then hardlinked
> to upper dir.
>
> Directories are copied up to indexdir, then an index entry is created

Makes me wonder:  do we need a workdir at all?  Temp files are already
differentiated by leading "#", so that takes care of cleanup.
Advantage: one less directory to worry about in implementation.

Thanks,
Miklos

Re: [PATCH v2 12/23] ovl: cleanup temp index entries

[Amir Goldstein]

On Tue, Jan 9, 2018 at 5:25 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> A previous failed attempt to create or whiteout a directory index may leave
>> index entries named '#%x' in the index dir.
>> Cleanup those temp entries on mount instead of failing the mount.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/namei.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> index e53c54ecb7cc..4987d7df1581 100644
>> --- a/fs/overlayfs/namei.c
>> +++ b/fs/overlayfs/namei.c
>> @@ -9,6 +9,7 @@
>>
>>  #include <linux/fs.h>
>>  #include <linux/cred.h>
>> +#include <linux/ctype.h>
>>  #include <linux/namei.h>
>>  #include <linux/xattr.h>
>>  #include <linux/ratelimit.h>
>> @@ -452,6 +453,24 @@ static struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
>>         return upper.dentry;
>>  }
>>
>> +/* Is this a leftover from create/whiteout of directory index entry? */
>> +static bool ovl_is_temp_index(struct dentry *index)
>> +{
>> +       const char *p = index->d_name.name;
>> +       int len = index->d_name.len;
>> +
>> +       if (!d_is_dir(index) && !ovl_is_whiteout(index))
>> +               return false;
>> +
>> +       if (*p++ != '#' || len < 2)
>> +               return false;
>
> I don't see a point in extensive verification that it's a well formed
> temp file name.   Just checking that the first character is "#" should
> be enough, no?
>

Sure.

Thanks,
Amir.

Re: [PATCH v2 17/23] ovl: whiteout index when union nlink drops to zero

[Miklos Szeredi]

On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> With the 'verify' feature enabled, when overlay inode nlink drops to
> zero, instead of removing the index entry, replace it with a whiteout
> index entry.
>
> This is needed for NFS export in order to prevent future open by handle
> from opening the lower file directly.

Just thinking out loud: how could we get rid of index whiteouts in the
"rm -rf" case?  Only way is if encoding parent as well, and we
recursively check for a whiteout in the ancestors.   Probably not
worth bothering with in the first version, but later maybe?

Thanks,
Miklos

Re: [PATCH v2 15/23] ovl: index directories on copy up with 'verify=on'

[Amir Goldstein]

On Tue, Jan 9, 2018 at 6:08 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> With the 'verify' feature enabled, all dirs are indexed on copy up.
>> Non-dir files are copied up directly to indexdir and then hardlinked
>> to upper dir.
>>
>> Directories are copied up to indexdir, then an index entry is created
>
> Makes me wonder:  do we need a workdir at all?  Temp files are already
> differentiated by leading "#", so that takes care of cleanup.
> Advantage: one less directory to worry about in implementation.
>

One advantage of keeping legacy work dir is that existence of index dir
is the one and only "feature bit" in overlayfs on-disk format and I wouldn't
like that bit to get lost. We can also look for non # entries in index as
indication to past index=on mount, but a simple exists indication is nicer.

I guess we could always use indexdir for copy up with index=on,
but I don't know that is going to simplify code, given that this (dir index)
patch didn't even need to change struct ovl_copy_up_ctx.

Thanks,
Amir.

Re: [PATCH v2 15/23] ovl: index directories on copy up with 'verify=on'

[Miklos Szeredi]

On Tue, Jan 9, 2018 at 5:31 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Jan 9, 2018 at 6:08 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> With the 'verify' feature enabled, all dirs are indexed on copy up.
>>> Non-dir files are copied up directly to indexdir and then hardlinked
>>> to upper dir.
>>>
>>> Directories are copied up to indexdir, then an index entry is created
>>
>> Makes me wonder:  do we need a workdir at all?  Temp files are already
>> differentiated by leading "#", so that takes care of cleanup.
>> Advantage: one less directory to worry about in implementation.
>>
>
> One advantage of keeping legacy work dir is that existence of index dir
> is the one and only "feature bit" in overlayfs on-disk format and I wouldn't
> like that bit to get lost. We can also look for non # entries in index as
> indication to past index=on mount, but a simple exists indication is nicer.
>
> I guess we could always use indexdir for copy up with index=on,
> but I don't know that is going to simplify code, given that this (dir index)
> patch didn't even need to change struct ovl_copy_up_ctx.

We could always use index dir for everything.  Since workdir lifetime
is for the duration of the mount, no back compatibility issues with
just not using it at all.

Thanks,
Miklos

Re: [PATCH v2 17/23] ovl: whiteout index when union nlink drops to zero

[Amir Goldstein]

On Tue, Jan 9, 2018 at 6:28 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> With the 'verify' feature enabled, when overlay inode nlink drops to
>> zero, instead of removing the index entry, replace it with a whiteout
>> index entry.
>>
>> This is needed for NFS export in order to prevent future open by handle
>> from opening the lower file directly.
>
> Just thinking out loud: how could we get rid of index whiteouts in the
> "rm -rf" case?  Only way is if encoding parent as well, and we
> recursively check for a whiteout in the ancestors.   Probably not
> worth bothering with in the first version, but later maybe?
>

Right.

Current implementation already checks for whiteout in ancestors
for dir file handles, so in theory, cleanup of dir whiteouts is not far
from reach, except for the fact that decoding dir file handles is expensive,
so every cleanup attempt is going to cost.

For non-dir this is a more complicated.
If you encode parent (i.e. connectable file handle) then for lower hardlinks
you find a specific parent, but not all hardlink parents.

Anyway, maybe later.

Thanks,
Amir.

Re: [PATCH v2 14/23] ovl: index all files on copy up with 'verify=on'

[Vivek Goyal]

On Thu, Jan 04, 2018 at 06:40:09PM +0200, Amir Goldstein wrote:
> With the 'verify' feature enabled, all files are indexed on copy up.
> The copy up origin inode of an indexed non-dir can be used as a unique
> identifier of the overlay object.
> 
> This is going to be used for NFS export.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/util.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 0254d7f0d401..f40351dcea7b 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -491,6 +491,10 @@ bool ovl_need_index(struct dentry *dentry)
>  	if (!lower || !ovl_indexdir(dentry->d_sb))
>  		return false;
>  
> +	/* Index all files for unique origin verification */
> +	if (!d_is_dir(lower) && ovl_verify(dentry->d_sb))
> +		return true;

If all files are indexd up with verify=on, what is this check about
"!d_is_dir()"?

Vivek

> +
>  	/* Index only lower hardlinks on copy up */
>  	if (!d_is_dir(lower) && d_inode(lower)->i_nlink > 1)
>  		return true;
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 15/23] ovl: index directories on copy up with 'verify=on'

[Amir Goldstein]

On Tue, Jan 9, 2018 at 6:35 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Jan 9, 2018 at 5:31 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Tue, Jan 9, 2018 at 6:08 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> With the 'verify' feature enabled, all dirs are indexed on copy up.
>>>> Non-dir files are copied up directly to indexdir and then hardlinked
>>>> to upper dir.
>>>>
>>>> Directories are copied up to indexdir, then an index entry is created
>>>
>>> Makes me wonder:  do we need a workdir at all?  Temp files are already
>>> differentiated by leading "#", so that takes care of cleanup.
>>> Advantage: one less directory to worry about in implementation.
>>>
>>
>> One advantage of keeping legacy work dir is that existence of index dir
>> is the one and only "feature bit" in overlayfs on-disk format and I wouldn't
>> like that bit to get lost. We can also look for non # entries in index as
>> indication to past index=on mount, but a simple exists indication is nicer.
>>
>> I guess we could always use indexdir for copy up with index=on,
>> but I don't know that is going to simplify code, given that this (dir index)
>> patch didn't even need to change struct ovl_copy_up_ctx.
>
> We could always use index dir for everything.  Since workdir lifetime
> is for the duration of the mount, no back compatibility issues with
> just not using it at all.
>

I got that.
I referred to loosing the information of whether or not overlay was ever
mounted with index=on (a.k.a. index feature bit is set).

Today, workdir/index exists => overlay was mounted with index=on.
If we always use workdir/index instead of workdir/work, we will loose
this simple and quick test. Not the end of the world, but nice to have.

OTOH, my POC for implementing overlay features depends on
existence of indexdir and on mount calling ovl_indexdir_cleanup():
https://github.com/amir73il/linux/commits/ovl-features
So if something like that is adopted, my "feature bit" concern is
addressed.

Thanks,
Amir.

Re: [PATCH v2 14/23] ovl: index all files on copy up with 'verify=on'

[Amir Goldstein]

On Tue, Jan 9, 2018 at 6:45 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Jan 04, 2018 at 06:40:09PM +0200, Amir Goldstein wrote:
>> With the 'verify' feature enabled, all files are indexed on copy up.
>> The copy up origin inode of an indexed non-dir can be used as a unique
>> identifier of the overlay object.
>>
>> This is going to be used for NFS export.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/util.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
>> index 0254d7f0d401..f40351dcea7b 100644
>> --- a/fs/overlayfs/util.c
>> +++ b/fs/overlayfs/util.c
>> @@ -491,6 +491,10 @@ bool ovl_need_index(struct dentry *dentry)
>>       if (!lower || !ovl_indexdir(dentry->d_sb))
>>               return false;
>>
>> +     /* Index all files for unique origin verification */
>> +     if (!d_is_dir(lower) && ovl_verify(dentry->d_sb))
>> +             return true;
>
> If all files are indexd up with verify=on, what is this check about
> "!d_is_dir()"?
>


Removed by next patch that implements indexing of dir on copy up.

Thanks,
Amir.

Re: [PATCH v2 23/23] ovl: copy up of disconnected dentries

[Miklos Szeredi]

On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> With NFS export, some operations on decoded file handles (e.g. open,
> link, setattr, xattr_set) may call copy up with a disconnected non-dir.
> In this case, we will copy up lower inode to index dir without
> linking it to upper dir.

Did we not have plans to link-up from index on lookup, when upper link
is missing?  For consistency sake, that would make a lot of sense
after such a disconnected copy-up, and also for the hardlink copy-up.

Yes, things work without the upper link, but restoring the upper link
makes sense, and lookup is perhaps the most logical place to do that
restoration.

Thanks,
Miklos

Re: [PATCH v2 23/23] ovl: copy up of disconnected dentries

[Amir Goldstein]

On Thu, Jan 11, 2018 at 4:47 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Jan 4, 2018 at 5:40 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> With NFS export, some operations on decoded file handles (e.g. open,
>> link, setattr, xattr_set) may call copy up with a disconnected non-dir.
>> In this case, we will copy up lower inode to index dir without
>> linking it to upper dir.
>
> Did we not have plans to link-up from index on lookup, when upper link
> is missing?  For consistency sake, that would make a lot of sense
> after such a disconnected copy-up, and also for the hardlink copy-up.
>
> Yes, things work without the upper link, but restoring the upper link
> makes sense, and lookup is perhaps the most logical place to do that
> restoration.
>

Indeed, disconnected copy-up is a lot like hardlink copy-up+unlink.
I guess the link-up on lookup is independent of the series, but should
be doable.

Thanks,
Amir.

Re: [PATCH v2 00/23] Overlayfs consistency verification with full index

[Amir Goldstein]

On Thu, Jan 4, 2018 at 6:39 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Miklos,
>
> This is the 2nd revision of the prep patch series for NFS export.
> I am posing the prep patches only to overlayfs list and will post
> NFS export implementation to fsdevel.
>

Miklos,

3rd revision of prep patches after addressing V2 comments is
available at [1].

Changes since v2:
- Rename mount/config option from "verify" to "nfs_export"
- Rename dir index entries xattr from ".origin" to ".upper"
- Re-factor ovl_{get|set|verify}_origin() helpers
- Abandon ovl_dentry_is_renamed() test for lower st_ino
- Document overhead on mount with full index
- Document change of behavior when verifying lower origin

After you are done with review of NFS export patches
and I will post V3 of prep patches to list.

> In the 1st revision the mount option 'index=all' was used to request
> a full index. This revision re-brands the feature as 'verify=on',
> including a config/module option and documentation that explains the
> benefits of full index (i.e. avoid multiple redirect).
>

In V3, as agreed, the feature name is "nfs_export". There are 2 helpers
called ovl_index_all() and ovl_index_lower() both return the value of
ofs->coinfig.nfs_export, each used in relevant code.

> The prep series goes on a bit beyond implementation of 'verify=on'.
> Patches 17-23 create the whiteout index entries and make some more
> changes that are prerequisites for NFS export implementation.
>

Patches 17-23 as well as NFS export patches have been rebased and
tested. They are mostly unmodified since V2. The complete work is
availabe at [2].

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/ovl-index-all-v3
[2] https://github.com/amir73il/linux/commits/ovl-nfs-export