[PATCH v12 00/17] overlayfs: Delayed copy up of data

[PATCH v12 00/17] overlayfs: Delayed copy up of data

[Vivek Goyal]

Hi,

Here are V12 of the metadata only copy up patches. 

Patches are also available here.

https://github.com/rhvgoyal/linux/tree/metacopy-v12

Changes from V11:

- Rebased on latest kernel
- Follow metacopy source using REDIRECT instead of ORIGIN
- Support metacopy xattr in mid layer.

Please have a look.

Amir, few patches have changed and you might have to look at it
again.

Also, I have disabled metacopy support when nfs_export=on. I am not
sure if I can make it work for nfs_export easily or not. Will need
to spend more time understanding nfs_export stuff.

Thanks
Vivek

Vivek Goyal (17):
  ovl: redirect_dir=nofollow can follow redirect for opaque lower
  ovl: Provide a mount option metacopy=on/off for metadata copyup
  ovl: During copy up, first copy up metadata and then data
  ovl: Move the copy up helpers to copy_up.c
  ovl: Copy up only metadata during copy up where it makes sense
  ovl: Add helper ovl_already_copied_up()
  ovl: A new xattr OVL_XATTR_METACOPY for file on upper
  ovl: Modify ovl_lookup() and friends to lookup metacopy dentry
  ovl: Do not mark a non dir as _OVL_PATH_MERGE in ovl_path_type()
  ovl: Copy up meta inode data from lowest data inode
  ovl: Fix ovl_getattr() to get number of blocks from lower
  ovl: Do not expose metacopy only upper dentry from d_real()
  ovl: Check redirects for metacopy files
  ovl: Set redirect on metacopy files upon rename
  ovl: Remove redirect when data of a metacopy file is copied up
  ovl: Set redirect on upper inode when it is linked
  ovl: Enable metadata only feature

 Documentation/filesystems/overlayfs.txt |  30 +++++-
 fs/overlayfs/Kconfig                    |  17 ++++
 fs/overlayfs/copy_up.c                  | 165 +++++++++++++++++++++++++-------
 fs/overlayfs/dir.c                      |  32 +++++--
 fs/overlayfs/export.c                   |   3 +
 fs/overlayfs/inode.c                    |  49 ++++------
 fs/overlayfs/namei.c                    | 142 +++++++++++++++++++++++----
 fs/overlayfs/overlayfs.h                |  22 ++++-
 fs/overlayfs/ovl_entry.h                |   1 +
 fs/overlayfs/super.c                    |  62 +++++++++++-
 fs/overlayfs/util.c                     | 138 +++++++++++++++++++++++++-
 11 files changed, 554 insertions(+), 107 deletions(-)

-- 
2.13.6

[PATCH v12 01/17] ovl: redirect_dir=nofollow can follow redirect for opaque lower

[Vivek Goyal]

redirect_dir=nofollow should not follow a redirect. But in a specific
configuration it can still follow it.  For example try this.

$ mkdir -p lower0 lower1/foo upper work merged
$ touch lower1/foo/lower-file.txt
$ setfattr -n "trusted.overlay.opaque" -v "y" lower1/foo
$ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=on none merged
$ cd merged
$ mv foo foo-renamed
$ umount merged

# mount again. This time with redirect_dir=nofollow
$ mount -t overlay -o lowerdir=lower1:lower0,workdir=work,upperdir=upper,redirect_dir=nofollow none merged
$ ls merged/foo-renamed/
# This lists lower-file.txt, while it should not have.

Basically, we are doing redirect check after we check for d.stop. And
if this is not last lower, and we find an opaque lower, d.stop will be
set.

ovl_lookup_single()
        if (!d->last && ovl_is_opaquedir(this)) {
                d->stop = d->opaque = true;
                goto out;
        }

To fix this, first check redirect is allowed. And after that check if
d.stop has been set or not.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index de3e6da1d5a5..70fcfcc684cc 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -913,9 +913,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		stack[ctr].layer = lower.layer;
 		ctr++;
 
-		if (d.stop)
-			break;
-
 		/*
 		 * Following redirects can have security consequences: it's like
 		 * a symlink into the lower layer without the permission checks.
@@ -933,6 +930,9 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			goto out_put;
 		}
 
+		if (d.stop)
+			break;
+
 		if (d.redirect && d.redirect[0] == '/' && poe != roe) {
 			poe = roe;
 			/* Find the current layer on the root dentry */
-- 
2.13.6

[PATCH v12 02/17] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Vivek Goyal]

By default metadata only copy up is disabled. Provide a mount option so
that users can choose one way or other.

Also provide a kernel config and module option to enable/disable
metacopy feature.

metacopy feature requires redirect_dir=on when upper is present. Otherwise,
it requires redirect_dir=follow atleast.

Like index feature, we verify on mount that upper root is not being
reused with a different lower root. This hopes to get the configuration
right and detect the copied layers use case. But this does only so
much as we don't verify all the lowers. So it is possible that a lower is
missing and later data copy up fails.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 Documentation/filesystems/overlayfs.txt | 30 ++++++++++++++++++++++++-
 fs/overlayfs/Kconfig                    | 17 ++++++++++++++
 fs/overlayfs/ovl_entry.h                |  1 +
 fs/overlayfs/super.c                    | 40 ++++++++++++++++++++++++++++++++-
 4 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
index 6ea1e64d1464..b7720e61973c 100644
--- a/Documentation/filesystems/overlayfs.txt
+++ b/Documentation/filesystems/overlayfs.txt
@@ -249,6 +249,30 @@ rightmost one and going left.  In the above example lower1 will be the
 top, lower2 the middle and lower3 the bottom layer.
 
 
+Metadata only copyup
+--------------------
+
+When metadata only copy up feature is enabled, overlayfs will only copy
+up metadata (as opposed to whole file), when a metadata specific operation
+like chown/chmod is performed. Full file will be copied up later when
+file is opened for WRITE operation.
+
+IOW, this is delayed data copy up operation and data is copied up when
+there is a need to actually modify data.
+
+There are multiple ways to enable/disable this feature. A config option
+CONFIG_OVERLAY_FS_METACOPY can be set/unset to enable/disable this feature
+by default. Or one can enable/disable it at module load time with module
+parameter metacopy=on/off. Lastly, there is also a per mount option
+metacopy=on/off to enable/disable this feature per mount.
+
+Do not use metacopy=on with untrusted upper/lower directories. Otherwise
+it is possible that an attacker can create an handcrafted file with
+appropriate REDIRECT and METACOPY xattrs, and gain access to file on lower
+pointed by REDIRECT. This should not be possible on local system as setting
+"trusted." xattrs will require CAP_SYS_ADMIN. But it should be possible
+for untrusted layers like from a pen drive.
+
 Sharing and copying layers
 --------------------------
 
@@ -267,7 +291,7 @@ though it will not result in a crash or deadlock.
 Mounting an overlay using an upper layer path, where the upper layer path
 was previously used by another mounted overlay in combination with a
 different lower layer path, is allowed, unless the "inodes index" feature
-is enabled.
+or "metadata only copyup" feature is enabled.
 
 With the "inodes index" feature, on the first time mount, an NFS file
 handle of the lower layer root directory, along with the UUID of the lower
@@ -280,6 +304,10 @@ lower root origin, mount will fail with ESTALE.  An overlayfs mount with
 does not support NFS export, lower filesystem does not have a valid UUID or
 if the upper filesystem does not support extended attributes.
 
+For "metadata only copyup" feature there is no verification mechanism at
+mount time. So if same upper is mouted with different set of lower, mount
+probably will succeed but expect the unexpected later on. So don't do it.
+
 It is quite a common practice to copy overlay layers to a different
 directory tree on the same or different underlying filesystem, and even
 to a different machine.  With the "inodes index" feature, trying to mount
diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index 406e72de88f6..e1974db486a9 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -72,3 +72,20 @@ config OVERLAY_FS_NFS_EXPORT
 	  Note, that the NFS export feature is not backward compatible.
 	  That is, mounting an overlay which has a full index on a kernel
 	  that doesn't support this feature will have unexpected results.
+
+config OVERLAY_FS_METACOPY
+	bool "Overlayfs: turn on metadata only copy up feature by default"
+	depends on OVERLAY_FS
+	depends on !OVERLAY_FS_NFS_EXPORT
+	select OVERLAY_FS_REDIRECT_DIR
+	help
+	  If this config option is enabled then overlay filesystems will
+	  copy up only metadata where appropriate and data copy up will
+	  happen when a file is opended for WRITE operation. It is still
+	  possible to turn off this feature globally with the "metacopy=off"
+	  module option or on a filesystem instance basis with the
+	  "metacopy=off" mount option.
+
+	  Note, that this feature is not backward compatible.  That is,
+	  mounting an overlay which has metacopy only inodes on a kernel
+	  that doesn't support this feature will have unexpected results.
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index bfef6edcc111..7dc55628080d 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -18,6 +18,7 @@ struct ovl_config {
 	const char *redirect_mode;
 	bool index;
 	bool nfs_export;
+	bool metacopy;
 };
 
 struct ovl_layer {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 9ee37c76091d..0e124b9d902d 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -58,6 +58,11 @@ static void ovl_entry_stack_free(struct ovl_entry *oe)
 		dput(oe->lowerstack[i].dentry);
 }
 
+static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
+module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
+MODULE_PARM_DESC(ovl_metacopy_def,
+		 "Default to on or off for the metadata only copy up feature");
+
 static void ovl_dentry_release(struct dentry *dentry)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
@@ -350,6 +355,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 	if (ofs->config.nfs_export != ovl_nfs_export_def)
 		seq_printf(m, ",nfs_export=%s", ofs->config.nfs_export ?
 						"on" : "off");
+	if (ofs->config.metacopy != ovl_metacopy_def)
+		seq_printf(m, ",metacopy=%s",
+			   ofs->config.metacopy ? "on" : "off");
 	return 0;
 }
 
@@ -384,6 +392,8 @@ enum {
 	OPT_INDEX_OFF,
 	OPT_NFS_EXPORT_ON,
 	OPT_NFS_EXPORT_OFF,
+	OPT_METACOPY_ON,
+	OPT_METACOPY_OFF,
 	OPT_ERR,
 };
 
@@ -397,6 +407,8 @@ static const match_table_t ovl_tokens = {
 	{OPT_INDEX_OFF,			"index=off"},
 	{OPT_NFS_EXPORT_ON,		"nfs_export=on"},
 	{OPT_NFS_EXPORT_OFF,		"nfs_export=off"},
+	{OPT_METACOPY_ON,		"metacopy=on"},
+	{OPT_METACOPY_OFF,		"metacopy=off"},
 	{OPT_ERR,			NULL}
 };
 
@@ -511,6 +523,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			config->nfs_export = false;
 			break;
 
+		case OPT_METACOPY_ON:
+			config->metacopy = true;
+			break;
+
+		case OPT_METACOPY_OFF:
+			config->metacopy = false;
+			break;
+
 		default:
 			pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
 			return -EINVAL;
@@ -993,7 +1013,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.metacopy = false;
+		pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off and metacopy=off.\n");
 		err = 0;
 	} else {
 		vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
@@ -1012,6 +1033,11 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 		ofs->config.nfs_export = false;
 	}
 
+	/* metacopy feature with upper requires redirect_dir=on */
+	if (ofs->config.metacopy && !ofs->config.redirect_dir) {
+		pr_warn("overlayfs: metadata only copyup requires \"redirect_dir=on\", falling back to metacopy=off.\n");
+		ofs->config.metacopy = false;
+	}
 out:
 	mnt_drop_write(mnt);
 	return err;
@@ -1188,6 +1214,12 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 		ofs->config.nfs_export = false;
 	}
 
+	if (!ofs->config.upperdir && ofs->config.metacopy &&
+	    !ofs->config.redirect_follow) {
+		ofs->config.metacopy = false;
+		pr_warn("overlayfs: metadata only copyup requires \"redirect_dir=follow\" on non-upper mount, falling back to metacopy=off.\n");
+	}
+
 	err = -ENOMEM;
 	stack = kcalloc(stacklen, sizeof(struct path), GFP_KERNEL);
 	if (!stack)
@@ -1263,6 +1295,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
 	ofs->config.index = ovl_index_def;
 	ofs->config.nfs_export = ovl_nfs_export_def;
+	ofs->config.metacopy = ovl_metacopy_def;
 	err = ovl_parse_opt((char *) data, &ofs->config);
 	if (err)
 		goto out_err;
@@ -1331,6 +1364,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		}
 	}
 
+	if (ofs->config.metacopy && ofs->config.nfs_export) {
+		pr_warn("overlayfs: Metadata copy up requires NFS export disabled, falling back to metacopy=off.\n");
+		ofs->config.metacopy = false;
+	}
+
 	if (ofs->config.nfs_export)
 		sb->s_export_op = &ovl_export_operations;
 
-- 
2.13.6

[PATCH v12 03/17] ovl: During copy up, first copy up metadata and then data

[Vivek Goyal]

Just a little re-ordering of code. This helps with next patch where after
copying up metadata, we skip data copying step, if needed.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d855f508fa20..2de4ab3254a4 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -536,28 +536,10 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 {
 	int err;
 
-	if (S_ISREG(c->stat.mode)) {
-		struct path upperpath;
-
-		ovl_path_upper(c->dentry, &upperpath);
-		BUG_ON(upperpath.dentry != NULL);
-		upperpath.dentry = temp;
-
-		err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
-		if (err)
-			return err;
-	}
-
 	err = ovl_copy_xattr(c->lowerpath.dentry, temp);
 	if (err)
 		return err;
 
-	inode_lock(temp->d_inode);
-	err = ovl_set_attr(temp, &c->stat);
-	inode_unlock(temp->d_inode);
-	if (err)
-		return err;
-
 	/*
 	 * Store identifier of lower inode in upper inode xattr to
 	 * allow lookup of the copy up origin inode.
@@ -571,7 +553,23 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 			return err;
 	}
 
-	return 0;
+	if (S_ISREG(c->stat.mode)) {
+		struct path upperpath;
+
+		ovl_path_upper(c->dentry, &upperpath);
+		BUG_ON(upperpath.dentry != NULL);
+		upperpath.dentry = temp;
+
+		err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
+		if (err)
+			return err;
+	}
+
+	inode_lock(temp->d_inode);
+	err = ovl_set_attr(temp, &c->stat);
+	inode_unlock(temp->d_inode);
+
+	return err;
 }
 
 static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
-- 
2.13.6

[PATCH v12 04/17] ovl: Move the copy up helpers to copy_up.c

[Vivek Goyal]

Right now two copy up helpers are in inode.c. Amir suggested it might
be better to move these to copy_up.c.

There will one more related function which will come in later patch.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c   | 32 ++++++++++++++++++++++++++++++++
 fs/overlayfs/inode.c     | 32 --------------------------------
 fs/overlayfs/overlayfs.h |  2 +-
 3 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 2de4ab3254a4..98403b1d40c2 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -826,6 +826,38 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 	return err;
 }
 
+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) ||
+	     (dentry->d_flags & DCACHE_DISCONNECTED)))
+		return false;
+
+	if (special_file(d_inode(dentry)->i_mode))
+		return false;
+
+	if (!(OPEN_FMODE(flags) & FMODE_WRITE) && !(flags & O_TRUNC))
+		return false;
+
+	return true;
+}
+
+int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags)
+{
+	int err = 0;
+
+	if (ovl_open_need_copy_up(dentry, file_flags)) {
+		err = ovl_want_write(dentry);
+		if (!err) {
+			err = ovl_copy_up_flags(dentry, file_flags);
+			ovl_drop_write(dentry);
+		}
+	}
+
+	return err;
+}
+
 int ovl_copy_up(struct dentry *dentry)
 {
 	return ovl_copy_up_flags(dentry, 0);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index fcd97b783fa1..165e1b52f619 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -349,38 +349,6 @@ struct posix_acl *ovl_get_acl(struct inode *inode, int type)
 	return acl;
 }
 
-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) ||
-	     (dentry->d_flags & DCACHE_DISCONNECTED)))
-		return false;
-
-	if (special_file(d_inode(dentry)->i_mode))
-		return false;
-
-	if (!(OPEN_FMODE(flags) & FMODE_WRITE) && !(flags & O_TRUNC))
-		return false;
-
-	return true;
-}
-
-int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags)
-{
-	int err = 0;
-
-	if (ovl_open_need_copy_up(dentry, file_flags)) {
-		err = ovl_want_write(dentry);
-		if (!err) {
-			err = ovl_copy_up_flags(dentry, file_flags);
-			ovl_drop_write(dentry);
-		}
-	}
-
-	return err;
-}
-
 int ovl_update_time(struct inode *inode, struct timespec *ts, int flags)
 {
 	struct dentry *alias;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 0df25a9c94bd..21f3bfea665c 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -320,7 +320,6 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
 		  void *value, size_t size);
 ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size);
 struct posix_acl *ovl_get_acl(struct inode *inode, int type);
-int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags);
 int ovl_update_time(struct inode *inode, struct timespec *ts, int flags);
 bool ovl_is_private_xattr(const char *name);
 
@@ -358,6 +357,7 @@ int ovl_cleanup(struct inode *dir, struct dentry *dentry);
 /* copy_up.c */
 int ovl_copy_up(struct dentry *dentry);
 int ovl_copy_up_flags(struct dentry *dentry, int flags);
+int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_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 *real, bool is_upper);
-- 
2.13.6

[PATCH v12 05/17] ovl: Copy up only metadata during copy up where it makes sense

[Vivek Goyal]

If it makes sense to copy up only metadata during copy up, do it. This
is done for regular files which are not opened for WRITE and have origin
being saved.

Right now ->metacopy is set to 0 always. Last patch in the series will
remove the hard coded statement and enable metacopy feature.

Currently ovl_set_origin() returns success even if specified xattr could
not be set due to -EOPNOTSUPP returned by upper. IOW, ovl_set_origin()
operation is optional and copy up operation continues.

With metadata copy up, ovl_set_origin() can't be optional. We need to know
if origin could be set or not. If it could not be set, then either disable
metacopy or abort copy up operation. I have take then later approach and
disable metacopy for this inode.

Normally we should not run into this path as metacopy will not be enabled
if upper does not support xattr. This check is done during mount. This
path can only hit if checks during mount pass but at some point of time
later upper still returns -EOPNOTSUPP. So its a saftey mechanism to handle
that unexpected -EOPNOTSUPP.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 98403b1d40c2..45e396a0fd6a 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -232,6 +232,26 @@ int ovl_set_attr(struct dentry *upperdentry, struct kstat *stat)
 	return err;
 }
 
+static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
+				  int flags)
+{
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
+
+	/* TODO: Will enable metacopy in last patch of series */
+	return false;
+
+	if (!ofs->config.metacopy)
+		return false;
+
+	if (!S_ISREG(mode))
+		return false;
+
+	if (flags && ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC)))
+		return false;
+
+	return true;
+}
+
 struct ovl_fh *ovl_encode_fh(struct dentry *real, bool is_upper)
 {
 	struct ovl_fh *fh;
@@ -416,6 +436,7 @@ struct ovl_copy_up_ctx {
 	bool tmpfile;
 	bool origin;
 	bool indexed;
+	bool metacopy;
 };
 
 static int ovl_link_up(struct ovl_copy_up_ctx *c)
@@ -553,7 +574,7 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 			return err;
 	}
 
-	if (S_ISREG(c->stat.mode)) {
+	if (S_ISREG(c->stat.mode) && !c->metacopy) {
 		struct path upperpath;
 
 		ovl_path_upper(c->dentry, &upperpath);
@@ -729,6 +750,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	if (err)
 		return err;
 
+	ctx.metacopy = ovl_need_meta_copy_up(dentry, ctx.stat.mode, flags);
+
 	if (parent) {
 		ovl_path_upper(parent, &parentpath);
 		ctx.destdir = parentpath.dentry;
-- 
2.13.6

[PATCH v12 06/17] ovl: Add helper ovl_already_copied_up()

[Vivek Goyal]

There are couple of places where we need to know if file is already copied
up (in lockless manner). Right now its open coded and there are only
two conditions to check. Soon this patch series will introduce another
condition to check and Amir wants to introduce one more. So introduce
a helper instead to check this so that code is easier to read.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   | 20 ++------------------
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 26 +++++++++++++++++++++++++-
 3 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 45e396a0fd6a..8d9af7fdc8a4 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -810,21 +810,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		struct dentry *next;
 		struct dentry *parent = NULL;
 
-		/*
-		 * Check if copy-up has happened as well as for upper alias (in
-		 * case of hard links) is there.
-		 *
-		 * Both checks are lockless:
-		 *  - false negatives: will recheck under oi->lock
-		 *  - false positives:
-		 *    + ovl_dentry_upper() uses memory barriers to ensure the
-		 *      upper dentry is up-to-date
-		 *    + ovl_dentry_has_upper_alias() relies on locking of
-		 *      upper parent i_rwsem to prevent reordering copy-up
-		 *      with rename.
-		 */
-		if (ovl_dentry_upper(dentry) &&
-		    (ovl_dentry_has_upper_alias(dentry) || disconnected))
+		if (ovl_already_copied_up(dentry))
 			break;
 
 		next = dget(dentry);
@@ -852,9 +838,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 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) ||
-	     (dentry->d_flags & DCACHE_DISCONNECTED)))
+	if (ovl_already_copied_up(dentry))
 		return false;
 
 	if (special_file(d_inode(dentry)->i_mode))
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 21f3bfea665c..11f87f3099a1 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -241,6 +241,7 @@ bool ovl_is_whiteout(struct dentry *dentry);
 struct file *ovl_path_open(struct path *path, int flags);
 int ovl_copy_up_start(struct dentry *dentry);
 void ovl_copy_up_end(struct dentry *dentry);
+bool ovl_already_copied_up(struct dentry *dentry);
 bool ovl_check_origin_xattr(struct dentry *dentry);
 bool ovl_check_dir_xattr(struct dentry *dentry, const char *name);
 int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 930784a26623..4e537eeb594e 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -339,13 +339,37 @@ struct file *ovl_path_open(struct path *path, int flags)
 	return dentry_open(path, flags | O_NOATIME, current_cred());
 }
 
+bool ovl_already_copied_up(struct dentry *dentry)
+{
+	bool disconnected = dentry->d_flags & DCACHE_DISCONNECTED;
+
+	/*
+	 * Check if copy-up has happened as well as for upper alias (in
+	 * case of hard links) is there.
+	 *
+	 * Both checks are lockless:
+	 *  - false negatives: will recheck under oi->lock
+	 *  - false positives:
+	 *    + ovl_dentry_upper() uses memory barriers to ensure the
+	 *      upper dentry is up-to-date
+	 *    + ovl_dentry_has_upper_alias() relies on locking of
+	 *      upper parent i_rwsem to prevent reordering copy-up
+	 *      with rename.
+	 */
+	if (ovl_dentry_upper(dentry) &&
+	    (ovl_dentry_has_upper_alias(dentry) || disconnected))
+		return true;
+
+	return false;
+}
+
 int ovl_copy_up_start(struct dentry *dentry)
 {
 	struct ovl_inode *oi = OVL_I(d_inode(dentry));
 	int err;
 
 	err = mutex_lock_interruptible(&oi->lock);
-	if (!err && ovl_dentry_has_upper_alias(dentry)) {
+	if (!err && ovl_already_copied_up(dentry)) {
 		err = 1; /* Already copied up */
 		mutex_unlock(&oi->lock);
 	}
-- 
2.13.6

[PATCH v12 07/17] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Vivek Goyal]

Now we will have the capability to have upper inodes which might be only
metadata copy up and data is still on lower inode. So add a new xattr
OVL_XATTR_METACOPY to distinguish between two cases.

Presence of OVL_XATTR_METACOPY reflects that file has been copied up
metadata only and and data will be copied up later from lower origin.
So this xattr is set when a metadata copy takes place and cleared when
data copy takes place.

We also use a bit in ovl_inode->flags to cache OVL_UPPERDATA which reflects
whether ovl inode has data or not (as opposed to metadata only copy up).

If a file is copied up metadata only and later when same file is opened
for WRITE, then data copy up takes place. We copy up data, remove METACOPY
xattr and then set the UPPERDATA flag in ovl_inode->flags. While all
these operations happen with oi->lock held, read side of oi->flags can be
lockless. That is another thread on another cpu can check if UPPERDATA
flag is set or not.

So this gives us an ordering requirement w.r.t UPPERDATA flag. That is, if
another cpu sees UPPERDATA flag set, then it should be guaranteed that
effects of data copy up and remove xattr operations are also visible.

For example.

	CPU1				CPU2
ovl_d_real()				acquire(oi->lock)
 ovl_open_maybe_copy_up()                ovl_copy_up_data()
  open_open_need_copy_up()		 vfs_removexattr()
   ovl_already_copied_up()
    ovl_dentry_needs_data_copy_up()	 ovl_set_flag(OVL_UPPERDATA)
     ovl_test_flag(OVL_UPPERDATA)       release(oi->lock)

Say CPU2 is copying up data and in the end sets UPPERDATA flag. But if
CPU1 perceives the effects of setting UPPERDATA flag but not the effects
of preceeding operations (ex. upper that is not fully copied up), it will be
a problem.

Hence this patch introduces smp_wmb() on setting UPPERDATA flag operation
and smp_rmb() on UPPERDATA flag test operation.

May be some other lock or barrier is already covering it. But I am not sure
what that is and is it obvious enough that we will not break it in future.

So hence trying to be safe here and introducing barriers explicitly for
UPPERDATA flag/bit.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c   | 56 ++++++++++++++++++++++++++++++----
 fs/overlayfs/dir.c       |  1 +
 fs/overlayfs/overlayfs.h | 18 +++++++++--
 fs/overlayfs/super.c     |  1 +
 fs/overlayfs/util.c      | 78 +++++++++++++++++++++++++++++++++++++++++++++---
 5 files changed, 143 insertions(+), 11 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 8d9af7fdc8a4..9801ae7baa5d 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -195,6 +195,16 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 	return error;
 }
 
+static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
+{
+	struct iattr attr = {
+		.ia_valid = ATTR_SIZE,
+		.ia_size = stat->size,
+	};
+
+	return notify_change(upperdentry, &attr, NULL);
+}
+
 static int ovl_set_timestamps(struct dentry *upperdentry, struct kstat *stat)
 {
 	struct iattr attr = {
@@ -586,8 +596,18 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 			return err;
 	}
 
+	if (c->metacopy) {
+		err = ovl_check_setxattr(c->dentry, temp, OVL_XATTR_METACOPY,
+					 NULL, 0, -EOPNOTSUPP);
+		if (err)
+			return err;
+	}
+
 	inode_lock(temp->d_inode);
-	err = ovl_set_attr(temp, &c->stat);
+	if (c->metacopy)
+		err = ovl_set_size(temp, &c->stat);
+	if (!err)
+		err = ovl_set_attr(temp, &c->stat);
 	inode_unlock(temp->d_inode);
 
 	return err;
@@ -625,6 +645,8 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
 	if (err)
 		goto out_cleanup;
 
+	if (!c->metacopy)
+		ovl_set_upperdata(d_inode(c->dentry));
 	inode = d_inode(c->dentry);
 	ovl_inode_update(inode, newdentry);
 	if (S_ISDIR(inode->i_mode))
@@ -729,6 +751,28 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 	return err;
 }
 
+/* Copy up data of an inode which was copied up metadata only in the past. */
+static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
+{
+	struct path upperpath;
+	int err;
+
+	ovl_path_upper(c->dentry, &upperpath);
+	if (WARN_ON(upperpath.dentry == NULL))
+		return -EIO;
+
+	err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
+	if (err)
+		return err;
+
+	err = vfs_removexattr(upperpath.dentry, OVL_XATTR_METACOPY);
+	if (err)
+		return err;
+
+	ovl_set_upperdata(d_inode(c->dentry));
+	return err;
+}
+
 static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 			   int flags)
 {
@@ -775,7 +819,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	}
 	ovl_do_check_copy_up(ctx.lowerpath.dentry);
 
