[PATCH v9 00/15] overlayfs: Delayed copy up of data

[PATCH v9 00/15] overlayfs: Delayed copy up of data

[Vivek Goyal]

Hi,

Please find attached V9 of the patches. Minor changes to take care of
Amir's comments. I have also dropped RFC tag. If there are no concerns,
then I would like these patches to be included.

These patches apply on top of overlayfs-next branch plus the fix I posted
with subject "ovl: Pass ovl_get_nlink() parameters in right order"

https://www.spinics.net/lists/stable/msg200902.html

Changes from V8.

- Fixed ovl_get_nlink() to take index as parameter instead of
  upperdentry.

- Ran checkpatch on patches and fixed issues.

- Updated help text of config option OVERLAY_FS_METACOPY
 

Vivek

Amir Goldstein (1):
  ovl: disable redirect_dir and index when no xattr support

Vivek Goyal (14):
  ovl: Do not look for OVL_XATTR_NLINK if index is not there
  ovl: ovl_check_setxattr() get rid of redundant -EOPNOTSUPP check
  ovl: Create origin xattr on copy up for all files
  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: Fix ovl_getattr() to get number of blocks from lower
  ovl: Set OVL_UPPERDATA flag during ovl_lookup()
  ovl: Do not expose metacopy only upper dentry from d_real()
  ovl: Fix encryption/compression status of a metacopy only file
  ovl: Enable metadata only feature

 fs/overlayfs/Kconfig     |  15 +++++
 fs/overlayfs/copy_up.c   | 162 ++++++++++++++++++++++++++++++++++-------------
 fs/overlayfs/dir.c       |   1 +
 fs/overlayfs/inode.c     |  73 ++++++++++-----------
 fs/overlayfs/namei.c     |  40 ++++++++++++
 fs/overlayfs/overlayfs.h |  21 +++++-
 fs/overlayfs/ovl_entry.h |   1 +
 fs/overlayfs/super.c     |  62 ++++++++++++++++--
 fs/overlayfs/util.c      | 105 ++++++++++++++++++++++++++----
 9 files changed, 376 insertions(+), 104 deletions(-)

-- 
2.13.6

[PATCH v9 01/15] ovl: Do not look for OVL_XATTR_NLINK if index is not there

[Vivek Goyal]

Soon, we will be creating OVL_TYPE_ORIGIN entries even for hardlinked
files with index=off. That means, it is possible that there is no
index and hence no OVL_XATTR_NLINK set on upperdentry but lowerdentry
is still there. In that case current implementation gets -ENODATA
from vfs_getxattr)() and it warns and returns fallback. I can get
following warning.

"overlayfs: failed to get index nlink ....."

Pass in index instead of upperdentry in ovl_get_nlink(). That way, if
index is not there, we know that OVL_XATTR_NLINK should not be
present and just return fallback.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/inode.c     | 13 ++++++-------
 fs/overlayfs/overlayfs.h |  2 +-
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 00b6b294272a..903548a6b825 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -543,7 +543,7 @@ int ovl_set_nlink_lower(struct dentry *dentry)
 }
 
 unsigned int ovl_get_nlink(struct dentry *lowerdentry,
-			   struct dentry *upperdentry,
+			   struct dentry *index,
 			   unsigned int fallback)
 {
 	int nlink_diff;
@@ -551,10 +551,10 @@ unsigned int ovl_get_nlink(struct dentry *lowerdentry,
 	char buf[13];
 	int err;
 
-	if (!lowerdentry || !upperdentry || d_inode(lowerdentry)->i_nlink == 1)
+	if (!lowerdentry || !index || d_inode(lowerdentry)->i_nlink == 1)
 		return fallback;
 
-	err = vfs_getxattr(upperdentry, OVL_XATTR_NLINK, &buf, sizeof(buf) - 1);
+	err = vfs_getxattr(index, OVL_XATTR_NLINK, &buf, sizeof(buf) - 1);
 	if (err < 0)
 		goto fail;
 
@@ -567,7 +567,7 @@ unsigned int ovl_get_nlink(struct dentry *lowerdentry,
 	if (err < 0)
 		goto fail;
 
-	nlink = d_inode(buf[0] == 'L' ? lowerdentry : upperdentry)->i_nlink;
+	nlink = d_inode(buf[0] == 'L' ? lowerdentry : index)->i_nlink;
 	nlink += nlink_diff;
 
 	if (nlink <= 0)
@@ -577,7 +577,7 @@ unsigned int ovl_get_nlink(struct dentry *lowerdentry,
 
 fail:
 	pr_warn_ratelimited("overlayfs: failed to get index nlink (%pd2, err=%i)\n",
-			    upperdentry, err);
+			    index, err);
 	return fallback;
 }
 
@@ -670,8 +670,7 @@ struct inode *ovl_get_inode(struct dentry *dentry, struct dentry *upperdentry,
 			goto out;
 		}
 
-		nlink = ovl_get_nlink(lowerdentry, upperdentry,
-				      realinode->i_nlink);
+		nlink = ovl_get_nlink(lowerdentry, index, realinode->i_nlink);
 		set_nlink(inode, nlink);
 	} else {
 		inode = new_inode(dentry->d_sb);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 13eab09a6b6f..4e505cb86f8c 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -274,7 +274,7 @@ int ovl_indexdir_cleanup(struct dentry *dentry, struct vfsmount *mnt,
 int ovl_set_nlink_upper(struct dentry *dentry);
 int ovl_set_nlink_lower(struct dentry *dentry);
 unsigned int ovl_get_nlink(struct dentry *lowerdentry,
-			   struct dentry *upperdentry,
+			   struct dentry *index,
 			   unsigned int fallback);
 int ovl_setattr(struct dentry *dentry, struct iattr *attr);
 int ovl_getattr(const struct path *path, struct kstat *stat,
-- 
2.13.6

Re: [PATCH v9 01/15] ovl: Do not look for OVL_XATTR_NLINK if index is not there

[Amir Goldstein]

On Wed, Nov 29, 2017 at 5:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Soon, we will be creating OVL_TYPE_ORIGIN entries even for hardlinked
> files with index=off. That means, it is possible that there is no
> index and hence no OVL_XATTR_NLINK set on upperdentry but lowerdentry
> is still there. In that case current implementation gets -ENODATA
> from vfs_getxattr)() and it warns and returns fallback. I can get
> following warning.
>
> "overlayfs: failed to get index nlink ....."
>
> Pass in index instead of upperdentry in ovl_get_nlink(). That way, if
> index is not there, we know that OVL_XATTR_NLINK should not be
> present and just return fallback.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

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

[PATCH v9 02/15] ovl: disable redirect_dir and index when no xattr support

[Vivek Goyal]

From: Amir Goldstein <amir73il@gmail.com>

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

The redirect_dir feature is implicitly disabled when upper fs does not
support xattr via the check in ovl_redirect_dir(). Make the feature
explicitly disabled in this case by emitting a warning at mount time
and setting redirect_dir to off so its true state is visible in
/proc/mounts.

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

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index be03578181d2..ee41bde87b14 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -921,7 +921,9 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 	err = ovl_do_setxattr(ofs->workdir, OVL_XATTR_OPAQUE, "0", 1, 0);
 	if (err) {
 		ofs->noxattr = true;
-		pr_warn("overlayfs: upper fs does not support xattr.\n");
+		ofs->config.redirect_dir = false;
+		ofs->config.index = false;
+		pr_warn("overlayfs: upper fs does not support xattr, falling back to redirect_dir=off, index=off and no opaque dir.\n");
 	} else {
 		vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
 	}
-- 
2.13.6

[PATCH v9 03/15] ovl: ovl_check_setxattr() get rid of redundant -EOPNOTSUPP check

[Vivek Goyal]

At mount time we check if upper supports xattr or not and set ofs->noxattr
field accordingly. But in ovl_check_setxattr() still seems to have logic
which expects that ovl_do_setxattr() can return -EOPNOTSUPP. Not sure when
and how can we hit this code. Feels redundant to me. So I am removing
this code.

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

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index d6bb1c9f5e7a..77dc00438d54 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -365,21 +365,12 @@ int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
 		       const char *name, const void *value, size_t size,
 		       int xerr)
 {
-	int err;
 	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 
 	if (ofs->noxattr)
 		return xerr;
 
-	err = ovl_do_setxattr(upperdentry, name, value, size, 0);
-
-	if (err == -EOPNOTSUPP) {
-		pr_warn("overlayfs: cannot set %s xattr on upper\n", name);
-		ofs->noxattr = true;
-		return xerr;
-	}
-
-	return err;
+	return ovl_do_setxattr(upperdentry, name, value, size, 0);
 }
 
 int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
-- 
2.13.6

[PATCH v9 04/15] ovl: Create origin xattr on copy up for all files

[Vivek Goyal]

Right now my understanding is that origin xattr is created for all copied
up files if index=on. And if index=off, then we create it for all type
of files except hardlinks (nlink != 1).

With metadata only copy up, I will still require origin xattr to copy up
data later, so create it even for hardlinks even with index=off.

Given ->origin is always true now, get rid of this field from
"struct ovl_copy_up_ctx".

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

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index eb3b8d39fb61..f55bece3688e 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -326,7 +326,6 @@ struct ovl_copy_up_ctx {
 	struct qstr destname;
 	struct dentry *workdir;
 	bool tmpfile;
-	bool origin;
 };
 
 static int ovl_link_up(struct ovl_copy_up_ctx *c)
@@ -469,15 +468,10 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 	/*
 	 * Store identifier of lower inode in upper inode xattr to
 	 * allow lookup of the copy up origin inode.
-	 *
-	 * Don't set origin when we are breaking the association with a lower
-	 * hard link.
 	 */
-	if (c->origin) {
-		err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
-		if (err)
-			return err;
-	}
+	err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
+	if (err)
+		return err;
 
 	return 0;
 }
@@ -542,9 +536,6 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 	    c->stat.nlink > 1)
 		indexed = true;
 
