overlayfs: Delayed copy up of data

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 <[email protected]>

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.

Quoted 4 lines · 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).

Quoted 2 lines

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.

Quoted 8 lines · Miklos Szeredi

Quoted 6 lines · 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.

Quoted 4 lines

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.

Quoted 6 lines

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.

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 <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/copy_up.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

fs/overlayfs/copy_up.c +22 -4

index 31711edf5bde..c5804b87f3c7 100644
@@ -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)

Quoted 64 lines · 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 <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/copy_up.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

fs/overlayfs/copy_up.c +22 -4

index 31711edf5bde..c5804b87f3c7 100644
@@ -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;
 };
 

fs/overlayfs/copy_up.c static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp) · line 478

Quoted 12 lines

fs/overlayfs/copy_up.c +5 -2

 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()?

Quoted 65 lines · Miklos Szeredi

Quoted 64 lines · 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 <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/copy_up.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

fs/overlayfs/copy_up.c +22 -4

index 31711edf5bde..c5804b87f3c7 100644
@@ -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;
 };
 

Quoted 14 lines

fs/overlayfs/copy_up.c static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp) · line 478

Quoted 12 lines

fs/overlayfs/copy_up.c +5 -2

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

Quoted 116 lines · Vivek Goyal

Quoted 65 lines · Miklos Szeredi

Quoted 64 lines · 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 <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/copy_up.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

fs/overlayfs/copy_up.c +22 -4

index 31711edf5bde..c5804b87f3c7 100644
@@ -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;
 };
 

Quoted 14 lines

fs/overlayfs/copy_up.c static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp) · line 478

Quoted 12 lines

fs/overlayfs/copy_up.c +5 -2

 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.

Quoted 3 lines

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.

Quoted 117 lines · Miklos Szeredi

Quoted 116 lines · Vivek Goyal

Quoted 65 lines · Miklos Szeredi

Quoted 64 lines · 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 <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/copy_up.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

fs/overlayfs/copy_up.c +22 -4

index 31711edf5bde..c5804b87f3c7 100644
@@ -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;
 };
 

Quoted 14 lines

fs/overlayfs/copy_up.c static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp) · line 478

Quoted 12 lines

fs/overlayfs/copy_up.c +5 -2

 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.

Quoted 7 lines

Quoted 3 lines

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;
}

Quoted 131 lines · Vivek Goyal

Quoted 117 lines · Miklos Szeredi

Quoted 116 lines · Vivek Goyal

Quoted 65 lines · Miklos Szeredi

Quoted 64 lines · 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 <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/copy_up.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

fs/overlayfs/copy_up.c +22 -4

index 31711edf5bde..c5804b87f3c7 100644
@@ -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;
 };
 

Quoted 14 lines

fs/overlayfs/copy_up.c static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp) · line 478

Quoted 12 lines

fs/overlayfs/copy_up.c +5 -2

 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.

Quoted 7 lines

Quoted 3 lines

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

Quoted 8 lines

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.

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.

Quoted 3 lines · 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.

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.

Quoted 9 lines · Miklos Szeredi

Quoted 3 lines · 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.

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

Quoted 22 lines · Vivek Goyal

Quoted 9 lines · Miklos Szeredi

Quoted 3 lines · 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.

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;

}

Quoted 3 lines

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

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 <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/inode.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

fs/overlayfs/inode.c +21 -1

index 605308a9d60f..0b83efaac9e0 100644
@@ -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) {
 			/*

Signature · 6 lines

2.13.6

--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 7 lines · 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.

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.

Quoted 23 lines · Vivek Goyal

Quoted 7 lines · 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.

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.

Quoted 8 lines

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.

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.

Quoted 6 lines · 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.

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.

Quoted 8 lines · Amir Goldstein

Quoted 6 lines · 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.

Quoted 6 lines

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.

Quoted 16 lines · Vivek Goyal

Quoted 8 lines · Amir Goldstein

Quoted 6 lines · 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.

Quoted 6 lines

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?

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 16 lines · Vivek Goyal

Quoted 8 lines · Amir Goldstein

Quoted 6 lines · 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.

Quoted 6 lines

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?

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 37 lines · Amir Goldstein

Quoted 16 lines · Vivek Goyal

Quoted 8 lines · Amir Goldstein

Quoted 6 lines · 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.

Quoted 6 lines

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?

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 6 lines

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?

Quoted 53 lines · Vivek Goyal

Quoted 37 lines · Amir Goldstein

Quoted 16 lines · Vivek Goyal

Quoted 8 lines · Amir Goldstein

Quoted 6 lines · 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.

Quoted 6 lines

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?

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 6 lines

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?

Quoted 4 lines

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.

Quoted 37 lines · Amir Goldstein

Quoted 16 lines · Vivek Goyal

Quoted 8 lines · Amir Goldstein

Quoted 6 lines · 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.

Quoted 6 lines

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?

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 6 lines

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?

Quoted 32 lines

Quoted 25 lines

Quoted 5 lines

Quoted 4 lines

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?

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

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?

Quoted 7 lines

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.

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

[..]

Quoted 58 lines

Quoted 32 lines

Quoted 25 lines

Quoted 5 lines

Quoted 4 lines

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?

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

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?

Quoted 7 lines

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?

Quoted 6 lines

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.

Quoted 68 lines · Vivek Goyal

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

[..]

Quoted 58 lines

Quoted 32 lines

Quoted 25 lines

Quoted 5 lines

Quoted 4 lines

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?

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

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?

Quoted 7 lines

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?

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 3 lines · Miklos Szeredi

Quoted 2 lines · Vivek Goyal

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

Quoted 15 lines

Quoted 14 lines

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 4 lines · Vivek Goyal

Quoted 3 lines · Miklos Szeredi

Quoted 2 lines · Vivek Goyal

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

[...]

Quoted 21 lines

Quoted 20 lines

Quoted 8 lines

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 27 lines · Amir Goldstein

Quoted 4 lines · Vivek Goyal

Quoted 3 lines · Miklos Szeredi

Quoted 2 lines · Vivek Goyal

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

[...]

Quoted 21 lines

Quoted 20 lines

Quoted 8 lines

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 38 lines · Miklos Szeredi

Quoted 27 lines · Amir Goldstein

Quoted 4 lines · Vivek Goyal

Quoted 3 lines · Miklos Szeredi

Quoted 2 lines · Vivek Goyal

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

[...]

Quoted 21 lines

Quoted 20 lines

Quoted 8 lines

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 3 lines

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.