-	err = ovl_copy_up_start(dentry);
+	err = ovl_copy_up_start(dentry, flags);
 	/* err < 0: interrupted, err > 0: raced with another copy-up */
 	if (unlikely(err)) {
 		if (err > 0)
@@ -785,6 +829,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 			err = ovl_do_copy_up(&ctx);
 		if (!err && parent && !ovl_dentry_has_upper_alias(dentry))
 			err = ovl_link_up(&ctx);
+		if (!err && ovl_dentry_needs_data_copy_up_locked(dentry, flags))
+			err = ovl_copy_up_meta_inode_data(&ctx);
 		ovl_copy_up_end(dentry);
 	}
 	do_delayed_call(&done);
@@ -810,7 +856,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		struct dentry *next;
 		struct dentry *parent = NULL;
 
-		if (ovl_already_copied_up(dentry))
+		if (ovl_already_copied_up(dentry, flags))
 			break;
 
 		next = dget(dentry);
@@ -838,13 +884,13 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
 {
 	/* Copy up of disconnected dentry does not set upper alias */
-	if (ovl_already_copied_up(dentry))
+	if (ovl_already_copied_up(dentry, flags))
 		return false;
 
 	if (special_file(d_inode(dentry)->i_mode))
 		return false;
 
-	if (!(OPEN_FMODE(flags) & FMODE_WRITE) && !(flags & O_TRUNC))
+	if (!ovl_open_flags_need_copy_up(flags))
 		return false;
 
 	return true;
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 839709c7803a..cdeae4bdbfce 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -189,6 +189,7 @@ static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
 	ovl_dentry_version_inc(dentry->d_parent, false);
 	ovl_dentry_set_upper_alias(dentry);
 	if (!hardlink) {
+		ovl_set_upperdata(inode);
 		ovl_inode_update(inode, newdentry);
 		ovl_copyattr(newdentry->d_inode, inode);
 	} else {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 11f87f3099a1..b48bd73786f1 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -28,6 +28,7 @@ enum ovl_path_type {
 #define OVL_XATTR_IMPURE OVL_XATTR_PREFIX "impure"
 #define OVL_XATTR_NLINK OVL_XATTR_PREFIX "nlink"
 #define OVL_XATTR_UPPER OVL_XATTR_PREFIX "upper"
+#define OVL_XATTR_METACOPY OVL_XATTR_PREFIX "metacopy"
 
 enum ovl_inode_flag {
 	/* Pure upper dir that may contain non pure upper entries */
@@ -35,6 +36,7 @@ enum ovl_inode_flag {
 	/* Non-merge dir that may contain whiteout entries */
 	OVL_WHITEOUTS,
 	OVL_INDEX,
+	OVL_UPPERDATA,
 };
 
 enum ovl_entry_flag {
@@ -195,6 +197,14 @@ static inline struct dentry *ovl_do_tmpfile(struct dentry *dentry, umode_t mode)
 	return ret;
 }
 
+static inline bool ovl_open_flags_need_copy_up(int flags)
+{
+	if (!flags)
+		return false;
+
+	return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));
+}
+
 /* util.c */
 int ovl_want_write(struct dentry *dentry);
 void ovl_drop_write(struct dentry *dentry);
@@ -229,6 +239,10 @@ bool ovl_dentry_is_whiteout(struct dentry *dentry);
 void ovl_dentry_set_opaque(struct dentry *dentry);
 bool ovl_dentry_has_upper_alias(struct dentry *dentry);
 void ovl_dentry_set_upper_alias(struct dentry *dentry);
+bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags);
+bool ovl_dentry_needs_data_copy_up_locked(struct dentry *dentry, int flags);
+bool ovl_has_upperdata(struct dentry *dentry);
+void ovl_set_upperdata(struct inode *inode);
 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);
@@ -239,9 +253,9 @@ void ovl_dentry_version_inc(struct dentry *dentry, bool impurity);
 u64 ovl_dentry_version_get(struct dentry *dentry);
 bool ovl_is_whiteout(struct dentry *dentry);
 struct file *ovl_path_open(struct path *path, int flags);
-int ovl_copy_up_start(struct dentry *dentry);
+int ovl_copy_up_start(struct dentry *dentry, int flags);
 void ovl_copy_up_end(struct dentry *dentry);
-bool ovl_already_copied_up(struct dentry *dentry);
+bool ovl_already_copied_up(struct dentry *dentry, int flags);
 bool ovl_check_origin_xattr(struct dentry *dentry);
 bool ovl_check_dir_xattr(struct dentry *dentry, const char *name);
 int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 0e124b9d902d..d3dbdd695722 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1397,6 +1397,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
 	/* Root is always merge -> can have whiteouts */
 	ovl_set_flag(OVL_WHITEOUTS, d_inode(root_dentry));
+	ovl_set_upperdata(d_inode(root_dentry));
 	ovl_inode_init(d_inode(root_dentry), upperpath.dentry,
 		       ovl_dentry_lower(root_dentry));
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 4e537eeb594e..7929cc872df6 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -256,6 +256,62 @@ void ovl_dentry_set_upper_alias(struct dentry *dentry)
 	ovl_dentry_set_flag(OVL_E_UPPER_ALIAS, dentry);
 }
 
+static bool ovl_should_check_upperdata(struct dentry *dentry)
+{
+	if (!S_ISREG(d_inode(dentry)->i_mode))
+		return false;
+
+	if (!ovl_dentry_lower(dentry))
+		return false;
+
+	return true;
+}
+
+bool ovl_has_upperdata(struct dentry *dentry)
+{
+	if (!ovl_should_check_upperdata(dentry))
+		return true;
+
+	if (!ovl_test_flag(OVL_UPPERDATA, d_inode(dentry)))
+		return false;
+	/*
+	 * Pairs with smp_wmb() in ovl_set_upperdata(). Main user of
+	 * ovl_has_upperdata() is ovl_copy_up_meta_inode_data(). Make sure
+	 * if setting of OVL_UPPERDATA is visible, then effects of writes
+	 * before that are visible too.
+	 */
+	smp_rmb();
+	return true;
+}
+
+void ovl_set_upperdata(struct inode *inode)
+{
+	/*
+	 * Pairs with smp_rmb() in ovl_has_upperdata(). Make sure
+	 * if OVL_UPPERDATA flag is visible, then effects of write operations
+	 * before it are visible as well.
+	 */
+	smp_wmb();
+	ovl_set_flag(OVL_UPPERDATA, inode);
+}
+
+/* Caller should hold ovl_inode->lock */
+bool ovl_dentry_needs_data_copy_up_locked(struct dentry *dentry, int flags)
+{
+	if (!ovl_open_flags_need_copy_up(flags))
+		return false;
+
+	return !ovl_test_flag(OVL_UPPERDATA, d_inode(dentry));
+}
+
+bool ovl_dentry_needs_data_copy_up(struct dentry *dentry, int flags)
+{
+	if (!ovl_open_flags_need_copy_up(flags))
+		return false;
+
+	return !ovl_has_upperdata(dentry);
+}
+
 bool ovl_redirect_dir(struct super_block *sb)
 {
 	struct ovl_fs *ofs = sb->s_fs_info;
@@ -339,7 +395,20 @@ struct file *ovl_path_open(struct path *path, int flags)
 	return dentry_open(path, flags | O_NOATIME, current_cred());
 }
 
-bool ovl_already_copied_up(struct dentry *dentry)
+/* Caller should hold ovl_inode->lock */
+static bool ovl_already_copied_up_locked(struct dentry *dentry, int flags)
+{
+	bool disconnected = dentry->d_flags & DCACHE_DISCONNECTED;
+
+	if (ovl_dentry_upper(dentry) &&
+	    (ovl_dentry_has_upper_alias(dentry) || disconnected) &&
+	    !ovl_dentry_needs_data_copy_up_locked(dentry, flags))
+		return true;
+
+	return false;
+}
+
+bool ovl_already_copied_up(struct dentry *dentry, int flags)
 {
 	bool disconnected = dentry->d_flags & DCACHE_DISCONNECTED;
 
@@ -357,19 +426,20 @@ bool ovl_already_copied_up(struct dentry *dentry)
 	 *      with rename.
 	 */
 	if (ovl_dentry_upper(dentry) &&
-	    (ovl_dentry_has_upper_alias(dentry) || disconnected))
+	    (ovl_dentry_has_upper_alias(dentry) || disconnected) &&
+	    !ovl_dentry_needs_data_copy_up(dentry, flags))
 		return true;
 
 	return false;
 }
 
-int ovl_copy_up_start(struct dentry *dentry)
+int ovl_copy_up_start(struct dentry *dentry, int flags)
 {
 	struct ovl_inode *oi = OVL_I(d_inode(dentry));
 	int err;
 
 	err = mutex_lock_interruptible(&oi->lock);
-	if (!err && ovl_already_copied_up(dentry)) {
+	if (!err && ovl_already_copied_up_locked(dentry, flags)) {
 		err = 1; /* Already copied up */
 		mutex_unlock(&oi->lock);
 	}
-- 
2.13.6

[PATCH v12 08/17] ovl: Modify ovl_lookup() and friends to lookup metacopy dentry

[Vivek Goyal]

This patch modifies ovl_lookup() and friends to lookup metacopy dentries.
It also allows for presence of metacopy dentries in lower layer.

During lookup, check for presence of OVL_XATTR_METACOPY and if not present,
set OVL_UPPERDATA bit in flags.

OVL_UPPERDATA flag is set unconditionally if upper inode exists.

Do not follow metacopy origin if we find a metacopy only inode and metacopy
feature is not enabled for that mount. Like redirect, this can have security
implications where an attacker could hand craft upper and try to gain
access to file on lower which it should not have to begin with.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/export.c |   3 ++
 fs/overlayfs/namei.c  | 121 ++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 115 insertions(+), 9 deletions(-)

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index bb94ce9da5c8..35f2d4eb0d7e 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -192,6 +192,9 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 	if (index)
 		ovl_set_flag(OVL_INDEX, inode);
 
+	if (upper)
+		ovl_set_flag(OVL_UPPERDATA, inode);
+
 	dentry = d_find_any_alias(inode);
 	if (!dentry) {
 		dentry = d_alloc_anon(inode->i_sb);
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 70fcfcc684cc..220e754c974b 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -24,6 +24,7 @@ struct ovl_lookup_data {
 	bool stop;
 	bool last;
 	char *redirect;
+	bool metacopy;
 };
 
 static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
@@ -208,6 +209,28 @@ struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt)
 	return real;
 }
 
+/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
+static int ovl_check_metacopy_xattr(struct dentry *dentry)
+{
+	int res;
+
+	/* Only regular files can have metacopy xattr */
+	if (!S_ISREG(d_inode(dentry)->i_mode))
+		return 0;
+
+	res = vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
+	if (res < 0) {
+		if (res == -ENODATA || res == -EOPNOTSUPP)
+			return 0;
+		goto out;
+	}
+
+	return 1;
+out:
+	pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
+	return res;
+}
+
 static bool ovl_is_opaquedir(struct dentry *dentry)
 {
 	return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);
@@ -242,9 +265,16 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
 		goto put_and_out;
 	}
 	if (!d_can_lookup(this)) {
-		d->stop = true;
 		if (d->is_dir)
 			goto put_and_out;
+		err = ovl_check_metacopy_xattr(this);
+		if (err < 0)
+			goto out_err;
+		if (!err) {
+			d->stop = true;
+			d->metacopy = false;
+		} else
+			d->metacopy = true;
 		goto out;
 	}
 	d->is_dir = true;
@@ -799,7 +829,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 	struct ovl_entry *poe = dentry->d_parent->d_fsdata;
 	struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
-	struct ovl_path *stack = NULL;
+	struct ovl_path *stack = NULL, *origin_path = NULL;
 	struct dentry *upperdir, *upperdentry = NULL;
 	struct dentry *origin = NULL;
 	struct dentry *index = NULL;
@@ -810,6 +840,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	struct dentry *this;
 	unsigned int i;
 	int err;
+	bool metacopy = false;
 	struct ovl_lookup_data d = {
 		.name = dentry->d_name,
 		.is_dir = false,
@@ -817,6 +848,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		.stop = false,
 		.last = !poe->numlower,
 		.redirect = NULL,
+		.metacopy = false,
 	};
 
 	if (dentry->d_name.len > ofs->namelen)
@@ -835,7 +867,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			goto out;
 		}
 		if (upperdentry && !d.is_dir) {
-			BUG_ON(!d.stop || d.redirect);
+			unsigned int origin_ctr = 0;
+			BUG_ON(d.redirect);
 			/*
 			 * Lookup copy up origin by decoding origin file handle.
 			 * We may get a disconnected dentry, which is fine,
@@ -846,16 +879,20 @@ 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(ofs, upperdentry, &stack, &ctr);
+			err = ovl_check_origin(ofs, upperdentry, &origin_path,
+					       &origin_ctr);
 			if (err)
 				goto out_put_upper;
+
+			if (d.metacopy)
+				metacopy = true;
 		}
 
 		if (d.redirect) {
 			err = -ENOMEM;
 			upperredirect = kstrdup(d.redirect, GFP_KERNEL);
 			if (!upperredirect)
-				goto out_put_upper;
+				goto out_put_origin;
 			if (d.redirect[0] == '/')
 				poe = roe;
 		}
@@ -867,7 +904,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		stack = kcalloc(ofs->numlower, sizeof(struct ovl_path),
 				GFP_KERNEL);
 		if (!stack)
-			goto out_put_upper;
+			goto out_put_origin;
 	}
 
 	for (i = 0; !d.stop && i < poe->numlower; i++) {
@@ -885,7 +922,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		 * If no origin fh is stored in upper of a merge dir, store fh
 		 * of lower dir and set upper parent "impure".
 		 */
-		if (upperdentry && !ctr && !ofs->noxattr) {
+		if (upperdentry && !ctr && !ofs->noxattr && d.is_dir) {
 			err = ovl_fix_origin(dentry, this, upperdentry);
 			if (err) {
 				dput(this);
@@ -898,7 +935,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		 * lower 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_lower(dentry->d_sb)) {
+		if (upperdentry && !ctr && d.is_dir &&
+		    ovl_verify_lower(dentry->d_sb)) {
 			err = ovl_verify_origin(upperdentry, this, false);
 			if (err) {
 				dput(this);
@@ -909,6 +947,29 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			origin = this;
 		}
 
+		/*
+		 * For non-dir dentry, make sure dentry found by lookup
+		 * matches the origin stored in upper
+		 */
+		if (!d.is_dir && upperdentry && !ctr && origin_path) {
+			err = ovl_verify_origin(upperdentry, this, false);
+			if (err) {
+				dput(this);
+				goto out_put;
+			}
+		}
+
+		if (d.metacopy)
+			metacopy = true;
+		/*
+		 * Do not store intermediate metacopy dentries in chain,
+		 * except top most lower metacopy dentry
+		 */
+		if (d.metacopy && ctr) {
+			dput(this);
+			continue;
+		}
+
 		stack[ctr].dentry = this;
 		stack[ctr].layer = lower.layer;
 		ctr++;
@@ -940,6 +1001,34 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 	}
 
+	if (metacopy) {
+		BUG_ON(d.is_dir);
+		/*
+		 * Found a metacopy dentry but did not find corresponding
+		 * data dentry
+		 */
+		if (d.metacopy) {
+			err = -ESTALE;
+			goto out_put;
+		}
+
+		err = -EPERM;
+		if (!ofs->config.metacopy) {
+			pr_warn_ratelimited("overlay: refusing to follow"
+					    " metacopy origin for (%pd2)\n",
+					    dentry);
+			goto out_put;
+		}
+	} else if (!d.is_dir && upperdentry && !ctr && origin_path) {
+		if (WARN_ON(stack != NULL)) {
+			err = -EIO;
+			goto out_put;
+		}
+		stack = origin_path;
+		ctr = 1;
+		origin_path = NULL;
+	}
+
 	/*
 	 * Lookup index by lower inode and verify it matches upper inode.
 	 * We only trust dir index if we verified that lower dir matches
@@ -972,8 +1061,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 	if (upperdentry)
 		ovl_dentry_set_upper_alias(dentry);
-	else if (index)
+	else if (index) {
 		upperdentry = dget(index);
+		metacopy = ovl_check_metacopy_xattr(index);
+	}
 
 	if (upperdentry || ctr) {
 		if (ctr)
@@ -987,9 +1078,16 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		OVL_I(inode)->redirect = upperredirect;
 		if (index)
 			ovl_set_flag(OVL_INDEX, inode);
+
+		if (upperdentry && !metacopy)
+			ovl_set_flag(OVL_UPPERDATA, inode);
 	}
 
 	revert_creds(old_cred);
+	if (origin_path) {
+		dput(origin_path->dentry);
+		kfree(origin_path);
+	}
 	dput(index);
 	kfree(stack);
 	kfree(d.redirect);
@@ -1003,6 +1101,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	for (i = 0; i < ctr; i++)
 		dput(stack[i].dentry);
 	kfree(stack);
+out_put_origin:
+	if (origin_path) {
+		dput(origin_path->dentry);
+		kfree(origin_path);
+	}
 out_put_upper:
 	dput(upperdentry);
 	kfree(upperredirect);
-- 
2.13.6

[PATCH v12 09/17] ovl: Do not mark a non dir as _OVL_PATH_MERGE in ovl_path_type()

[Vivek Goyal]

With the addition of an origin chain for regular files, it is possible
that we don't have an upperdentry and oe->numlower > 1 for non-dir. So
mark a path __OVL_TYPE_MERGE only if it is a directory.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 7929cc872df6..19b936b0a551 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -122,7 +122,7 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
 				type |= __OVL_PATH_MERGE;
 		}
 	} else {
-		if (oe->numlower > 1)
+		if (oe->numlower > 1 && d_is_dir(dentry))
 			type |= __OVL_PATH_MERGE;
 	}
 	return type;
-- 
2.13.6

[PATCH v12 10/17] ovl: Copy up meta inode data from lowest data inode

[Vivek Goyal]

So far lower could not be a meta inode. So whenever it was time to copy
up data of a meta inode, we could copy it up from top most lower dentry.

But now lower itself can be a metacopy inode. That means data copy up
needs to take place from a data inode in metacopy inode chain. Find
lower data inode in the chain and use that for data copy up.

Introduced a helper called ovl_path_lowerdata() to find the lower
data inode chain.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c   | 14 ++++++++++----
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 14 ++++++++++++++
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 9801ae7baa5d..0c8d2755bd25 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -585,13 +585,15 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 	}
 
 	if (S_ISREG(c->stat.mode) && !c->metacopy) {
-		struct path upperpath;
+		struct path upperpath, datapath;
 
 		ovl_path_upper(c->dentry, &upperpath);
 		BUG_ON(upperpath.dentry != NULL);
 		upperpath.dentry = temp;
 
-		err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
+		ovl_path_lowerdata(c->dentry, &datapath);
+		BUG_ON(datapath.dentry == NULL);
+		err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
 		if (err)
 			return err;
 	}
@@ -754,14 +756,18 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 /* Copy up data of an inode which was copied up metadata only in the past. */
 static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 {
-	struct path upperpath;
+	struct path upperpath, datapath;
 	int err;
 
 	ovl_path_upper(c->dentry, &upperpath);
 	if (WARN_ON(upperpath.dentry == NULL))
 		return -EIO;
 
-	err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
+	ovl_path_lowerdata(c->dentry, &datapath);
+	if (WARN_ON(datapath.dentry == NULL))
+		return -EIO;
+
+	err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
 	if (err)
 		return err;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index b48bd73786f1..d999813f9aeb 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -221,6 +221,7 @@ bool ovl_dentry_weird(struct dentry *dentry);
 enum ovl_path_type ovl_path_type(struct dentry *dentry);
 void ovl_path_upper(struct dentry *dentry, struct path *path);
 void ovl_path_lower(struct dentry *dentry, struct path *path);
+void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
 struct dentry *ovl_dentry_lower(struct dentry *dentry);
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 19b936b0a551..22df74cb1def 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -148,6 +148,20 @@ void ovl_path_lower(struct dentry *dentry, struct path *path)
 	}
 }
 
+void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
+{
+	struct ovl_entry *oe = dentry->d_fsdata;
+	int idx = oe->numlower - 1;
+
+	if (!oe->numlower) {
+		*path = (struct path) { };
+		return;
+	}
+
+	path->mnt = oe->lowerstack[idx].layer->mnt;
+	path->dentry = oe->lowerstack[idx].dentry;
+}
+
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path)
 {
 	enum ovl_path_type type = ovl_path_type(dentry);
-- 
2.13.6

[PATCH v12 11/17] ovl: Fix ovl_getattr() to get number of blocks from lower

[Vivek Goyal]

If an inode has been copied up metadata only, then we need to query the
number of blocks from lower and fill up the stat->st_blocks.

We need to be careful about races where we are doing stat on one cpu and
data copy up is taking place on other cpu. We want to return
stat->st_blocks either from lower or stable upper and not something in
between. Hence, ovl_has_upperdata() is called first to figure out whether
block reporting will take place from lower or upper.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/inode.c     | 17 ++++++++++++++++-
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 16 ++++++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 165e1b52f619..150929a1fcdf 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -76,6 +76,9 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 	bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
 	bool samefs = ovl_same_sb(dentry->d_sb);
 	int err;
+	bool metacopy = false;
+
+	metacopy = ovl_is_metacopy_dentry(dentry);
 
 	type = ovl_path_real(dentry, &realpath);
 	old_cred = ovl_override_creds(dentry->d_sb);
@@ -93,7 +96,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 	if (!is_dir || samefs) {
 		if (OVL_TYPE_ORIGIN(type)) {
 			struct kstat lowerstat;
-			u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
+			u32 lowermask = STATX_INO | STATX_BLOCKS |
+					(!is_dir ? STATX_NLINK : 0);
 
 			ovl_path_lower(dentry, &realpath);
 			err = vfs_getattr(&realpath, &lowerstat,
@@ -126,6 +130,17 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 			else
 				stat->dev = ovl_get_pseudo_dev(dentry);
 		}
+		if (metacopy) {
+			struct kstat lowerdatastat;
+			u32 lowermask = STATX_BLOCKS;
+
+			ovl_path_lowerdata(dentry, &realpath);
+			err = vfs_getattr(&realpath, &lowerdatastat,
+					  lowermask, flags);
+			if (err)
+				goto out;
+			stat->blocks = lowerdatastat.blocks;
+		}
 		if (samefs) {
 			/*
 			 * When all layers are on the same fs, all real inode
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index d999813f9aeb..2d682923252e 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -272,6 +272,7 @@ 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);
+bool ovl_is_metacopy_dentry(struct dentry *dentry);
 
 static inline bool ovl_is_impuredir(struct dentry *dentry)
 {
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 22df74cb1def..274bbfc855e0 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -754,3 +754,19 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
 	pr_err("overlayfs: failed to lock workdir+upperdir\n");
 	return -EIO;
 }
+
+bool ovl_is_metacopy_dentry(struct dentry *dentry)
+{
+	struct ovl_entry *oe = dentry->d_fsdata;
+
+	if (!d_is_reg(dentry))
+		return false;
+
+	if (ovl_dentry_upper(dentry)) {
+		if (!ovl_has_upperdata(dentry))
+			return true;
+		return false;
+	}
+
+	return (oe->numlower > 1);
+}
-- 
2.13.6

[PATCH v12 12/17] ovl: Do not expose metacopy only upper dentry from d_real()

[Vivek Goyal]

d_real() can make a upper metacopy dentry/inode visible to the vfs layer.
This is something new and vfs layer does not know that this inode contains
only metadata and not data. And this could break things.

So to be safe, do not expose metacopy only dentry/inode to vfs using
d_real().

IOW, d_real() will not reuturn metacopy dentry. Instead, it will return
dentry corresponding lower dentry/inode which has file data.

For regular d_real() call (inode == NULL, D_REAL_UPPER not set), if upper
dentry inode is metacopy only and does not have data, return lower dentry.

If d_real() is called with flag D_REAL_UPPER, return upper dentry only if
it has data (flag OVL_UPPERDATA is set).

Similiarly, if d_real(inode=X) is called, a warning is emitted if returned
dentry/inode does not have OVL_UPPERDATA set. This should not happen as
we never made this metacopy inode visible to vfs so nobody should be
calling overlayfs back with inode=metacopy_inode.

I scanned the code and I don't think it breaks any of the existing code.
There are two users of D_REAL_UPPER. may_write_real() and
update_ovl_inode_times().

may_write_real(), will get an NULL dentry if upper inode is metacopy only
and it will return -EPERM. Effectively, we are disallowing modifications
to metacopy only inode from this interface. Though there is opportunity
to improve it. (Allow chattr on metacopy inodes).

update_ovl_inode_times() gets inode mtime and ctime from real inode. It
should not be broken for metacopy inode as well for following reasons.

- For any metadata operations (setattr, acl etc), overlay always calls
  ovl_copyattr() and updates ovl inode mtime and ctime. So there is no
  need to update mtime and ctime in this case. Its already updated, hence
  even if d_real(D_REAL_UPPER) returns nil, it should be fine.

- For metadata inode, mtime should be same as lower and not change. (data
  can't be modified on metadata inode without copyup). IOW, mtime of
  ovl dentry should be same as mtime of underlying metadata inode on upper
  always. So there is no need to update it.

- For file writes, ctime and mtime will be updated. But in that case
  first data will be copied up and this will not be a metadata inode
  anymore. And furthr call to d_real(D_REAL_UPPER) will return upper
  inode and new mtime and ctime will be obtainable.

So atime updates should work just fine for metacopy inodes. I think only
corner case is if somehow underlying filesystem changes ctime of upper
metadata inode without overlay knowing about it. Not sure how that
can happen. If somehow is affected by that, then we probably can implement
another flag which will allow caller to get metacopy inode as well.
Something like d_real(D_REAL_UPPER | D_METACOPY). And that should solve
this issue.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/super.c     | 21 +++++++++++++++++----
 fs/overlayfs/util.c      |  8 ++++++++
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 2d682923252e..24725b6668b9 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -225,6 +225,7 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
 struct dentry *ovl_dentry_lower(struct dentry *dentry);
+struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
 struct dentry *ovl_dentry_real(struct dentry *dentry);
 struct dentry *ovl_i_dentry_upper(struct inode *inode);
 struct inode *ovl_inode_upper(struct inode *inode);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index d3dbdd695722..4be4e47cbf57 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -96,8 +96,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 	struct dentry *real;
 	int err;
 
-	if (flags & D_REAL_UPPER)
-		return ovl_dentry_upper(dentry);
+	if (flags & D_REAL_UPPER) {
+		real = ovl_dentry_upper(dentry);
+		if (!real)
+			return NULL;
+		if (!ovl_has_upperdata(dentry))
+			return NULL;
+		return real;
+	}
 
 	if (!d_is_reg(dentry)) {
 		if (!inode || inode == d_inode(dentry))
@@ -113,15 +119,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 
 	real = ovl_dentry_upper(dentry);
 	if (real && (!inode || inode == d_inode(real))) {
+		bool metacopy = !ovl_has_upperdata(dentry);
 		if (!inode) {
 			err = ovl_check_append_only(d_inode(real), open_flags);
 			if (err)
 				return ERR_PTR(err);
-		}
+
+			if (unlikely(metacopy))
+				goto lower;
+		} else if (unlikely(metacopy))
+			goto bug;
+
 		return real;
 	}
 
-	real = ovl_dentry_lower(dentry);
+lower:
+	real = ovl_dentry_lowerdata(dentry);
 	if (!real)
 		goto bug;
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 274bbfc855e0..36d41f7001e3 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -186,6 +186,14 @@ struct dentry *ovl_dentry_lower(struct dentry *dentry)
 	return oe->numlower ? oe->lowerstack[0].dentry : NULL;
 }
 
+struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
+{
+	struct ovl_entry *oe = dentry->d_fsdata;
+	int idx = oe->numlower - 1;
+
+	return oe->lowerstack[idx].dentry;
+}
+
 struct dentry *ovl_dentry_real(struct dentry *dentry)
 {
 	return ovl_dentry_upper(dentry) ?: ovl_dentry_lower(dentry);
-- 
2.13.6

[PATCH v12 13/17] ovl: Check redirects for metacopy files

[Vivek Goyal]

Right now we rely on path based lookup for data origin of metacopy upper.
This will work only if upper has not been renamed. We solved this problem
already for merged directories using redirect. Use same logic for metacopy
files.

This patch just goes on to check redirects for metacopy files.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 220e754c974b..a4a5c5f5540d 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -265,22 +265,22 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
 		goto put_and_out;
 	}
 	if (!d_can_lookup(this)) {
-		if (d->is_dir)
-			goto put_and_out;
+		d->is_dir = false;
 		err = ovl_check_metacopy_xattr(this);
 		if (err < 0)
 			goto out_err;
 		if (!err) {
 			d->stop = true;
 			d->metacopy = false;
+			goto out;
 		} else
 			d->metacopy = true;
-		goto out;
-	}
-	d->is_dir = true;
-	if (!d->last && ovl_is_opaquedir(this)) {
-		d->stop = d->opaque = true;
-		goto out;
+	} else {
+		d->is_dir = true;
+		if (!d->last && ovl_is_opaquedir(this)) {
+			d->stop = d->opaque = true;
+			goto out;
+		}
 	}
 	err = ovl_check_redirect(this, d, prelen, post);
 	if (err)
@@ -868,7 +868,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 		if (upperdentry && !d.is_dir) {
 			unsigned int origin_ctr = 0;
-			BUG_ON(d.redirect);
 			/*
 			 * Lookup copy up origin by decoding origin file handle.
 			 * We may get a disconnected dentry, which is fine,
-- 
2.13.6

[PATCH v12 14/17] ovl: Set redirect on metacopy files upon rename

[Vivek Goyal]

Set redirect on metacopy files upon rename. This will help find data dentry
in lower dirs.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/dir.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index cdeae4bdbfce..b955f6fede06 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -1048,9 +1048,11 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
 			err = ovl_set_redirect(old, samedir);
 		else if (!old_opaque && ovl_type_merge(new->d_parent))
 			err = ovl_set_opaque_xerr(old, olddentry, -EXDEV);
-		if (err)
-			goto out_dput;
-	}
+	} else if (ovl_is_metacopy_dentry(old))
+			err = ovl_set_redirect(old, false);
+	if (err)
+		goto out_dput;
+
 	if (!overwrite && new_is_dir) {
 		if (ovl_type_merge_or_lower(new))
 			err = ovl_set_redirect(new, samedir);
@@ -1058,7 +1060,10 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
 			err = ovl_set_opaque_xerr(new, newdentry, -EXDEV);
 		if (err)
 			goto out_dput;
-	}
+	} else if (!overwrite && ovl_is_metacopy_dentry(new))
+			err = ovl_set_redirect(new, false);
+	if (err)
+		goto out_dput;
 
 	err = ovl_do_rename(old_upperdir->d_inode, olddentry,
 			    new_upperdir->d_inode, newdentry, flags);
-- 
2.13.6

[PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Vivek Goyal]

When a metacopy file is no longer a metacopy and data has been copied up,
remove REDIRECT xattr. Its not needed anymore.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 0c8d2755bd25..704febd2e2fa 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 	if (err)
 		return err;
 
+	/*
+	 * A metacopy files does not need redirect xattr once data has
+	 * been copied up.
+	 */
+	err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
+	if (err && err != -ENODATA && err != -EOPNOTSUPP)
+		return err;
+
+	err = 0;
 	ovl_set_upperdata(d_inode(c->dentry));
 	return err;
 }
-- 
2.13.6

[PATCH v12 16/17] ovl: Set redirect on upper inode when it is linked

[Vivek Goyal]

When we create a hardlink to a metacopy upper file, first the redirect
on that inode. Path based lookup will not work with newly created link
and redirect will solve that issue.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/dir.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index b955f6fede06..da5c4b8ee919 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -24,6 +24,8 @@ module_param_named(redirect_max, ovl_redirect_max, ushort, 0644);
 MODULE_PARM_DESC(ovl_redirect_max,
 		 "Maximum length of absolute redirect xattr value");
 
+static int ovl_set_redirect(struct dentry *dentry, bool samedir);
+
 int ovl_cleanup(struct inode *wdir, struct dentry *wdentry)
 {
 	int err;
@@ -468,6 +470,9 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 	const struct cred *old_cred;
 	struct cred *override_cred;
 	struct dentry *parent = dentry->d_parent;
+	struct dentry *hardlink_upper;
+
+	hardlink_upper = hardlink ? ovl_dentry_upper(hardlink) : NULL;
 
 	err = ovl_copy_up(parent);
 	if (err)
@@ -502,12 +507,18 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 		put_cred(override_creds(override_cred));
 		put_cred(override_cred);
 
+		if (hardlink && ovl_is_metacopy_dentry(hardlink)) {
+			err = ovl_set_redirect(hardlink, false);
+			if (err)
+				goto out_revert_creds;
+		}
+
 		if (!ovl_dentry_is_whiteout(dentry))
 			err = ovl_create_upper(dentry, inode, attr,
-						hardlink);
+					       hardlink_upper);
 		else
 			err = ovl_create_over_whiteout(dentry, inode, attr,
-							hardlink);
+					               hardlink_upper);
 	}
 out_revert_creds:
 	revert_creds(old_cred);
@@ -602,8 +613,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
 	inode = d_inode(old);
 	ihold(inode);
 
-	err = ovl_create_or_link(new, inode, NULL, ovl_dentry_upper(old),
-				 ovl_type_origin(old));
+	err = ovl_create_or_link(new, inode, NULL, old, ovl_type_origin(old));
 	if (err)
 		iput(inode);
 
-- 
2.13.6

[PATCH v12 17/17] ovl: Enable metadata only feature

[Vivek Goyal]

All the bits are in patches before this. So it is time to enable the
metadata only copy up feature.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 704febd2e2fa..0fbc35fdcb06 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -247,9 +247,6 @@ static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
 {
 	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 
-	/* TODO: Will enable metacopy in last patch of series */
-	return false;
-
 	if (!ofs->config.metacopy)
 		return false;
 
-- 
2.13.6

Re: [PATCH v12 09/17] ovl: Do not mark a non dir as _OVL_PATH_MERGE in ovl_path_type()

[Amir Goldstein]

On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> With the addition of an origin chain for regular files, it is possible
> that we don't have an upperdentry and oe->numlower > 1 for non-dir. So
> mark a path __OVL_TYPE_MERGE only if it is a directory.
>

Okay, but what's wrong with marking a non-dir as TYPE MERGE?
It is, after all, a sort of merged entry.

> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/util.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 7929cc872df6..19b936b0a551 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -122,7 +122,7 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
>                                 type |= __OVL_PATH_MERGE;
>                 }
>         } else {
> -               if (oe->numlower > 1)
> +               if (oe->numlower > 1 && d_is_dir(dentry))
>                         type |= __OVL_PATH_MERGE;
>         }
>         return type;
> --
> 2.13.6
>

Re: [PATCH v12 12/17] ovl: Do not expose metacopy only upper dentry from d_real()

[Amir Goldstein]

On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> d_real() can make a upper metacopy dentry/inode visible to the vfs layer.
> This is something new and vfs layer does not know that this inode contains
> only metadata and not data. And this could break things.
>
> So to be safe, do not expose metacopy only dentry/inode to vfs using
> d_real().
>
> IOW, d_real() will not reuturn metacopy dentry. Instead, it will return
> dentry corresponding lower dentry/inode which has file data.
>
> For regular d_real() call (inode == NULL, D_REAL_UPPER not set), if upper
> dentry inode is metacopy only and does not have data, return lower dentry.
>
> If d_real() is called with flag D_REAL_UPPER, return upper dentry only if
> it has data (flag OVL_UPPERDATA is set).
>
> Similiarly, if d_real(inode=X) is called, a warning is emitted if returned
> dentry/inode does not have OVL_UPPERDATA set. This should not happen as
> we never made this metacopy inode visible to vfs so nobody should be
> calling overlayfs back with inode=metacopy_inode.
>
> I scanned the code and I don't think it breaks any of the existing code.
> There are two users of D_REAL_UPPER. may_write_real() and
> update_ovl_inode_times().
>
> may_write_real(), will get an NULL dentry if upper inode is metacopy only
> and it will return -EPERM. Effectively, we are disallowing modifications
> to metacopy only inode from this interface. Though there is opportunity
> to improve it. (Allow chattr on metacopy inodes).
>
> update_ovl_inode_times() gets inode mtime and ctime from real inode. It
> should not be broken for metacopy inode as well for following reasons.
>
> - For any metadata operations (setattr, acl etc), overlay always calls
>   ovl_copyattr() and updates ovl inode mtime and ctime. So there is no
>   need to update mtime and ctime in this case. Its already updated, hence
>   even if d_real(D_REAL_UPPER) returns nil, it should be fine.
>
> - For metadata inode, mtime should be same as lower and not change. (data
>   can't be modified on metadata inode without copyup). IOW, mtime of
>   ovl dentry should be same as mtime of underlying metadata inode on upper
>   always. So there is no need to update it.
>
> - For file writes, ctime and mtime will be updated. But in that case
>   first data will be copied up and this will not be a metadata inode
>   anymore. And furthr call to d_real(D_REAL_UPPER) will return upper
>   inode and new mtime and ctime will be obtainable.
>
> So atime updates should work just fine for metacopy inodes. I think only
> corner case is if somehow underlying filesystem changes ctime of upper
> metadata inode without overlay knowing about it. Not sure how that
> can happen. If somehow is affected by that, then we probably can implement
> another flag which will allow caller to get metacopy inode as well.
> Something like d_real(D_REAL_UPPER | D_METACOPY). And that should solve
> this issue.
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/super.c     | 21 +++++++++++++++++----
>  fs/overlayfs/util.c      |  8 ++++++++
>  3 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 2d682923252e..24725b6668b9 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -225,6 +225,7 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
>  enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
>  struct dentry *ovl_dentry_upper(struct dentry *dentry);
>  struct dentry *ovl_dentry_lower(struct dentry *dentry);
> +struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
>  struct dentry *ovl_dentry_real(struct dentry *dentry);
>  struct dentry *ovl_i_dentry_upper(struct inode *inode);
>  struct inode *ovl_inode_upper(struct inode *inode);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index d3dbdd695722..4be4e47cbf57 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -96,8 +96,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>         struct dentry *real;
>         int err;
>
> -       if (flags & D_REAL_UPPER)
> -               return ovl_dentry_upper(dentry);
> +       if (flags & D_REAL_UPPER) {
> +               real = ovl_dentry_upper(dentry);
> +               if (!real)
> +                       return NULL;
> +               if (!ovl_has_upperdata(dentry))
> +                       return NULL;
> +               return real;
> +       }
>
>         if (!d_is_reg(dentry)) {
>                 if (!inode || inode == d_inode(dentry))
> @@ -113,15 +119,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>
>         real = ovl_dentry_upper(dentry);
>         if (real && (!inode || inode == d_inode(real))) {
> +               bool metacopy = !ovl_has_upperdata(dentry);
>                 if (!inode) {
>                         err = ovl_check_append_only(d_inode(real), open_flags);
>                         if (err)
>                                 return ERR_PTR(err);
> -               }
> +
> +                       if (unlikely(metacopy))
> +                               goto lower;
> +               } else if (unlikely(metacopy))
> +                       goto bug;
> +
>                 return real;
>         }
>
> -       real = ovl_dentry_lower(dentry);
> +lower:
> +       real = ovl_dentry_lowerdata(dentry);
>         if (!real)
>                 goto bug;
>
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 274bbfc855e0..36d41f7001e3 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -186,6 +186,14 @@ struct dentry *ovl_dentry_lower(struct dentry *dentry)
>         return oe->numlower ? oe->lowerstack[0].dentry : NULL;
>  }
>
> +struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
> +{
> +       struct ovl_entry *oe = dentry->d_fsdata;
> +       int idx = oe->numlower - 1;
> +
> +       return oe->lowerstack[idx].dentry;
> +}
> +

This new change is not in line with the subject line.
Either change the commit message to fit or better split this
small change to a new patch because the commit message is long
enough as it is.

Thanks,
Amir.

Re: [PATCH v12 10/17] ovl: Copy up meta inode data from lowest data inode

[Amir Goldstein]

On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> So far lower could not be a meta inode. So whenever it was time to copy
> up data of a meta inode, we could copy it up from top most lower dentry.
>
> But now lower itself can be a metacopy inode. That means data copy up
> needs to take place from a data inode in metacopy inode chain. Find
> lower data inode in the chain and use that for data copy up.
>
> Introduced a helper called ovl_path_lowerdata() to find the lower
> data inode chain.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/copy_up.c   | 14 ++++++++++----
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/util.c      | 14 ++++++++++++++
>  3 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 9801ae7baa5d..0c8d2755bd25 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -585,13 +585,15 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>         }
>
>         if (S_ISREG(c->stat.mode) && !c->metacopy) {
> -               struct path upperpath;
> +               struct path upperpath, datapath;
>
>                 ovl_path_upper(c->dentry, &upperpath);
>                 BUG_ON(upperpath.dentry != NULL);
>                 upperpath.dentry = temp;
>
> -               err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
> +               ovl_path_lowerdata(c->dentry, &datapath);
> +               BUG_ON(datapath.dentry == NULL);
> +               err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
>                 if (err)
>                         return err;
>         }
> @@ -754,14 +756,18 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
>  /* Copy up data of an inode which was copied up metadata only in the past. */
>  static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>  {
> -       struct path upperpath;
> +       struct path upperpath, datapath;
>         int err;
>
>         ovl_path_upper(c->dentry, &upperpath);
>         if (WARN_ON(upperpath.dentry == NULL))
>                 return -EIO;
>
> -       err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
> +       ovl_path_lowerdata(c->dentry, &datapath);
> +       if (WARN_ON(datapath.dentry == NULL))
> +               return -EIO;
> +
> +       err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
>         if (err)
>                 return err;
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index b48bd73786f1..d999813f9aeb 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -221,6 +221,7 @@ bool ovl_dentry_weird(struct dentry *dentry);
>  enum ovl_path_type ovl_path_type(struct dentry *dentry);
>  void ovl_path_upper(struct dentry *dentry, struct path *path);
>  void ovl_path_lower(struct dentry *dentry, struct path *path);
> +void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
>  enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
>  struct dentry *ovl_dentry_upper(struct dentry *dentry);
>  struct dentry *ovl_dentry_lower(struct dentry *dentry);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 19b936b0a551..22df74cb1def 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -148,6 +148,20 @@ void ovl_path_lower(struct dentry *dentry, struct path *path)
>         }
>  }
>