-	if (S_ISDIR(c->stat.mode) || c->stat.nlink == 1 || indexed)
-		c->origin = true;
-
 	if (indexed) {
 		c->destdir = ovl_indexdir(c->dentry->d_sb);
 		err = ovl_get_index_name(c->lowerpath.dentry, &c->destname);
-- 
2.13.6

Re: [PATCH v9 04/15] ovl: Create origin xattr on copy up for all files

[Miklos Szeredi]

On Wed, Nov 29, 2017 at 4:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Right now my understanding is that origin xattr is created for all copied
> up files if index=on. And if index=off, then we create it for all type
> of files except hardlinks (nlink != 1).
>
> With metadata only copy up, I will still require origin xattr to copy up
> data later, so create it even for hardlinks even with index=off.

This doesn't look right.  Without index we can't do hard links
properly, doing metacopy in that case will complicate the situation
further and can result in weirdness (e.g. two files that have
different attributes but same inode number).

So I think we should either require index for metacopy, or disable
metacopy only for nlink != 1 files, but otherwise enable it even in
case of index=off.

Thanks,
Miklos

Re: [PATCH v9 04/15] ovl: Create origin xattr on copy up for all files

[Amir Goldstein]

On Mon, Jan 8, 2018 at 12:16 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Wed, Nov 29, 2017 at 4:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> Right now my understanding is that origin xattr is created for all copied
>> up files if index=on. And if index=off, then we create it for all type
>> of files except hardlinks (nlink != 1).
>>
>> With metadata only copy up, I will still require origin xattr to copy up
>> data later, so create it even for hardlinks even with index=off.
>
> This doesn't look right.  Without index we can't do hard links
> properly, doing metacopy in that case will complicate the situation
> further and can result in weirdness (e.g. two files that have
> different attributes but same inode number).

But ovl_getattr() does not return lower st_ino for non-indexed
upper with lowerstat.nlink != 1.

>
> So I think we should either require index for metacopy, or disable
> metacopy only for nlink != 1 files, but otherwise enable it even in
> case of index=off.
>

IIRC, commit 6eaf011144af ("ovl: fix EIO from lookup of non-indexed
upper") was a result of a conversation about this very patch and this
very issue. After this commit, the situation of non-indexed upper with
origin is handled correctly on lookup (hash by upper inode) and I
couldn't find a reason why setting origin for metacopy with index=off
is wrong. Or anything that could go wrong for setting origin on every
copy up.

Am I missing something?

Amir.

Re: [PATCH v9 04/15] ovl: Create origin xattr on copy up for all files

[Vivek Goyal]

On Mon, Jan 08, 2018 at 01:18:05PM +0200, Amir Goldstein wrote:
> On Mon, Jan 8, 2018 at 12:16 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > On Wed, Nov 29, 2017 at 4:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> Right now my understanding is that origin xattr is created for all copied
> >> up files if index=on. And if index=off, then we create it for all type
> >> of files except hardlinks (nlink != 1).
> >>
> >> With metadata only copy up, I will still require origin xattr to copy up
> >> data later, so create it even for hardlinks even with index=off.
> >
> > This doesn't look right.  Without index we can't do hard links
> > properly, doing metacopy in that case will complicate the situation
> > further and can result in weirdness (e.g. two files that have
> > different attributes but same inode number).
> 
> But ovl_getattr() does not return lower st_ino for non-indexed
> upper with lowerstat.nlink != 1.

Hi Miklos,

Thanks Amir for ovl_gettr() behavior.

So there are 3 ways to deal with this.

A. Keep ORIGIN but do not report lower inode number if index=off, instead
   report upper inode number.

B. Do not create ORIGIN to begin with if index=off and disable metacopy
   for hardlinked files.

C. Make metacopy dependent on index=on.

currently I take approach A. That is always create ORIGIN xattr but
do not use it when we think this might be a problem. (Like for the
cases of broken hardlinks).

If you still don't like this approach, please let me know and I will
change it. And switch to say approach B?

Thanks
Vivek

> 
> >
> > So I think we should either require index for metacopy, or disable
> > metacopy only for nlink != 1 files, but otherwise enable it even in
> > case of index=off.
> >
> 
> IIRC, commit 6eaf011144af ("ovl: fix EIO from lookup of non-indexed
> upper") was a result of a conversation about this very patch and this
> very issue. After this commit, the situation of non-indexed upper with
> origin is handled correctly on lookup (hash by upper inode) and I
> couldn't find a reason why setting origin for metacopy with index=off
> is wrong. Or anything that could go wrong for setting origin on every
> copy up.
> 
> Am I missing something?
> 
> Amir.

[PATCH v9 05/15] 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.

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>
---
 fs/overlayfs/Kconfig     | 15 +++++++++++++++
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     | 40 +++++++++++++++++++++++++++++++++++++---
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index cbfc196e5dc5..df18c0ad534d 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -43,3 +43,18 @@ config OVERLAY_FS_INDEX
 	  outcomes.  However, mounting the same overlay with an old kernel
 	  read-write and then mounting it again with a new kernel, will have
 	  unexpected results.
+
+config OVERLAY_FS_METACOPY
+	bool "Overlayfs: turn on metadata only copy up feature by default"
+	depends on OVERLAY_FS
+	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 93eb6a044dd2..2720ed21ad53 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -15,6 +15,7 @@ struct ovl_config {
 	bool default_permissions;
 	bool redirect_dir;
 	bool index;
+	bool metacopy;
 };
 
 struct ovl_layer {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index ee41bde87b14..8b9ddcba4b04 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -46,6 +46,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;
@@ -319,6 +324,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 	if (ofs->config.index != ovl_index_def)
 		seq_printf(m, ",index=%s",
 			   ofs->config.index ? "on" : "off");
+	if (ofs->config.metacopy != ovl_metacopy_def)
+		seq_printf(m, ",metacopy=%s",
+			   ofs->config.metacopy ? "on" : "off");
 	return 0;
 }
 
@@ -352,6 +360,8 @@ enum {
 	OPT_REDIRECT_DIR_OFF,
 	OPT_INDEX_ON,
 	OPT_INDEX_OFF,
+	OPT_METACOPY_ON,
+	OPT_METACOPY_OFF,
 	OPT_ERR,
 };
 
@@ -364,6 +374,8 @@ static const match_table_t ovl_tokens = {
 	{OPT_REDIRECT_DIR_OFF,		"redirect_dir=off"},
 	{OPT_INDEX_ON,			"index=on"},
 	{OPT_INDEX_OFF,			"index=off"},
+	{OPT_METACOPY_ON,		"metacopy=on"},
+	{OPT_METACOPY_OFF,		"metacopy=off"},
 	{OPT_ERR,			NULL}
 };
 
@@ -444,6 +456,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			config->index = 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;
@@ -657,9 +677,11 @@ static int ovl_lower_dir(const char *name, struct path *path,
 	 * The inodes index feature needs to encode and decode file
 	 * handles, so it requires that all layers support them.
 	 */
-	if (ofs->config.index && !ovl_can_decode_fh(path->dentry->d_sb)) {
+	if ((ofs->config.index || ofs->config.metacopy) &&
+	     !ovl_can_decode_fh(path->dentry->d_sb)) {
+		pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off and metacopy=off.\n", name);
 		ofs->config.index = false;
-		pr_warn("overlayfs: fs on '%s' does not support file handles, falling back to index=off.\n", name);
+		ofs->config.metacopy = false;
 	}
 
 	return 0;
@@ -923,7 +945,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 		ofs->noxattr = true;
 		ofs->config.redirect_dir = false;
 		ofs->config.index = false;
-		pr_warn("overlayfs: upper fs does not support xattr, falling back to redirect_dir=off, index=off and no opaque dir.\n");
+		ofs->config.metacopy = false;
+		pr_warn("overlayfs: upper fs does not support xattr, falling back to redirect_dir=off, index=off, metacopy=off and no opaque dir.\n");
 	} else {
 		vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
 	}
@@ -1164,6 +1187,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
 	ofs->config.redirect_dir = ovl_redirect_dir_def;
 	ofs->config.index = ovl_index_def;
+	ofs->config.metacopy = ovl_metacopy_def;
 	err = ovl_parse_opt((char *) data, &ofs->config);
 	if (err)
 		goto out_err;
@@ -1209,6 +1233,16 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	else if (ofs->upper_mnt->mnt_sb != ofs->same_sb)
 		ofs->same_sb = NULL;
 
+	if (ofs->upper_mnt && ofs->config.metacopy) {
+		/* Verify lower root is upper root origin */
+		err = ovl_verify_origin(upperpath.dentry,
+					oe->lowerstack[0].dentry, false, true);
+		if (err) {
+			pr_err("overlayfs: failed to verify upper root origin\n");
+			goto out_free_oe;
+		}
+	}
+
 	if (!(ovl_force_readonly(ofs)) && ofs->config.index) {
 		err = ovl_get_indexdir(ofs, oe, &upperpath);
 		if (err)
-- 
2.13.6

[PATCH v9 06/15] 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 | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index f55bece3688e..303794bb9127 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -443,6 +443,19 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 {
 	int err;
 
+	err = ovl_copy_xattr(c->lowerpath.dentry, temp);
+	if (err)
+		return err;
+
+	/*
+	 * Store identifier of lower inode in upper inode xattr to
+	 * allow lookup of the copy up origin inode.
+	 *
+	 */
+	err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
+	if (err)
+		return err;
+
 	if (S_ISREG(c->stat.mode)) {
 		struct path upperpath;
 
@@ -455,25 +468,11 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 			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.
-	 */
-	err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
-	if (err)
-		return err;
 
-	return 0;
+	return err;
 }
 
 static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
-- 
2.13.6

[PATCH v9 07/15] 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   | 30 ++++++++++++++++++++++++++++++
 fs/overlayfs/inode.c     | 30 ------------------------------
 fs/overlayfs/overlayfs.h |  2 +-
 3 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 303794bb9127..31711edf5bde 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -686,6 +686,36 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 	return err;
 }
 
+static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
+{
+	if (ovl_dentry_upper(dentry) &&
+	    ovl_dentry_has_upper_alias(dentry))
+		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 903548a6b825..5239a1ff7211 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -341,36 +341,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)
-{
-	if (ovl_dentry_upper(dentry) &&
-	    ovl_dentry_has_upper_alias(dentry))
-		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 4e505cb86f8c..b0a691b463d6 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -286,7 +286,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);
 
@@ -319,6 +318,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 *lower, bool is_upper);
-- 
2.13.6

[PATCH v9 08/15] 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.

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

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 31711edf5bde..c5804b87f3c7 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 *lower, bool is_upper)
 {
 	struct ovl_fh *fh;
@@ -305,11 +325,8 @@ static int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
 			return PTR_ERR(fh);
 	}
 
-	/*
-	 * Do not fail when upper doesn't support xattrs.
-	 */
 	err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh,
-				 fh ? fh->len : 0, 0);
+				 fh ? fh->len : 0, -EOPNOTSUPP);
 	kfree(fh);
 
 	return err;
@@ -326,6 +343,7 @@ struct ovl_copy_up_ctx {
 	struct qstr destname;
 	struct dentry *workdir;
 	bool tmpfile;
+	bool metacopy;
 };
 
 static int ovl_link_up(struct ovl_copy_up_ctx *c)
@@ -453,10 +471,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 	 *
 	 */
 	err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
-	if (err)
-		return err;
+	if (err) {
+		if (err != -EOPNOTSUPP)
+			return err;
+		/* Upper does not support xattr. Copy up data as well */
+		c->metacopy = false;
+	}
 
-	if (S_ISREG(c->stat.mode)) {
+	if (S_ISREG(c->stat.mode) && !c->metacopy) {
 		struct path upperpath;
 
 		ovl_path_upper(c->dentry, &upperpath);
@@ -601,6 +623,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);
+
 	ovl_path_upper(parent, &parentpath);
 	ctx.destdir = parentpath.dentry;
 	ctx.destname = dentry->d_name;
-- 
2.13.6

Re: [PATCH v9 08/15] ovl: Copy up only metadata during copy up where it makes sense

[Miklos Szeredi]

On Wed, Nov 29, 2017 at 4:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> 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.
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/copy_up.c | 38 +++++++++++++++++++++++++++++++-------
>  1 file changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 31711edf5bde..c5804b87f3c7 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 *lower, bool is_upper)
>  {
>         struct ovl_fh *fh;
> @@ -305,11 +325,8 @@ static int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
>                         return PTR_ERR(fh);
>         }
>
> -       /*
> -        * Do not fail when upper doesn't support xattrs.
> -        */
>         err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh,
> -                                fh ? fh->len : 0, 0);
> +                                fh ? fh->len : 0, -EOPNOTSUPP);
>         kfree(fh);
>
>         return err;
> @@ -326,6 +343,7 @@ struct ovl_copy_up_ctx {
>         struct qstr destname;
>         struct dentry *workdir;
>         bool tmpfile;
> +       bool metacopy;
>  };
>
>  static int ovl_link_up(struct ovl_copy_up_ctx *c)
> @@ -453,10 +471,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>          *
>          */
>         err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
> -       if (err)
> -               return err;
> +       if (err) {
> +               if (err != -EOPNOTSUPP)
> +                       return err;
> +               /* Upper does not support xattr. Copy up data as well */
> +               c->metacopy = false;

Isn't this supposed to be checked in ovl_get_super()?

Thanks,
Miklos

Re: [PATCH v9 08/15] ovl: Copy up only metadata during copy up where it makes sense

[Vivek Goyal]

On Mon, Jan 08, 2018 at 11:35:09AM +0100, Miklos Szeredi wrote:
> On Wed, Nov 29, 2017 at 4:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > 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.
> >
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/copy_up.c | 38 +++++++++++++++++++++++++++++++-------
> >  1 file changed, 31 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 31711edf5bde..c5804b87f3c7 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 *lower, bool is_upper)
> >  {
> >         struct ovl_fh *fh;
> > @@ -305,11 +325,8 @@ static int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
> >                         return PTR_ERR(fh);
> >         }
> >
> > -       /*
> > -        * Do not fail when upper doesn't support xattrs.
> > -        */
> >         err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh,
> > -                                fh ? fh->len : 0, 0);
> > +                                fh ? fh->len : 0, -EOPNOTSUPP);
> >         kfree(fh);
> >
> >         return err;
> > @@ -326,6 +343,7 @@ struct ovl_copy_up_ctx {
> >         struct qstr destname;
> >         struct dentry *workdir;
> >         bool tmpfile;
> > +       bool metacopy;
> >  };
> >
> >  static int ovl_link_up(struct ovl_copy_up_ctx *c)
> > @@ -453,10 +471,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> >          *
> >          */
> >         err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
> > -       if (err)
> > -               return err;
> > +       if (err) {
> > +               if (err != -EOPNOTSUPP)
> > +                       return err;
> > +               /* Upper does not support xattr. Copy up data as well */
> > +               c->metacopy = false;
> 
> Isn't this supposed to be checked in ovl_get_super()?

Hi Miklos,

Right. I think I was confused by code in ovl_check_setxattr().

int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
                       const char *name, const void *value, size_t size,
                       int xerr)
{
        int err;
        struct ovl_fs *ofs = dentry->d_sb->s_fs_info;

        if (ofs->noxattr)
                return xerr;

        err = ovl_do_setxattr(upperdentry, name, value, size, 0);

        if (err == -EOPNOTSUPP) {
                pr_warn("overlayfs: cannot set %s xattr on upper\n", name);
                ofs->noxattr = true;
                return xerr;
        }

        return err;
}