A comment here why  ovl_path_lowerdata is needed would be nice.

> +void ovl_path_lowerdata(struct dentry *dentry, struct path *path)
> +{
> +       struct ovl_entry *oe = dentry->d_fsdata;
> +       int idx = oe->numlower - 1;
> +
> +       if (!oe->numlower) {
> +               *path = (struct path) { };
> +               return;
> +       }
> +
> +       path->mnt = oe->lowerstack[idx].layer->mnt;
> +       path->dentry = oe->lowerstack[idx].dentry;
> +}
> +
>  enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path)
>  {
>         enum ovl_path_type type = ovl_path_type(dentry);
> --
> 2.13.6
>

Re: [PATCH v12 14/17] ovl: Set redirect on metacopy files upon rename

[Amir Goldstein]

On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Set redirect on metacopy files upon rename. This will help find data dentry
> in lower dirs.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/dir.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index cdeae4bdbfce..b955f6fede06 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -1048,9 +1048,11 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
>                         err = ovl_set_redirect(old, samedir);
>                 else if (!old_opaque && ovl_type_merge(new->d_parent))
>                         err = ovl_set_opaque_xerr(old, olddentry, -EXDEV);
> -               if (err)
> -                       goto out_dput;
> -       }
> +       } else if (ovl_is_metacopy_dentry(old))
> +                       err = ovl_set_redirect(old, false);

You should use {} in the else statement as well.

Q: why do you set samedir: = false here?
A: because other hardlink aliasses cannot follow a relative redirect, right?

This is a subtle detail that should be documented, but also
maybe do use samedir if nlink == 1?

> +       if (err)
> +               goto out_dput;
> +
>         if (!overwrite && new_is_dir) {
>                 if (ovl_type_merge_or_lower(new))
>                         err = ovl_set_redirect(new, samedir);
> @@ -1058,7 +1060,10 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
>                         err = ovl_set_opaque_xerr(new, newdentry, -EXDEV);
>                 if (err)
>                         goto out_dput;
> -       }
> +       } else if (!overwrite && ovl_is_metacopy_dentry(new))
> +                       err = ovl_set_redirect(new, false);

Same here for both comments.

Thanks,
Amir.

Re: [PATCH v12 16/17] ovl: Set redirect on upper inode when it is linked

[Amir Goldstein]

On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> When we create a hardlink to a metacopy upper file, first the redirect
> on that inode. Path based lookup will not work with newly created link
> and redirect will solve that issue.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/dir.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index b955f6fede06..da5c4b8ee919 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -24,6 +24,8 @@ module_param_named(redirect_max, ovl_redirect_max, ushort, 0644);
>  MODULE_PARM_DESC(ovl_redirect_max,
>                  "Maximum length of absolute redirect xattr value");
>
> +static int ovl_set_redirect(struct dentry *dentry, bool samedir);
> +
>  int ovl_cleanup(struct inode *wdir, struct dentry *wdentry)
>  {
>         int err;
> @@ -468,6 +470,9 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>         const struct cred *old_cred;
>         struct cred *override_cred;
>         struct dentry *parent = dentry->d_parent;
> +       struct dentry *hardlink_upper;
> +
> +       hardlink_upper = hardlink ? ovl_dentry_upper(hardlink) : NULL;

IMO it would be nicer to pass overlay hardlink dentry all the way to
ovl_create_real() and then just:

        if (hardlink) {
                err = ovl_do_link(ovl_dentry_upper(hardlink), dir,
newdentry, debug);

Thanks,
Amir.

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Amir Goldstein]

On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> When a metacopy file is no longer a metacopy and data has been copied up,
> remove REDIRECT xattr. Its not needed anymore.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 0c8d2755bd25..704febd2e2fa 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>         if (err)
>                 return err;
>
> +       /*
> +        * A metacopy files does not need redirect xattr once data has
> +        * been copied up.
> +        */
> +       err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
> +       if (err && err != -ENODATA && err != -EOPNOTSUPP)
> +               return err;
> +
> +       err = 0;
>         ovl_set_upperdata(d_inode(c->dentry));
>         return err;

By intuition, I would say that removing redirect should be done after setting
upperdata flag. Not sure if it really matters in real life.
Maybe when racing a lookup of a metacopy hardlink and copy up data of
an upper alias?

Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
probably use a helper ovl_clear_redirect() for the locking.

But that highlights a serious problem with current patches -
Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()
and additionally with dentry->d_lock in ovl_rename()
That is sufficient for directories which can only have a single dentry
alias to an
inode but not at all sufficient for non-directories.

Thanks,
Amir.

Re: [PATCH v12 02/17] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Amir Goldstein]

On Tue, Mar 6, 2018 at 10:53 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> By default metadata only copy up is disabled. Provide a mount option so
> that users can choose one way or other.
>
> Also provide a kernel config and module option to enable/disable
> metacopy feature.
>
> metacopy feature requires redirect_dir=on when upper is present. Otherwise,
> it requires redirect_dir=follow atleast.
>
> Like index feature, we verify on mount that upper root is not being
> reused with a different lower root. This hopes to get the configuration
> right and detect the copied layers use case. But this does only so
> much as we don't verify all the lowers. So it is possible that a lower is
> missing and later data copy up fails.
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  Documentation/filesystems/overlayfs.txt | 30 ++++++++++++++++++++++++-
>  fs/overlayfs/Kconfig                    | 17 ++++++++++++++
>  fs/overlayfs/ovl_entry.h                |  1 +
>  fs/overlayfs/super.c                    | 40 ++++++++++++++++++++++++++++++++-
>  4 files changed, 86 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> index 6ea1e64d1464..b7720e61973c 100644
> --- a/Documentation/filesystems/overlayfs.txt
> +++ b/Documentation/filesystems/overlayfs.txt
> @@ -249,6 +249,30 @@ rightmost one and going left.  In the above example lower1 will be the
>  top, lower2 the middle and lower3 the bottom layer.
>
>
> +Metadata only copyup
> +--------------------
> +
> +When metadata only copy up feature is enabled, overlayfs will only copy
> +up metadata (as opposed to whole file), when a metadata specific operation
> +like chown/chmod is performed. Full file will be copied up later when
> +file is opened for WRITE operation.
> +
> +IOW, this is delayed data copy up operation and data is copied up when
> +there is a need to actually modify data.
> +
> +There are multiple ways to enable/disable this feature. A config option
> +CONFIG_OVERLAY_FS_METACOPY can be set/unset to enable/disable this feature
> +by default. Or one can enable/disable it at module load time with module
> +parameter metacopy=on/off. Lastly, there is also a per mount option
> +metacopy=on/off to enable/disable this feature per mount.
> +
> +Do not use metacopy=on with untrusted upper/lower directories. Otherwise
> +it is possible that an attacker can create an handcrafted file with
> +appropriate REDIRECT and METACOPY xattrs, and gain access to file on lower
> +pointed by REDIRECT. This should not be possible on local system as setting
> +"trusted." xattrs will require CAP_SYS_ADMIN. But it should be possible
> +for untrusted layers like from a pen drive.
> +
>  Sharing and copying layers
>  --------------------------
>
> @@ -267,7 +291,7 @@ though it will not result in a crash or deadlock.
>  Mounting an overlay using an upper layer path, where the upper layer path
>  was previously used by another mounted overlay in combination with a
>  different lower layer path, is allowed, unless the "inodes index" feature
> -is enabled.
> +or "metadata only copyup" feature is enabled.
>
>  With the "inodes index" feature, on the first time mount, an NFS file
>  handle of the lower layer root directory, along with the UUID of the lower
> @@ -280,6 +304,10 @@ lower root origin, mount will fail with ESTALE.  An overlayfs mount with
>  does not support NFS export, lower filesystem does not have a valid UUID or
>  if the upper filesystem does not support extended attributes.
>
> +For "metadata only copyup" feature there is no verification mechanism at
> +mount time. So if same upper is mouted with different set of lower, mount
> +probably will succeed but expect the unexpected later on. So don't do it.
> +
>  It is quite a common practice to copy overlay layers to a different
>  directory tree on the same or different underlying filesystem, and even
>  to a different machine.  With the "inodes index" feature, trying to mount
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index 406e72de88f6..e1974db486a9 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -72,3 +72,20 @@ config OVERLAY_FS_NFS_EXPORT
>           Note, that the NFS export feature is not backward compatible.
>           That is, mounting an overlay which has a full index on a kernel
>           that doesn't support this feature will have unexpected results.
> +
> +config OVERLAY_FS_METACOPY
> +       bool "Overlayfs: turn on metadata only copy up feature by default"
> +       depends on OVERLAY_FS
> +       depends on !OVERLAY_FS_NFS_EXPORT
> +       select OVERLAY_FS_REDIRECT_DIR
> +       help
> +         If this config option is enabled then overlay filesystems will
> +         copy up only metadata where appropriate and data copy up will
> +         happen when a file is opended for WRITE operation. It is still
> +         possible to turn off this feature globally with the "metacopy=off"
> +         module option or on a filesystem instance basis with the
> +         "metacopy=off" mount option.
> +
> +         Note, that this feature is not backward compatible.  That is,
> +         mounting an overlay which has metacopy only inodes on a kernel
> +         that doesn't support this feature will have unexpected results.
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index bfef6edcc111..7dc55628080d 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -18,6 +18,7 @@ struct ovl_config {
>         const char *redirect_mode;
>         bool index;
>         bool nfs_export;
> +       bool metacopy;
>  };
>
>  struct ovl_layer {
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 9ee37c76091d..0e124b9d902d 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -58,6 +58,11 @@ static void ovl_entry_stack_free(struct ovl_entry *oe)
>                 dput(oe->lowerstack[i].dentry);
>  }
>
> +static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
> +module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
> +MODULE_PARM_DESC(ovl_metacopy_def,
> +                "Default to on or off for the metadata only copy up feature");
> +
>  static void ovl_dentry_release(struct dentry *dentry)
>  {
>         struct ovl_entry *oe = dentry->d_fsdata;
> @@ -350,6 +355,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>         if (ofs->config.nfs_export != ovl_nfs_export_def)
>                 seq_printf(m, ",nfs_export=%s", ofs->config.nfs_export ?
>                                                 "on" : "off");
> +       if (ofs->config.metacopy != ovl_metacopy_def)
> +               seq_printf(m, ",metacopy=%s",
> +                          ofs->config.metacopy ? "on" : "off");
>         return 0;
>  }
>
> @@ -384,6 +392,8 @@ enum {
>         OPT_INDEX_OFF,
>         OPT_NFS_EXPORT_ON,
>         OPT_NFS_EXPORT_OFF,
> +       OPT_METACOPY_ON,
> +       OPT_METACOPY_OFF,
>         OPT_ERR,
>  };
>
> @@ -397,6 +407,8 @@ static const match_table_t ovl_tokens = {
>         {OPT_INDEX_OFF,                 "index=off"},
>         {OPT_NFS_EXPORT_ON,             "nfs_export=on"},
>         {OPT_NFS_EXPORT_OFF,            "nfs_export=off"},
> +       {OPT_METACOPY_ON,               "metacopy=on"},
> +       {OPT_METACOPY_OFF,              "metacopy=off"},
>         {OPT_ERR,                       NULL}
>  };
>
> @@ -511,6 +523,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>                         config->nfs_export = false;
>                         break;
>
> +               case OPT_METACOPY_ON:
> +                       config->metacopy = true;
> +                       break;
> +
> +               case OPT_METACOPY_OFF:
> +                       config->metacopy = false;
> +                       break;
> +
>                 default:
>                         pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
>                         return -EINVAL;
> @@ -993,7 +1013,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.metacopy = false;
> +               pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off and metacopy=off.\n");
>                 err = 0;
>         } else {
>                 vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
> @@ -1012,6 +1033,11 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
>                 ofs->config.nfs_export = false;
>         }
>
> +       /* metacopy feature with upper requires redirect_dir=on */
> +       if (ofs->config.metacopy && !ofs->config.redirect_dir) {
> +               pr_warn("overlayfs: metadata only copyup requires \"redirect_dir=on\", falling back to metacopy=off.\n");
> +               ofs->config.metacopy = false;
> +       }
>  out:
>         mnt_drop_write(mnt);
>         return err;
> @@ -1188,6 +1214,12 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>                 ofs->config.nfs_export = false;
>         }
>
> +       if (!ofs->config.upperdir && ofs->config.metacopy &&
> +           !ofs->config.redirect_follow) {
> +               ofs->config.metacopy = false;
> +               pr_warn("overlayfs: metadata only copyup requires \"redirect_dir=follow\" on non-upper mount, falling back to metacopy=off.\n");
> +       }
> +
>         err = -ENOMEM;
>         stack = kcalloc(stacklen, sizeof(struct path), GFP_KERNEL);
>         if (!stack)
> @@ -1263,6 +1295,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>
>         ofs->config.index = ovl_index_def;
>         ofs->config.nfs_export = ovl_nfs_export_def;
> +       ofs->config.metacopy = ovl_metacopy_def;
>         err = ovl_parse_opt((char *) data, &ofs->config);
>         if (err)
>                 goto out_err;
> @@ -1331,6 +1364,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>                 }
>         }
>
> +       if (ofs->config.metacopy && ofs->config.nfs_export) {
> +               pr_warn("overlayfs: Metadata copy up requires NFS export disabled, falling back to metacopy=off.\n");
> +               ofs->config.metacopy = false;
> +       }
> +

Is should probably be the other way around - metacopy should prevail
and we should fall back to nfs_export=off (just like non-upper mount
with redirect_follow)
it's rather user can access file data then export to NFS.

If user prefers to risk not being able to access file data in favor of
NFS export, then
user should opt-in with metacopy=off, just like user needs to opt-in
to redirect_dir=nofollow
in order to NFS export a non-upper overlayfs.

Thanks,
Amir.

Re: [PATCH v12 13/17] ovl: Check redirects for metacopy files

[Amir Goldstein]

On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Right now we rely on path based lookup for data origin of metacopy upper.
> This will work only if upper has not been renamed. We solved this problem
> already for merged directories using redirect. Use same logic for metacopy
> files.
>
> This patch just goes on to check redirects for metacopy files.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/namei.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 220e754c974b..a4a5c5f5540d 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -265,22 +265,22 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
>                 goto put_and_out;
>         }
>         if (!d_can_lookup(this)) {
> -               if (d->is_dir)
> -                       goto put_and_out;
> +               d->is_dir = false;

That test was supposed to catch non-dir under dir from an upper layer
but you are loosing the information stored in d->dir.

>                 err = ovl_check_metacopy_xattr(this);
>                 if (err < 0)
>                         goto out_err;
>                 if (!err) {
>                         d->stop = true;
>                         d->metacopy = false;
> +                       goto out;
>                 } else
>                         d->metacopy = true;
> -               goto out;
> -       }
> -       d->is_dir = true;
> -       if (!d->last && ovl_is_opaquedir(this)) {
> -               d->stop = d->opaque = true;
> -               goto out;
> +       } else {
> +               d->is_dir = true;
> +               if (!d->last && ovl_is_opaquedir(this)) {
> +                       d->stop = d->opaque = true;
> +                       goto out;

I think there is a bug here - not related to your change, but semi related
to your recent fix patch (patch 1 in this series).

d->last is set to true when lookup in parent poe->numlower layer,
but parent may be pure upper for example and redirect from child can still
continue lookup to lower layers. If a directory is marked both "redirect" and
"opaque" (which is an inconsistency). In that case, d->last will be true
and opaque xattr will not be checked, but redirect will be checked.

Since AFAIK d->last is an optimization, I think it could be relaxed to
lower.layer->idx == roe->numlower - 1
and then for the d->last case, we can skip both opaque and redirect checks
and skip redirect check for both directory and metadata.


> +               }
>         }
>         err = ovl_check_redirect(this, d, prelen, post);
>         if (err)

Thanks,
Amir.

Re: [PATCH v12 09/17] ovl: Do not mark a non dir as _OVL_PATH_MERGE in ovl_path_type()

[Vivek Goyal]

On Wed, Mar 07, 2018 at 09:07:22AM +0200, Amir Goldstein wrote:
> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > With the addition of an origin chain for regular files, it is possible
> > that we don't have an upperdentry and oe->numlower > 1 for non-dir. So
> > mark a path __OVL_TYPE_MERGE only if it is a directory.
> >
> 
> Okay, but what's wrong with marking a non-dir as TYPE MERGE?
> It is, after all, a sort of merged entry.

Conceptually, I can't think of anything wrong. Just that currently we use
MERGE in the context of directory and we don't need it in the context of
non-dir. So this is just trying to be safe to make sure not to break any
existing code. 

I guess we could mark metacopy regular files as MERGE as well, if need be.
But that could be part of a separate patch series where we could run
various tests and make sure nothing is broken.

Vivek

> 
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/util.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 7929cc872df6..19b936b0a551 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -122,7 +122,7 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
> >                                 type |= __OVL_PATH_MERGE;
> >                 }
> >         } else {
> > -               if (oe->numlower > 1)
> > +               if (oe->numlower > 1 && d_is_dir(dentry))
> >                         type |= __OVL_PATH_MERGE;
> >         }
> >         return type;
> > --
> > 2.13.6
> >

Re: [PATCH v12 12/17] ovl: Do not expose metacopy only upper dentry from d_real()

[Vivek Goyal]

On Wed, Mar 07, 2018 at 09:15:40AM +0200, Amir Goldstein wrote:
> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > d_real() can make a upper metacopy dentry/inode visible to the vfs layer.
> > This is something new and vfs layer does not know that this inode contains
> > only metadata and not data. And this could break things.
> >
> > So to be safe, do not expose metacopy only dentry/inode to vfs using
> > d_real().
> >
> > IOW, d_real() will not reuturn metacopy dentry. Instead, it will return
> > dentry corresponding lower dentry/inode which has file data.
> >
> > For regular d_real() call (inode == NULL, D_REAL_UPPER not set), if upper
> > dentry inode is metacopy only and does not have data, return lower dentry.
> >
> > If d_real() is called with flag D_REAL_UPPER, return upper dentry only if
> > it has data (flag OVL_UPPERDATA is set).
> >
> > Similiarly, if d_real(inode=X) is called, a warning is emitted if returned
> > dentry/inode does not have OVL_UPPERDATA set. This should not happen as
> > we never made this metacopy inode visible to vfs so nobody should be
> > calling overlayfs back with inode=metacopy_inode.
> >
> > I scanned the code and I don't think it breaks any of the existing code.
> > There are two users of D_REAL_UPPER. may_write_real() and
> > update_ovl_inode_times().
> >
> > may_write_real(), will get an NULL dentry if upper inode is metacopy only
> > and it will return -EPERM. Effectively, we are disallowing modifications
> > to metacopy only inode from this interface. Though there is opportunity
> > to improve it. (Allow chattr on metacopy inodes).
> >
> > update_ovl_inode_times() gets inode mtime and ctime from real inode. It
> > should not be broken for metacopy inode as well for following reasons.
> >
> > - For any metadata operations (setattr, acl etc), overlay always calls
> >   ovl_copyattr() and updates ovl inode mtime and ctime. So there is no
> >   need to update mtime and ctime in this case. Its already updated, hence
> >   even if d_real(D_REAL_UPPER) returns nil, it should be fine.
> >
> > - For metadata inode, mtime should be same as lower and not change. (data
> >   can't be modified on metadata inode without copyup). IOW, mtime of
> >   ovl dentry should be same as mtime of underlying metadata inode on upper
> >   always. So there is no need to update it.
> >
> > - For file writes, ctime and mtime will be updated. But in that case
> >   first data will be copied up and this will not be a metadata inode
> >   anymore. And furthr call to d_real(D_REAL_UPPER) will return upper
> >   inode and new mtime and ctime will be obtainable.
> >
> > So atime updates should work just fine for metacopy inodes. I think only
> > corner case is if somehow underlying filesystem changes ctime of upper
> > metadata inode without overlay knowing about it. Not sure how that
> > can happen. If somehow is affected by that, then we probably can implement
> > another flag which will allow caller to get metacopy inode as well.
> > Something like d_real(D_REAL_UPPER | D_METACOPY). And that should solve
> > this issue.
> >
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/overlayfs.h |  1 +
> >  fs/overlayfs/super.c     | 21 +++++++++++++++++----
> >  fs/overlayfs/util.c      |  8 ++++++++
> >  3 files changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index 2d682923252e..24725b6668b9 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -225,6 +225,7 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
> >  enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
> >  struct dentry *ovl_dentry_upper(struct dentry *dentry);
> >  struct dentry *ovl_dentry_lower(struct dentry *dentry);
> > +struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
> >  struct dentry *ovl_dentry_real(struct dentry *dentry);
> >  struct dentry *ovl_i_dentry_upper(struct inode *inode);
> >  struct inode *ovl_inode_upper(struct inode *inode);
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index d3dbdd695722..4be4e47cbf57 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -96,8 +96,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
> >         struct dentry *real;
> >         int err;
> >
> > -       if (flags & D_REAL_UPPER)
> > -               return ovl_dentry_upper(dentry);
> > +       if (flags & D_REAL_UPPER) {
> > +               real = ovl_dentry_upper(dentry);
> > +               if (!real)
> > +                       return NULL;
> > +               if (!ovl_has_upperdata(dentry))
> > +                       return NULL;
> > +               return real;
> > +       }
> >
> >         if (!d_is_reg(dentry)) {
> >                 if (!inode || inode == d_inode(dentry))
> > @@ -113,15 +119,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
> >
> >         real = ovl_dentry_upper(dentry);
> >         if (real && (!inode || inode == d_inode(real))) {
> > +               bool metacopy = !ovl_has_upperdata(dentry);
> >                 if (!inode) {
> >                         err = ovl_check_append_only(d_inode(real), open_flags);
> >                         if (err)
> >                                 return ERR_PTR(err);
> > -               }
> > +
> > +                       if (unlikely(metacopy))
> > +                               goto lower;
> > +               } else if (unlikely(metacopy))
> > +                       goto bug;
> > +
> >                 return real;
> >         }
> >
> > -       real = ovl_dentry_lower(dentry);
> > +lower:
> > +       real = ovl_dentry_lowerdata(dentry);
> >         if (!real)
> >                 goto bug;
> >
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 274bbfc855e0..36d41f7001e3 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -186,6 +186,14 @@ struct dentry *ovl_dentry_lower(struct dentry *dentry)
> >         return oe->numlower ? oe->lowerstack[0].dentry : NULL;
> >  }
> >
> > +struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
> > +{
> > +       struct ovl_entry *oe = dentry->d_fsdata;
> > +       int idx = oe->numlower - 1;
> > +
> > +       return oe->lowerstack[idx].dentry;
> > +}
> > +
> 
> This new change is not in line with the subject line.
> Either change the commit message to fit or better split this
> small change to a new patch because the commit message is long
> enough as it is.

Ok, I will move this helper in a separate patch before this patch.

Vivek

Re: [PATCH v12 10/17] ovl: Copy up meta inode data from lowest data inode

[Vivek Goyal]

On Wed, Mar 07, 2018 at 09:19:07AM +0200, Amir Goldstein wrote:

[..]
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index b48bd73786f1..d999813f9aeb 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -221,6 +221,7 @@ bool ovl_dentry_weird(struct dentry *dentry);
> >  enum ovl_path_type ovl_path_type(struct dentry *dentry);
> >  void ovl_path_upper(struct dentry *dentry, struct path *path);
> >  void ovl_path_lower(struct dentry *dentry, struct path *path);
> > +void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
> >  enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
> >  struct dentry *ovl_dentry_upper(struct dentry *dentry);
> >  struct dentry *ovl_dentry_lower(struct dentry *dentry);
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 19b936b0a551..22df74cb1def 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -148,6 +148,20 @@ void ovl_path_lower(struct dentry *dentry, struct path *path)
> >         }
> >  }
> >
> 
> A comment here why  ovl_path_lowerdata is needed would be nice.

Ok, will add a comment.

Vivek

Re: [PATCH v12 09/17] ovl: Do not mark a non dir as _OVL_PATH_MERGE in ovl_path_type()

[Amir Goldstein]

On Wed, Mar 7, 2018 at 3:21 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Mar 07, 2018 at 09:07:22AM +0200, Amir Goldstein wrote:
>> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > With the addition of an origin chain for regular files, it is possible
>> > that we don't have an upperdentry and oe->numlower > 1 for non-dir. So
>> > mark a path __OVL_TYPE_MERGE only if it is a directory.
>> >
>>
>> Okay, but what's wrong with marking a non-dir as TYPE MERGE?
>> It is, after all, a sort of merged entry.
>
> Conceptually, I can't think of anything wrong. Just that currently we use
> MERGE in the context of directory and we don't need it in the context of
> non-dir. So this is just trying to be safe to make sure not to break any
> existing code.
>
> I guess we could mark metacopy regular files as MERGE as well, if need be.
> But that could be part of a separate patch series where we could run
> various tests and make sure nothing is broken.
>

I did a quick scan on all uses of OVL_TYPE_MERGE() they
all have is_dir check in front of them or checked on a parent object.

In fact, in the case of ovl_rename(), for directory, the test is
   if (ovl_type_merge_or_lower(old))
      err = ovl_set_redirect(old, samedir);


You could have removed the is_dir condition before this code instead
of adding a different case for is_metacopy, although you did use a different
argument for samedir in the metacopy case...

Thanks,
Amir.

Re: [PATCH v12 12/17] ovl: Do not expose metacopy only upper dentry from d_real()

[Amir Goldstein]

On Wed, Mar 7, 2018 at 3:29 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Mar 07, 2018 at 09:15:40AM +0200, Amir Goldstein wrote:
>> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > d_real() can make a upper metacopy dentry/inode visible to the vfs layer.
>> > This is something new and vfs layer does not know that this inode contains
>> > only metadata and not data. And this could break things.
>> >
>> > So to be safe, do not expose metacopy only dentry/inode to vfs using
>> > d_real().
>> >
>> > IOW, d_real() will not reuturn metacopy dentry. Instead, it will return
>> > dentry corresponding lower dentry/inode which has file data.
>> >
>> > For regular d_real() call (inode == NULL, D_REAL_UPPER not set), if upper
>> > dentry inode is metacopy only and does not have data, return lower dentry.
>> >
>> > If d_real() is called with flag D_REAL_UPPER, return upper dentry only if
>> > it has data (flag OVL_UPPERDATA is set).
>> >
>> > Similiarly, if d_real(inode=X) is called, a warning is emitted if returned
>> > dentry/inode does not have OVL_UPPERDATA set. This should not happen as
>> > we never made this metacopy inode visible to vfs so nobody should be
>> > calling overlayfs back with inode=metacopy_inode.
>> >
>> > I scanned the code and I don't think it breaks any of the existing code.
>> > There are two users of D_REAL_UPPER. may_write_real() and
>> > update_ovl_inode_times().
>> >
>> > may_write_real(), will get an NULL dentry if upper inode is metacopy only
>> > and it will return -EPERM. Effectively, we are disallowing modifications
>> > to metacopy only inode from this interface. Though there is opportunity
>> > to improve it. (Allow chattr on metacopy inodes).
>> >
>> > update_ovl_inode_times() gets inode mtime and ctime from real inode. It
>> > should not be broken for metacopy inode as well for following reasons.
>> >
>> > - For any metadata operations (setattr, acl etc), overlay always calls
>> >   ovl_copyattr() and updates ovl inode mtime and ctime. So there is no
>> >   need to update mtime and ctime in this case. Its already updated, hence
>> >   even if d_real(D_REAL_UPPER) returns nil, it should be fine.
>> >
>> > - For metadata inode, mtime should be same as lower and not change. (data
>> >   can't be modified on metadata inode without copyup). IOW, mtime of
>> >   ovl dentry should be same as mtime of underlying metadata inode on upper
>> >   always. So there is no need to update it.
>> >
>> > - For file writes, ctime and mtime will be updated. But in that case
>> >   first data will be copied up and this will not be a metadata inode
>> >   anymore. And furthr call to d_real(D_REAL_UPPER) will return upper
>> >   inode and new mtime and ctime will be obtainable.
>> >
>> > So atime updates should work just fine for metacopy inodes. I think only
>> > corner case is if somehow underlying filesystem changes ctime of upper
>> > metadata inode without overlay knowing about it. Not sure how that
>> > can happen. If somehow is affected by that, then we probably can implement
>> > another flag which will allow caller to get metacopy inode as well.
>> > Something like d_real(D_REAL_UPPER | D_METACOPY). And that should solve
>> > this issue.
>> >
>> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > ---
>> >  fs/overlayfs/overlayfs.h |  1 +
>> >  fs/overlayfs/super.c     | 21 +++++++++++++++++----
>> >  fs/overlayfs/util.c      |  8 ++++++++
>> >  3 files changed, 26 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>> > index 2d682923252e..24725b6668b9 100644
>> > --- a/fs/overlayfs/overlayfs.h
>> > +++ b/fs/overlayfs/overlayfs.h
>> > @@ -225,6 +225,7 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
>> >  enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
>> >  struct dentry *ovl_dentry_upper(struct dentry *dentry);
>> >  struct dentry *ovl_dentry_lower(struct dentry *dentry);
>> > +struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
>> >  struct dentry *ovl_dentry_real(struct dentry *dentry);
>> >  struct dentry *ovl_i_dentry_upper(struct inode *inode);
>> >  struct inode *ovl_inode_upper(struct inode *inode);
>> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> > index d3dbdd695722..4be4e47cbf57 100644
>> > --- a/fs/overlayfs/super.c
>> > +++ b/fs/overlayfs/super.c
>> > @@ -96,8 +96,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>> >         struct dentry *real;
>> >         int err;
>> >
>> > -       if (flags & D_REAL_UPPER)
>> > -               return ovl_dentry_upper(dentry);
>> > +       if (flags & D_REAL_UPPER) {
>> > +               real = ovl_dentry_upper(dentry);
>> > +               if (!real)
>> > +                       return NULL;
>> > +               if (!ovl_has_upperdata(dentry))
>> > +                       return NULL;
>> > +               return real;
>> > +       }
>> >
>> >         if (!d_is_reg(dentry)) {
>> >                 if (!inode || inode == d_inode(dentry))
>> > @@ -113,15 +119,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>> >
>> >         real = ovl_dentry_upper(dentry);
>> >         if (real && (!inode || inode == d_inode(real))) {
>> > +               bool metacopy = !ovl_has_upperdata(dentry);
>> >                 if (!inode) {
>> >                         err = ovl_check_append_only(d_inode(real), open_flags);
>> >                         if (err)
>> >                                 return ERR_PTR(err);
>> > -               }
>> > +
>> > +                       if (unlikely(metacopy))
>> > +                               goto lower;
>> > +               } else if (unlikely(metacopy))
>> > +                       goto bug;
>> > +
>> >                 return real;
>> >         }
>> >
>> > -       real = ovl_dentry_lower(dentry);
>> > +lower:
>> > +       real = ovl_dentry_lowerdata(dentry);
>> >         if (!real)
>> >                 goto bug;
>> >
>> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
>> > index 274bbfc855e0..36d41f7001e3 100644
>> > --- a/fs/overlayfs/util.c
>> > +++ b/fs/overlayfs/util.c
>> > @@ -186,6 +186,14 @@ struct dentry *ovl_dentry_lower(struct dentry *dentry)
>> >         return oe->numlower ? oe->lowerstack[0].dentry : NULL;
>> >  }
>> >
>> > +struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
>> > +{
>> > +       struct ovl_entry *oe = dentry->d_fsdata;
>> > +       int idx = oe->numlower - 1;
>> > +
>> > +       return oe->lowerstack[idx].dentry;
>> > +}
>> > +
>>
>> This new change is not in line with the subject line.
>> Either change the commit message to fit or better split this
>> small change to a new patch because the commit message is long
>> enough as it is.
>
> Ok, I will move this helper in a separate patch before this patch.
>

It's not just the helper. The subject says "Don't expose metacopy upper"
but this helper is used to "not expose metacopy lower", so either amend
the commit message or fix exposing metacopy lower in a separate patch.

Thanks,
Amir.

Re: [PATCH v12 08/17] ovl: Modify ovl_lookup() and friends to lookup metacopy dentry

[Amir Goldstein]

On Tue, Mar 6, 2018 at 10:53 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> This patch modifies ovl_lookup() and friends to lookup metacopy dentries.
> It also allows for presence of metacopy dentries in lower layer.
>
> During lookup, check for presence of OVL_XATTR_METACOPY and if not present,
> set OVL_UPPERDATA bit in flags.
>
> OVL_UPPERDATA flag is set unconditionally if upper inode exists.

You mean if *only* upper inode exists?

>
> Do not follow metacopy origin if we find a metacopy only inode and metacopy
> feature is not enabled for that mount. Like redirect, this can have security
> implications where an attacker could hand craft upper and try to gain
> access to file on lower which it should not have to begin with.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/export.c |   3 ++
>  fs/overlayfs/namei.c  | 121 ++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 115 insertions(+), 9 deletions(-)
>
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index bb94ce9da5c8..35f2d4eb0d7e 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -192,6 +192,9 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
>         if (index)
>                 ovl_set_flag(OVL_INDEX, inode);
>
> +       if (upper)
> +               ovl_set_flag(OVL_UPPERDATA, inode);
> +

FYI, in this function upper may be a disconnected dentry, so
for future metacopy+nfs_export support you will need to make sure that
either:
- We can always get datalower from upper by origin and not by path
- We set absolute path redirect on encode of non-indexed metacopy upper
- If metacopy upper is indexed by the lowerdata inode then all is good,
  because we find the lowerdata by file handle and upper metacopy by index,
  just need to  ovl_check_metacopy_xattr(upper) before setting upperdata
  here above

Lower layer metacopies are going to be a problem with nfs export.
The only way to solve it would be to follow to lowerdata by origin.