ofs->noxattr will be set at mount time. Still we check for -EOPNOTSUPP
and set ofs->noxattr again. Is there a situation where this can happen.
Is it intentional. Can we get rid of this code.

To take care of this situation, I put that code. That is at mount time
xattr was fine and hence ofs->noxattr=false. But later somehow we 
encountered -EOPNOTSUPP while doing ovl_set_origin(). Current code
does not fail copy up operation and I tried to retain same behavior.
That is just disable metacopy in such case but continue.

So if we don't expect all this to happen at all, then I can get rid
of this -EOPNOTSUPP check both from ovl_check_setxattr() as well
as ovl_copy_up_inode().

Vivek

Re: [PATCH v9 08/15] ovl: Copy up only metadata during copy up where it makes sense

[Miklos Szeredi]

On Mon, Jan 8, 2018 at 6:03 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Mon, Jan 08, 2018 at 11:35:09AM +0100, Miklos Szeredi wrote:
>> On Wed, Nov 29, 2017 at 4:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > 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.
>> >
>> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> > ---
>> >  fs/overlayfs/copy_up.c | 38 +++++++++++++++++++++++++++++++-------
>> >  1 file changed, 31 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> > index 31711edf5bde..c5804b87f3c7 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 *lower, bool is_upper)
>> >  {
>> >         struct ovl_fh *fh;
>> > @@ -305,11 +325,8 @@ static int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
>> >                         return PTR_ERR(fh);
>> >         }
>> >
>> > -       /*
>> > -        * Do not fail when upper doesn't support xattrs.
>> > -        */
>> >         err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh,
>> > -                                fh ? fh->len : 0, 0);
>> > +                                fh ? fh->len : 0, -EOPNOTSUPP);
>> >         kfree(fh);
>> >
>> >         return err;
>> > @@ -326,6 +343,7 @@ struct ovl_copy_up_ctx {
>> >         struct qstr destname;
>> >         struct dentry *workdir;
>> >         bool tmpfile;
>> > +       bool metacopy;
>> >  };
>> >
>> >  static int ovl_link_up(struct ovl_copy_up_ctx *c)
>> > @@ -453,10 +471,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>> >          *
>> >          */
>> >         err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
>> > -       if (err)
>> > -               return err;
>> > +       if (err) {
>> > +               if (err != -EOPNOTSUPP)
>> > +                       return err;
>> > +               /* Upper does not support xattr. Copy up data as well */
>> > +               c->metacopy = false;
>>
>> Isn't this supposed to be checked in ovl_get_super()?
>
> Hi Miklos,
>
> Right. I think I was confused by code in ovl_check_setxattr().
>
> int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
>                        const char *name, const void *value, size_t size,
>                        int xerr)
> {
>         int err;
>         struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
>
>         if (ofs->noxattr)
>                 return xerr;
>
>         err = ovl_do_setxattr(upperdentry, name, value, size, 0);
>
>         if (err == -EOPNOTSUPP) {
>                 pr_warn("overlayfs: cannot set %s xattr on upper\n", name);
>                 ofs->noxattr = true;
>                 return xerr;
>         }
>
>         return err;
> }
>
> ofs->noxattr will be set at mount time. Still we check for -EOPNOTSUPP
> and set ofs->noxattr again. Is there a situation where this can happen.
> Is it intentional. Can we get rid of this code.
>
> To take care of this situation, I put that code. That is at mount time
> xattr was fine and hence ofs->noxattr=false. But later somehow we
> encountered -EOPNOTSUPP while doing ovl_set_origin(). Current code
> does not fail copy up operation and I tried to retain same behavior.
> That is just disable metacopy in such case but continue.
>
> So if we don't expect all this to happen at all, then I can get rid
> of this -EOPNOTSUPP check both from ovl_check_setxattr() as well
> as ovl_copy_up_inode().

It would be really weird to encounter this, so I think possibly a
WARN_ON(err == -EOPNOTSUPP) is enough to make sure reality matches our
expectations.

Thanks,
Miklos

Re: [PATCH v9 08/15] ovl: Copy up only metadata during copy up where it makes sense

[Vivek Goyal]

On Tue, Jan 09, 2018 at 11:49:11AM +0100, Miklos Szeredi wrote:
> On Mon, Jan 8, 2018 at 6:03 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Mon, Jan 08, 2018 at 11:35:09AM +0100, Miklos Szeredi wrote:
> >> On Wed, Nov 29, 2017 at 4:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > 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.
> >> >
> >> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> >> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> >> > ---
> >> >  fs/overlayfs/copy_up.c | 38 +++++++++++++++++++++++++++++++-------
> >> >  1 file changed, 31 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> >> > index 31711edf5bde..c5804b87f3c7 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 *lower, bool is_upper)
> >> >  {
> >> >         struct ovl_fh *fh;
> >> > @@ -305,11 +325,8 @@ static int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
> >> >                         return PTR_ERR(fh);
> >> >         }
> >> >
> >> > -       /*
> >> > -        * Do not fail when upper doesn't support xattrs.
> >> > -        */
> >> >         err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh,
> >> > -                                fh ? fh->len : 0, 0);
> >> > +                                fh ? fh->len : 0, -EOPNOTSUPP);
> >> >         kfree(fh);
> >> >
> >> >         return err;
> >> > @@ -326,6 +343,7 @@ struct ovl_copy_up_ctx {
> >> >         struct qstr destname;
> >> >         struct dentry *workdir;
> >> >         bool tmpfile;
> >> > +       bool metacopy;
> >> >  };
> >> >
> >> >  static int ovl_link_up(struct ovl_copy_up_ctx *c)
> >> > @@ -453,10 +471,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> >> >          *
> >> >          */
> >> >         err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
> >> > -       if (err)
> >> > -               return err;
> >> > +       if (err) {
> >> > +               if (err != -EOPNOTSUPP)
> >> > +                       return err;
> >> > +               /* Upper does not support xattr. Copy up data as well */
> >> > +               c->metacopy = false;
> >>
> >> Isn't this supposed to be checked in ovl_get_super()?
> >
> > Hi Miklos,
> >
> > Right. I think I was confused by code in ovl_check_setxattr().
> >
> > int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
> >                        const char *name, const void *value, size_t size,
> >                        int xerr)
> > {
> >         int err;
> >         struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> >
> >         if (ofs->noxattr)
> >                 return xerr;
> >
> >         err = ovl_do_setxattr(upperdentry, name, value, size, 0);
> >
> >         if (err == -EOPNOTSUPP) {
> >                 pr_warn("overlayfs: cannot set %s xattr on upper\n", name);
> >                 ofs->noxattr = true;
> >                 return xerr;
> >         }
> >
> >         return err;
> > }
> >
> > ofs->noxattr will be set at mount time. Still we check for -EOPNOTSUPP
> > and set ofs->noxattr again. Is there a situation where this can happen.
> > Is it intentional. Can we get rid of this code.
> >
> > To take care of this situation, I put that code. That is at mount time
> > xattr was fine and hence ofs->noxattr=false. But later somehow we
> > encountered -EOPNOTSUPP while doing ovl_set_origin(). Current code
> > does not fail copy up operation and I tried to retain same behavior.
> > That is just disable metacopy in such case but continue.
> >
> > So if we don't expect all this to happen at all, then I can get rid
> > of this -EOPNOTSUPP check both from ovl_check_setxattr() as well
> > as ovl_copy_up_inode().
> 
> It would be really weird to encounter this, so I think possibly a
> WARN_ON(err == -EOPNOTSUPP) is enough to make sure reality matches our
> expectations.

Hi Miklos,

Effectively that's what current code is doing. Only difference is that it
does a pr_warn() instead of WARN_ON().

So that means I leave current code as it is. Or replace pr_warn() with
WARN_ON(). Something like.

if (WARN_ON(err == -EOPNOTSUPP)) {
	ofs->noxattr = true;
	return xerr;
}

And my code in ovl_copy_up_inode() will continue to remain same as it
will need to handle unexpected -EOPNOTSUPP and disable metacopy operation
for that particular copy up.

Vivek

Re: [PATCH v9 08/15] ovl: Copy up only metadata during copy up where it makes sense

[Amir Goldstein]

On Tue, Jan 9, 2018 at 3:26 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Jan 09, 2018 at 11:49:11AM +0100, Miklos Szeredi wrote:
>> On Mon, Jan 8, 2018 at 6:03 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Mon, Jan 08, 2018 at 11:35:09AM +0100, Miklos Szeredi wrote:
>> >> On Wed, Nov 29, 2017 at 4:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> > 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.
>> >> >
>> >> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>> >> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>> >> > ---
>> >> >  fs/overlayfs/copy_up.c | 38 +++++++++++++++++++++++++++++++-------
>> >> >  1 file changed, 31 insertions(+), 7 deletions(-)
>> >> >
>> >> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> >> > index 31711edf5bde..c5804b87f3c7 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 *lower, bool is_upper)
>> >> >  {
>> >> >         struct ovl_fh *fh;
>> >> > @@ -305,11 +325,8 @@ static int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
>> >> >                         return PTR_ERR(fh);
>> >> >         }
>> >> >
>> >> > -       /*
>> >> > -        * Do not fail when upper doesn't support xattrs.
>> >> > -        */
>> >> >         err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh,
>> >> > -                                fh ? fh->len : 0, 0);
>> >> > +                                fh ? fh->len : 0, -EOPNOTSUPP);
>> >> >         kfree(fh);
>> >> >
>> >> >         return err;
>> >> > @@ -326,6 +343,7 @@ struct ovl_copy_up_ctx {
>> >> >         struct qstr destname;
>> >> >         struct dentry *workdir;
>> >> >         bool tmpfile;
>> >> > +       bool metacopy;
>> >> >  };
>> >> >
>> >> >  static int ovl_link_up(struct ovl_copy_up_ctx *c)
>> >> > @@ -453,10 +471,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>> >> >          *
>> >> >          */
>> >> >         err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
>> >> > -       if (err)
>> >> > -               return err;
>> >> > +       if (err) {
>> >> > +               if (err != -EOPNOTSUPP)
>> >> > +                       return err;
>> >> > +               /* Upper does not support xattr. Copy up data as well */
>> >> > +               c->metacopy = false;
>> >>
>> >> Isn't this supposed to be checked in ovl_get_super()?
>> >
>> > Hi Miklos,
>> >
>> > Right. I think I was confused by code in ovl_check_setxattr().
>> >
>> > int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
>> >                        const char *name, const void *value, size_t size,
>> >                        int xerr)
>> > {
>> >         int err;
>> >         struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
>> >
>> >         if (ofs->noxattr)
>> >                 return xerr;
>> >
>> >         err = ovl_do_setxattr(upperdentry, name, value, size, 0);
>> >
>> >         if (err == -EOPNOTSUPP) {
>> >                 pr_warn("overlayfs: cannot set %s xattr on upper\n", name);
>> >                 ofs->noxattr = true;
>> >                 return xerr;
>> >         }
>> >
>> >         return err;
>> > }
>> >
>> > ofs->noxattr will be set at mount time. Still we check for -EOPNOTSUPP
>> > and set ofs->noxattr again. Is there a situation where this can happen.
>> > Is it intentional. Can we get rid of this code.
>> >
>> > To take care of this situation, I put that code. That is at mount time
>> > xattr was fine and hence ofs->noxattr=false. But later somehow we
>> > encountered -EOPNOTSUPP while doing ovl_set_origin(). Current code
>> > does not fail copy up operation and I tried to retain same behavior.
>> > That is just disable metacopy in such case but continue.
>> >
>> > So if we don't expect all this to happen at all, then I can get rid
>> > of this -EOPNOTSUPP check both from ovl_check_setxattr() as well
>> > as ovl_copy_up_inode().
>>
>> It would be really weird to encounter this, so I think possibly a
>> WARN_ON(err == -EOPNOTSUPP) is enough to make sure reality matches our
>> expectations.
>
> Hi Miklos,
>
> Effectively that's what current code is doing. Only difference is that it
> does a pr_warn() instead of WARN_ON().
>
> So that means I leave current code as it is. Or replace pr_warn() with
> WARN_ON(). Something like.
>
> if (WARN_ON(err == -EOPNOTSUPP)) {
>         ofs->noxattr = true;
>         return xerr;
> }
>

That gives less informative IMO than the pr_warn, because maybe
EOPNOTSUPP can be returned for a specific xattr name by some fs
and as far as the stack trace of WARN_ON, I think if we know the name
of the xattr, we probably already know the overlay code path anyway...

Amir.

Re: [PATCH v9 08/15] ovl: Copy up only metadata during copy up where it makes sense

[Vivek Goyal]