>         dentry = d_find_any_alias(inode);
>         if (!dentry) {
>                 dentry = d_alloc_anon(inode->i_sb);
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 70fcfcc684cc..220e754c974b 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -24,6 +24,7 @@ struct ovl_lookup_data {
>         bool stop;
>         bool last;
>         char *redirect;
> +       bool metacopy;
>  };
>
>  static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
> @@ -208,6 +209,28 @@ struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt)
>         return real;
>  }
>
> +/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
> +static int ovl_check_metacopy_xattr(struct dentry *dentry)
> +{
> +       int res;
> +
> +       /* Only regular files can have metacopy xattr */
> +       if (!S_ISREG(d_inode(dentry)->i_mode))
> +               return 0;
> +
> +       res = vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
> +       if (res < 0) {
> +               if (res == -ENODATA || res == -EOPNOTSUPP)
> +                       return 0;
> +               goto out;
> +       }
> +
> +       return 1;
> +out:
> +       pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
> +       return res;
> +}
> +
>  static bool ovl_is_opaquedir(struct dentry *dentry)
>  {
>         return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);
> @@ -242,9 +265,16 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
>                 goto put_and_out;
>         }
>         if (!d_can_lookup(this)) {
> -               d->stop = true;
>                 if (d->is_dir)

You need to set d->stop here because this is a non-dir below upper dir.

>                         goto put_and_out;
> +               err = ovl_check_metacopy_xattr(this);
> +               if (err < 0)
> +                       goto out_err;
> +               if (!err) {
> +                       d->stop = true;
> +                       d->metacopy = false;
> +               } else
> +                       d->metacopy = true;

Need to have {} in both if and else, but better not use if at all:
d->stop = !err;
d->metacopy = !!err;


>                 goto out;
>         }
>         d->is_dir = true;
> @@ -799,7 +829,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
>         struct ovl_entry *poe = dentry->d_parent->d_fsdata;
>         struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
> -       struct ovl_path *stack = NULL;
> +       struct ovl_path *stack = NULL, *origin_path = NULL;
>         struct dentry *upperdir, *upperdentry = NULL;
>         struct dentry *origin = NULL;
>         struct dentry *index = NULL;
> @@ -810,6 +840,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         struct dentry *this;
>         unsigned int i;
>         int err;
> +       bool metacopy = false;
>         struct ovl_lookup_data d = {
>                 .name = dentry->d_name,
>                 .is_dir = false,
> @@ -817,6 +848,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 .stop = false,
>                 .last = !poe->numlower,
>                 .redirect = NULL,
> +               .metacopy = false,
>         };
>
>         if (dentry->d_name.len > ofs->namelen)
> @@ -835,7 +867,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         goto out;
>                 }
>                 if (upperdentry && !d.is_dir) {
> -                       BUG_ON(!d.stop || d.redirect);
> +                       unsigned int origin_ctr = 0;
> +                       BUG_ON(d.redirect);
>                         /*
>                          * Lookup copy up origin by decoding origin file handle.
>                          * We may get a disconnected dentry, which is fine,
> @@ -846,16 +879,20 @@ 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(ofs, upperdentry, &stack, &ctr);
> +                       err = ovl_check_origin(ofs, upperdentry, &origin_path,
> +                                              &origin_ctr);
>                         if (err)
>                                 goto out_put_upper;
> +
> +                       if (d.metacopy)
> +                               metacopy = true;
>                 }
>
>                 if (d.redirect) {
>                         err = -ENOMEM;
>                         upperredirect = kstrdup(d.redirect, GFP_KERNEL);
>                         if (!upperredirect)
> -                               goto out_put_upper;
> +                               goto out_put_origin;
>                         if (d.redirect[0] == '/')
>                                 poe = roe;
>                 }
> @@ -867,7 +904,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 stack = kcalloc(ofs->numlower, sizeof(struct ovl_path),
>                                 GFP_KERNEL);
>                 if (!stack)
> -                       goto out_put_upper;
> +                       goto out_put_origin;
>         }
>
>         for (i = 0; !d.stop && i < poe->numlower; i++) {
> @@ -885,7 +922,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                  * If no origin fh is stored in upper of a merge dir, store fh
>                  * of lower dir and set upper parent "impure".
>                  */
> -               if (upperdentry && !ctr && !ofs->noxattr) {
> +               if (upperdentry && !ctr && !ofs->noxattr && d.is_dir) {
>                         err = ovl_fix_origin(dentry, this, upperdentry);
>                         if (err) {
>                                 dput(this);
> @@ -898,7 +935,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                  * lower 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_lower(dentry->d_sb)) {
> +               if (upperdentry && !ctr && d.is_dir &&
> +                   ovl_verify_lower(dentry->d_sb)) {
>                         err = ovl_verify_origin(upperdentry, this, false);
>                         if (err) {
>                                 dput(this);
> @@ -909,6 +947,29 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         origin = this;
>                 }
>
> +               /*
> +                * For non-dir dentry, make sure dentry found by lookup
> +                * matches the origin stored in upper
> +                */
> +               if (!d.is_dir && upperdentry && !ctr && origin_path) {
> +                       err = ovl_verify_origin(upperdentry, this, false);
> +                       if (err) {
> +                               dput(this);
> +                               goto out_put;
> +                       }
> +               }
> +

Why is this code duplicated from d.is_dir case?


> +               if (d.metacopy)
> +                       metacopy = true;
> +               /*
> +                * Do not store intermediate metacopy dentries in chain,
> +                * except top most lower metacopy dentry
> +                */
> +               if (d.metacopy && ctr) {
> +                       dput(this);
> +                       continue;
> +               }
> +
>                 stack[ctr].dentry = this;
>                 stack[ctr].layer = lower.layer;
>                 ctr++;
> @@ -940,6 +1001,34 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 }
>         }
>
> +       if (metacopy) {
> +               BUG_ON(d.is_dir);
> +               /*
> +                * Found a metacopy dentry but did not find corresponding
> +                * data dentry
> +                */
> +               if (d.metacopy) {
> +                       err = -ESTALE;

Set err = -ESTALE before if (d.metacopy).
I think this case deserves a warning as well.

> +                       goto out_put;
> +               }
> +
> +               err = -EPERM;
> +               if (!ofs->config.metacopy) {
> +                       pr_warn_ratelimited("overlay: refusing to follow"
> +                                           " metacopy origin for (%pd2)\n",
> +                                           dentry);
> +                       goto out_put;
> +               }
> +       } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
> +               if (WARN_ON(stack != NULL)) {
> +                       err = -EIO;
> +                       goto out_put;
> +               }
> +               stack = origin_path;
> +               ctr = 1;
> +               origin_path = NULL;

I am having a hard time understanding why origin_path is needed at all
and why not lookup only by path and then ovl_verify_origin().
The only case I can think of where following by origin is required if for
decoding a non-indexed upper file handle which is also a metacopy,
but that has nothing to do with ovl_lookup().

Please enlighten me.

> +       }
> +
>         /*
>          * Lookup index by lower inode and verify it matches upper inode.
>          * We only trust dir index if we verified that lower dir matches
> @@ -972,8 +1061,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>
>         if (upperdentry)
>                 ovl_dentry_set_upper_alias(dentry);
> -       else if (index)
> +       else if (index) {
>                 upperdentry = dget(index);
> +               metacopy = ovl_check_metacopy_xattr(index);
> +       }

{} should be on both if and else statements.


Thanks,
Amir.

Re: [PATCH v12 14/17] ovl: Set redirect on metacopy files upon rename

[Vivek Goyal]

On Wed, Mar 07, 2018 at 09:48:22AM +0200, Amir Goldstein wrote:
> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Set redirect on metacopy files upon rename. This will help find data dentry
> > in lower dirs.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/dir.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index cdeae4bdbfce..b955f6fede06 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -1048,9 +1048,11 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
> >                         err = ovl_set_redirect(old, samedir);
> >                 else if (!old_opaque && ovl_type_merge(new->d_parent))
> >                         err = ovl_set_opaque_xerr(old, olddentry, -EXDEV);
> > -               if (err)
> > -                       goto out_dput;
> > -       }
> > +       } else if (ovl_is_metacopy_dentry(old))
> > +                       err = ovl_set_redirect(old, false);
> 
> You should use {} in the else statement as well.

ok.

> 
> Q: why do you set samedir: = false here?
> A: because other hardlink aliasses cannot follow a relative redirect, right?

Right. If we create a hardlink later then it will need absolute redirect
if both dentries are not in same dir.

> 
> This is a subtle detail that should be documented,

Ok, will do.

> but also
> maybe do use samedir if nlink == 1?

Hmm.., so initially we could put a relative redirct (if nlink=1) and later
if we create a link, we could replace relative redirect with an absolute
redirect? I see we already have logic to do that for the case of rename.

Now only thing I need to figure out in ovl_link() whethre two dentries
are in same dir or not. I am assuming I can just check parent dentry
pointers and see if these two have same parent or not.

In fact, we probably don't even have to check for nlink=1. Only when
we create a upper hard link, then we need to make sure we replace relative
hardlink with absolute one. I will play with it and see how it goes.

Vivek

> 
> > +       if (err)
> > +               goto out_dput;
> > +
> >         if (!overwrite && new_is_dir) {
> >                 if (ovl_type_merge_or_lower(new))
> >                         err = ovl_set_redirect(new, samedir);
> > @@ -1058,7 +1060,10 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
> >                         err = ovl_set_opaque_xerr(new, newdentry, -EXDEV);
> >                 if (err)
> >                         goto out_dput;
> > -       }
> > +       } else if (!overwrite && ovl_is_metacopy_dentry(new))
> > +                       err = ovl_set_redirect(new, false);
> 
> Same here for both comments.
> 
> Thanks,
> Amir.

Re: [PATCH v12 16/17] ovl: Set redirect on upper inode when it is linked

[Vivek Goyal]

On Wed, Mar 07, 2018 at 10:02:20AM +0200, Amir Goldstein wrote:
> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > When we create a hardlink to a metacopy upper file, first the redirect
> > on that inode. Path based lookup will not work with newly created link
> > and redirect will solve that issue.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/dir.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index b955f6fede06..da5c4b8ee919 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -24,6 +24,8 @@ module_param_named(redirect_max, ovl_redirect_max, ushort, 0644);
> >  MODULE_PARM_DESC(ovl_redirect_max,
> >                  "Maximum length of absolute redirect xattr value");
> >
> > +static int ovl_set_redirect(struct dentry *dentry, bool samedir);
> > +
> >  int ovl_cleanup(struct inode *wdir, struct dentry *wdentry)
> >  {
> >         int err;
> > @@ -468,6 +470,9 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> >         const struct cred *old_cred;
> >         struct cred *override_cred;
> >         struct dentry *parent = dentry->d_parent;
> > +       struct dentry *hardlink_upper;
> > +
> > +       hardlink_upper = hardlink ? ovl_dentry_upper(hardlink) : NULL;
> 
> IMO it would be nicer to pass overlay hardlink dentry all the way to
> ovl_create_real() and then just:
> 
>         if (hardlink) {
>                 err = ovl_do_link(ovl_dentry_upper(hardlink), dir,
> newdentry, debug);

Ok, I will look into it and modify if it does not turn out to be
significant change.

Vivek
> 
> Thanks,
> Amir.

Re: [PATCH v12 02/17] ovl: Provide a mount option metacopy=on/off for metadata copyup

[Vivek Goyal]

On Wed, Mar 07, 2018 at 10:47:13AM +0200, Amir Goldstein wrote:
> On Tue, Mar 6, 2018 at 10:53 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > By default metadata only copy up is disabled. Provide a mount option so
> > that users can choose one way or other.
> >
> > Also provide a kernel config and module option to enable/disable
> > metacopy feature.
> >
> > metacopy feature requires redirect_dir=on when upper is present. Otherwise,
> > it requires redirect_dir=follow atleast.
> >
> > Like index feature, we verify on mount that upper root is not being
> > reused with a different lower root. This hopes to get the configuration
> > right and detect the copied layers use case. But this does only so
> > much as we don't verify all the lowers. So it is possible that a lower is
> > missing and later data copy up fails.
> >
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  Documentation/filesystems/overlayfs.txt | 30 ++++++++++++++++++++++++-
> >  fs/overlayfs/Kconfig                    | 17 ++++++++++++++
> >  fs/overlayfs/ovl_entry.h                |  1 +
> >  fs/overlayfs/super.c                    | 40 ++++++++++++++++++++++++++++++++-
> >  4 files changed, 86 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
> > index 6ea1e64d1464..b7720e61973c 100644
> > --- a/Documentation/filesystems/overlayfs.txt
> > +++ b/Documentation/filesystems/overlayfs.txt
> > @@ -249,6 +249,30 @@ rightmost one and going left.  In the above example lower1 will be the
> >  top, lower2 the middle and lower3 the bottom layer.
> >
> >
> > +Metadata only copyup
> > +--------------------
> > +
> > +When metadata only copy up feature is enabled, overlayfs will only copy
> > +up metadata (as opposed to whole file), when a metadata specific operation
> > +like chown/chmod is performed. Full file will be copied up later when
> > +file is opened for WRITE operation.
> > +
> > +IOW, this is delayed data copy up operation and data is copied up when
> > +there is a need to actually modify data.
> > +
> > +There are multiple ways to enable/disable this feature. A config option
> > +CONFIG_OVERLAY_FS_METACOPY can be set/unset to enable/disable this feature
> > +by default. Or one can enable/disable it at module load time with module
> > +parameter metacopy=on/off. Lastly, there is also a per mount option
> > +metacopy=on/off to enable/disable this feature per mount.
> > +
> > +Do not use metacopy=on with untrusted upper/lower directories. Otherwise
> > +it is possible that an attacker can create an handcrafted file with
> > +appropriate REDIRECT and METACOPY xattrs, and gain access to file on lower
> > +pointed by REDIRECT. This should not be possible on local system as setting
> > +"trusted." xattrs will require CAP_SYS_ADMIN. But it should be possible
> > +for untrusted layers like from a pen drive.
> > +
> >  Sharing and copying layers
> >  --------------------------
> >
> > @@ -267,7 +291,7 @@ though it will not result in a crash or deadlock.
> >  Mounting an overlay using an upper layer path, where the upper layer path
> >  was previously used by another mounted overlay in combination with a
> >  different lower layer path, is allowed, unless the "inodes index" feature
> > -is enabled.
> > +or "metadata only copyup" feature is enabled.
> >
> >  With the "inodes index" feature, on the first time mount, an NFS file
> >  handle of the lower layer root directory, along with the UUID of the lower
> > @@ -280,6 +304,10 @@ lower root origin, mount will fail with ESTALE.  An overlayfs mount with
> >  does not support NFS export, lower filesystem does not have a valid UUID or
> >  if the upper filesystem does not support extended attributes.
> >
> > +For "metadata only copyup" feature there is no verification mechanism at
> > +mount time. So if same upper is mouted with different set of lower, mount
> > +probably will succeed but expect the unexpected later on. So don't do it.
> > +
> >  It is quite a common practice to copy overlay layers to a different
> >  directory tree on the same or different underlying filesystem, and even
> >  to a different machine.  With the "inodes index" feature, trying to mount
> > diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> > index 406e72de88f6..e1974db486a9 100644
> > --- a/fs/overlayfs/Kconfig
> > +++ b/fs/overlayfs/Kconfig
> > @@ -72,3 +72,20 @@ config OVERLAY_FS_NFS_EXPORT
> >           Note, that the NFS export feature is not backward compatible.
> >           That is, mounting an overlay which has a full index on a kernel
> >           that doesn't support this feature will have unexpected results.
> > +
> > +config OVERLAY_FS_METACOPY
> > +       bool "Overlayfs: turn on metadata only copy up feature by default"
> > +       depends on OVERLAY_FS
> > +       depends on !OVERLAY_FS_NFS_EXPORT
> > +       select OVERLAY_FS_REDIRECT_DIR
> > +       help
> > +         If this config option is enabled then overlay filesystems will
> > +         copy up only metadata where appropriate and data copy up will
> > +         happen when a file is opended for WRITE operation. It is still
> > +         possible to turn off this feature globally with the "metacopy=off"
> > +         module option or on a filesystem instance basis with the
> > +         "metacopy=off" mount option.
> > +
> > +         Note, that this feature is not backward compatible.  That is,
> > +         mounting an overlay which has metacopy only inodes on a kernel
> > +         that doesn't support this feature will have unexpected results.
> > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > index bfef6edcc111..7dc55628080d 100644
> > --- a/fs/overlayfs/ovl_entry.h
> > +++ b/fs/overlayfs/ovl_entry.h
> > @@ -18,6 +18,7 @@ struct ovl_config {
> >         const char *redirect_mode;
> >         bool index;
> >         bool nfs_export;
> > +       bool metacopy;
> >  };
> >
> >  struct ovl_layer {
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 9ee37c76091d..0e124b9d902d 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -58,6 +58,11 @@ static void ovl_entry_stack_free(struct ovl_entry *oe)
> >                 dput(oe->lowerstack[i].dentry);
> >  }
> >
> > +static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
> > +module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
> > +MODULE_PARM_DESC(ovl_metacopy_def,
> > +                "Default to on or off for the metadata only copy up feature");
> > +
> >  static void ovl_dentry_release(struct dentry *dentry)
> >  {
> >         struct ovl_entry *oe = dentry->d_fsdata;
> > @@ -350,6 +355,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
> >         if (ofs->config.nfs_export != ovl_nfs_export_def)
> >                 seq_printf(m, ",nfs_export=%s", ofs->config.nfs_export ?
> >                                                 "on" : "off");
> > +       if (ofs->config.metacopy != ovl_metacopy_def)
> > +               seq_printf(m, ",metacopy=%s",
> > +                          ofs->config.metacopy ? "on" : "off");
> >         return 0;
> >  }
> >
> > @@ -384,6 +392,8 @@ enum {
> >         OPT_INDEX_OFF,
> >         OPT_NFS_EXPORT_ON,
> >         OPT_NFS_EXPORT_OFF,
> > +       OPT_METACOPY_ON,
> > +       OPT_METACOPY_OFF,
> >         OPT_ERR,
> >  };
> >
> > @@ -397,6 +407,8 @@ static const match_table_t ovl_tokens = {
> >         {OPT_INDEX_OFF,                 "index=off"},
> >         {OPT_NFS_EXPORT_ON,             "nfs_export=on"},
> >         {OPT_NFS_EXPORT_OFF,            "nfs_export=off"},
> > +       {OPT_METACOPY_ON,               "metacopy=on"},
> > +       {OPT_METACOPY_OFF,              "metacopy=off"},
> >         {OPT_ERR,                       NULL}
> >  };
> >
> > @@ -511,6 +523,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> >                         config->nfs_export = false;
> >                         break;
> >
> > +               case OPT_METACOPY_ON:
> > +                       config->metacopy = true;
> > +                       break;
> > +
> > +               case OPT_METACOPY_OFF:
> > +                       config->metacopy = false;
> > +                       break;
> > +
> >                 default:
> >                         pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
> >                         return -EINVAL;
> > @@ -993,7 +1013,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.metacopy = false;
> > +               pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off and metacopy=off.\n");
> >                 err = 0;
> >         } else {
> >                 vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
> > @@ -1012,6 +1033,11 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
> >                 ofs->config.nfs_export = false;
> >         }
> >
> > +       /* metacopy feature with upper requires redirect_dir=on */
> > +       if (ofs->config.metacopy && !ofs->config.redirect_dir) {
> > +               pr_warn("overlayfs: metadata only copyup requires \"redirect_dir=on\", falling back to metacopy=off.\n");
> > +               ofs->config.metacopy = false;
> > +       }
> >  out:
> >         mnt_drop_write(mnt);
> >         return err;
> > @@ -1188,6 +1214,12 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
> >                 ofs->config.nfs_export = false;
> >         }
> >
> > +       if (!ofs->config.upperdir && ofs->config.metacopy &&
> > +           !ofs->config.redirect_follow) {
> > +               ofs->config.metacopy = false;
> > +               pr_warn("overlayfs: metadata only copyup requires \"redirect_dir=follow\" on non-upper mount, falling back to metacopy=off.\n");
> > +       }
> > +
> >         err = -ENOMEM;
> >         stack = kcalloc(stacklen, sizeof(struct path), GFP_KERNEL);
> >         if (!stack)
> > @@ -1263,6 +1295,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> >
> >         ofs->config.index = ovl_index_def;
> >         ofs->config.nfs_export = ovl_nfs_export_def;
> > +       ofs->config.metacopy = ovl_metacopy_def;
> >         err = ovl_parse_opt((char *) data, &ofs->config);
> >         if (err)
> >                 goto out_err;
> > @@ -1331,6 +1364,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> >                 }
> >         }
> >
> > +       if (ofs->config.metacopy && ofs->config.nfs_export) {
> > +               pr_warn("overlayfs: Metadata copy up requires NFS export disabled, falling back to metacopy=off.\n");
> > +               ofs->config.metacopy = false;
> > +       }
> > +
> 
> Is should probably be the other way around - metacopy should prevail
> and we should fall back to nfs_export=off (just like non-upper mount
> with redirect_follow)
> it's rather user can access file data then export to NFS.
> 
> If user prefers to risk not being able to access file data in favor of
> NFS export, then
> user should opt-in with metacopy=off, just like user needs to opt-in
> to redirect_dir=nofollow
> in order to NFS export a non-upper overlayfs.

Ok, makes sense. I will change it. Fall back to nfs_export=off.

Vivek

Re: [PATCH v12 14/17] ovl: Set redirect on metacopy files upon rename

[Amir Goldstein]

On Wed, Mar 7, 2018 at 5:15 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Mar 07, 2018 at 09:48:22AM +0200, Amir Goldstein wrote:
>> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > Set redirect on metacopy files upon rename. This will help find data dentry
>> > in lower dirs.
>> >
>> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > ---
>> >  fs/overlayfs/dir.c | 13 +++++++++----
>> >  1 file changed, 9 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>> > index cdeae4bdbfce..b955f6fede06 100644
>> > --- a/fs/overlayfs/dir.c
>> > +++ b/fs/overlayfs/dir.c
>> > @@ -1048,9 +1048,11 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
>> >                         err = ovl_set_redirect(old, samedir);
>> >                 else if (!old_opaque && ovl_type_merge(new->d_parent))
>> >                         err = ovl_set_opaque_xerr(old, olddentry, -EXDEV);
>> > -               if (err)
>> > -                       goto out_dput;
>> > -       }
>> > +       } else if (ovl_is_metacopy_dentry(old))
>> > +                       err = ovl_set_redirect(old, false);
>>
>> You should use {} in the else statement as well.
>
> ok.
>
>>
>> Q: why do you set samedir: = false here?
>> A: because other hardlink aliasses cannot follow a relative redirect, right?
>
> Right. If we create a hardlink later then it will need absolute redirect
> if both dentries are not in same dir.
>
>>
>> This is a subtle detail that should be documented,
>
> Ok, will do.
>
>> but also
>> maybe do use samedir if nlink == 1?
>
> Hmm.., so initially we could put a relative redirct (if nlink=1) and later
> if we create a link, we could replace relative redirect with an absolute
> redirect? I see we already have logic to do that for the case of rename.
>
> Now only thing I need to figure out in ovl_link() whethre two dentries
> are in same dir or not. I am assuming I can just check parent dentry
> pointers and see if these two have same parent or not.

Yes or we can just convert to absolute path anyway for nlink > 1.
>
> In fact, we probably don't even have to check for nlink=1. Only when
> we create a upper hard link, then we need to make sure we replace relative
> hardlink with absolute one. I will play with it and see how it goes.
>

Yes. alomst true. but we do need to check for lower nlink > 1,
because in that case (when index=on) upper hardlinks are created on
copy up not only on ovl_link(), so easiest is to just start with
absolute redirect
on rename of lower hardlink.

Thanks,
Amir.

Re: [PATCH v12 13/17] ovl: Check redirects for metacopy files

[Vivek Goyal]

On Wed, Mar 07, 2018 at 02:16:25PM +0200, Amir Goldstein wrote:
> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Right now we rely on path based lookup for data origin of metacopy upper.
> > This will work only if upper has not been renamed. We solved this problem
> > already for merged directories using redirect. Use same logic for metacopy
> > files.
> >
> > This patch just goes on to check redirects for metacopy files.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/namei.c | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index 220e754c974b..a4a5c5f5540d 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -265,22 +265,22 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
> >                 goto put_and_out;
> >         }
> >         if (!d_can_lookup(this)) {
> > -               if (d->is_dir)
> > -                       goto put_and_out;
> > +               d->is_dir = false;
> 
> That test was supposed to catch non-dir under dir from an upper layer
> but you are loosing the information stored in d->dir.
> 
Can you elaborate a bit more. I never understood this check.

This probably worked in the past. But now absolute redirects can be put
on regular files as well. So it is very much possible that d->is_dir
is set and then we find a non-dir dentry and in that case we don't
want to jump to "put_and_out"?

IOW, this check worked as long as redirects were supposed to be used
only for directories. Now with metacopy, redirects can be put on on
non-dir as well.

What I can probably do is that leave d->is_dir untouched. But not sure
what does that buy us. When we return back to ovl_lookup() it does not
mean anything. returned dentry could be a directory or non-directory.

If we set it to d->is_dir=false, atleast this info is meaningful in
ovl_lookup and we know if returned dentry is a direcotry or non-dir.

> >                 err = ovl_check_metacopy_xattr(this);
> >                 if (err < 0)
> >                         goto out_err;
> >                 if (!err) {
> >                         d->stop = true;
> >                         d->metacopy = false;
> > +                       goto out;
> >                 } else
> >                         d->metacopy = true;
> > -               goto out;
> > -       }
> > -       d->is_dir = true;
> > -       if (!d->last && ovl_is_opaquedir(this)) {
> > -               d->stop = d->opaque = true;
> > -               goto out;
> > +       } else {
> > +               d->is_dir = true;
> > +               if (!d->last && ovl_is_opaquedir(this)) {
> > +                       d->stop = d->opaque = true;
> > +                       goto out;
> 
> I think there is a bug here - not related to your change, but semi related
> to your recent fix patch (patch 1 in this series).
> 
> d->last is set to true when lookup in parent poe->numlower layer,
> but parent may be pure upper for example and redirect from child can still
> continue lookup to lower layers. If a directory is marked both "redirect" and
> "opaque" (which is an inconsistency). In that case, d->last will be true
> and opaque xattr will not be checked, but redirect will be checked.

I am not sure I understand the concern. d->last will be set only if this
is last layer we are looking into and in that case it does not matter if
we process opaque or not.

Say we had pure upper parent and child directory had both opaque and redirect
set. If redirect is relative, then we will not even search in lower pas
poe->numlower=0. If redirect is absolute, then poe is reset to roe and we
will start searching from root and d->last will be set when searching in
last layer. 

So I can't see what's the issue. Can you give an example.

Thanks
Vivek

> 
> Since AFAIK d->last is an optimization, I think it could be relaxed to
> lower.layer->idx == roe->numlower - 1
> and then for the d->last case, we can skip both opaque and redirect checks
> and skip redirect check for both directory and metadata.
> 
> 
> > +               }
> >         }
> >         err = ovl_check_redirect(this, d, prelen, post);
> >         if (err)
> 
> Thanks,
> Amir.

Re: [PATCH v12 12/17] ovl: Do not expose metacopy only upper dentry from d_real()

[Vivek Goyal]

On Wed, Mar 07, 2018 at 03:40:02PM +0200, Amir Goldstein wrote:
> On Wed, Mar 7, 2018 at 3:29 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Mar 07, 2018 at 09:15:40AM +0200, Amir Goldstein wrote:
> >> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > d_real() can make a upper metacopy dentry/inode visible to the vfs layer.
> >> > This is something new and vfs layer does not know that this inode contains
> >> > only metadata and not data. And this could break things.
> >> >
> >> > So to be safe, do not expose metacopy only dentry/inode to vfs using
> >> > d_real().
> >> >
> >> > IOW, d_real() will not reuturn metacopy dentry. Instead, it will return
> >> > dentry corresponding lower dentry/inode which has file data.
> >> >
> >> > For regular d_real() call (inode == NULL, D_REAL_UPPER not set), if upper
> >> > dentry inode is metacopy only and does not have data, return lower dentry.
> >> >
> >> > If d_real() is called with flag D_REAL_UPPER, return upper dentry only if
> >> > it has data (flag OVL_UPPERDATA is set).
> >> >
> >> > Similiarly, if d_real(inode=X) is called, a warning is emitted if returned
> >> > dentry/inode does not have OVL_UPPERDATA set. This should not happen as
> >> > we never made this metacopy inode visible to vfs so nobody should be
> >> > calling overlayfs back with inode=metacopy_inode.
> >> >
> >> > I scanned the code and I don't think it breaks any of the existing code.
> >> > There are two users of D_REAL_UPPER. may_write_real() and
> >> > update_ovl_inode_times().
> >> >
> >> > may_write_real(), will get an NULL dentry if upper inode is metacopy only
> >> > and it will return -EPERM. Effectively, we are disallowing modifications
> >> > to metacopy only inode from this interface. Though there is opportunity
> >> > to improve it. (Allow chattr on metacopy inodes).
> >> >
> >> > update_ovl_inode_times() gets inode mtime and ctime from real inode. It
> >> > should not be broken for metacopy inode as well for following reasons.
> >> >
> >> > - For any metadata operations (setattr, acl etc), overlay always calls
> >> >   ovl_copyattr() and updates ovl inode mtime and ctime. So there is no
> >> >   need to update mtime and ctime in this case. Its already updated, hence
> >> >   even if d_real(D_REAL_UPPER) returns nil, it should be fine.
> >> >
> >> > - For metadata inode, mtime should be same as lower and not change. (data
> >> >   can't be modified on metadata inode without copyup). IOW, mtime of
> >> >   ovl dentry should be same as mtime of underlying metadata inode on upper
> >> >   always. So there is no need to update it.
> >> >
> >> > - For file writes, ctime and mtime will be updated. But in that case
> >> >   first data will be copied up and this will not be a metadata inode
> >> >   anymore. And furthr call to d_real(D_REAL_UPPER) will return upper
> >> >   inode and new mtime and ctime will be obtainable.
> >> >
> >> > So atime updates should work just fine for metacopy inodes. I think only
> >> > corner case is if somehow underlying filesystem changes ctime of upper
> >> > metadata inode without overlay knowing about it. Not sure how that
> >> > can happen. If somehow is affected by that, then we probably can implement
> >> > another flag which will allow caller to get metacopy inode as well.
> >> > Something like d_real(D_REAL_UPPER | D_METACOPY). And that should solve
> >> > this issue.
> >> >
> >> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> >> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> >> > ---
> >> >  fs/overlayfs/overlayfs.h |  1 +
> >> >  fs/overlayfs/super.c     | 21 +++++++++++++++++----
> >> >  fs/overlayfs/util.c      |  8 ++++++++
> >> >  3 files changed, 26 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> >> > index 2d682923252e..24725b6668b9 100644
> >> > --- a/fs/overlayfs/overlayfs.h
> >> > +++ b/fs/overlayfs/overlayfs.h
> >> > @@ -225,6 +225,7 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
> >> >  enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
> >> >  struct dentry *ovl_dentry_upper(struct dentry *dentry);
> >> >  struct dentry *ovl_dentry_lower(struct dentry *dentry);
> >> > +struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
> >> >  struct dentry *ovl_dentry_real(struct dentry *dentry);
> >> >  struct dentry *ovl_i_dentry_upper(struct inode *inode);
> >> >  struct inode *ovl_inode_upper(struct inode *inode);
> >> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> >> > index d3dbdd695722..4be4e47cbf57 100644
> >> > --- a/fs/overlayfs/super.c
> >> > +++ b/fs/overlayfs/super.c
> >> > @@ -96,8 +96,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
> >> >         struct dentry *real;
> >> >         int err;
> >> >
> >> > -       if (flags & D_REAL_UPPER)
> >> > -               return ovl_dentry_upper(dentry);
> >> > +       if (flags & D_REAL_UPPER) {
> >> > +               real = ovl_dentry_upper(dentry);
> >> > +               if (!real)
> >> > +                       return NULL;
> >> > +               if (!ovl_has_upperdata(dentry))
> >> > +                       return NULL;
> >> > +               return real;
> >> > +       }
> >> >
> >> >         if (!d_is_reg(dentry)) {
> >> >                 if (!inode || inode == d_inode(dentry))
> >> > @@ -113,15 +119,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
> >> >
> >> >         real = ovl_dentry_upper(dentry);
> >> >         if (real && (!inode || inode == d_inode(real))) {
> >> > +               bool metacopy = !ovl_has_upperdata(dentry);
> >> >                 if (!inode) {
> >> >                         err = ovl_check_append_only(d_inode(real), open_flags);
> >> >                         if (err)
> >> >                                 return ERR_PTR(err);
> >> > -               }
> >> > +
> >> > +                       if (unlikely(metacopy))
> >> > +                               goto lower;
> >> > +               } else if (unlikely(metacopy))
> >> > +                       goto bug;
> >> > +
> >> >                 return real;
> >> >         }
> >> >
> >> > -       real = ovl_dentry_lower(dentry);
> >> > +lower:
> >> > +       real = ovl_dentry_lowerdata(dentry);
> >> >         if (!real)
> >> >                 goto bug;
> >> >
> >> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> >> > index 274bbfc855e0..36d41f7001e3 100644
> >> > --- a/fs/overlayfs/util.c
> >> > +++ b/fs/overlayfs/util.c
> >> > @@ -186,6 +186,14 @@ struct dentry *ovl_dentry_lower(struct dentry *dentry)
> >> >         return oe->numlower ? oe->lowerstack[0].dentry : NULL;
> >> >  }
> >> >
> >> > +struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
> >> > +{
> >> > +       struct ovl_entry *oe = dentry->d_fsdata;
> >> > +       int idx = oe->numlower - 1;
> >> > +
> >> > +       return oe->lowerstack[idx].dentry;
> >> > +}
> >> > +
> >>
> >> This new change is not in line with the subject line.
> >> Either change the commit message to fit or better split this
> >> small change to a new patch because the commit message is long
> >> enough as it is.
> >
> > Ok, I will move this helper in a separate patch before this patch.
> >
> 
> It's not just the helper. The subject says "Don't expose metacopy upper"
> but this helper is used to "not expose metacopy lower", so either amend
> the commit message or fix exposing metacopy lower in a separate patch.

Ok, I will modify subject also. This subject is vestige of previous
versions where metacopy dentry was only in upper layer. Now I support
metacopy in mid layer also and don't want to expose it either. So will
modify subject as well.

Vivek

Re: [PATCH v12 08/17] ovl: Modify ovl_lookup() and friends to lookup metacopy dentry

[Vivek Goyal]

On Wed, Mar 07, 2018 at 04:42:54PM +0200, Amir Goldstein wrote:
> On Tue, Mar 6, 2018 at 10:53 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > This patch modifies ovl_lookup() and friends to lookup metacopy dentries.
> > It also allows for presence of metacopy dentries in lower layer.
> >
> > During lookup, check for presence of OVL_XATTR_METACOPY and if not present,
> > set OVL_UPPERDATA bit in flags.
> >
> > OVL_UPPERDATA flag is set unconditionally if upper inode exists.
> 
> You mean if *only* upper inode exists?

This message is not very clear. Will modify it. Basically want to say
if upperdentry exists and it is not a metacopy, then OVL_UPPERDATA is
set.

> 
> >
> > Do not follow metacopy origin if we find a metacopy only inode and metacopy
> > feature is not enabled for that mount. Like redirect, this can have security
> > implications where an attacker could hand craft upper and try to gain
> > access to file on lower which it should not have to begin with.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/export.c |   3 ++
> >  fs/overlayfs/namei.c  | 121 ++++++++++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 115 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > index bb94ce9da5c8..35f2d4eb0d7e 100644
> > --- a/fs/overlayfs/export.c
> > +++ b/fs/overlayfs/export.c
> > @@ -192,6 +192,9 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
> >         if (index)
> >                 ovl_set_flag(OVL_INDEX, inode);
> >
> > +       if (upper)
> > +               ovl_set_flag(OVL_UPPERDATA, inode);
> > +
> 
> FYI, in this function upper may be a disconnected dentry, so
> for future metacopy+nfs_export support you will need to make sure that
> either:
> - We can always get datalower from upper by origin and not by path
> - We set absolute path redirect on encode of non-indexed metacopy upper
> - If metacopy upper is indexed by the lowerdata inode then all is good,
>   because we find the lowerdata by file handle and upper metacopy by index,
>   just need to  ovl_check_metacopy_xattr(upper) before setting upperdata
>   here above
> 
> Lower layer metacopies are going to be a problem with nfs export.
> The only way to solve it would be to follow to lowerdata by origin.

Need to think a little about above points. Will respond to it later.

> 
> >         dentry = d_find_any_alias(inode);
> >         if (!dentry) {
> >                 dentry = d_alloc_anon(inode->i_sb);
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index 70fcfcc684cc..220e754c974b 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -24,6 +24,7 @@ struct ovl_lookup_data {
> >         bool stop;
> >         bool last;
> >         char *redirect;
> > +       bool metacopy;
> >  };
> >
> >  static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
> > @@ -208,6 +209,28 @@ struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt)
> >         return real;
> >  }
> >
> > +/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
> > +static int ovl_check_metacopy_xattr(struct dentry *dentry)
> > +{
> > +       int res;
> > +
> > +       /* Only regular files can have metacopy xattr */
> > +       if (!S_ISREG(d_inode(dentry)->i_mode))
> > +               return 0;
> > +
> > +       res = vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
> > +       if (res < 0) {
> > +               if (res == -ENODATA || res == -EOPNOTSUPP)
> > +                       return 0;
> > +               goto out;
> > +       }
> > +
> > +       return 1;
> > +out:
> > +       pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
> > +       return res;
> > +}
> > +
> >  static bool ovl_is_opaquedir(struct dentry *dentry)
> >  {
> >         return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);
> > @@ -242,9 +265,16 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
> >                 goto put_and_out;
> >         }
> >         if (!d_can_lookup(this)) {
> > -               d->stop = true;
> >                 if (d->is_dir)
> 
> You need to set d->stop here because this is a non-dir below upper dir.

With metacopy dentry on regular files, we want to do d->stop=true only 
if this is not a metacopy. Otherwise we want to continue to do path
based lookup in lower layers. That's the whole point of this patch
series. 

> 
> >                         goto put_and_out;
> > +               err = ovl_check_metacopy_xattr(this);
> > +               if (err < 0)
> > +                       goto out_err;
> > +               if (!err) {
> > +                       d->stop = true;
> > +                       d->metacopy = false;
> > +               } else
> > +                       d->metacopy = true;
> 
> Need to have {} in both if and else, but better not use if at all:
> d->stop = !err;
> d->metacopy = !!err;

I think I will put {}. Its much easier to read the code that way.

> 
> 
> >                 goto out;
> >         }
> >         d->is_dir = true;
> > @@ -799,7 +829,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >         struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> >         struct ovl_entry *poe = dentry->d_parent->d_fsdata;
> >         struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
> > -       struct ovl_path *stack = NULL;
> > +       struct ovl_path *stack = NULL, *origin_path = NULL;
> >         struct dentry *upperdir, *upperdentry = NULL;
> >         struct dentry *origin = NULL;
> >         struct dentry *index = NULL;
> > @@ -810,6 +840,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >         struct dentry *this;
> >         unsigned int i;
> >         int err;
> > +       bool metacopy = false;
> >         struct ovl_lookup_data d = {
> >                 .name = dentry->d_name,
> >                 .is_dir = false,
> > @@ -817,6 +848,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                 .stop = false,
> >                 .last = !poe->numlower,
> >                 .redirect = NULL,
> > +               .metacopy = false,
> >         };
> >
> >         if (dentry->d_name.len > ofs->namelen)
> > @@ -835,7 +867,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                         goto out;
> >                 }
> >                 if (upperdentry && !d.is_dir) {
> > -                       BUG_ON(!d.stop || d.redirect);
> > +                       unsigned int origin_ctr = 0;
> > +                       BUG_ON(d.redirect);
> >                         /*
> >                          * Lookup copy up origin by decoding origin file handle.
> >                          * We may get a disconnected dentry, which is fine,
> > @@ -846,16 +879,20 @@ 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(ofs, upperdentry, &stack, &ctr);
> > +                       err = ovl_check_origin(ofs, upperdentry, &origin_path,
> > +                                              &origin_ctr);
> >                         if (err)
> >                                 goto out_put_upper;
> > +
> > +                       if (d.metacopy)
> > +                               metacopy = true;
> >                 }
> >
> >                 if (d.redirect) {
> >                         err = -ENOMEM;
> >                         upperredirect = kstrdup(d.redirect, GFP_KERNEL);
> >                         if (!upperredirect)
> > -                               goto out_put_upper;
> > +                               goto out_put_origin;
> >                         if (d.redirect[0] == '/')
> >                                 poe = roe;
> >                 }
> > @@ -867,7 +904,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                 stack = kcalloc(ofs->numlower, sizeof(struct ovl_path),
> >                                 GFP_KERNEL);
> >                 if (!stack)
> > -                       goto out_put_upper;
> > +                       goto out_put_origin;
> >         }
> >
> >         for (i = 0; !d.stop && i < poe->numlower; i++) {
> > @@ -885,7 +922,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                  * If no origin fh is stored in upper of a merge dir, store fh
> >                  * of lower dir and set upper parent "impure".
> >                  */
> > -               if (upperdentry && !ctr && !ofs->noxattr) {
> > +               if (upperdentry && !ctr && !ofs->noxattr && d.is_dir) {
> >                         err = ovl_fix_origin(dentry, this, upperdentry);
> >                         if (err) {
> >                                 dput(this);
> > @@ -898,7 +935,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                  * lower 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_lower(dentry->d_sb)) {
> > +               if (upperdentry && !ctr && d.is_dir &&
> > +                   ovl_verify_lower(dentry->d_sb)) {
> >                         err = ovl_verify_origin(upperdentry, this, false);
> >                         if (err) {
> >                                 dput(this);
> > @@ -909,6 +947,29 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                         origin = this;
> >                 }
> >
> > +               /*
> > +                * For non-dir dentry, make sure dentry found by lookup
> > +                * matches the origin stored in upper
> > +                */
> > +               if (!d.is_dir && upperdentry && !ctr && origin_path) {
> > +                       err = ovl_verify_origin(upperdentry, this, false);
> > +                       if (err) {
> > +                               dput(this);
> > +                               goto out_put;
> > +                       }
> > +               }
> > +
> 
> Why is this code duplicated from d.is_dir case?

Primarily because for the case of dir, you are verifying origin only
if nfs_export is enabled. While I doing this verification even when
nfs_export is not enabled.

So far by default we always installed origin in lowerstack[0] for non-dir.
Now we are doing lookup for non-dir files and looked up file might
be different from origin. So we need to make sure file found by lookup
is same as pointed by origin, otherwise something is not right?

> 
> 
> > +               if (d.metacopy)
> > +                       metacopy = true;
> > +               /*
> > +                * Do not store intermediate metacopy dentries in chain,
> > +                * except top most lower metacopy dentry
> > +                */
> > +               if (d.metacopy && ctr) {
> > +                       dput(this);
> > +                       continue;
> > +               }
> > +
> >                 stack[ctr].dentry = this;
> >                 stack[ctr].layer = lower.layer;
> >                 ctr++;
> > @@ -940,6 +1001,34 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                 }
> >         }
> >
> > +       if (metacopy) {
> > +               BUG_ON(d.is_dir);
> > +               /*
> > +                * Found a metacopy dentry but did not find corresponding
> > +                * data dentry
> > +                */
> > +               if (d.metacopy) {
> > +                       err = -ESTALE;
> 
> Set err = -ESTALE before if (d.metacopy).
> I think this case deserves a warning as well.

Ok, Will put a warning.

> 
> > +                       goto out_put;
> > +               }
> > +
> > +               err = -EPERM;
> > +               if (!ofs->config.metacopy) {
> > +                       pr_warn_ratelimited("overlay: refusing to follow"
> > +                                           " metacopy origin for (%pd2)\n",
> > +                                           dentry);
> > +                       goto out_put;
> > +               }
> > +       } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
> > +               if (WARN_ON(stack != NULL)) {
> > +                       err = -EIO;
> > +                       goto out_put;
> > +               }
> > +               stack = origin_path;
> > +               ctr = 1;
> > +               origin_path = NULL;
> 
> I am having a hard time understanding why origin_path is needed at all
> and why not lookup only by path and then ovl_verify_origin().

Right now we look for non-dir file in lower only if upper is metacopy. So
if upper is not metacopy, then origin has to be installed in
lowerstack[0]. That's why I save origin in origin_path, and install it
in lowerstack[0] when there is no lower.

Now other way could be that we lookup for dentry in lower even for
non-metacopy non-dir files and do ovl_verify_origin(). I think that's
what you are referring to. But that seems like unnecessary lookup in
lower. We don't have to do it.

> The only case I can think of where following by origin is required if for
> decoding a non-indexed upper file handle which is also a metacopy,
> but that has nothing to do with ovl_lookup().
> 
> Please enlighten me.
> 
> > +       }
> > +
> >         /*
> >          * Lookup index by lower inode and verify it matches upper inode.
> >          * We only trust dir index if we verified that lower dir matches
> > @@ -972,8 +1061,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >
> >         if (upperdentry)
> >                 ovl_dentry_set_upper_alias(dentry);
> > -       else if (index)
> > +       else if (index) {
> >                 upperdentry = dget(index);
> > +               metacopy = ovl_check_metacopy_xattr(index);
> > +       }
> 
> {} should be on both if and else statements.

Will do.

Vivek

Re: [PATCH v12 14/17] ovl: Set redirect on metacopy files upon rename

[Vivek Goyal]

On Wed, Mar 07, 2018 at 06:26:01PM +0200, Amir Goldstein wrote:
> On Wed, Mar 7, 2018 at 5:15 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Mar 07, 2018 at 09:48:22AM +0200, Amir Goldstein wrote:
> >> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > Set redirect on metacopy files upon rename. This will help find data dentry
> >> > in lower dirs.
> >> >
> >> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> >> > ---
> >> >  fs/overlayfs/dir.c | 13 +++++++++----
> >> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> >> > index cdeae4bdbfce..b955f6fede06 100644
> >> > --- a/fs/overlayfs/dir.c
> >> > +++ b/fs/overlayfs/dir.c
> >> > @@ -1048,9 +1048,11 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
> >> >                         err = ovl_set_redirect(old, samedir);
> >> >                 else if (!old_opaque && ovl_type_merge(new->d_parent))
> >> >                         err = ovl_set_opaque_xerr(old, olddentry, -EXDEV);
> >> > -               if (err)
> >> > -                       goto out_dput;
> >> > -       }
> >> > +       } else if (ovl_is_metacopy_dentry(old))
> >> > +                       err = ovl_set_redirect(old, false);
> >>
> >> You should use {} in the else statement as well.
> >
> > ok.
> >
> >>
> >> Q: why do you set samedir: = false here?
> >> A: because other hardlink aliasses cannot follow a relative redirect, right?
> >
> > Right. If we create a hardlink later then it will need absolute redirect
> > if both dentries are not in same dir.
> >
> >>
> >> This is a subtle detail that should be documented,
> >
> > Ok, will do.
> >
> >> but also
> >> maybe do use samedir if nlink == 1?
> >
> > Hmm.., so initially we could put a relative redirct (if nlink=1) and later
> > if we create a link, we could replace relative redirect with an absolute
> > redirect? I see we already have logic to do that for the case of rename.
> >
> > Now only thing I need to figure out in ovl_link() whethre two dentries
> > are in same dir or not. I am assuming I can just check parent dentry
> > pointers and see if these two have same parent or not.
> 
> Yes or we can just convert to absolute path anyway for nlink > 1.
> >
> > In fact, we probably don't even have to check for nlink=1. Only when
> > we create a upper hard link, then we need to make sure we replace relative
> > hardlink with absolute one. I will play with it and see how it goes.
> >
> 
> Yes. alomst true. but we do need to check for lower nlink > 1,
> because in that case (when index=on) upper hardlinks are created on
> copy up not only on ovl_link(), so easiest is to just start with
> absolute redirect
> on rename of lower hardlink.

Hmm..., even in that case it should work. For example, say foo.txt and
bar.txt are hardlinked in lower. hence nlink=2. And now I rename foo.txt
to foo-upper.txt, then it will be copied up (with index hardlinked) and
then a redirect will be set (foo.txt). And now, bar.txt can be looked up
without redirct and foo-upper.txt can be looked up with redirect. So it
should work even in nlink>1 in lower. Can you give a specific example
where it will be broken.

Having said that I don't mind to always set absolute redirect whenever
nlink > 1 (both in lower and upper) and that simplifies the logic a bit.

Vivek

Re: [PATCH v12 14/17] ovl: Set redirect on metacopy files upon rename

[Amir Goldstein]

On Wed, Mar 7, 2018 at 10:43 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
[...]
>> >>
>> >> Q: why do you set samedir: = false here?
>> >> A: because other hardlink aliasses cannot follow a relative redirect, right?
>> >
>> > Right. If we create a hardlink later then it will need absolute redirect
>> > if both dentries are not in same dir.
>> >
>> >>
>> >> This is a subtle detail that should be documented,
>> >
>> > Ok, will do.
>> >
>> >> but also
>> >> maybe do use samedir if nlink == 1?
>> >
>> > Hmm.., so initially we could put a relative redirct (if nlink=1) and later
>> > if we create a link, we could replace relative redirect with an absolute
>> > redirect? I see we already have logic to do that for the case of rename.
>> >
>> > Now only thing I need to figure out in ovl_link() whethre two dentries
>> > are in same dir or not. I am assuming I can just check parent dentry
>> > pointers and see if these two have same parent or not.
>>
>> Yes or we can just convert to absolute path anyway for nlink > 1.
>> >
>> > In fact, we probably don't even have to check for nlink=1. Only when
>> > we create a upper hard link, then we need to make sure we replace relative
>> > hardlink with absolute one. I will play with it and see how it goes.
>> >
>>
>> Yes. alomst true. but we do need to check for lower nlink > 1,
>> because in that case (when index=on) upper hardlinks are created on
>> copy up not only on ovl_link(), so easiest is to just start with
>> absolute redirect
>> on rename of lower hardlink.
>
> Hmm..., even in that case it should work. For example, say foo.txt and
> bar.txt are hardlinked in lower. hence nlink=2. And now I rename foo.txt
> to foo-upper.txt, then it will be copied up (with index hardlinked) and
> then a redirect will be set (foo.txt). And now, bar.txt can be looked up
> without redirct and foo-upper.txt can be looked up with redirect. So it
> should work even in nlink>1 in lower. Can you give a specific example
> where it will be broken.
>

Suppose bar/bar.txt and foo/foo.txt. On metacopy up of bar.txt you need
to convert to absolute redirect. Because (with index=on) bar.txt will be
"linked up" to foo-upper.txt, which has a redirect xattr of "foo.txt".
On cold cache lookup of bar/bar.txt, metacopy lookup will follow "foo.txt"
redirect to "bar/foo.txt", which does not exist.

> Having said that I don't mind to always set absolute redirect whenever
> nlink > 1 (both in lower and upper) and that simplifies the logic a bit.
>

Yeh, we optimize the common case of nlink == 1 and same dir, because
absolute redirect is heavy on lookup, and we leave the less common case
of upper or lower hardlink simple and not optimized.

Thanks,
Amir.

Re: [PATCH v12 08/17] ovl: Modify ovl_lookup() and friends to lookup metacopy dentry

[Amir Goldstein]

On Wed, Mar 7, 2018 at 10:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Mar 07, 2018 at 04:42:54PM +0200, Amir Goldstein wrote:
[...]

>> >  static bool ovl_is_opaquedir(struct dentry *dentry)
>> >  {
>> >         return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);
>> > @@ -242,9 +265,16 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
>> >                 goto put_and_out;
>> >         }
>> >         if (!d_can_lookup(this)) {
>> > -               d->stop = true;
>> >                 if (d->is_dir)
>>
>> You need to set d->stop here because this is a non-dir below upper dir.
>
> With metacopy dentry on regular files, we want to do d->stop=true only
> if this is not a metacopy. Otherwise we want to continue to do path
> based lookup in lower layers. That's the whole point of this patch
> series.
>

Yes we do, but the point that yo missed about (d->is_dir) check is that it means
that upper layer was a directory (e.g. upperdir/foo) and underneath it we find
a non-dir (e.g. lowerdir/foo). This is a case were lookup needs to stop and
result in a non-merge pure upper dir.

>>
>> >                         goto put_and_out;
>> > +               err = ovl_check_metacopy_xattr(this);
>> > +               if (err < 0)
>> > +                       goto out_err;
>> > +               if (!err) {
>> > +                       d->stop = true;
>> > +                       d->metacopy = false;
>> > +               } else
>> > +                       d->metacopy = true;
>>
>> Need to have {} in both if and else, but better not use if at all:
>> d->stop = !err;
>> d->metacopy = !!err;
>
> I think I will put {}. Its much easier to read the code that way.

Well, execution branch is less efficient and I don't know if compiler
will optimize it away, but in any case I think d->metacopy = err is
nicer because  ovl_check_metacopy_xattr() returns an error or
an answer to is_metacopy.

[...]

>> > +               /*
>> > +                * For non-dir dentry, make sure dentry found by lookup
>> > +                * matches the origin stored in upper
>> > +                */
>> > +               if (!d.is_dir && upperdentry && !ctr && origin_path) {
>> > +                       err = ovl_verify_origin(upperdentry, this, false);
>> > +                       if (err) {
>> > +                               dput(this);
>> > +                               goto out_put;
>> > +                       }
>> > +               }
>> > +
>>
>> Why is this code duplicated from d.is_dir case?
>
> Primarily because for the case of dir, you are verifying origin only
> if nfs_export is enabled. While I doing this verification even when
> nfs_export is not enabled.

(d.is_dir && ovl_verify_lower(dentry->d_sb)) || (!d.is_dir && origin_path)

The case of verifying dir found by path and file found by found is exactly
the same case. You should just include the new case in the comment above
current code. The idea is to make metacopy lookup as similar as possible
to merge dir lookup, not to have a different code path for it.

>
> So far by default we always installed origin in lowerstack[0] for non-dir.
> Now we are doing lookup for non-dir files and looked up file might
> be different from origin. So we need to make sure file found by lookup
> is same as pointed by origin, otherwise something is not right?
>

[...]

>> > +       } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
>> > +               if (WARN_ON(stack != NULL)) {
>> > +                       err = -EIO;
>> > +                       goto out_put;
>> > +               }
>> > +               stack = origin_path;
>> > +               ctr = 1;
>> > +               origin_path = NULL;
>>
>> I am having a hard time understanding why origin_path is needed at all
>> and why not lookup only by path and then ovl_verify_origin().
>
> Right now we look for non-dir file in lower only if upper is metacopy. So
> if upper is not metacopy, then origin has to be installed in
> lowerstack[0]. That's why I save origin in origin_path, and install it
> in lowerstack[0] when there is no lower.

OK. you don't need to allocate origin_path, because it is limited to stack
size of 1. See how ovl_verify_index() initializes stock and origin vars.

>
> Now other way could be that we lookup for dentry in lower even for
> non-metacopy non-dir files and do ovl_verify_origin(). I think that's
> what you are referring to. But that seems like unnecessary lookup in
> lower. We don't have to do it.
>

Nah, that will require also doing redirect on non-dir without metacopy.

Thanks,
Amir.

Re: [PATCH v12 13/17] ovl: Check redirects for metacopy files

[Amir Goldstein]

On Wed, Mar 7, 2018 at 8:52 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Mar 07, 2018 at 02:16:25PM +0200, Amir Goldstein wrote:
>> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > Right now we rely on path based lookup for data origin of metacopy upper.
>> > This will work only if upper has not been renamed. We solved this problem
>> > already for merged directories using redirect. Use same logic for metacopy
>> > files.
>> >
>> > This patch just goes on to check redirects for metacopy files.
>> >
>> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > ---
>> >  fs/overlayfs/namei.c | 17 ++++++++---------
>> >  1 file changed, 8 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> > index 220e754c974b..a4a5c5f5540d 100644
>> > --- a/fs/overlayfs/namei.c
>> > +++ b/fs/overlayfs/namei.c
>> > @@ -265,22 +265,22 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
>> >                 goto put_and_out;
>> >         }
>> >         if (!d_can_lookup(this)) {
>> > -               if (d->is_dir)
>> > -                       goto put_and_out;
>> > +               d->is_dir = false;
>>
>> That test was supposed to catch non-dir under dir from an upper layer
>> but you are loosing the information stored in d->dir.
>>
> Can you elaborate a bit more. I never understood this check.
>
> This probably worked in the past. But now absolute redirects can be put
> on regular files as well. So it is very much possible that d->is_dir
> is set and then we find a non-dir dentry and in that case we don't
> want to jump to "put_and_out"?
>
> IOW, this check worked as long as redirects were supposed to be used
> only for directories. Now with metacopy, redirects can be put on on
> non-dir as well.
>
> What I can probably do is that leave d->is_dir untouched. But not sure
> what does that buy us. When we return back to ovl_lookup() it does not
> mean anything. returned dentry could be a directory or non-directory.
>
> If we set it to d->is_dir=false, atleast this info is meaningful in
> ovl_lookup and we know if returned dentry is a direcotry or non-dir.

d->is_dir means that layer above was a directory if you find non-dir underneath
a directory, lookup should stop.

>
>> >                 err = ovl_check_metacopy_xattr(this);
>> >                 if (err < 0)
>> >                         goto out_err;
>> >                 if (!err) {
>> >                         d->stop = true;
>> >                         d->metacopy = false;
>> > +                       goto out;
>> >                 } else
>> >                         d->metacopy = true;
>> > -               goto out;
>> > -       }
>> > -       d->is_dir = true;
>> > -       if (!d->last && ovl_is_opaquedir(this)) {
>> > -               d->stop = d->opaque = true;
>> > -               goto out;
>> > +       } else {
>> > +               d->is_dir = true;
>> > +               if (!d->last && ovl_is_opaquedir(this)) {
>> > +                       d->stop = d->opaque = true;
>> > +                       goto out;
>>
>> I think there is a bug here - not related to your change, but semi related
>> to your recent fix patch (patch 1 in this series).
>>
>> d->last is set to true when lookup in parent poe->numlower layer,
>> but parent may be pure upper for example and redirect from child can still
>> continue lookup to lower layers. If a directory is marked both "redirect" and
>> "opaque" (which is an inconsistency). In that case, d->last will be true
>> and opaque xattr will not be checked, but redirect will be checked.
>
> I am not sure I understand the concern. d->last will be set only if this
> is last layer we are looking into and in that case it does not matter if
> we process opaque or not.
>
> Say we had pure upper parent and child directory had both opaque and redirect
> set. If redirect is relative, then we will not even search in lower pas
> poe->numlower=0. If redirect is absolute, then poe is reset to roe and we
> will start searching from root and d->last will be set when searching in
> last layer.
>
> So I can't see what's the issue. Can you give an example.
>

/upperdir/pure (opaque=y)
/upperdir/pure/foo (opaque=y,redirect=/bar)
/lowerdir/bar (redirect=blah)

/pure/foo is both opaque and redirect and poe->numlower == 0 so d->last is true.

In this case (which is an inconsistency) /upperdir/pure/foo *will* be
merged with
/lowerdir/bar and I don't think that redirect should overrule opaque, but the
other way around.

It is also worth noting that we do not optimize away checking redirect on
/lowerdir/bar exactly because we d->last is not set correctly.
If we set d.last = lower.layer->idx == roe->numlower - 1
then is will be correct to optimize away both opaque and redirect checks
for d->last true case.

If this is not clear enough I will send out a patch.

Thanks,
Amir.

Re: [PATCH v12 08/17] ovl: Modify ovl_lookup() and friends to lookup metacopy dentry

[Vivek Goyal]

On Thu, Mar 08, 2018 at 10:43:09AM +0200, Amir Goldstein wrote:
> On Wed, Mar 7, 2018 at 10:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Mar 07, 2018 at 04:42:54PM +0200, Amir Goldstein wrote:
> [...]
> 
> >> >  static bool ovl_is_opaquedir(struct dentry *dentry)
> >> >  {
> >> >         return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);
> >> > @@ -242,9 +265,16 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
> >> >                 goto put_and_out;
> >> >         }
> >> >         if (!d_can_lookup(this)) {
> >> > -               d->stop = true;
> >> >                 if (d->is_dir)
> >>
> >> You need to set d->stop here because this is a non-dir below upper dir.
> >
> > With metacopy dentry on regular files, we want to do d->stop=true only
> > if this is not a metacopy. Otherwise we want to continue to do path
> > based lookup in lower layers. That's the whole point of this patch
> > series.
> >
> 
> Yes we do, but the point that yo missed about (d->is_dir) check is that it means
> that upper layer was a directory (e.g. upperdir/foo) and underneath it we find
> a non-dir (e.g. lowerdir/foo). This is a case were lookup needs to stop and
> result in a non-merge pure upper dir.

But d->is_dir does not necessarily reflect the state of upper. It
reflects the state of last found dentry. And in case of redirect, last
found dentry can very well be parent of current dentry.

For example, say you have upperdir/foo and it has redirect set to 
lowerdir/bar/bar-child.txt. Now, by the time we find bar-child.txt
d->is_dir is always true because it reflects the state of parent "bar"
dentry which is always directory. I mean it does not reflect state
of upperdir/foo. And that state is already lost.

If we really want to make sure that we don't merge upper dir with lower
non-dir, then we need to do this check in ovl_lookup() and not in
ovl_lookup_single(), IMHO.

> 
> >>
> >> >                         goto put_and_out;
> >> > +               err = ovl_check_metacopy_xattr(this);
> >> > +               if (err < 0)
> >> > +                       goto out_err;
> >> > +               if (!err) {
> >> > +                       d->stop = true;
> >> > +                       d->metacopy = false;
> >> > +               } else
> >> > +                       d->metacopy = true;
> >>
> >> Need to have {} in both if and else, but better not use if at all:
> >> d->stop = !err;
> >> d->metacopy = !!err;
> >
> > I think I will put {}. Its much easier to read the code that way.
> 
> Well, execution branch is less efficient and I don't know if compiler
> will optimize it away, but in any case I think d->metacopy = err is
> nicer because  ovl_check_metacopy_xattr() returns an error or
> an answer to is_metacopy.
> 
> [...]
> 
> >> > +               /*
> >> > +                * For non-dir dentry, make sure dentry found by lookup
> >> > +                * matches the origin stored in upper
> >> > +                */
> >> > +               if (!d.is_dir && upperdentry && !ctr && origin_path) {
> >> > +                       err = ovl_verify_origin(upperdentry, this, false);
> >> > +                       if (err) {
> >> > +                               dput(this);
> >> > +                               goto out_put;
> >> > +                       }
> >> > +               }
> >> > +
> >>
> >> Why is this code duplicated from d.is_dir case?
> >
> > Primarily because for the case of dir, you are verifying origin only
> > if nfs_export is enabled. While I doing this verification even when
> > nfs_export is not enabled.
> 
> (d.is_dir && ovl_verify_lower(dentry->d_sb)) || (!d.is_dir && origin_path)
> 
> The case of verifying dir found by path and file found by found is exactly
> the same case. You should just include the new case in the comment above
> current code. The idea is to make metacopy lookup as similar as possible
> to merge dir lookup, not to have a different code path for it.

Ok, will do.

> 
> >
> > So far by default we always installed origin in lowerstack[0] for non-dir.
> > Now we are doing lookup for non-dir files and looked up file might
> > be different from origin. So we need to make sure file found by lookup
> > is same as pointed by origin, otherwise something is not right?
> >
> 
> [...]
> 
> >> > +       } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
> >> > +               if (WARN_ON(stack != NULL)) {
> >> > +                       err = -EIO;
> >> > +                       goto out_put;
> >> > +               }
> >> > +               stack = origin_path;
> >> > +               ctr = 1;
> >> > +               origin_path = NULL;
> >>
> >> I am having a hard time understanding why origin_path is needed at all
> >> and why not lookup only by path and then ovl_verify_origin().
> >
> > Right now we look for non-dir file in lower only if upper is metacopy. So
> > if upper is not metacopy, then origin has to be installed in
> > lowerstack[0]. That's why I save origin in origin_path, and install it
> > in lowerstack[0] when there is no lower.
> 
> OK. you don't need to allocate origin_path, because it is limited to stack
> size of 1. See how ovl_verify_index() initializes stock and origin vars.

Ok, I can allocate origin_path on stack in caller instead of relying on
ovl_check_origin_fh() doing kmalloc.

> 
> >
> > Now other way could be that we lookup for dentry in lower even for
> > non-metacopy non-dir files and do ovl_verify_origin(). I think that's
> > what you are referring to. But that seems like unnecessary lookup in
> > lower. We don't have to do it.
> >
> 
> Nah, that will require also doing redirect on non-dir without metacopy.

Thanks
Vivek

Re: [PATCH v12 08/17] ovl: Modify ovl_lookup() and friends to lookup metacopy dentry

[Amir Goldstein]

On Thu, Mar 8, 2018 at 7:03 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Mar 08, 2018 at 10:43:09AM +0200, Amir Goldstein wrote:
>> On Wed, Mar 7, 2018 at 10:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Wed, Mar 07, 2018 at 04:42:54PM +0200, Amir Goldstein wrote:
>> [...]
>>
>> >> >  static bool ovl_is_opaquedir(struct dentry *dentry)
>> >> >  {
>> >> >         return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);
>> >> > @@ -242,9 +265,16 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
>> >> >                 goto put_and_out;
>> >> >         }
>> >> >         if (!d_can_lookup(this)) {
>> >> > -               d->stop = true;
>> >> >                 if (d->is_dir)
>> >>
>> >> You need to set d->stop here because this is a non-dir below upper dir.
>> >
>> > With metacopy dentry on regular files, we want to do d->stop=true only
>> > if this is not a metacopy. Otherwise we want to continue to do path
>> > based lookup in lower layers. That's the whole point of this patch
>> > series.
>> >
>>
>> Yes we do, but the point that yo missed about (d->is_dir) check is that it means
>> that upper layer was a directory (e.g. upperdir/foo) and underneath it we find
>> a non-dir (e.g. lowerdir/foo). This is a case were lookup needs to stop and
>> result in a non-merge pure upper dir.
>
> But d->is_dir does not necessarily reflect the state of upper. It
> reflects the state of last found dentry. And in case of redirect, last
> found dentry can very well be parent of current dentry.
>
> For example, say you have upperdir/foo and it has redirect set to
> lowerdir/bar/bar-child.txt. Now, by the time we find bar-child.txt
> d->is_dir is always true because it reflects the state of parent "bar"
> dentry which is always directory. I mean it does not reflect state
> of upperdir/foo. And that state is already lost.
>
> If we really want to make sure that we don't merge upper dir with lower
> non-dir, then we need to do this check in ovl_lookup() and not in
> ovl_lookup_single(), IMHO.
>

Maybe. need to see a patch.

Currently, d->is_dir can be used to stop merge dir lookup,
but with introduction of a merge-non-dir (i.e. metacopy)
ovl_lookup_layer() is broken, because it looses previous layer
d->is_dir information while doing absolute path lookup.

If I am not mistaken, it looks like ovl_lookup_layer() is already
broken w.r.t d->opaque. It should not be setting d->opaque
when doing ovl_lookup_single() on a non-last path element
if it find an opaquedir.

You can tell if ovl_lookup_single() is being called for a last path
element from ovl_lookup_layer() if 'post' is an empty string.
You can use that to determine if you need to set d->is_dir for
directory and d->stop for non-dir if (d->is_dir).

I lean towards getting the semantics of ovl_lookup_layer()
ovl_lookup_single() and the ovl_lookup_data state adjusted
to metacopy lookup and getting rid of the hidden assumptions
that are inside current code.

Thanks,
Amir.

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Vivek Goyal]