On Tue, Jan 09, 2018 at 03:33:48PM +0200, Amir Goldstein wrote:
> On Tue, Jan 9, 2018 at 3:26 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Tue, Jan 09, 2018 at 11:49:11AM +0100, Miklos Szeredi wrote:
> >> On Mon, Jan 8, 2018 at 6:03 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > On Mon, Jan 08, 2018 at 11:35:09AM +0100, Miklos Szeredi wrote:
> >> >> On Wed, Nov 29, 2017 at 4:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> >> > 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.
> >> >> >
> >> >> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> >> >> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> >> >> > ---
> >> >> >  fs/overlayfs/copy_up.c | 38 +++++++++++++++++++++++++++++++-------
> >> >> >  1 file changed, 31 insertions(+), 7 deletions(-)
> >> >> >
> >> >> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> >> >> > index 31711edf5bde..c5804b87f3c7 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 *lower, bool is_upper)
> >> >> >  {
> >> >> >         struct ovl_fh *fh;
> >> >> > @@ -305,11 +325,8 @@ static int ovl_set_origin(struct dentry *dentry, struct dentry *lower,
> >> >> >                         return PTR_ERR(fh);
> >> >> >         }
> >> >> >
> >> >> > -       /*
> >> >> > -        * Do not fail when upper doesn't support xattrs.
> >> >> > -        */
> >> >> >         err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh,
> >> >> > -                                fh ? fh->len : 0, 0);
> >> >> > +                                fh ? fh->len : 0, -EOPNOTSUPP);
> >> >> >         kfree(fh);
> >> >> >
> >> >> >         return err;
> >> >> > @@ -326,6 +343,7 @@ struct ovl_copy_up_ctx {
> >> >> >         struct qstr destname;
> >> >> >         struct dentry *workdir;
> >> >> >         bool tmpfile;
> >> >> > +       bool metacopy;
> >> >> >  };
> >> >> >
> >> >> >  static int ovl_link_up(struct ovl_copy_up_ctx *c)
> >> >> > @@ -453,10 +471,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> >> >> >          *
> >> >> >          */
> >> >> >         err = ovl_set_origin(c->dentry, c->lowerpath.dentry, temp);
> >> >> > -       if (err)
> >> >> > -               return err;
> >> >> > +       if (err) {
> >> >> > +               if (err != -EOPNOTSUPP)
> >> >> > +                       return err;
> >> >> > +               /* Upper does not support xattr. Copy up data as well */
> >> >> > +               c->metacopy = false;
> >> >>
> >> >> Isn't this supposed to be checked in ovl_get_super()?
> >> >
> >> > Hi Miklos,
> >> >
> >> > Right. I think I was confused by code in ovl_check_setxattr().
> >> >
> >> > int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
> >> >                        const char *name, const void *value, size_t size,
> >> >                        int xerr)
> >> > {
> >> >         int err;
> >> >         struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> >> >
> >> >         if (ofs->noxattr)
> >> >                 return xerr;
> >> >
> >> >         err = ovl_do_setxattr(upperdentry, name, value, size, 0);
> >> >
> >> >         if (err == -EOPNOTSUPP) {
> >> >                 pr_warn("overlayfs: cannot set %s xattr on upper\n", name);
> >> >                 ofs->noxattr = true;
> >> >                 return xerr;
> >> >         }
> >> >
> >> >         return err;
> >> > }
> >> >
> >> > ofs->noxattr will be set at mount time. Still we check for -EOPNOTSUPP
> >> > and set ofs->noxattr again. Is there a situation where this can happen.
> >> > Is it intentional. Can we get rid of this code.
> >> >
> >> > To take care of this situation, I put that code. That is at mount time
> >> > xattr was fine and hence ofs->noxattr=false. But later somehow we
> >> > encountered -EOPNOTSUPP while doing ovl_set_origin(). Current code
> >> > does not fail copy up operation and I tried to retain same behavior.
> >> > That is just disable metacopy in such case but continue.
> >> >
> >> > So if we don't expect all this to happen at all, then I can get rid
> >> > of this -EOPNOTSUPP check both from ovl_check_setxattr() as well
> >> > as ovl_copy_up_inode().
> >>
> >> It would be really weird to encounter this, so I think possibly a
> >> WARN_ON(err == -EOPNOTSUPP) is enough to make sure reality matches our
> >> expectations.
> >
> > Hi Miklos,
> >
> > Effectively that's what current code is doing. Only difference is that it
> > does a pr_warn() instead of WARN_ON().
> >
> > So that means I leave current code as it is. Or replace pr_warn() with
> > WARN_ON(). Something like.
> >
> > if (WARN_ON(err == -EOPNOTSUPP)) {
> >         ofs->noxattr = true;
> >         return xerr;
> > }
> >
> 
> That gives less informative IMO than the pr_warn, because maybe
> EOPNOTSUPP can be returned for a specific xattr name by some fs
> and as far as the stack trace of WARN_ON, I think if we know the name
> of the xattr, we probably already know the overlay code path anyway...

I just realized that in this patch series I already removed this special
handling of -EOPNOTSUPP in patch3.

"[PATCH v9 03/15] ovl: ovl_check_setxattr() get rid of redundant -EOPNOTSUPP check"

Vivek
> 
> Amir.

[PATCH v9 09/15] 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   | 19 ++-----------------
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 24 +++++++++++++++++++++++-
 3 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index c5804b87f3c7..65e993e931ba 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -671,21 +671,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		struct dentry *next;
 		struct dentry *parent;
 
-		/*
-		 * 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))
+		if (ovl_already_copied_up(dentry))
 			break;
 
 		next = dget(dentry);
@@ -712,8 +698,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 
 static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
 {
-	if (ovl_dentry_upper(dentry) &&
-	    ovl_dentry_has_upper_alias(dentry))
+	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 b0a691b463d6..8ff920949004 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -227,6 +227,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 77dc00438d54..572bf46e9f2e 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -314,13 +314,35 @@ 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)
+{
+	/*
+	 * 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))
+		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 v9 10/15] 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      | 76 +++++++++++++++++++++++++++++++++++++++++++++---
 5 files changed, 141 insertions(+), 11 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 65e993e931ba..0bfc1df13b79 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 = {
@@ -490,8 +500,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;
@@ -523,6 +543,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))
@@ -602,6 +624,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)
 {
@@ -645,7 +689,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)
@@ -655,6 +699,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 			err = ovl_do_copy_up(&ctx);
 		if (!err && !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);
@@ -671,7 +717,7 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		struct dentry *next;
 		struct dentry *parent;
 
-		if (ovl_already_copied_up(dentry))
+		if (ovl_already_copied_up(dentry, flags))
 			break;
 
 		next = dget(dentry);
@@ -698,13 +744,13 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 
 static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
 {
-	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 e13921824c70..6ebd351d8a4c 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -158,6 +158,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 8ff920949004..5c5b91deed17 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -27,6 +27,7 @@ enum ovl_path_type {
 #define OVL_XATTR_ORIGIN OVL_XATTR_PREFIX "origin"
 #define OVL_XATTR_IMPURE OVL_XATTR_PREFIX "impure"
 #define OVL_XATTR_NLINK OVL_XATTR_PREFIX "nlink"
+#define OVL_XATTR_METACOPY OVL_XATTR_PREFIX "metacopy"
 
 enum ovl_flag {
 	/* Pure upper dir that may contain non pure upper entries */
@@ -34,6 +35,7 @@ enum ovl_flag {
 	/* Non-merge dir that may contain whiteout entries */
 	OVL_WHITEOUTS,
 	OVL_INDEX,
+	OVL_UPPERDATA,
 };
 
 /*
@@ -186,6 +188,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);
@@ -215,6 +225,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);
@@ -225,9 +239,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 8b9ddcba4b04..3ff60e6381c4 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1281,6 +1281,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 572bf46e9f2e..6d076a420a10 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -231,6 +231,62 @@ void ovl_dentry_set_upper_alias(struct dentry *dentry)
 	oe->has_upper = true;
 }
 
+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;
@@ -314,7 +370,18 @@ 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)
+{
+	if (ovl_dentry_upper(dentry) &&
+	    ovl_dentry_has_upper_alias(dentry) &&
+	    !ovl_dentry_needs_data_copy_up_locked(dentry, flags))
+		return true;
+
+	return false;
+}
+
+bool ovl_already_copied_up(struct dentry *dentry, int flags)
 {
 	/*
 	 * Check if copy-up has happened as well as for upper alias (in
@@ -330,19 +397,20 @@ bool ovl_already_copied_up(struct dentry *dentry)
 	 *      with rename.
 	 */
 	if (ovl_dentry_upper(dentry) &&
-	    ovl_dentry_has_upper_alias(dentry))
+	    ovl_dentry_has_upper_alias(dentry) &&
+	    !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

Re: [PATCH v9 10/15] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Miklos Szeredi]

On Wed, Nov 29, 2017 at 4:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> 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.

Maybe I missed something, but I think there's a bug in this patch:
truncate() is a metadata+data operation since it affects results of
read().  So it should be handled by doing the data copy-up like we did
before.

Thanks,
Miklos

Re: [PATCH v9 10/15] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Vivek Goyal]

On Mon, Jan 08, 2018 at 04:50:00PM +0100, Miklos Szeredi wrote:
> On Wed, Nov 29, 2017 at 4:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > 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.
> 
> Maybe I missed something, but I think there's a bug in this patch:
> truncate() is a metadata+data operation since it affects results of
> read().  So it should be handled by doing the data copy-up like we did
> before.

Hi Miklos,

For truncate operation, we don't do metadata only copy up and fall back
to regular operation.

ovl_need_meta_copy_up() {
        if (flags && ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC)))
                return false;

}

So behavior should not change with metacopy for truncate operation.

Am I missing something?

Thanks
Vivek

Re: [PATCH v9 10/15] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Miklos Szeredi]

On Mon, Jan 8, 2018 at 5:17 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Mon, Jan 08, 2018 at 04:50:00PM +0100, Miklos Szeredi wrote:
>> On Wed, Nov 29, 2017 at 4:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > 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.
>>
>> Maybe I missed something, but I think there's a bug in this patch:
>> truncate() is a metadata+data operation since it affects results of
>> read().  So it should be handled by doing the data copy-up like we did
>> before.
>
> Hi Miklos,
>
> For truncate operation, we don't do metadata only copy up and fall back
> to regular operation.
>
> ovl_need_meta_copy_up() {
>         if (flags && ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC)))
>                 return false;
>
> }
>
> So behavior should not change with metacopy for truncate operation.
>
> Am I missing something?

I'm talking about truncate(2).  That will bypass opening the file
completely, going only through ovl_setattr(), which will do a metadata
only copy up (AFAICS).

Thanks,
Miklos


>
> Thanks
> Vivek

Re: [PATCH v9 10/15] ovl: A new xattr OVL_XATTR_METACOPY for file on upper

[Miklos Szeredi]

On Mon, Jan 8, 2018 at 5:21 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Mon, Jan 8, 2018 at 5:17 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> On Mon, Jan 08, 2018 at 04:50:00PM +0100, Miklos Szeredi wrote:
>>> On Wed, Nov 29, 2017 at 4:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>> > 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.
>>>
>>> Maybe I missed something, but I think there's a bug in this patch:
>>> truncate() is a metadata+data operation since it affects results of
>>> read().  So it should be handled by doing the data copy-up like we did
>>> before.
>>
>> Hi Miklos,
>>
>> For truncate operation, we don't do metadata only copy up and fall back
>> to regular operation.
>>
>> ovl_need_meta_copy_up() {
>>         if (flags && ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC)))
>>                 return false;
>>
>> }
>>
>> So behavior should not change with metacopy for truncate operation.
>>
>> Am I missing something?
>
> I'm talking about truncate(2).  That will bypass opening the file
> completely, going only through ovl_setattr(), which will do a metadata
> only copy up (AFAICS).

Ah, we do do d_real(path->dentry, NULL, O_WRONLY, 0); from
vfs_truncate(), so that will indeed do a data copy up.

So it should be okay.

Thanks,
Miklos

[PATCH v9 11/15] 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 | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 5239a1ff7211..605308a9d60f 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -76,6 +76,10 @@ 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;
+
+	if (ovl_dentry_upper(dentry) && !ovl_has_upperdata(dentry))
+		metacopy = true;
 
 	type = ovl_path_real(dentry, &realpath);
 	old_cred = ovl_override_creds(dentry->d_sb);
@@ -93,7 +97,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,
@@ -117,6 +122,9 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 				WARN_ON_ONCE(stat->dev != lowerstat.dev);
 			else
 				stat->dev = ovl_get_pseudo_dev(dentry);