On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote:
> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > When a metacopy file is no longer a metacopy and data has been copied up,
> > remove REDIRECT xattr. Its not needed anymore.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 0c8d2755bd25..704febd2e2fa 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> >         if (err)
> >                 return err;
> >
> > +       /*
> > +        * A metacopy files does not need redirect xattr once data has
> > +        * been copied up.
> > +        */
> > +       err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
> > +       if (err && err != -ENODATA && err != -EOPNOTSUPP)
> > +               return err;
> > +
> > +       err = 0;
> >         ovl_set_upperdata(d_inode(c->dentry));
> >         return err;
> 
> By intuition, I would say that removing redirect should be done after setting
> upperdata flag. Not sure if it really matters in real life.
> Maybe when racing a lookup of a metacopy hardlink and copy up data of
> an upper alias?

I think you found a good race situation. 

>  
> Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
> probably use a helper ovl_clear_redirect() for the locking.
> 
> But that highlights a serious problem with current patches -
> Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()
> and additionally with dentry->d_lock in ovl_rename()
> That is sufficient for directories which can only have a single dentry
> alias to an
> inode but not at all sufficient for non-directories.

This is a good point. So we need to protect OVL_I(inode)->redirect with
oi->lock mutex as well (atleast for non-dirs). So ovl_rename() will nest
3 locks (which it already does for index case).

parent dir i_mutex.
 oi->lock
   dentry->d_lock().

I will try to write a patch for this and see what issues do I face

Vivek
> 
> Thanks,
> Amir.

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Vivek Goyal]

On Wed, Mar 14, 2018 at 03:15:33PM -0400, Vivek Goyal wrote:
> On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote:
> > On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > When a metacopy file is no longer a metacopy and data has been copied up,
> > > remove REDIRECT xattr. Its not needed anymore.
> > >
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  fs/overlayfs/copy_up.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > > index 0c8d2755bd25..704febd2e2fa 100644
> > > --- a/fs/overlayfs/copy_up.c
> > > +++ b/fs/overlayfs/copy_up.c
> > > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> > >         if (err)
> > >                 return err;
> > >
> > > +       /*
> > > +        * A metacopy files does not need redirect xattr once data has
> > > +        * been copied up.
> > > +        */
> > > +       err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
> > > +       if (err && err != -ENODATA && err != -EOPNOTSUPP)
> > > +               return err;
> > > +
> > > +       err = 0;
> > >         ovl_set_upperdata(d_inode(c->dentry));
> > >         return err;
> > 
> > By intuition, I would say that removing redirect should be done after setting
> > upperdata flag. Not sure if it really matters in real life.
> > Maybe when racing a lookup of a metacopy hardlink and copy up data of
> > an upper alias?
> 
> I think you found a good race situation. 
> 
> >  
> > Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
> > probably use a helper ovl_clear_redirect() for the locking.
> > 
> > But that highlights a serious problem with current patches -
> > Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()
> > and additionally with dentry->d_lock in ovl_rename()
> > That is sufficient for directories which can only have a single dentry
> > alias to an
> > inode but not at all sufficient for non-directories.
> 
> This is a good point. So we need to protect OVL_I(inode)->redirect with
> oi->lock mutex as well (atleast for non-dirs). So ovl_rename() will nest
> 3 locks (which it already does for index case).
> 
> parent dir i_mutex.
>  oi->lock
>    dentry->d_lock().
> 
> I will try to write a patch for this and see what issues do I face

Hi Amir,

I am trying to understand better how you are taking oi->lock w.r.t
nlink stuff and I am having a hard time.

- Why do you keep oi->locked for the duration of operation (link, unlink
  etc) using ovl_nlink_start() and ovl_nlink_end().

- What exactly is oi->lock protecting. I understand its protecting
  copy up. What else. Comment just says "synchronize copy up and more"
  and its not clear what is that "more".

  I need to understand that better to be able to use this lock for
  protecting "oi->redirect" for the case of hard links.

Vivek

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Amir Goldstein]

On Thu, Mar 15, 2018 at 8:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Mar 14, 2018 at 03:15:33PM -0400, Vivek Goyal wrote:
>> On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote:
>> > On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > > When a metacopy file is no longer a metacopy and data has been copied up,
>> > > remove REDIRECT xattr. Its not needed anymore.
>> > >
>> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > > ---
>> > >  fs/overlayfs/copy_up.c | 9 +++++++++
>> > >  1 file changed, 9 insertions(+)
>> > >
>> > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> > > index 0c8d2755bd25..704febd2e2fa 100644
>> > > --- a/fs/overlayfs/copy_up.c
>> > > +++ b/fs/overlayfs/copy_up.c
>> > > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>> > >         if (err)
>> > >                 return err;
>> > >
>> > > +       /*
>> > > +        * A metacopy files does not need redirect xattr once data has
>> > > +        * been copied up.
>> > > +        */
>> > > +       err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
>> > > +       if (err && err != -ENODATA && err != -EOPNOTSUPP)
>> > > +               return err;
>> > > +
>> > > +       err = 0;
>> > >         ovl_set_upperdata(d_inode(c->dentry));
>> > >         return err;
>> >
>> > By intuition, I would say that removing redirect should be done after setting
>> > upperdata flag. Not sure if it really matters in real life.
>> > Maybe when racing a lookup of a metacopy hardlink and copy up data of
>> > an upper alias?
>>
>> I think you found a good race situation.
>>
>> >
>> > Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
>> > probably use a helper ovl_clear_redirect() for the locking.
>> >
>> > But that highlights a serious problem with current patches -
>> > Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()
>> > and additionally with dentry->d_lock in ovl_rename()
>> > That is sufficient for directories which can only have a single dentry
>> > alias to an
>> > inode but not at all sufficient for non-directories.
>>
>> This is a good point. So we need to protect OVL_I(inode)->redirect with
>> oi->lock mutex as well (atleast for non-dirs). So ovl_rename() will nest
>> 3 locks (which it already does for index case).
>>
>> parent dir i_mutex.
>>  oi->lock
>>    dentry->d_lock().
>>
>> I will try to write a patch for this and see what issues do I face
>
> Hi Amir,
>
> I am trying to understand better how you are taking oi->lock w.r.t
> nlink stuff and I am having a hard time.
>
> - Why do you keep oi->locked for the duration of operation (link, unlink
>   etc) using ovl_nlink_start() and ovl_nlink_end().

As the comment above ovl_nlink_start() says, union nlink may be changed
by link(), unlink() and copyup. nlink is an overlay inode property, so we need
to protect its updates with a lock on the inode object, which in this level if
oi->lock. Also, in ovl_nlink_end() we cleanup the index on last union nlink drop
and we need to do that also under inode object lock.

>
> - What exactly is oi->lock protecting. I understand its protecting
>   copy up. What else. Comment just says "synchronize copy up and more"
>   and its not clear what is that "more".

It is protecting operations that need to be serialized on the overlay inode
object, while inode->i_rwsem cannot be used for that purpose, because
it is used earlier in the locking order by VFS layer.
Those operations include copyup and union nlink update. I don't recall
there are other users for oi->lock at the moment, but did not check.

>
>   I need to understand that better to be able to use this lock for
>   protecting "oi->redirect" for the case of hard links.
>

If I am not mistaken, as long as you preserve locking order, you
should be able to use oi->lock to protect oi->redirect updates.
After all oi->redirect is really a property on the overlay inode.
It just happens to be protected a dentry level lock, from the time
that redirect was stored in the overlay dentry and that is only
still correct because of the one to one mapping between directory
inode and dentry.

Cheers,
Amir.

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Vivek Goyal]

On Thu, Mar 15, 2018 at 10:42:11PM +0200, Amir Goldstein wrote:
> On Thu, Mar 15, 2018 at 8:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Mar 14, 2018 at 03:15:33PM -0400, Vivek Goyal wrote:
> >> On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote:
> >> > On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > > When a metacopy file is no longer a metacopy and data has been copied up,
> >> > > remove REDIRECT xattr. Its not needed anymore.
> >> > >
> >> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> >> > > ---
> >> > >  fs/overlayfs/copy_up.c | 9 +++++++++
> >> > >  1 file changed, 9 insertions(+)
> >> > >
> >> > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> >> > > index 0c8d2755bd25..704febd2e2fa 100644
> >> > > --- a/fs/overlayfs/copy_up.c
> >> > > +++ b/fs/overlayfs/copy_up.c
> >> > > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> >> > >         if (err)
> >> > >                 return err;
> >> > >
> >> > > +       /*
> >> > > +        * A metacopy files does not need redirect xattr once data has
> >> > > +        * been copied up.
> >> > > +        */
> >> > > +       err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
> >> > > +       if (err && err != -ENODATA && err != -EOPNOTSUPP)
> >> > > +               return err;
> >> > > +
> >> > > +       err = 0;
> >> > >         ovl_set_upperdata(d_inode(c->dentry));
> >> > >         return err;
> >> >
> >> > By intuition, I would say that removing redirect should be done after setting
> >> > upperdata flag. Not sure if it really matters in real life.
> >> > Maybe when racing a lookup of a metacopy hardlink and copy up data of
> >> > an upper alias?
> >>
> >> I think you found a good race situation.
> >>
> >> >
> >> > Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
> >> > probably use a helper ovl_clear_redirect() for the locking.
> >> >
> >> > But that highlights a serious problem with current patches -
> >> > Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()
> >> > and additionally with dentry->d_lock in ovl_rename()
> >> > That is sufficient for directories which can only have a single dentry
> >> > alias to an
> >> > inode but not at all sufficient for non-directories.
> >>
> >> This is a good point. So we need to protect OVL_I(inode)->redirect with
> >> oi->lock mutex as well (atleast for non-dirs). So ovl_rename() will nest
> >> 3 locks (which it already does for index case).
> >>
> >> parent dir i_mutex.
> >>  oi->lock
> >>    dentry->d_lock().
> >>
> >> I will try to write a patch for this and see what issues do I face
> >
> > Hi Amir,
> >
> > I am trying to understand better how you are taking oi->lock w.r.t
> > nlink stuff and I am having a hard time.
> >
> > - Why do you keep oi->locked for the duration of operation (link, unlink
> >   etc) using ovl_nlink_start() and ovl_nlink_end().
> 
> As the comment above ovl_nlink_start() says, union nlink may be changed
> by link(), unlink() and copyup. nlink is an overlay inode property, so we need
> to protect its updates with a lock on the inode object, which in this level if
> oi->lock. Also, in ovl_nlink_end() we cleanup the index on last union nlink drop
> and we need to do that also under inode object lock.

Sure. What I don't understand is that why do we have to continue to hold
the lock for the whole duration. Can we drop the lock and re-acquire it
before we cleanup index and change nlink value on ovelray inode?

Thanks
Vivek

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Amir Goldstein]

On Fri, Mar 16, 2018 at 2:52 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Mar 15, 2018 at 10:42:11PM +0200, Amir Goldstein wrote:
>> On Thu, Mar 15, 2018 at 8:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Wed, Mar 14, 2018 at 03:15:33PM -0400, Vivek Goyal wrote:
>> >> On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote:
>> >> > On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> > > When a metacopy file is no longer a metacopy and data has been copied up,
>> >> > > remove REDIRECT xattr. Its not needed anymore.
>> >> > >
>> >> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> >> > > ---
>> >> > >  fs/overlayfs/copy_up.c | 9 +++++++++
>> >> > >  1 file changed, 9 insertions(+)
>> >> > >
>> >> > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> >> > > index 0c8d2755bd25..704febd2e2fa 100644
>> >> > > --- a/fs/overlayfs/copy_up.c
>> >> > > +++ b/fs/overlayfs/copy_up.c
>> >> > > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>> >> > >         if (err)
>> >> > >                 return err;
>> >> > >
>> >> > > +       /*
>> >> > > +        * A metacopy files does not need redirect xattr once data has
>> >> > > +        * been copied up.
>> >> > > +        */
>> >> > > +       err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
>> >> > > +       if (err && err != -ENODATA && err != -EOPNOTSUPP)
>> >> > > +               return err;
>> >> > > +
>> >> > > +       err = 0;
>> >> > >         ovl_set_upperdata(d_inode(c->dentry));
>> >> > >         return err;
>> >> >
>> >> > By intuition, I would say that removing redirect should be done after setting
>> >> > upperdata flag. Not sure if it really matters in real life.
>> >> > Maybe when racing a lookup of a metacopy hardlink and copy up data of
>> >> > an upper alias?
>> >>
>> >> I think you found a good race situation.
>> >>
>> >> >
>> >> > Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
>> >> > probably use a helper ovl_clear_redirect() for the locking.
>> >> >
>> >> > But that highlights a serious problem with current patches -
>> >> > Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()
>> >> > and additionally with dentry->d_lock in ovl_rename()
>> >> > That is sufficient for directories which can only have a single dentry
>> >> > alias to an
>> >> > inode but not at all sufficient for non-directories.
>> >>
>> >> This is a good point. So we need to protect OVL_I(inode)->redirect with
>> >> oi->lock mutex as well (atleast for non-dirs). So ovl_rename() will nest
>> >> 3 locks (which it already does for index case).
>> >>
>> >> parent dir i_mutex.
>> >>  oi->lock
>> >>    dentry->d_lock().
>> >>
>> >> I will try to write a patch for this and see what issues do I face
>> >
>> > Hi Amir,
>> >
>> > I am trying to understand better how you are taking oi->lock w.r.t
>> > nlink stuff and I am having a hard time.
>> >
>> > - Why do you keep oi->locked for the duration of operation (link, unlink
>> >   etc) using ovl_nlink_start() and ovl_nlink_end().
>>
>> As the comment above ovl_nlink_start() says, union nlink may be changed
>> by link(), unlink() and copyup. nlink is an overlay inode property, so we need
>> to protect its updates with a lock on the inode object, which in this level if
>> oi->lock. Also, in ovl_nlink_end() we cleanup the index on last union nlink drop
>> and we need to do that also under inode object lock.
>
> Sure. What I don't understand is that why do we have to continue to hold
> the lock for the whole duration. Can we drop the lock and re-acquire it
> before we cleanup index and change nlink value on ovelray inode?
>

No. When copyup calls ovl_set_nlink_upper() the value to write in NLINK
xattr is computed from ovl_inode->i_link and realinode->i_nlink.
If we drop the lock between ovl_nlink_start() and ovl_nlink_end(), realinode
nlink can change while copyup is in progress and then union nlink will be
messed up.

What is exactly the problem that you are trying to solve?
It seems that you need to protect oi->redirect in copyup/rename/link.
copyup/link already take the oi->lock and rename takes oi->lock
on new inode in case of "overwrite".
A simple solution would be to call ovl_nlink_start()/ovl_nlink_end()
in rename for both old and new inodes, regardless of "overwrite".
It may be unneeded, but in fact, ovl_nlink_start() doesn't do
anything wrong, it just recomputes NLINK xattr and most of those
recomputes will store the same value anyway, unless machine crashes
during copyup between ovl_set_nlink_lower() and
ovl_set_nlink_upper() and leaves the value of NLINK xattr relative to
lower nlink.

Amir.

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Vivek Goyal]

On Fri, Mar 16, 2018 at 03:17:47PM +0200, Amir Goldstein wrote:
> On Fri, Mar 16, 2018 at 2:52 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Thu, Mar 15, 2018 at 10:42:11PM +0200, Amir Goldstein wrote:
> >> On Thu, Mar 15, 2018 at 8:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > On Wed, Mar 14, 2018 at 03:15:33PM -0400, Vivek Goyal wrote:
> >> >> On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote:
> >> >> > On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> >> > > When a metacopy file is no longer a metacopy and data has been copied up,
> >> >> > > remove REDIRECT xattr. Its not needed anymore.
> >> >> > >
> >> >> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> >> >> > > ---
> >> >> > >  fs/overlayfs/copy_up.c | 9 +++++++++
> >> >> > >  1 file changed, 9 insertions(+)
> >> >> > >
> >> >> > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> >> >> > > index 0c8d2755bd25..704febd2e2fa 100644
> >> >> > > --- a/fs/overlayfs/copy_up.c
> >> >> > > +++ b/fs/overlayfs/copy_up.c
> >> >> > > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> >> >> > >         if (err)
> >> >> > >                 return err;
> >> >> > >
> >> >> > > +       /*
> >> >> > > +        * A metacopy files does not need redirect xattr once data has
> >> >> > > +        * been copied up.
> >> >> > > +        */
> >> >> > > +       err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
> >> >> > > +       if (err && err != -ENODATA && err != -EOPNOTSUPP)
> >> >> > > +               return err;
> >> >> > > +
> >> >> > > +       err = 0;
> >> >> > >         ovl_set_upperdata(d_inode(c->dentry));
> >> >> > >         return err;
> >> >> >
> >> >> > By intuition, I would say that removing redirect should be done after setting
> >> >> > upperdata flag. Not sure if it really matters in real life.
> >> >> > Maybe when racing a lookup of a metacopy hardlink and copy up data of
> >> >> > an upper alias?
> >> >>
> >> >> I think you found a good race situation.
> >> >>
> >> >> >
> >> >> > Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
> >> >> > probably use a helper ovl_clear_redirect() for the locking.
> >> >> >
> >> >> > But that highlights a serious problem with current patches -
> >> >> > Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()
> >> >> > and additionally with dentry->d_lock in ovl_rename()
> >> >> > That is sufficient for directories which can only have a single dentry
> >> >> > alias to an
> >> >> > inode but not at all sufficient for non-directories.
> >> >>
> >> >> This is a good point. So we need to protect OVL_I(inode)->redirect with
> >> >> oi->lock mutex as well (atleast for non-dirs). So ovl_rename() will nest
> >> >> 3 locks (which it already does for index case).
> >> >>
> >> >> parent dir i_mutex.
> >> >>  oi->lock
> >> >>    dentry->d_lock().
> >> >>
> >> >> I will try to write a patch for this and see what issues do I face
> >> >
> >> > Hi Amir,
> >> >
> >> > I am trying to understand better how you are taking oi->lock w.r.t
> >> > nlink stuff and I am having a hard time.
> >> >
> >> > - Why do you keep oi->locked for the duration of operation (link, unlink
> >> >   etc) using ovl_nlink_start() and ovl_nlink_end().
> >>
> >> As the comment above ovl_nlink_start() says, union nlink may be changed
> >> by link(), unlink() and copyup. nlink is an overlay inode property, so we need
> >> to protect its updates with a lock on the inode object, which in this level if
> >> oi->lock. Also, in ovl_nlink_end() we cleanup the index on last union nlink drop
> >> and we need to do that also under inode object lock.
> >
> > Sure. What I don't understand is that why do we have to continue to hold
> > the lock for the whole duration. Can we drop the lock and re-acquire it
> > before we cleanup index and change nlink value on ovelray inode?
> >
> 
> No. When copyup calls ovl_set_nlink_upper() the value to write in NLINK
> xattr is computed from ovl_inode->i_link and realinode->i_nlink.
> If we drop the lock between ovl_nlink_start() and ovl_nlink_end(), realinode
> nlink can change while copyup is in progress and then union nlink will be
> messed up.

I am just trying to understand this nlink stuff and associated locking
better. It has confused me many a times.

Can you give me an example where things will go wrong if we drop the
lock after setting ovl_set_nlink_upper(). I have spent enough time
thinking about it and can't think what will go wrong.

> 
> What is exactly the problem that you are trying to solve?
> It seems that you need to protect oi->redirect in copyup/rename/link.
> copyup/link already take the oi->lock and rename takes oi->lock
> on new inode in case of "overwrite".
> A simple solution would be to call ovl_nlink_start()/ovl_nlink_end()
> in rename for both old and new inodes, regardless of "overwrite".
> It may be unneeded, but in fact, ovl_nlink_start() doesn't do
> anything wrong, it just recomputes NLINK xattr and most of those
> recomputes will store the same value anyway, unless machine crashes
> during copyup between ovl_set_nlink_lower() and
> ovl_set_nlink_upper() and leaves the value of NLINK xattr relative to
> lower nlink.

ovl_nlink_start() also assumes that file is indexed. metadata copy up
stuff does not have dependency on index.

So I am instead passing "locked" state to ovl_set_redirect() and
ovl_get_redirect(), and if oi->lock is not already held, then
these functions will acquire it for non-dir.

I meant to ask you one more question. Without indexing it is possible
that two upper layer hardlinks (broken hardlinks), have redirects to
same lower. I know that for the case of directories, you don't want
two redirects to same lower. I am wondering what's the problem it 
leads to and if same problem applies for non-dir as well?

Thanks
Vivek
> 
> Amir.

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Amir Goldstein]

On Fri, Mar 16, 2018 at 5:06 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Mar 16, 2018 at 03:17:47PM +0200, Amir Goldstein wrote:
[...]
>
> I am just trying to understand this nlink stuff and associated locking
> better. It has confused me many a times.
>

There are only two rules to understand:
1. The delta between upper nlink and union nlink doesn't change on link()
    unlink() rename()
2. The delta between lower nlink and union nlink doesn't change op copyup

So all we need to do to make union nlink crash consistent is make sure
that we store NLINK xattr relative to lower before copyup and store it
relative to upper nlink before link/unlink/rename.

If we allow copyup (of lower hardlink) and link (of upper hardlink) at the
same time, we cannot guaranty crash consistency of union nlink.

> Can you give me an example where things will go wrong if we drop the
> lock after setting ovl_set_nlink_upper(). I have spent enough time
> thinking about it and can't think what will go wrong.
>

lower nlink = 2
upper nlink = 2 (1 copy up and 1 index)
union nlink = 2
NLINK xattr = "U+0"

start link():
oi->lock
store NLINK xattr = "U+0"
oi->unlock
...
ovl_do_link() (but not yet inc_nlink(inode))

start copyup():
oi->lock
store NLINK xattr = "L+0"
copy up inode
store NLINK xattr = "U-2" (because upper nlink is now 4, but
inode->i_nlink is still 2)

CRASH

BOOT

ovl_get_nlink()

lower nlink = 2
upper nlink = 4 (2 copy ups, 1 hardlink and 1 index)
NLINK xattr = "U-2"
union nlink = 2 (WRONG should be 3)

Now unlink the 2 copy ups and the new hardlinks and you hit
WARN_ON(inode->i_nlink == 0) in drop_nlink()

Hope I got this right...


>>
>> What is exactly the problem that you are trying to solve?
>> It seems that you need to protect oi->redirect in copyup/rename/link.
>> copyup/link already take the oi->lock and rename takes oi->lock
>> on new inode in case of "overwrite".
>> A simple solution would be to call ovl_nlink_start()/ovl_nlink_end()
>> in rename for both old and new inodes, regardless of "overwrite".
>> It may be unneeded, but in fact, ovl_nlink_start() doesn't do
>> anything wrong, it just recomputes NLINK xattr and most of those
>> recomputes will store the same value anyway, unless machine crashes
>> during copyup between ovl_set_nlink_lower() and
>> ovl_set_nlink_upper() and leaves the value of NLINK xattr relative to
>> lower nlink.
>
> ovl_nlink_start() also assumes that file is indexed. metadata copy up
> stuff does not have dependency on index.

That's probably ok because you can set independent redirects on
different broken hardlinks of the same lower.

>
> So I am instead passing "locked" state to ovl_set_redirect() and
> ovl_get_redirect(), and if oi->lock is not already held, then
> these functions will acquire it for non-dir.

Sounds ok.

>
> I meant to ask you one more question. Without indexing it is possible
> that two upper layer hardlinks (broken hardlinks), have redirects to
> same lower. I know that for the case of directories, you don't want
> two redirects to same lower. I am wondering what's the problem it
> leads to and if same problem applies for non-dir as well?

Yes, see in the test:
https://github.com/amir73il/xfstests/blob/overlayfs-devel/tests/overlay/049#L98

two redirects can have the same st_ino if lower nlink == 1 and
they are not indexed.

Thanks,
Amir.

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Vivek Goyal]

On Fri, Mar 16, 2018 at 06:09:31PM +0200, Amir Goldstein wrote:
> On Fri, Mar 16, 2018 at 5:06 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Fri, Mar 16, 2018 at 03:17:47PM +0200, Amir Goldstein wrote:
> [...]
> >
> > I am just trying to understand this nlink stuff and associated locking
> > better. It has confused me many a times.
> >
> 
> There are only two rules to understand:
> 1. The delta between upper nlink and union nlink doesn't change on link()
>     unlink() rename()
> 2. The delta between lower nlink and union nlink doesn't change op copyup
> 
> So all we need to do to make union nlink crash consistent is make sure
> that we store NLINK xattr relative to lower before copyup and store it
> relative to upper nlink before link/unlink/rename.
> 
> If we allow copyup (of lower hardlink) and link (of upper hardlink) at the
> same time, we cannot guaranty crash consistency of union nlink.
> 
> > Can you give me an example where things will go wrong if we drop the
> > lock after setting ovl_set_nlink_upper(). I have spent enough time
> > thinking about it and can't think what will go wrong.
> >
> 
> lower nlink = 2
> upper nlink = 2 (1 copy up and 1 index)
> union nlink = 2
> NLINK xattr = "U+0"
> 
> start link():
> oi->lock
> store NLINK xattr = "U+0"
> oi->unlock
> ...
> ovl_do_link() (but not yet inc_nlink(inode))
> 
> start copyup():
> oi->lock
> store NLINK xattr = "L+0"
> copy up inode
> store NLINK xattr = "U-2" (because upper nlink is now 4, but
> inode->i_nlink is still 2)
> 
> CRASH
> 
> BOOT
> 
> ovl_get_nlink()
> 
> lower nlink = 2
> upper nlink = 4 (2 copy ups, 1 hardlink and 1 index)
> NLINK xattr = "U-2"
> union nlink = 2 (WRONG should be 3)
> 
> Now unlink the 2 copy ups and the new hardlinks and you hit
> WARN_ON(inode->i_nlink == 0) in drop_nlink()
> 
> Hope I got this right...