+
+			if (metacopy)
+				stat->blocks = lowerstat.blocks;
 		}
 		if (samefs) {
 			/*
-- 
2.13.6

[PATCH v9 12/15] ovl: Set OVL_UPPERDATA flag during ovl_lookup()

[Vivek Goyal]

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.

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

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 5ef69bc09e0c..518e048de2db 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -25,6 +25,28 @@ struct ovl_lookup_data {
 	char *redirect;
 };
 
+/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
+static int ovl_check_metacopy(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 int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,
 			      size_t prelen, const char *post)
 {
@@ -602,6 +624,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,
@@ -642,6 +665,20 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 					       roe->numlower, &stack, &ctr);
 			if (err)
 				goto out_put_upper;
+
+			err = ovl_check_metacopy(upperdentry);
+			metacopy = err;
+			if (err < 0)
+				goto out_put_upper;
+			if (metacopy && !ctr) {
+				/*
+				 * Found an upper with metacopy set but
+				 * at the same time there is no lower
+				 * dentry. Something is not right.
+				 */
+				err = -ESTALE;
+				goto out_put_upper;
+			}
 		}
 
 		if (d.redirect) {
@@ -726,6 +763,9 @@ 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);
-- 
2.13.6

[PATCH v9 13/15] 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().

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.

- For metadata inode, mtime should be same as lower and not change. (data
  can't be modified on metadata inode without copyup).

- 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.

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

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 3ff60e6381c4..97dc047ffe79 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -84,8 +84,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))
@@ -101,14 +107,21 @@ 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;
 	}
 
+lower:
 	real = ovl_dentry_lower(dentry);
 	if (!real)
 		goto bug;
-- 
2.13.6

[PATCH v9 14/15] ovl: Fix encryption/compression status of a metacopy only file

[Vivek Goyal]

If file is metacopy only, it is possible that lower is encrypted while
other is not. In that case, report file as encrypted (despite the fact
that only data is encrypted while metadata is not).

Similarly if lower is compressed, report that in stat for a metacopy
only file.

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

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 605308a9d60f..0b83efaac9e0 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -66,6 +66,24 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
 	return err;
 }
 
+#define OVL_STATX_ATTR_MASK	(STATX_ATTR_ENCRYPTED | STATX_ATTR_COMPRESSED)
+
+static void ovl_stat_fix_attributes(struct kstat *ustat, struct kstat *lstat)
+{
+	unsigned int attr_mask, attr;
+
+	attr_mask = lstat->attributes_mask & OVL_STATX_ATTR_MASK;
+	if (!attr_mask)
+		return;
+
+	attr = lstat->attributes & attr_mask;
+	if (!attr)
+		return;
+
+	ustat->attributes_mask |= attr_mask;
+	ustat->attributes |= attr;
+}
+
 int ovl_getattr(const struct path *path, struct kstat *stat,
 		u32 request_mask, unsigned int flags)
 {
@@ -123,8 +141,10 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 			else
 				stat->dev = ovl_get_pseudo_dev(dentry);
 
-			if (metacopy)
+			if (metacopy) {
 				stat->blocks = lowerstat.blocks;
+				ovl_stat_fix_attributes(stat, &lowerstat);
+			}
 		}
 		if (samefs) {
 			/*
-- 
2.13.6

Re: [PATCH v9 14/15] ovl: Fix encryption/compression status of a metacopy only file

[Vivek Goyal]

On Wed, Nov 29, 2017 at 10:54:47AM -0500, Vivek Goyal wrote:
> If file is metacopy only, it is possible that lower is encrypted while
> other is not. In that case, report file as encrypted (despite the fact
> that only data is encrypted while metadata is not).
> 
> Similarly if lower is compressed, report that in stat for a metacopy
> only file.
> 

Hi Amir,

Thinking more about this patch. Taking union of lower and upper attributes
(encryption and compressed) does not feel right.

Right now if lower is compressed and upper is not (metacopy), then we will
report file as compressed. But if lower is not compressed while upper is,
still we will report file as compressed. So there is an anomaly here. 

I think, compression and encryption primarily represent the data state.
So always report the state from inode which has file data (lower). And
don't take union with upper metacopy only inode. Because state in
upper inode does not matter till data is copied up.

This will also simplify my implementation when supporting metacopy in 
middle layer. That way I don't have to do getattr() on all intermediate
metacopy inodes in the chain and take a union. Instead I can go to lowest
most data inode and take encryption and compression attributes from there.

I am not sure why did we decide to take union of lower and upper attrs.
Right now it does not seem to make sense to me.

Thanks
Vivek

> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/inode.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 605308a9d60f..0b83efaac9e0 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -66,6 +66,24 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
>  	return err;
>  }
>  
> +#define OVL_STATX_ATTR_MASK	(STATX_ATTR_ENCRYPTED | STATX_ATTR_COMPRESSED)
> +
> +static void ovl_stat_fix_attributes(struct kstat *ustat, struct kstat *lstat)
> +{
> +	unsigned int attr_mask, attr;
> +
> +	attr_mask = lstat->attributes_mask & OVL_STATX_ATTR_MASK;
> +	if (!attr_mask)
> +		return;
> +
> +	attr = lstat->attributes & attr_mask;
> +	if (!attr)
> +		return;
> +
> +	ustat->attributes_mask |= attr_mask;
> +	ustat->attributes |= attr;
> +}
> +
>  int ovl_getattr(const struct path *path, struct kstat *stat,
>  		u32 request_mask, unsigned int flags)
>  {
> @@ -123,8 +141,10 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>  			else
>  				stat->dev = ovl_get_pseudo_dev(dentry);
>  
> -			if (metacopy)
> +			if (metacopy) {
>  				stat->blocks = lowerstat.blocks;
> +				ovl_stat_fix_attributes(stat, &lowerstat);
> +			}
>  		}
>  		if (samefs) {
>  			/*
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 14/15] ovl: Fix encryption/compression status of a metacopy only file

[Amir Goldstein]

On Thu, Jan 18, 2018 at 4:24 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Nov 29, 2017 at 10:54:47AM -0500, Vivek Goyal wrote:
>> If file is metacopy only, it is possible that lower is encrypted while
>> other is not. In that case, report file as encrypted (despite the fact
>> that only data is encrypted while metadata is not).
>>
>> Similarly if lower is compressed, report that in stat for a metacopy
>> only file.
>>
>
> Hi Amir,
>
> Thinking more about this patch. Taking union of lower and upper attributes
> (encryption and compressed) does not feel right.
>
> Right now if lower is compressed and upper is not (metacopy), then we will
> report file as compressed. But if lower is not compressed while upper is,
> still we will report file as compressed. So there is an anomaly here.
>
> I think, compression and encryption primarily represent the data state.
> So always report the state from inode which has file data (lower). And
> don't take union with upper metacopy only inode. Because state in
> upper inode does not matter till data is copied up.
>
> This will also simplify my implementation when supporting metacopy in
> middle layer. That way I don't have to do getattr() on all intermediate
> metacopy inodes in the chain and take a union. Instead I can go to lowest
> most data inode and take encryption and compression attributes from there.
>
> I am not sure why did we decide to take union of lower and upper attrs.
> Right now it does not seem to make sense to me.
>

Now I am not sure either. I suspect that userspace may need the ability to
know if a file is encrypted, but I can't say for sure.
If you want to know better ask the developers that work of fs/crypto.

I don't mind if you leave this patch out.

Cheers,
Amir.

Re: [PATCH v9 14/15] ovl: Fix encryption/compression status of a metacopy only file

[Vivek Goyal]

On Thu, Jan 18, 2018 at 04:32:01PM +0200, Amir Goldstein wrote:
> On Thu, Jan 18, 2018 at 4:24 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Nov 29, 2017 at 10:54:47AM -0500, Vivek Goyal wrote:
> >> If file is metacopy only, it is possible that lower is encrypted while
> >> other is not. In that case, report file as encrypted (despite the fact
> >> that only data is encrypted while metadata is not).
> >>
> >> Similarly if lower is compressed, report that in stat for a metacopy
> >> only file.
> >>
> >
> > Hi Amir,
> >
> > Thinking more about this patch. Taking union of lower and upper attributes
> > (encryption and compressed) does not feel right.
> >
> > Right now if lower is compressed and upper is not (metacopy), then we will
> > report file as compressed. But if lower is not compressed while upper is,
> > still we will report file as compressed. So there is an anomaly here.
> >
> > I think, compression and encryption primarily represent the data state.
> > So always report the state from inode which has file data (lower). And
> > don't take union with upper metacopy only inode. Because state in
> > upper inode does not matter till data is copied up.
> >
> > This will also simplify my implementation when supporting metacopy in
> > middle layer. That way I don't have to do getattr() on all intermediate
> > metacopy inodes in the chain and take a union. Instead I can go to lowest
> > most data inode and take encryption and compression attributes from there.
> >
> > I am not sure why did we decide to take union of lower and upper attrs.
> > Right now it does not seem to make sense to me.
> >
> 
> Now I am not sure either. I suspect that userspace may need the ability to
> know if a file is encrypted, but I can't say for sure.
> If you want to know better ask the developers that work of fs/crypto.
> 
> I don't mind if you leave this patch out.

For now I think I prefer to leave this patch out. If somebody cares,
fixing it is really easy. Just report encryption/compression state from
the inode which actually has data.

Vivek

[PATCH v9 15/15] 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 0bfc1df13b79..469552eaad5f 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 v9 00/15] overlayfs: Delayed copy up of data

[Amir Goldstein]

On Wed, Nov 29, 2017 at 5:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Hi,
>
> Please find attached V9 of the patches. Minor changes to take care of
> Amir's comments. I have also dropped RFC tag. If there are no concerns,
> then I would like these patches to be included.
>

Sorry Vivek, just realized some issues:

1. Considering Miklos' commit
    438c84c2f0c7 ovl: don't follow redirects if redirect_dir=off
    It is probably not a good idea to allow lookup of metacopy unless
    metacopy=on. Is that already the behavior in V9?

2. An upper layer with metacopy cannot be rotated as middle layer
    becasue non-dir origin is only followed from upper layer.
    This needs to be fixed (follow origin of metacopy from middle layer).

3. You really should write some tests to verify correctness of
    metadata before requesting to include the feature.

I recommend that you start with a simple xfstest that verify expected
behavior of a some basic use cases with
_require_scratch_feature metacopy.

Then, I suggest that you look into extending  unionmount-testsuite's
check_layer() to know about metacopy. Currently it checks that
objects that were supposed to be copied up (dentry.copied_up())
are on upper layer (dentry.on_upper()). It shouldn't be too hard to
extend that with dentry.data_copied_up() and dentry.data_on_upper().
to verify metacopy correctness.

Alas, there are currently no chmod/chown test cases in
unionmount-testsuite, so you will also need to add some test cases.

To properly test metacopy in middle layers (once it is implemented)
you can use ./run --ov=N. Currently, to verify redirect_dir,
upper layer is rotated on mkdir and rename dir. You will need
to add some relevant "rotate points" for the metacopy use cases.
For example, I added rotate and recycle points for testing
handlinks/index:
c427e85 - Cycle mount after link rename of non dir

I never got around to the TODO item
https://github.com/amir73il/overlayfs/wiki/Overlayfs-TODO#testing
"unionmount-testsuite configure rotate points"
I envisioned something like:
  ./run --ov=N rotate=mkdir,rename recycle=link
Instead of the hardcoded rotate/recycle points.

Well, you don't need to implement ALL of that ;-)

Cheers,
Amir.

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Vivek Goyal]

On Sat, Jan 06, 2018 at 09:38:07AM +0200, Amir Goldstein wrote:
> On Wed, Nov 29, 2017 at 5:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Hi,
> >
> > Please find attached V9 of the patches. Minor changes to take care of
> > Amir's comments. I have also dropped RFC tag. If there are no concerns,
> > then I would like these patches to be included.
> >
> 
> Sorry Vivek, just realized some issues:
> 
> 1. Considering Miklos' commit
>     438c84c2f0c7 ovl: don't follow redirects if redirect_dir=off
>     It is probably not a good idea to allow lookup of metacopy unless
>     metacopy=on. Is that already the behavior in V9?

Hi Amir,

Hmm.., no, that's not the behavior in V9. Remember, we wanted to follow
metacopy origin even if metacopy=off. That way a user can mount a
overlayfs with metacopy=off (which was previously mounted as metacopy=on)
and not be broken.

If we follow metacopy only if metacopy=on, then we really need some
mechanism which can atleast warn user that this overlay mount was 
mounted with metacopy=on in the past and expect some unexpected results
if mounted with metacopy=off.

Has there been any agreement on what mechanism to use to remember what
features have been turned on existing overlay mount.

> 
> 2. An upper layer with metacopy cannot be rotated as middle layer
>     becasue non-dir origin is only followed from upper layer.
>     This needs to be fixed (follow origin of metacopy from middle layer).

I was thinking about it. I did not have an immediate user of this
functinality to so I did not bother too much about it. I will look
into it and see how to implement it.

> 
> 3. You really should write some tests to verify correctness of
>     metadata before requesting to include the feature.

Was thinking about this too. Agreed, it is a good idea to write test
cases. Will do.

Vivek

> 
> I recommend that you start with a simple xfstest that verify expected
> behavior of a some basic use cases with
> _require_scratch_feature metacopy.
> 
> Then, I suggest that you look into extending  unionmount-testsuite's
> check_layer() to know about metacopy. Currently it checks that
> objects that were supposed to be copied up (dentry.copied_up())
> are on upper layer (dentry.on_upper()). It shouldn't be too hard to
> extend that with dentry.data_copied_up() and dentry.data_on_upper().
> to verify metacopy correctness.
> 
> Alas, there are currently no chmod/chown test cases in
> unionmount-testsuite, so you will also need to add some test cases.
> 
> To properly test metacopy in middle layers (once it is implemented)
> you can use ./run --ov=N. Currently, to verify redirect_dir,
> upper layer is rotated on mkdir and rename dir. You will need
> to add some relevant "rotate points" for the metacopy use cases.
> For example, I added rotate and recycle points for testing
> handlinks/index:
> c427e85 - Cycle mount after link rename of non dir
> 
> I never got around to the TODO item
> https://github.com/amir73il/overlayfs/wiki/Overlayfs-TODO#testing
> "unionmount-testsuite configure rotate points"
> I envisioned something like:
>   ./run --ov=N rotate=mkdir,rename recycle=link
> Instead of the hardcoded rotate/recycle points.
> 
> Well, you don't need to implement ALL of that ;-)
> 
> Cheers,
> Amir.

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Amir Goldstein]

On Mon, Jan 8, 2018 at 4:13 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Sat, Jan 06, 2018 at 09:38:07AM +0200, Amir Goldstein wrote:
>> On Wed, Nov 29, 2017 at 5:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > Hi,
>> >
>> > Please find attached V9 of the patches. Minor changes to take care of
>> > Amir's comments. I have also dropped RFC tag. If there are no concerns,
>> > then I would like these patches to be included.
>> >
>>
>> Sorry Vivek, just realized some issues:
>>
>> 1. Considering Miklos' commit
>>     438c84c2f0c7 ovl: don't follow redirects if redirect_dir=off
>>     It is probably not a good idea to allow lookup of metacopy unless
>>     metacopy=on. Is that already the behavior in V9?
>
> Hi Amir,
>
> Hmm.., no, that's not the behavior in V9. Remember, we wanted to follow
> metacopy origin even if metacopy=off. That way a user can mount a
> overlayfs with metacopy=off (which was previously mounted as metacopy=on)
> and not be broken.
>

User can also mount with redirect_dir=nofollow after previously mounting with
redirect_dir=on. It's the exact same thing.

> If we follow metacopy only if metacopy=on, then we really need some
> mechanism which can atleast warn user that this overlay mount was
> mounted with metacopy=on in the past and expect some unexpected results
> if mounted with metacopy=off.
>
> Has there been any agreement on what mechanism to use to remember what
> features have been turned on existing overlay mount.
>

There is no agreement, but there is code in upstream that "allows" the user
to make the same with redirect_dir. The consequences of this configuration is
-EPERM on lookup.
You actually have to allow this configuration for security reasons, the only
question is whether metacopy will have 3 modes (off/follow/on) or just on/off
where off implies nofollow.

Amir.

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Vivek Goyal]

On Mon, Jan 08, 2018 at 04:42:59PM +0200, Amir Goldstein wrote:
> On Mon, Jan 8, 2018 at 4:13 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Sat, Jan 06, 2018 at 09:38:07AM +0200, Amir Goldstein wrote:
> >> On Wed, Nov 29, 2017 at 5:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > Hi,
> >> >
> >> > Please find attached V9 of the patches. Minor changes to take care of
> >> > Amir's comments. I have also dropped RFC tag. If there are no concerns,
> >> > then I would like these patches to be included.
> >> >
> >>
> >> Sorry Vivek, just realized some issues:
> >>
> >> 1. Considering Miklos' commit
> >>     438c84c2f0c7 ovl: don't follow redirects if redirect_dir=off
> >>     It is probably not a good idea to allow lookup of metacopy unless
> >>     metacopy=on. Is that already the behavior in V9?
> >
> > Hi Amir,
> >
> > Hmm.., no, that's not the behavior in V9. Remember, we wanted to follow
> > metacopy origin even if metacopy=off. That way a user can mount a
> > overlayfs with metacopy=off (which was previously mounted as metacopy=on)
> > and not be broken.
> >
> 
> User can also mount with redirect_dir=nofollow after previously mounting with
> redirect_dir=on. It's the exact same thing.
> 
> > If we follow metacopy only if metacopy=on, then we really need some
> > mechanism which can atleast warn user that this overlay mount was
> > mounted with metacopy=on in the past and expect some unexpected results
> > if mounted with metacopy=off.
> >
> > Has there been any agreement on what mechanism to use to remember what
> > features have been turned on existing overlay mount.
> >
> 
> There is no agreement, but there is code in upstream that "allows" the user
> to make the same with redirect_dir. The consequences of this configuration is
> -EPERM on lookup.
> You actually have to allow this configuration for security reasons, the only
> question is whether metacopy will have 3 modes (off/follow/on) or just on/off
> where off implies nofollow.

Ok, I will also return -EPERM of metacopy xattr is found but metacopy=on
is not set.

We can introduce metacopy=follow later if need be. Right now I can't
think how it will be useful. Once we have a use case, adding it should
be easy as there are not backward compatibility issues to deal with.

Vivek

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Vivek Goyal]

On Mon, Jan 08, 2018 at 04:42:59PM +0200, Amir Goldstein wrote:
> On Mon, Jan 8, 2018 at 4:13 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Sat, Jan 06, 2018 at 09:38:07AM +0200, Amir Goldstein wrote:
> >> On Wed, Nov 29, 2017 at 5:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > Hi,
> >> >
> >> > Please find attached V9 of the patches. Minor changes to take care of
> >> > Amir's comments. I have also dropped RFC tag. If there are no concerns,
> >> > then I would like these patches to be included.
> >> >
> >>
> >> Sorry Vivek, just realized some issues:
> >>
> >> 1. Considering Miklos' commit
> >>     438c84c2f0c7 ovl: don't follow redirects if redirect_dir=off
> >>     It is probably not a good idea to allow lookup of metacopy unless
> >>     metacopy=on. Is that already the behavior in V9?
> >
> > Hi Amir,
> >
> > Hmm.., no, that's not the behavior in V9. Remember, we wanted to follow
> > metacopy origin even if metacopy=off. That way a user can mount a
> > overlayfs with metacopy=off (which was previously mounted as metacopy=on)
> > and not be broken.
> >
> 
> User can also mount with redirect_dir=nofollow after previously mounting with
> redirect_dir=on. It's the exact same thing.
> 
> > If we follow metacopy only if metacopy=on, then we really need some
> > mechanism which can atleast warn user that this overlay mount was
> > mounted with metacopy=on in the past and expect some unexpected results
> > if mounted with metacopy=off.
> >
> > Has there been any agreement on what mechanism to use to remember what
> > features have been turned on existing overlay mount.
> >
> 
> There is no agreement, but there is code in upstream that "allows" the user
> to make the same with redirect_dir. The consequences of this configuration is
> -EPERM on lookup.
> You actually have to allow this configuration for security reasons, the only
> question is whether metacopy will have 3 modes (off/follow/on) or just on/off
> where off implies nofollow.

Hi Miklos and Amir,

Thinking more about security implications of this. 

Can a user hand craft ORIGIN xattr? I mean, if inode number of lower file
is known, can a user come up with file handle of lower and put in ORIGIN
XATTR?

If yes, this sounds like a security concern. Then I as a user can simply
hand craft an upper file and point to any file in lower and put associated
ORIGIN and METACOPY xattr on upper and next time mount is done with
metacopy=on, I can get access to any lower file?

In fact, not just metacopy, if ORIGIN can be handcrafted, then we will have
to be very careful on when ORIGIN should be followed otherwise an
handcrafted upper can lead to unexpected security issues. (This is
assuming that we will use ORIGIN for more and more features).

Am I overthinking this?

Vivek

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Miklos Szeredi]

On Wed, Jan 10, 2018 at 3:56 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Mon, Jan 08, 2018 at 04:42:59PM +0200, Amir Goldstein wrote:
>> On Mon, Jan 8, 2018 at 4:13 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Sat, Jan 06, 2018 at 09:38:07AM +0200, Amir Goldstein wrote:
>> >> On Wed, Nov 29, 2017 at 5:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> > Hi,
>> >> >
>> >> > Please find attached V9 of the patches. Minor changes to take care of
>> >> > Amir's comments. I have also dropped RFC tag. If there are no concerns,
>> >> > then I would like these patches to be included.
>> >> >
>> >>
>> >> Sorry Vivek, just realized some issues:
>> >>
>> >> 1. Considering Miklos' commit
>> >>     438c84c2f0c7 ovl: don't follow redirects if redirect_dir=off
>> >>     It is probably not a good idea to allow lookup of metacopy unless
>> >>     metacopy=on. Is that already the behavior in V9?
>> >
>> > Hi Amir,
>> >
>> > Hmm.., no, that's not the behavior in V9. Remember, we wanted to follow
>> > metacopy origin even if metacopy=off. That way a user can mount a
>> > overlayfs with metacopy=off (which was previously mounted as metacopy=on)
>> > and not be broken.
>> >
>>
>> User can also mount with redirect_dir=nofollow after previously mounting with
>> redirect_dir=on. It's the exact same thing.
>>
>> > If we follow metacopy only if metacopy=on, then we really need some
>> > mechanism which can atleast warn user that this overlay mount was
>> > mounted with metacopy=on in the past and expect some unexpected results
>> > if mounted with metacopy=off.
>> >
>> > Has there been any agreement on what mechanism to use to remember what
>> > features have been turned on existing overlay mount.
>> >
>>
>> There is no agreement, but there is code in upstream that "allows" the user
>> to make the same with redirect_dir. The consequences of this configuration is
>> -EPERM on lookup.
>> You actually have to allow this configuration for security reasons, the only
>> question is whether metacopy will have 3 modes (off/follow/on) or just on/off
>> where off implies nofollow.
>
> Hi Miklos and Amir,
>
> Thinking more about security implications of this.
>
> Can a user hand craft ORIGIN xattr? I mean, if inode number of lower file
> is known, can a user come up with file handle of lower and put in ORIGIN
> XATTR?
>
> If yes, this sounds like a security concern. Then I as a user can simply
> hand craft an upper file and point to any file in lower and put associated
> ORIGIN and METACOPY xattr on upper and next time mount is done with
> metacopy=on, I can get access to any lower file?

"trusted." prefix xattrs need CAP_SYS_ADMIN, so no, it's not that
simple to exploit.

But if underlying layer comes from untrusted source (e.g. pendrive,
etc) then that could indeed be a security concern.

So, we should make sure users understand the risks associated with
overlay mounting.  And we'll need to be especially careful if we want
to allow unprivileged mount of overlays.

Thanks,
Miklos

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Vivek Goyal]

On Wed, Jan 10, 2018 at 04:08:56PM +0100, Miklos Szeredi wrote:
> On Wed, Jan 10, 2018 at 3:56 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Mon, Jan 08, 2018 at 04:42:59PM +0200, Amir Goldstein wrote:
> >> On Mon, Jan 8, 2018 at 4:13 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > On Sat, Jan 06, 2018 at 09:38:07AM +0200, Amir Goldstein wrote:
> >> >> On Wed, Nov 29, 2017 at 5:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> >> > Hi,
> >> >> >
> >> >> > Please find attached V9 of the patches. Minor changes to take care of
> >> >> > Amir's comments. I have also dropped RFC tag. If there are no concerns,
> >> >> > then I would like these patches to be included.
> >> >> >
> >> >>
> >> >> Sorry Vivek, just realized some issues:
> >> >>
> >> >> 1. Considering Miklos' commit
> >> >>     438c84c2f0c7 ovl: don't follow redirects if redirect_dir=off
> >> >>     It is probably not a good idea to allow lookup of metacopy unless
> >> >>     metacopy=on. Is that already the behavior in V9?
> >> >
> >> > Hi Amir,
> >> >
> >> > Hmm.., no, that's not the behavior in V9. Remember, we wanted to follow
> >> > metacopy origin even if metacopy=off. That way a user can mount a
> >> > overlayfs with metacopy=off (which was previously mounted as metacopy=on)
> >> > and not be broken.
> >> >
> >>
> >> User can also mount with redirect_dir=nofollow after previously mounting with
> >> redirect_dir=on. It's the exact same thing.
> >>
> >> > If we follow metacopy only if metacopy=on, then we really need some
> >> > mechanism which can atleast warn user that this overlay mount was
> >> > mounted with metacopy=on in the past and expect some unexpected results
> >> > if mounted with metacopy=off.
> >> >
> >> > Has there been any agreement on what mechanism to use to remember what
> >> > features have been turned on existing overlay mount.
> >> >
> >>
> >> There is no agreement, but there is code in upstream that "allows" the user
> >> to make the same with redirect_dir. The consequences of this configuration is
> >> -EPERM on lookup.
> >> You actually have to allow this configuration for security reasons, the only
> >> question is whether metacopy will have 3 modes (off/follow/on) or just on/off
> >> where off implies nofollow.
> >
> > Hi Miklos and Amir,
> >
> > Thinking more about security implications of this.
> >
> > Can a user hand craft ORIGIN xattr? I mean, if inode number of lower file
> > is known, can a user come up with file handle of lower and put in ORIGIN
> > XATTR?
> >
> > If yes, this sounds like a security concern. Then I as a user can simply
> > hand craft an upper file and point to any file in lower and put associated
> > ORIGIN and METACOPY xattr on upper and next time mount is done with
> > metacopy=on, I can get access to any lower file?
> 
> "trusted." prefix xattrs need CAP_SYS_ADMIN, so no, it's not that
> simple to exploit.

Aha..., forgot about that. So that will atleast make sure that  for
container use case it will be fine. Even if a process manages to get out
of container, it can't write these "trusted." xattrs and not gain 
additional privileges.

> 
> But if underlying layer comes from untrusted source (e.g. pendrive,
> etc) then that could indeed be a security concern.

Ok. I am writing small section on overlayfs.txt about "metadata only
copyup" and I will mention it there.

> 
> So, we should make sure users understand the risks associated with
> overlay mounting.  And we'll need to be especially careful if we want
> to allow unprivileged mount of overlays.

Unprivileged mounts of overlay will come up at some point of time. It
will be tricky.

Vivek

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Amir Goldstein]

On Wed, Jan 10, 2018 at 4:56 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Mon, Jan 08, 2018 at 04:42:59PM +0200, Amir Goldstein wrote:
>> On Mon, Jan 8, 2018 at 4:13 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Sat, Jan 06, 2018 at 09:38:07AM +0200, Amir Goldstein wrote:
>> >> On Wed, Nov 29, 2017 at 5:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> > Hi,
>> >> >
>> >> > Please find attached V9 of the patches. Minor changes to take care of
>> >> > Amir's comments. I have also dropped RFC tag. If there are no concerns,
>> >> > then I would like these patches to be included.
>> >> >
>> >>
>> >> Sorry Vivek, just realized some issues:
>> >>
>> >> 1. Considering Miklos' commit
>> >>     438c84c2f0c7 ovl: don't follow redirects if redirect_dir=off
>> >>     It is probably not a good idea to allow lookup of metacopy unless
>> >>     metacopy=on. Is that already the behavior in V9?
>> >
>> > Hi Amir,
>> >
>> > Hmm.., no, that's not the behavior in V9. Remember, we wanted to follow
>> > metacopy origin even if metacopy=off. That way a user can mount a
>> > overlayfs with metacopy=off (which was previously mounted as metacopy=on)
>> > and not be broken.
>> >
>>
>> User can also mount with redirect_dir=nofollow after previously mounting with
>> redirect_dir=on. It's the exact same thing.
>>
>> > If we follow metacopy only if metacopy=on, then we really need some
>> > mechanism which can atleast warn user that this overlay mount was
>> > mounted with metacopy=on in the past and expect some unexpected results
>> > if mounted with metacopy=off.
>> >
>> > Has there been any agreement on what mechanism to use to remember what
>> > features have been turned on existing overlay mount.
>> >
>>
>> There is no agreement, but there is code in upstream that "allows" the user
>> to make the same with redirect_dir. The consequences of this configuration is
>> -EPERM on lookup.
>> You actually have to allow this configuration for security reasons, the only
>> question is whether metacopy will have 3 modes (off/follow/on) or just on/off
>> where off implies nofollow.
>
> Hi Miklos and Amir,
>
> Thinking more about security implications of this.
>
> Can a user hand craft ORIGIN xattr? I mean, if inode number of lower file
> is known, can a user come up with file handle of lower and put in ORIGIN
> XATTR?

Yes, its quite easy if you know the underlying fs.
For example for ext4, you don't even need to guess the generation number,
you can provide 0 generation and ext4 treats it as ANY.

>
> If yes, this sounds like a security concern. Then I as a user can simply
> hand craft an upper file and point to any file in lower and put associated
> ORIGIN and METACOPY xattr on upper and next time mount is done with
> metacopy=on, I can get access to any lower file?
>
> In fact, not just metacopy, if ORIGIN can be handcrafted, then we will have
> to be very careful on when ORIGIN should be followed otherwise an
> handcrafted upper can lead to unexpected security issues. (This is
> assuming that we will use ORIGIN for more and more features).
>
> Am I overthinking this?
>

It is exactly as you wrote. Not any less or any more of a security concern
than a hand crafted redirect_dir. The only difference is that without
metacopy=on and without redirect_dir=origin, the only implication of
following an hand crafted origin would be to get a different st_dev/st_ino
and for example, to fake that 2 files/dirs are the same while one is actually
a rootkit/malware. So not that easy to exploit in current upstream.

Amir.

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Vivek Goyal]

On Wed, Jan 10, 2018 at 05:10:22PM +0200, Amir Goldstein wrote:

[..]
> >> >> 1. Considering Miklos' commit
> >> >>     438c84c2f0c7 ovl: don't follow redirects if redirect_dir=off
> >> >>     It is probably not a good idea to allow lookup of metacopy unless
> >> >>     metacopy=on. Is that already the behavior in V9?
> >> >
> >> > Hi Amir,
> >> >
> >> > Hmm.., no, that's not the behavior in V9. Remember, we wanted to follow
> >> > metacopy origin even if metacopy=off. That way a user can mount a
> >> > overlayfs with metacopy=off (which was previously mounted as metacopy=on)
> >> > and not be broken.
> >> >
> >>
> >> User can also mount with redirect_dir=nofollow after previously mounting with
> >> redirect_dir=on. It's the exact same thing.
> >>
> >> > If we follow metacopy only if metacopy=on, then we really need some
> >> > mechanism which can atleast warn user that this overlay mount was
> >> > mounted with metacopy=on in the past and expect some unexpected results
> >> > if mounted with metacopy=off.
> >> >
> >> > Has there been any agreement on what mechanism to use to remember what
> >> > features have been turned on existing overlay mount.
> >> >
> >>
> >> There is no agreement, but there is code in upstream that "allows" the user
> >> to make the same with redirect_dir. The consequences of this configuration is
> >> -EPERM on lookup.
> >> You actually have to allow this configuration for security reasons, the only
> >> question is whether metacopy will have 3 modes (off/follow/on) or just on/off
> >> where off implies nofollow.
> >
> > Hi Miklos and Amir,
> >
> > Thinking more about security implications of this.
> >
> > Can a user hand craft ORIGIN xattr? I mean, if inode number of lower file
> > is known, can a user come up with file handle of lower and put in ORIGIN
> > XATTR?
> 
> Yes, its quite easy if you know the underlying fs.
> For example for ext4, you don't even need to guess the generation number,
> you can provide 0 generation and ext4 treats it as ANY.
> 
> >
> > If yes, this sounds like a security concern. Then I as a user can simply
> > hand craft an upper file and point to any file in lower and put associated
> > ORIGIN and METACOPY xattr on upper and next time mount is done with
> > metacopy=on, I can get access to any lower file?
> >
> > In fact, not just metacopy, if ORIGIN can be handcrafted, then we will have
> > to be very careful on when ORIGIN should be followed otherwise an
> > handcrafted upper can lead to unexpected security issues. (This is
> > assuming that we will use ORIGIN for more and more features).
> >
> > Am I overthinking this?
> >
> 
> It is exactly as you wrote. Not any less or any more of a security concern
> than a hand crafted redirect_dir. The only difference is that without
> metacopy=on and without redirect_dir=origin, the only implication of
> following an hand crafted origin would be to get a different st_dev/st_ino
> and for example, to fake that 2 files/dirs are the same while one is actually
> a rootkit/malware. So not that easy to exploit in current upstream.

Right. Currently we seem to be using origin only for st_dev/st_ino so
no big impact. "metadata only copyup" is first feature which will make
data of lower file available using ORIGIN. So anymore features we add
using ORIGIN, we will have to be extra careful. Atleast make it
conditional on a mount option and document that using this mount option
on untrusted layer source can lead to privilege escalation.

Vivek

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Miklos Szeredi]

On Wed, Jan 10, 2018 at 4:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Jan 10, 2018 at 05:10:22PM +0200, Amir Goldstein wrote:
>
> [..]
>> >> >> 1. Considering Miklos' commit
>> >> >>     438c84c2f0c7 ovl: don't follow redirects if redirect_dir=off
>> >> >>     It is probably not a good idea to allow lookup of metacopy unless
>> >> >>     metacopy=on. Is that already the behavior in V9?
>> >> >
>> >> > Hi Amir,
>> >> >
>> >> > Hmm.., no, that's not the behavior in V9. Remember, we wanted to follow
>> >> > metacopy origin even if metacopy=off. That way a user can mount a
>> >> > overlayfs with metacopy=off (which was previously mounted as metacopy=on)
>> >> > and not be broken.
>> >> >
>> >>
>> >> User can also mount with redirect_dir=nofollow after previously mounting with
>> >> redirect_dir=on. It's the exact same thing.
>> >>
>> >> > If we follow metacopy only if metacopy=on, then we really need some
>> >> > mechanism which can atleast warn user that this overlay mount was
>> >> > mounted with metacopy=on in the past and expect some unexpected results
>> >> > if mounted with metacopy=off.
>> >> >
>> >> > Has there been any agreement on what mechanism to use to remember what
>> >> > features have been turned on existing overlay mount.
>> >> >
>> >>
>> >> There is no agreement, but there is code in upstream that "allows" the user
>> >> to make the same with redirect_dir. The consequences of this configuration is
>> >> -EPERM on lookup.
>> >> You actually have to allow this configuration for security reasons, the only
>> >> question is whether metacopy will have 3 modes (off/follow/on) or just on/off
>> >> where off implies nofollow.
>> >
>> > Hi Miklos and Amir,
>> >
>> > Thinking more about security implications of this.
>> >
>> > Can a user hand craft ORIGIN xattr? I mean, if inode number of lower file
>> > is known, can a user come up with file handle of lower and put in ORIGIN
>> > XATTR?
>>
>> Yes, its quite easy if you know the underlying fs.
>> For example for ext4, you don't even need to guess the generation number,
>> you can provide 0 generation and ext4 treats it as ANY.
>>
>> >
>> > If yes, this sounds like a security concern. Then I as a user can simply
>> > hand craft an upper file and point to any file in lower and put associated
>> > ORIGIN and METACOPY xattr on upper and next time mount is done with
>> > metacopy=on, I can get access to any lower file?
>> >
>> > In fact, not just metacopy, if ORIGIN can be handcrafted, then we will have
>> > to be very careful on when ORIGIN should be followed otherwise an
>> > handcrafted upper can lead to unexpected security issues. (This is
>> > assuming that we will use ORIGIN for more and more features).
>> >
>> > Am I overthinking this?
>> >
>>
>> It is exactly as you wrote. Not any less or any more of a security concern
>> than a hand crafted redirect_dir. The only difference is that without
>> metacopy=on and without redirect_dir=origin, the only implication of
>> following an hand crafted origin would be to get a different st_dev/st_ino
>> and for example, to fake that 2 files/dirs are the same while one is actually
>> a rootkit/malware. So not that easy to exploit in current upstream.
>
> Right. Currently we seem to be using origin only for st_dev/st_ino so
> no big impact. "metadata only copyup" is first feature which will make
> data of lower file available using ORIGIN. So anymore features we add
> using ORIGIN, we will have to be extra careful. Atleast make it
> conditional on a mount option and document that using this mount option
> on untrusted layer source can lead to privilege escalation.

One more reason to use redirect, rather than origin.   Redirect at
least constrains things to inside the overlay, while following origin
can lead to anywhere within the filesystem.

The other reason is backup+restore not breaking.

Thanks,
Miklos

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Vivek Goyal]

On Wed, Jan 10, 2018 at 04:38:22PM +0100, Miklos Szeredi wrote:
> On Wed, Jan 10, 2018 at 4:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Jan 10, 2018 at 05:10:22PM +0200, Amir Goldstein wrote:
> >
> > [..]
> >> >> >> 1. Considering Miklos' commit
> >> >> >>     438c84c2f0c7 ovl: don't follow redirects if redirect_dir=off
> >> >> >>     It is probably not a good idea to allow lookup of metacopy unless
> >> >> >>     metacopy=on. Is that already the behavior in V9?
> >> >> >
> >> >> > Hi Amir,
> >> >> >
> >> >> > Hmm.., no, that's not the behavior in V9. Remember, we wanted to follow
> >> >> > metacopy origin even if metacopy=off. That way a user can mount a
> >> >> > overlayfs with metacopy=off (which was previously mounted as metacopy=on)
> >> >> > and not be broken.
> >> >> >
> >> >>
> >> >> User can also mount with redirect_dir=nofollow after previously mounting with
> >> >> redirect_dir=on. It's the exact same thing.
> >> >>
> >> >> > If we follow metacopy only if metacopy=on, then we really need some
> >> >> > mechanism which can atleast warn user that this overlay mount was
> >> >> > mounted with metacopy=on in the past and expect some unexpected results
> >> >> > if mounted with metacopy=off.
> >> >> >
> >> >> > Has there been any agreement on what mechanism to use to remember what
> >> >> > features have been turned on existing overlay mount.
> >> >> >
> >> >>
> >> >> There is no agreement, but there is code in upstream that "allows" the user
> >> >> to make the same with redirect_dir. The consequences of this configuration is
> >> >> -EPERM on lookup.
> >> >> You actually have to allow this configuration for security reasons, the only
> >> >> question is whether metacopy will have 3 modes (off/follow/on) or just on/off
> >> >> where off implies nofollow.
> >> >
> >> > Hi Miklos and Amir,
> >> >
> >> > Thinking more about security implications of this.
> >> >
> >> > Can a user hand craft ORIGIN xattr? I mean, if inode number of lower file
> >> > is known, can a user come up with file handle of lower and put in ORIGIN
> >> > XATTR?
> >>
> >> Yes, its quite easy if you know the underlying fs.
> >> For example for ext4, you don't even need to guess the generation number,
> >> you can provide 0 generation and ext4 treats it as ANY.
> >>
> >> >
> >> > If yes, this sounds like a security concern. Then I as a user can simply
> >> > hand craft an upper file and point to any file in lower and put associated
> >> > ORIGIN and METACOPY xattr on upper and next time mount is done with
> >> > metacopy=on, I can get access to any lower file?
> >> >
> >> > In fact, not just metacopy, if ORIGIN can be handcrafted, then we will have
> >> > to be very careful on when ORIGIN should be followed otherwise an
> >> > handcrafted upper can lead to unexpected security issues. (This is
> >> > assuming that we will use ORIGIN for more and more features).
> >> >
> >> > Am I overthinking this?
> >> >
> >>
> >> It is exactly as you wrote. Not any less or any more of a security concern
> >> than a hand crafted redirect_dir. The only difference is that without
> >> metacopy=on and without redirect_dir=origin, the only implication of
> >> following an hand crafted origin would be to get a different st_dev/st_ino
> >> and for example, to fake that 2 files/dirs are the same while one is actually
> >> a rootkit/malware. So not that easy to exploit in current upstream.
> >
> > Right. Currently we seem to be using origin only for st_dev/st_ino so
> > no big impact. "metadata only copyup" is first feature which will make
> > data of lower file available using ORIGIN. So anymore features we add
> > using ORIGIN, we will have to be extra careful. Atleast make it
> > conditional on a mount option and document that using this mount option
> > on untrusted layer source can lead to privilege escalation.
> 
> One more reason to use redirect, rather than origin.   Redirect at
> least constrains things to inside the overlay, while following origin
> can lead to anywhere within the filesystem.
> 
> The other reason is backup+restore not breaking.

Agreed. Looks like using REDIRECT instead of ORIGIN is more appealing. I 
will give it a try and see what issues do I run into.

Vivek

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Amir Goldstein]

On Wed, Jan 10, 2018 at 5:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Jan 10, 2018 at 04:38:22PM +0100, Miklos Szeredi wrote:
>> On Wed, Jan 10, 2018 at 4:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Wed, Jan 10, 2018 at 05:10:22PM +0200, Amir Goldstein wrote:
>> >
[...]
>> >>
>> >> It is exactly as you wrote. Not any less or any more of a security concern
>> >> than a hand crafted redirect_dir. The only difference is that without
>> >> metacopy=on and without redirect_dir=origin, the only implication of
>> >> following an hand crafted origin would be to get a different st_dev/st_ino
>> >> and for example, to fake that 2 files/dirs are the same while one is actually
>> >> a rootkit/malware. So not that easy to exploit in current upstream.
>> >
>> > Right. Currently we seem to be using origin only for st_dev/st_ino so
>> > no big impact. "metadata only copyup" is first feature which will make
>> > data of lower file available using ORIGIN. So anymore features we add
>> > using ORIGIN, we will have to be extra careful. Atleast make it
>> > conditional on a mount option and document that using this mount option
>> > on untrusted layer source can lead to privilege escalation.
>>
>> One more reason to use redirect, rather than origin.   Redirect at
>> least constrains things to inside the overlay, while following origin
>> can lead to anywhere within the filesystem.
>>
>> The other reason is backup+restore not breaking.
>
> Agreed. Looks like using REDIRECT instead of ORIGIN is more appealing. I
> will give it a try and see what issues do I run into.
>

As long as we are listing the pros and cons, REDIRECT is limited by path length
and a non-secure decode of non-dir ORIGIN is much faster then following all
potential parent redirects.

You could also encode a "secure/connectable" ORIGIN for non-dir and verify
that decoded dentry is within layer bounds, same as I did for
redirect_dir=origin
for directory ORIGIN. However, decoding a connectable non-dir ORIGIN may
be a lot heavier than redirect in some cases... and it cannot replace the
non-connectable ORIGIN needed for hardlinks copy up.

Amir.

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Miklos Szeredi]

On Wed, Jan 10, 2018 at 4:54 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Jan 10, 2018 at 5:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> On Wed, Jan 10, 2018 at 04:38:22PM +0100, Miklos Szeredi wrote:
>>> On Wed, Jan 10, 2018 at 4:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>> > On Wed, Jan 10, 2018 at 05:10:22PM +0200, Amir Goldstein wrote:
>>> >
> [...]
>>> >>
>>> >> It is exactly as you wrote. Not any less or any more of a security concern
>>> >> than a hand crafted redirect_dir. The only difference is that without
>>> >> metacopy=on and without redirect_dir=origin, the only implication of
>>> >> following an hand crafted origin would be to get a different st_dev/st_ino
>>> >> and for example, to fake that 2 files/dirs are the same while one is actually
>>> >> a rootkit/malware. So not that easy to exploit in current upstream.
>>> >
>>> > Right. Currently we seem to be using origin only for st_dev/st_ino so
>>> > no big impact. "metadata only copyup" is first feature which will make
>>> > data of lower file available using ORIGIN. So anymore features we add
>>> > using ORIGIN, we will have to be extra careful. Atleast make it
>>> > conditional on a mount option and document that using this mount option
>>> > on untrusted layer source can lead to privilege escalation.
>>>
>>> One more reason to use redirect, rather than origin.   Redirect at
>>> least constrains things to inside the overlay, while following origin
>>> can lead to anywhere within the filesystem.
>>>
>>> The other reason is backup+restore not breaking.
>>
>> Agreed. Looks like using REDIRECT instead of ORIGIN is more appealing. I
>> will give it a try and see what issues do I run into.
>>
>
> As long as we are listing the pros and cons, REDIRECT is limited by path length
> and a non-secure decode of non-dir ORIGIN is much faster then following all
> potential parent redirects.

True.

So there may be use case for following ORIGIN, but it should be
optional and the security and other implications clearly spelled out
in the doc.

Thanks,
Miklos

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Vivek Goyal]

On Wed, Jan 10, 2018 at 05:03:07PM +0100, Miklos Szeredi wrote:
> On Wed, Jan 10, 2018 at 4:54 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Wed, Jan 10, 2018 at 5:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> On Wed, Jan 10, 2018 at 04:38:22PM +0100, Miklos Szeredi wrote:
> >>> On Wed, Jan 10, 2018 at 4:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >>> > On Wed, Jan 10, 2018 at 05:10:22PM +0200, Amir Goldstein wrote:
> >>> >
> > [...]
> >>> >>
> >>> >> It is exactly as you wrote. Not any less or any more of a security concern
> >>> >> than a hand crafted redirect_dir. The only difference is that without
> >>> >> metacopy=on and without redirect_dir=origin, the only implication of
> >>> >> following an hand crafted origin would be to get a different st_dev/st_ino
> >>> >> and for example, to fake that 2 files/dirs are the same while one is actually
> >>> >> a rootkit/malware. So not that easy to exploit in current upstream.
> >>> >
> >>> > Right. Currently we seem to be using origin only for st_dev/st_ino so
> >>> > no big impact. "metadata only copyup" is first feature which will make
> >>> > data of lower file available using ORIGIN. So anymore features we add
> >>> > using ORIGIN, we will have to be extra careful. Atleast make it
> >>> > conditional on a mount option and document that using this mount option
> >>> > on untrusted layer source can lead to privilege escalation.
> >>>
> >>> One more reason to use redirect, rather than origin.   Redirect at
> >>> least constrains things to inside the overlay, while following origin
> >>> can lead to anywhere within the filesystem.
> >>>
> >>> The other reason is backup+restore not breaking.
> >>
> >> Agreed. Looks like using REDIRECT instead of ORIGIN is more appealing. I
> >> will give it a try and see what issues do I run into.
> >>
> >
> > As long as we are listing the pros and cons, REDIRECT is limited by path length
> > and a non-secure decode of non-dir ORIGIN is much faster then following all
> > potential parent redirects.
> 
> True.
> 
> So there may be use case for following ORIGIN, but it should be
> optional and the security and other implications clearly spelled out
> in the doc.

Hmm..., So does that mean we should create both REDIRCT and ORIGIN
during metadata copyup and use ORIGIN if available otherwise fallback
to slower REDIRECT. 

Or something else.

Vivek

Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data

[Amir Goldstein]

On Wed, Jan 10, 2018 at 6:30 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Jan 10, 2018 at 05:03:07PM +0100, Miklos Szeredi wrote:
>> On Wed, Jan 10, 2018 at 4:54 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> > On Wed, Jan 10, 2018 at 5:47 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> On Wed, Jan 10, 2018 at 04:38:22PM +0100, Miklos Szeredi wrote:
>> >>> On Wed, Jan 10, 2018 at 4:27 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >>> > On Wed, Jan 10, 2018 at 05:10:22PM +0200, Amir Goldstein wrote:
>> >>> >
>> > [...]
>> >>> >>
>> >>> >> It is exactly as you wrote. Not any less or any more of a security concern
>> >>> >> than a hand crafted redirect_dir. The only difference is that without
>> >>> >> metacopy=on and without redirect_dir=origin, the only implication of
>> >>> >> following an hand crafted origin would be to get a different st_dev/st_ino
>> >>> >> and for example, to fake that 2 files/dirs are the same while one is actually
>> >>> >> a rootkit/malware. So not that easy to exploit in current upstream.
>> >>> >
>> >>> > Right. Currently we seem to be using origin only for st_dev/st_ino so
>> >>> > no big impact. "metadata only copyup" is first feature which will make
>> >>> > data of lower file available using ORIGIN. So anymore features we add
>> >>> > using ORIGIN, we will have to be extra careful. Atleast make it
>> >>> > conditional on a mount option and document that using this mount option
>> >>> > on untrusted layer source can lead to privilege escalation.
>> >>>
>> >>> One more reason to use redirect, rather than origin.   Redirect at
>> >>> least constrains things to inside the overlay, while following origin
>> >>> can lead to anywhere within the filesystem.
>> >>>
>> >>> The other reason is backup+restore not breaking.
>> >>
>> >> Agreed. Looks like using REDIRECT instead of ORIGIN is more appealing. I
>> >> will give it a try and see what issues do I run into.
>> >>
>> >
>> > As long as we are listing the pros and cons, REDIRECT is limited by path length
>> > and a non-secure decode of non-dir ORIGIN is much faster then following all
>> > potential parent redirects.
>>
>> True.
>>
>> So there may be use case for following ORIGIN, but it should be
>> optional and the security and other implications clearly spelled out
>> in the doc.
>
> Hmm..., So does that mean we should create both REDIRCT and ORIGIN
> during metadata copyup and use ORIGIN if available otherwise fallback
> to slower REDIRECT.
>

Technically, for non-hardlinks you only need to set REDIRECT on rename,
same as for dir, when renaming a non-dir METACOPY upper.
Things get more complicated with METACOPY and hardlinks.
A METACOPY upper with hardlinks, must use an absolute REDIRECT,
(to any of the lower hardlinks) otherwise you may be redirecting a relative
name from the wrong upper alias to nowhere.

The simplest thing is to store absolute REDIRECT on every metacopy up.
Next simple and less brutal is to store absolute REDIRECT on the first rename
or link of METACOPY upper.

Amir.