Aha... I get it now. This is a good example which shows why we need
to keep holding the ovl_inode->lock. Thanks.

> 
> 
> >>
> >> What is exactly the problem that you are trying to solve?
> >> It seems that you need to protect oi->redirect in copyup/rename/link.
> >> copyup/link already take the oi->lock and rename takes oi->lock
> >> on new inode in case of "overwrite".
> >> A simple solution would be to call ovl_nlink_start()/ovl_nlink_end()
> >> in rename for both old and new inodes, regardless of "overwrite".
> >> It may be unneeded, but in fact, ovl_nlink_start() doesn't do
> >> anything wrong, it just recomputes NLINK xattr and most of those
> >> recomputes will store the same value anyway, unless machine crashes
> >> during copyup between ovl_set_nlink_lower() and
> >> ovl_set_nlink_upper() and leaves the value of NLINK xattr relative to
> >> lower nlink.
> >
> > ovl_nlink_start() also assumes that file is indexed. metadata copy up
> > stuff does not have dependency on index.
> 
> That's probably ok because you can set independent redirects on
> different broken hardlinks of the same lower.
> 
> >
> > So I am instead passing "locked" state to ovl_set_redirect() and
> > ovl_get_redirect(), and if oi->lock is not already held, then
> > these functions will acquire it for non-dir.
> 
> Sounds ok.
> 
> >
> > I meant to ask you one more question. Without indexing it is possible
> > that two upper layer hardlinks (broken hardlinks), have redirects to
> > same lower. I know that for the case of directories, you don't want
> > two redirects to same lower. I am wondering what's the problem it
> > leads to and if same problem applies for non-dir as well?
> 
> Yes, see in the test:
> https://github.com/amir73il/xfstests/blob/overlayfs-devel/tests/overlay/049#L98
> 
> two redirects can have the same st_ino if lower nlink == 1 and
> they are not indexed.

Ok, thanks. I will need to spend some more time on this and see if
I should make index=on mandatory for metacopy=on.

Vivek

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Vivek Goyal]

On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote:
> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > When a metacopy file is no longer a metacopy and data has been copied up,
> > remove REDIRECT xattr. Its not needed anymore.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 0c8d2755bd25..704febd2e2fa 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> >         if (err)
> >                 return err;
> >
> > +       /*
> > +        * A metacopy files does not need redirect xattr once data has
> > +        * been copied up.
> > +        */
> > +       err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
> > +       if (err && err != -ENODATA && err != -EOPNOTSUPP)
> > +               return err;
> > +
> > +       err = 0;
> >         ovl_set_upperdata(d_inode(c->dentry));
> >         return err;
> 
> By intuition, I would say that removing redirect should be done after setting
> upperdata flag. Not sure if it really matters in real life.
> Maybe when racing a lookup of a metacopy hardlink and copy up data of
> an upper alias?
> 
> Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
> probably use a helper ovl_clear_redirect() for the locking.
> 
> But that highlights a serious problem with current patches -
> Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()
> and additionally with dentry->d_lock in ovl_rename()
> That is sufficient for directories which can only have a single dentry
> alias to an
> inode but not at all sufficient for non-directories.

Hi Amir, 

I think I have got the locking of ovl_inode right now. Given this
situation, I am looking at ovl_lookup() and wondering, why certain
properties which belong to inode are still being initialized during
dentry initialization (That too without any lock). That sounds wrong.

For example.

ovl_lookup() {
	if (index)
		ovl_set_flag(OVL_INDEX, inode);
}

This is ovl_inode() property and should be initialized when inode is
created first time in cache? And updated later in copy up path as index
is created later. What's the reason to update inode property in
ovl_lookup() if inode is alreday in cache.

I have similar question for oi->redirect. This is inode property as
well and should be set in ovl_get_inode(), if inode was allocated
new. And don't do anything for an existing inode.

Thanks
Vivek

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Vivek Goyal]

On Tue, Mar 20, 2018 at 03:26:19PM -0400, Vivek Goyal wrote:
> On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote:
> > On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > When a metacopy file is no longer a metacopy and data has been copied up,
> > > remove REDIRECT xattr. Its not needed anymore.
> > >
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  fs/overlayfs/copy_up.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > > index 0c8d2755bd25..704febd2e2fa 100644
> > > --- a/fs/overlayfs/copy_up.c
> > > +++ b/fs/overlayfs/copy_up.c
> > > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> > >         if (err)
> > >                 return err;
> > >
> > > +       /*
> > > +        * A metacopy files does not need redirect xattr once data has
> > > +        * been copied up.
> > > +        */
> > > +       err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
> > > +       if (err && err != -ENODATA && err != -EOPNOTSUPP)
> > > +               return err;
> > > +
> > > +       err = 0;
> > >         ovl_set_upperdata(d_inode(c->dentry));
> > >         return err;
> > 
> > By intuition, I would say that removing redirect should be done after setting
> > upperdata flag. Not sure if it really matters in real life.
> > Maybe when racing a lookup of a metacopy hardlink and copy up data of
> > an upper alias?
> > 
> > Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
> > probably use a helper ovl_clear_redirect() for the locking.
> > 
> > But that highlights a serious problem with current patches -
> > Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()
> > and additionally with dentry->d_lock in ovl_rename()
> > That is sufficient for directories which can only have a single dentry
> > alias to an
> > inode but not at all sufficient for non-directories.
> 
> Hi Amir, 
> 
> I think I have got the locking of ovl_inode right now. Given this
> situation, I am looking at ovl_lookup() and wondering, why certain
> properties which belong to inode are still being initialized during
> dentry initialization (That too without any lock). That sounds wrong.
> 
> For example.
> 
> ovl_lookup() {
> 	if (index)
> 		ovl_set_flag(OVL_INDEX, inode);
> }
> 
> This is ovl_inode() property and should be initialized when inode is
> created first time in cache? And updated later in copy up path as index
> is created later. What's the reason to update inode property in
> ovl_lookup() if inode is alreday in cache.

IOW, will a patch as below fly?

---
 fs/overlayfs/export.c |    3 ---
 fs/overlayfs/inode.c  |    3 +++
 fs/overlayfs/namei.c  |    2 --
 3 files changed, 3 insertions(+), 5 deletions(-)

Index: rhvgoyal-linux/fs/overlayfs/inode.c
===================================================================
--- rhvgoyal-linux.orig/fs/overlayfs/inode.c	2018-03-20 16:32:30.137452244 -0400
+++ rhvgoyal-linux/fs/overlayfs/inode.c	2018-03-20 16:32:56.000452244 -0400
@@ -760,6 +760,9 @@ struct inode *ovl_get_inode(struct super
 	if (upperdentry && ovl_is_impuredir(upperdentry))
 		ovl_set_flag(OVL_IMPURE, inode);
 
+	if (index)
+		ovl_set_flag(OVL_INDEX, inode);
+
 	/* Check for non-merge dir that may have whiteouts */
 	if (is_dir) {
 		if (((upperdentry && lowerdentry) || numlower > 1) ||
Index: rhvgoyal-linux/fs/overlayfs/namei.c
===================================================================
--- rhvgoyal-linux.orig/fs/overlayfs/namei.c	2018-03-20 16:32:30.137452244 -0400
+++ rhvgoyal-linux/fs/overlayfs/namei.c	2018-03-20 16:32:56.005452244 -0400
@@ -985,8 +985,6 @@ struct dentry *ovl_lookup(struct inode *
 			goto out_free_oe;
 
 		OVL_I(inode)->redirect = upperredirect;
-		if (index)
-			ovl_set_flag(OVL_INDEX, inode);
 	}
 
 	revert_creds(old_cred);
Index: rhvgoyal-linux/fs/overlayfs/export.c
===================================================================
--- rhvgoyal-linux.orig/fs/overlayfs/export.c	2018-03-20 16:32:54.067452244 -0400
+++ rhvgoyal-linux/fs/overlayfs/export.c	2018-03-20 16:33:37.893452244 -0400
@@ -311,9 +311,6 @@ static struct dentry *ovl_obtain_alias(s
 		return ERR_CAST(inode);
 	}
 
-	if (index)
-		ovl_set_flag(OVL_INDEX, inode);
-
 	dentry = d_find_any_alias(inode);
 	if (!dentry) {
 		dentry = d_alloc_anon(inode->i_sb);

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Amir Goldstein]

On Tue, Mar 20, 2018 at 10:35 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Mar 20, 2018 at 03:26:19PM -0400, Vivek Goyal wrote:
>> On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote:
>> > On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > > When a metacopy file is no longer a metacopy and data has been copied up,
>> > > remove REDIRECT xattr. Its not needed anymore.
>> > >
>> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > > ---
>> > >  fs/overlayfs/copy_up.c | 9 +++++++++
>> > >  1 file changed, 9 insertions(+)
>> > >
>> > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> > > index 0c8d2755bd25..704febd2e2fa 100644
>> > > --- a/fs/overlayfs/copy_up.c
>> > > +++ b/fs/overlayfs/copy_up.c
>> > > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>> > >         if (err)
>> > >                 return err;
>> > >
>> > > +       /*
>> > > +        * A metacopy files does not need redirect xattr once data has
>> > > +        * been copied up.
>> > > +        */
>> > > +       err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
>> > > +       if (err && err != -ENODATA && err != -EOPNOTSUPP)
>> > > +               return err;
>> > > +
>> > > +       err = 0;
>> > >         ovl_set_upperdata(d_inode(c->dentry));
>> > >         return err;
>> >
>> > By intuition, I would say that removing redirect should be done after setting
>> > upperdata flag. Not sure if it really matters in real life.
>> > Maybe when racing a lookup of a metacopy hardlink and copy up data of
>> > an upper alias?
>> >
>> > Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
>> > probably use a helper ovl_clear_redirect() for the locking.
>> >
>> > But that highlights a serious problem with current patches -
>> > Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()
>> > and additionally with dentry->d_lock in ovl_rename()
>> > That is sufficient for directories which can only have a single dentry
>> > alias to an
>> > inode but not at all sufficient for non-directories.
>>
>> Hi Amir,
>>
>> I think I have got the locking of ovl_inode right now. Given this
>> situation, I am looking at ovl_lookup() and wondering, why certain
>> properties which belong to inode are still being initialized during
>> dentry initialization (That too without any lock). That sounds wrong.
>>
>> For example.
>>
>> ovl_lookup() {
>>       if (index)
>>               ovl_set_flag(OVL_INDEX, inode);
>> }
>>
>> This is ovl_inode() property and should be initialized when inode is
>> created first time in cache? And updated later in copy up path as index
>> is created later. What's the reason to update inode property in
>> ovl_lookup() if inode is alreday in cache.

Historic. It was there before index was passed in to ovl_get_inode()

>
> IOW, will a patch as below fly?

Yes, looks good.

>
> ---
>  fs/overlayfs/export.c |    3 ---
>  fs/overlayfs/inode.c  |    3 +++
>  fs/overlayfs/namei.c  |    2 --
>  3 files changed, 3 insertions(+), 5 deletions(-)
>
> Index: rhvgoyal-linux/fs/overlayfs/inode.c
> ===================================================================
> --- rhvgoyal-linux.orig/fs/overlayfs/inode.c    2018-03-20 16:32:30.137452244 -0400
> +++ rhvgoyal-linux/fs/overlayfs/inode.c 2018-03-20 16:32:56.000452244 -0400
> @@ -760,6 +760,9 @@ struct inode *ovl_get_inode(struct super
>         if (upperdentry && ovl_is_impuredir(upperdentry))
>                 ovl_set_flag(OVL_IMPURE, inode);
>
> +       if (index)
> +               ovl_set_flag(OVL_INDEX, inode);
> +
>         /* Check for non-merge dir that may have whiteouts */
>         if (is_dir) {
>                 if (((upperdentry && lowerdentry) || numlower > 1) ||
> Index: rhvgoyal-linux/fs/overlayfs/namei.c
> ===================================================================
> --- rhvgoyal-linux.orig/fs/overlayfs/namei.c    2018-03-20 16:32:30.137452244 -0400
> +++ rhvgoyal-linux/fs/overlayfs/namei.c 2018-03-20 16:32:56.005452244 -0400
> @@ -985,8 +985,6 @@ struct dentry *ovl_lookup(struct inode *
>                         goto out_free_oe;
>
>                 OVL_I(inode)->redirect = upperredirect;
> -               if (index)
> -                       ovl_set_flag(OVL_INDEX, inode);
>         }
>
>         revert_creds(old_cred);
> Index: rhvgoyal-linux/fs/overlayfs/export.c
> ===================================================================
> --- rhvgoyal-linux.orig/fs/overlayfs/export.c   2018-03-20 16:32:54.067452244 -0400
> +++ rhvgoyal-linux/fs/overlayfs/export.c        2018-03-20 16:33:37.893452244 -0400
> @@ -311,9 +311,6 @@ static struct dentry *ovl_obtain_alias(s
>                 return ERR_CAST(inode);
>         }
>
> -       if (index)
> -               ovl_set_flag(OVL_INDEX, inode);
> -
>         dentry = d_find_any_alias(inode);
>         if (!dentry) {
>                 dentry = d_alloc_anon(inode->i_sb);

Re: [PATCH v12 09/17] ovl: Do not mark a non dir as _OVL_PATH_MERGE in ovl_path_type()

[Vivek Goyal]

On Wed, Mar 07, 2018 at 03:37:10PM +0200, Amir Goldstein wrote:
> On Wed, Mar 7, 2018 at 3:21 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Mar 07, 2018 at 09:07:22AM +0200, Amir Goldstein wrote:
> >> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > With the addition of an origin chain for regular files, it is possible
> >> > that we don't have an upperdentry and oe->numlower > 1 for non-dir. So
> >> > mark a path __OVL_TYPE_MERGE only if it is a directory.
> >> >
> >>
> >> Okay, but what's wrong with marking a non-dir as TYPE MERGE?
> >> It is, after all, a sort of merged entry.
> >
> > Conceptually, I can't think of anything wrong. Just that currently we use
> > MERGE in the context of directory and we don't need it in the context of
> > non-dir. So this is just trying to be safe to make sure not to break any
> > existing code.
> >
> > I guess we could mark metacopy regular files as MERGE as well, if need be.
> > But that could be part of a separate patch series where we could run
> > various tests and make sure nothing is broken.
> >
> 
> I did a quick scan on all uses of OVL_TYPE_MERGE() they
> all have is_dir check in front of them or checked on a parent object.
> 
> In fact, in the case of ovl_rename(), for directory, the test is
>    if (ovl_type_merge_or_lower(old))
>       err = ovl_set_redirect(old, samedir);
> 
> 
> You could have removed the is_dir condition before this code instead
> of adding a different case for is_metacopy, although you did use a different
> argument for samedir in the metacopy case...

Hi Amir,

I am looking into merging code path for dir and non-dir. What I can't
understand is that why do we check for "ovl_type_merge_or_lower()" and
not just "ovl_type_merge()"?

By now, "old" dentry has been copied up anyway. So no point is checking
if this is lower or not? It will always be "upper"? 

What am I missing?

Vivek

Re: [PATCH v12 09/17] ovl: Do not mark a non dir as _OVL_PATH_MERGE in ovl_path_type()

[Amir Goldstein]

On Wed, Mar 28, 2018 at 10:43 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Mar 07, 2018 at 03:37:10PM +0200, Amir Goldstein wrote:
>> On Wed, Mar 7, 2018 at 3:21 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Wed, Mar 07, 2018 at 09:07:22AM +0200, Amir Goldstein wrote:
>> >> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> > With the addition of an origin chain for regular files, it is possible
>> >> > that we don't have an upperdentry and oe->numlower > 1 for non-dir. So
>> >> > mark a path __OVL_TYPE_MERGE only if it is a directory.
>> >> >
>> >>
>> >> Okay, but what's wrong with marking a non-dir as TYPE MERGE?
>> >> It is, after all, a sort of merged entry.
>> >
>> > Conceptually, I can't think of anything wrong. Just that currently we use
>> > MERGE in the context of directory and we don't need it in the context of
>> > non-dir. So this is just trying to be safe to make sure not to break any
>> > existing code.
>> >
>> > I guess we could mark metacopy regular files as MERGE as well, if need be.
>> > But that could be part of a separate patch series where we could run
>> > various tests and make sure nothing is broken.
>> >
>>
>> I did a quick scan on all uses of OVL_TYPE_MERGE() they
>> all have is_dir check in front of them or checked on a parent object.
>>
>> In fact, in the case of ovl_rename(), for directory, the test is
>>    if (ovl_type_merge_or_lower(old))
>>       err = ovl_set_redirect(old, samedir);
>>
>>
>> You could have removed the is_dir condition before this code instead
>> of adding a different case for is_metacopy, although you did use a different
>> argument for samedir in the metacopy case...
>
> Hi Amir,
>
> I am looking into merging code path for dir and non-dir. What I can't
> understand is that why do we check for "ovl_type_merge_or_lower()" and
> not just "ovl_type_merge()"?
>
> By now, "old" dentry has been copied up anyway. So no point is checking
> if this is lower or not? It will always be "upper"?
>
> What am I missing?
>

I don;t think you are not missing anything.
Maybe its a relic from the past.
Only the use of ovl_type_merge_or_lower() in ovl_can_move() is really
needed.

Thanks,
Amir.

Re: [PATCH v12 16/17] ovl: Set redirect on upper inode when it is linked

[Vivek Goyal]

On Wed, Mar 07, 2018 at 10:02:20AM +0200, Amir Goldstein wrote:
> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > When we create a hardlink to a metacopy upper file, first the redirect
> > on that inode. Path based lookup will not work with newly created link
> > and redirect will solve that issue.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/dir.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index b955f6fede06..da5c4b8ee919 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -24,6 +24,8 @@ module_param_named(redirect_max, ovl_redirect_max, ushort, 0644);
> >  MODULE_PARM_DESC(ovl_redirect_max,
> >                  "Maximum length of absolute redirect xattr value");
> >
> > +static int ovl_set_redirect(struct dentry *dentry, bool samedir);
> > +
> >  int ovl_cleanup(struct inode *wdir, struct dentry *wdentry)
> >  {
> >         int err;
> > @@ -468,6 +470,9 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> >         const struct cred *old_cred;
> >         struct cred *override_cred;
> >         struct dentry *parent = dentry->d_parent;
> > +       struct dentry *hardlink_upper;
> > +
> > +       hardlink_upper = hardlink ? ovl_dentry_upper(hardlink) : NULL;
> 
> IMO it would be nicer to pass overlay hardlink dentry all the way to
> ovl_create_real() and then just:
> 
>         if (hardlink) {
>                 err = ovl_do_link(ovl_dentry_upper(hardlink), dir,
> newdentry, debug);

Amir,

Are you particular about this change? I looked at ovl_create_real() and
I see that right now both "newdentry" and "hardlink" are real dentries.
And I like it. Mixing real dentries and overlay dentries in functions
is often very confusing, especially given the fact that we don't have
any prefix/postfix to differentiate between two.

So I would rather prefer to leave it as it is.

Thanks
Vivek

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Vivek Goyal]

On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote:
> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > When a metacopy file is no longer a metacopy and data has been copied up,
> > remove REDIRECT xattr. Its not needed anymore.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 0c8d2755bd25..704febd2e2fa 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> >         if (err)
> >                 return err;
> >
> > +       /*
> > +        * A metacopy files does not need redirect xattr once data has
> > +        * been copied up.
> > +        */
> > +       err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
> > +       if (err && err != -ENODATA && err != -EOPNOTSUPP)
> > +               return err;
> > +
> > +       err = 0;
> >         ovl_set_upperdata(d_inode(c->dentry));
> >         return err;
> 
> By intuition, I would say that removing redirect should be done after setting
> upperdata flag. Not sure if it really matters in real life.
> Maybe when racing a lookup of a metacopy hardlink and copy up data of
> an upper alias?

I have taken care of races now. All the redirect operations are done
under ovl_inode->lock (especially for metacopy files).

I am keeping ovl_set_upperdata() in the end. This signifies that all
the operations on the file (copy up and removal of metacopy xattr and
removal of redirect) are done. So IMO, we should set ovl_set_upperdata()
in the end and not before removal of redirect.

> 
> Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
> probably use a helper ovl_clear_redirect() for the locking.

Also took care of this in next set of patches I am planning to post soon

> 
> But that highlights a serious problem with current patches -
> Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()
> and additionally with dentry->d_lock in ovl_rename()
> That is sufficient for directories which can only have a single dentry
> alias to an
> inode but not at all sufficient for non-directories.

Forced taking ovl_inode->lock() when redirects need to be set on non-dir and
hopefully that will take care of such races.

Vivek

Re: [PATCH v12 16/17] ovl: Set redirect on upper inode when it is linked

[Amir Goldstein]

On Thu, Mar 29, 2018 at 5:01 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Mar 07, 2018 at 10:02:20AM +0200, Amir Goldstein wrote:
>> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > When we create a hardlink to a metacopy upper file, first the redirect
>> > on that inode. Path based lookup will not work with newly created link
>> > and redirect will solve that issue.
>> >
>> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > ---
>> >  fs/overlayfs/dir.c | 18 ++++++++++++++----
>> >  1 file changed, 14 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>> > index b955f6fede06..da5c4b8ee919 100644
>> > --- a/fs/overlayfs/dir.c
>> > +++ b/fs/overlayfs/dir.c
>> > @@ -24,6 +24,8 @@ module_param_named(redirect_max, ovl_redirect_max, ushort, 0644);
>> >  MODULE_PARM_DESC(ovl_redirect_max,
>> >                  "Maximum length of absolute redirect xattr value");
>> >
>> > +static int ovl_set_redirect(struct dentry *dentry, bool samedir);
>> > +
>> >  int ovl_cleanup(struct inode *wdir, struct dentry *wdentry)
>> >  {
>> >         int err;
>> > @@ -468,6 +470,9 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>> >         const struct cred *old_cred;
>> >         struct cred *override_cred;
>> >         struct dentry *parent = dentry->d_parent;
>> > +       struct dentry *hardlink_upper;
>> > +
>> > +       hardlink_upper = hardlink ? ovl_dentry_upper(hardlink) : NULL;
>>
>> IMO it would be nicer to pass overlay hardlink dentry all the way to
>> ovl_create_real() and then just:
>>
>>         if (hardlink) {
>>                 err = ovl_do_link(ovl_dentry_upper(hardlink), dir,
>> newdentry, debug);
>
> Amir,
>
> Are you particular about this change? I looked at ovl_create_real() and
> I see that right now both "newdentry" and "hardlink" are real dentries.
> And I like it. Mixing real dentries and overlay dentries in functions
> is often very confusing, especially given the fact that we don't have
> any prefix/postfix to differentiate between two.
>
> So I would rather prefer to leave it as it is.
>

I can live with that :)

Thanks,
Amir.

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Vivek Goyal]

On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote:
> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > When a metacopy file is no longer a metacopy and data has been copied up,
> > remove REDIRECT xattr. Its not needed anymore.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 0c8d2755bd25..704febd2e2fa 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
> >         if (err)
> >                 return err;
> >
> > +       /*
> > +        * A metacopy files does not need redirect xattr once data has
> > +        * been copied up.
> > +        */
> > +       err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
> > +       if (err && err != -ENODATA && err != -EOPNOTSUPP)
> > +               return err;
> > +
> > +       err = 0;
> >         ovl_set_upperdata(d_inode(c->dentry));
> >         return err;
> 
> By intuition, I would say that removing redirect should be done after setting
> upperdata flag. Not sure if it really matters in real life.
> Maybe when racing a lookup of a metacopy hardlink and copy up data of
> an upper alias?
> 
> Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
> probably use a helper ovl_clear_redirect() for the locking.
> 
> But that highlights a serious problem with current patches -
> Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()

> and additionally with dentry->d_lock in ovl_rename()
> That is sufficient for directories which can only have a single dentry
> alias to an
> inode but not at all sufficient for non-directories.

Quick question on this. For non-dir files, will ovl_inode->redirect be
protected by VFS locking. Atleast for our use case. We seem to be touching
ovl_inode->redirect in rename, link and lookup path.

VFS rename locks both source and destination inodes and link path will
lock source and destination as well. So I think a rename and link can
not make progress in parallel if any or the source or target is files
are common. If that's the case two redirect writers can't stomp over
each other.

Lookup path will only set ovl_inode->redirect only if inode state is
I_NEW and that means other vfs operations on same inode could not be
making any progress.

Now comes question of copy up path when we remove redirect xattr and
modify ovl_inode->redirect. I don't know if we can assume that
VFS is holding inode lock when copy up happens. If this assumption is
valid, then it means rename and link can't make progress and it is
safe to modify ovl_inode->redirect.

I guess this assumption might not be valid, and that's why we need
ovl_inode->lock. Not sure though. Can you throw some light at it.

Thanks
Vivek

Re: [PATCH v12 15/17] ovl: Remove redirect when data of a metacopy file is copied up

[Amir Goldstein]

On Wed, Apr 4, 2018 at 4:41 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Mar 07, 2018 at 10:21:30AM +0200, Amir Goldstein wrote:
>> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > When a metacopy file is no longer a metacopy and data has been copied up,
>> > remove REDIRECT xattr. Its not needed anymore.
>> >
>> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > ---
>> >  fs/overlayfs/copy_up.c | 9 +++++++++
>> >  1 file changed, 9 insertions(+)
>> >
>> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> > index 0c8d2755bd25..704febd2e2fa 100644
>> > --- a/fs/overlayfs/copy_up.c
>> > +++ b/fs/overlayfs/copy_up.c
>> > @@ -775,6 +775,15 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>> >         if (err)
>> >                 return err;
>> >
>> > +       /*
>> > +        * A metacopy files does not need redirect xattr once data has
>> > +        * been copied up.
>> > +        */
>> > +       err = vfs_removexattr(upperpath.dentry, OVL_XATTR_REDIRECT);
>> > +       if (err && err != -ENODATA && err != -EOPNOTSUPP)
>> > +               return err;
>> > +
>> > +       err = 0;
>> >         ovl_set_upperdata(d_inode(c->dentry));
>> >         return err;
>>
>> By intuition, I would say that removing redirect should be done after setting
>> upperdata flag. Not sure if it really matters in real life.
>> Maybe when racing a lookup of a metacopy hardlink and copy up data of
>> an upper alias?
>>
>> Also, it would make sense to also ovl_dentry_set_redirect(c->dentry, NULL)
>> probably use a helper ovl_clear_redirect() for the locking.
>>
>> But that highlights a serious problem with current patches -
>> Access to OVL_I(inode)->redirect is protected with parent mutex in ovl_lookup()
>
>> and additionally with dentry->d_lock in ovl_rename()
>> That is sufficient for directories which can only have a single dentry
>> alias to an
>> inode but not at all sufficient for non-directories.
>
> Quick question on this. For non-dir files, will ovl_inode->redirect be
> protected by VFS locking. Atleast for our use case. We seem to be touching
> ovl_inode->redirect in rename, link and lookup path.
>
> VFS rename locks both source and destination inodes and link path will
> lock source and destination as well. So I think a rename and link can
> not make progress in parallel if any or the source or target is files
> are common. If that's the case two redirect writers can't stomp over
> each other.
>
> Lookup path will only set ovl_inode->redirect only if inode state is
> I_NEW and that means other vfs operations on same inode could not be
> making any progress.
>
> Now comes question of copy up path when we remove redirect xattr and
> modify ovl_inode->redirect. I don't know if we can assume that
> VFS is holding inode lock when copy up happens. If this assumption is
> valid, then it means rename and link can't make progress and it is
> safe to modify ovl_inode->redirect.
>
> I guess this assumption might not be valid, and that's why we need
> ovl_inode->lock. Not sure though. Can you throw some light at it.
>

copy up from open() file for write doesn't hold any VFS lock.

I guess before your metacopy changes, it would have been safe
to set redirect without ovl_inode lock if we would only take the
patch to move setting redirect into I_NEW, but anyway, if for no
other reason, it is good practice to also protect multiple access in
overlayfs layer, unless there is a good reason to rely on VFS locks.

Thanks,
Amir.