overlayfs: Delayed copy up of data

ovl_get_inode() right now has 5 parameters. Soon this patch series will add 2 more and suddenly argument list starts looking too long.

Hence pass arguments to ovl_get_inode() in a structure and it looks little cleaner.

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

---
 fs/overlayfs/export.c    |  4 +++-
 fs/overlayfs/inode.c     | 19 ++++++++++---------
 fs/overlayfs/namei.c     |  6 ++++--
 fs/overlayfs/overlayfs.h |  4 +---
 fs/overlayfs/ovl_entry.h |  8 ++++++++
 5 files changed, 26 insertions(+), 15 deletions(-)

fs/overlayfs/export.c +0 -0

index 425a94672300..867946adcbc5 100644

fs/overlayfs/export.c +2 -1

        /* We get overlay directory dentries with ovl_lookup_real() */
        if (d_is_dir(upper ?: lower))
                return ERR_PTR(-EIO);
 
-       inode = ovl_get_inode(sb, dget(upper), lowerpath, index, !!lower);
+       oip.upperdentry = dget(upper);
+       inode = ovl_get_inode(&oip);
        if (IS_ERR(inode)) {
                dput(upper);
                return ERR_CAST(inode);

fs/overlayfs/inode.c +0 -0

index 7abcf96e94fc..2fe9538fffc9 100644

Quoted 7 lines · Vivek Goyal

ovl_get_inode() right now has 5 parameters. Soon this patch series will add 2 more and suddenly argument list starts looking too long.

Hence pass arguments to ovl_get_inode() in a structure and it looks little cleaner.

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

Reviewed-by Amir Goldstein <[email protected]>

After fixing some nits

Quoted 8 lines

---
 fs/overlayfs/export.c    |  4 +++-
 fs/overlayfs/inode.c     | 19 ++++++++++---------
 fs/overlayfs/namei.c     |  6 ++++--
 fs/overlayfs/overlayfs.h |  4 +---
 fs/overlayfs/ovl_entry.h |  8 ++++++++
 5 files changed, 26 insertions(+), 15 deletions(-)

fs/overlayfs/export.c static struct dentry *ovl_obtain_alias(struct super_block *sb, · line 303

Quoted 9 lines

fs/overlayfs/export.c +1 -0

index 425a94672300..867946adcbc5 100644
@@ -300,12 +300,14 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
        struct dentry *dentry;
        struct inode *inode;
        struct ovl_entry *oe;
+       struct ovl_inode_params oip = {sb, NULL, lowerpath, index, !!lower};

{
.index = index,
...

Quoted 20 lines

fs/overlayfs/export.c +2 -1

        /* We get overlay directory dentries with ovl_lookup_real() */
        if (d_is_dir(upper ?: lower))
                return ERR_PTR(-EIO);
 
-       inode = ovl_get_inode(sb, dget(upper), lowerpath, index, !!lower);
+       oip.upperdentry = dget(upper);
+       inode = ovl_get_inode(&oip);
        if (IS_ERR(inode)) {
                dput(upper);
                return ERR_CAST(inode);

fs/overlayfs/inode.c +0 -0

index 7abcf96e94fc..2fe9538fffc9 100644
@@ -792,15 +792,16 @@ static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper,
        return true;
 }
 

fs/overlayfs/inode.c static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper, · line 795

Quoted 4 lines

fs/overlayfs/inode.c +1 -3

-struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
-                           struct ovl_path *lowerpath, struct dentry *index,
-                           unsigned int numlower)
+struct inode *ovl_get_inode(struct ovl_inode_params *oip)

IMO, sb should not be part of ovl_inode_params
it should be passed as a separate arg

and ovl_inode_params
should be passed to ovl_inode_init() as well.

Quoted 21 lines · Amir Goldstein

Quoted 7 lines · Vivek Goyal

ovl_get_inode() right now has 5 parameters. Soon this patch series will add 2 more and suddenly argument list starts looking too long.

Hence pass arguments to ovl_get_inode() in a structure and it looks little cleaner.

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

Reviewed-by Amir Goldstein <[email protected]>

After fixing some nits

Quoted 8 lines

---
 fs/overlayfs/export.c    |  4 +++-
 fs/overlayfs/inode.c     | 19 ++++++++++---------
 fs/overlayfs/namei.c     |  6 ++++--
 fs/overlayfs/overlayfs.h |  4 +---
 fs/overlayfs/ovl_entry.h |  8 ++++++++
 5 files changed, 26 insertions(+), 15 deletions(-)

Quoted 13 lines

fs/overlayfs/export.c static struct dentry *ovl_obtain_alias(struct super_block *sb, · line 303

Quoted 9 lines

fs/overlayfs/export.c +1 -0

index 425a94672300..867946adcbc5 100644
@@ -300,12 +300,14 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
        struct dentry *dentry;
        struct inode *inode;
        struct ovl_entry *oe;
+       struct ovl_inode_params oip = {sb, NULL, lowerpath, index, !!lower};

{
.index = index,
...

Will fix this.

Quoted 20 lines

Quoted 20 lines

fs/overlayfs/export.c +2 -1

        /* We get overlay directory dentries with ovl_lookup_real() */
        if (d_is_dir(upper ?: lower))
                return ERR_PTR(-EIO);
 
-       inode = ovl_get_inode(sb, dget(upper), lowerpath, index, !!lower);
+       oip.upperdentry = dget(upper);
+       inode = ovl_get_inode(&oip);
        if (IS_ERR(inode)) {
                dput(upper);
                return ERR_CAST(inode);

fs/overlayfs/inode.c +0 -0

index 7abcf96e94fc..2fe9538fffc9 100644
@@ -792,15 +792,16 @@ static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper,
        return true;
 }
 

Quoted 7 lines

fs/overlayfs/inode.c static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper, · line 795

Quoted 4 lines

fs/overlayfs/inode.c +1 -3

-struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
-                           struct ovl_path *lowerpath, struct dentry *index,
-                           unsigned int numlower)
+struct inode *ovl_get_inode(struct ovl_inode_params *oip)

IMO, sb should not be part of ovl_inode_params
it should be passed as a separate arg

Will do.

Quoted 2 lines

and ovl_inode_params
should be passed to ovl_inode_init() as well.

I want to avoid making this change as part of this series. Right now it has 3 args and my patches add one more. Four arguments are not a lot. And even if we pass oip, only 3 will go in it. inode param remains outside of it.

And it is being called from super.c as well as inode.c. So while fixable, but it increases the code further. At this point of time, trying to limit the changes to which are really needed and are easy to do.

Quoted 3 lines · Vivek Goyal

ovl_get_inode() right now has 5 parameters. Soon this patch series will add 2 more and suddenly argument list starts looking too long.

Quoted 4 lines

Hence pass arguments to ovl_get_inode() in a structure and it looks little cleaner.

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

Reviewed-by Amir Goldstein <[email protected]>

After fixing some nits

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

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

We don't support metacopy feature with nfs_export. So in nfs_export code, we set OVL_UPPERDATA flag set unconditionally if upper inode exists.

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

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

---
 fs/overlayfs/export.c    |   3 ++
 fs/overlayfs/inode.c     |  11 ++++-
 fs/overlayfs/namei.c     | 108 +++++++++++++++++++++++++++++++++++++++++------
 fs/overlayfs/overlayfs.h |   1 +
 fs/overlayfs/util.c      |  22 ++++++++++
 5 files changed, 130 insertions(+), 15 deletions(-)

fs/overlayfs/export.c +3 -0

index 0549286cc55e..52a09a9f74b7 100644
@@ -314,6 +314,9 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
                return ERR_CAST(inode);
        }
 
+       if (upper)
+               ovl_set_flag(OVL_UPPERDATA, inode);
+
        dentry = d_find_any_alias(inode);
        if (!dentry) {
                dentry = d_alloc_anon(inode->i_sb);

fs/overlayfs/inode.c +10 -1

index c128d5d54d0f..83b276ce0240 100644
@@ -770,7 +770,7 @@ struct inode *ovl_get_inode(struct ovl_inode_params *oip)
        bool bylower = ovl_hash_bylower(oip->sb, upperdentry, lowerdentry,
                                        oip->index);
        int fsid = bylower ? oip->lowerpath->layer->fsid : 0;
-       bool is_dir;
+       bool is_dir, metacopy = false;
        unsigned long ino = 0;
        int err = -ENOMEM;
 

@@ -830,6 +830,15 @@ struct inode *ovl_get_inode(struct ovl_inode_params *oip)
        if (oip->index)
                ovl_set_flag(OVL_INDEX, inode);
 
+       if (upperdentry) {
+               err = ovl_check_metacopy_xattr(upperdentry);
+               if (err < 0)
+                       goto out_err;
+               metacopy = err;
+               if (!metacopy)
+                       ovl_set_flag(OVL_UPPERDATA, inode);
+       }
+
        OVL_I(inode)->redirect = oip->redirect;
 
        /* Check for non-merge dir that may have whiteouts */

fs/overlayfs/namei.c +94 -14

index 8fd817bf5529..b2ff08985e29 100644
@@ -24,6 +24,7 @@ struct ovl_lookup_data {
        bool stop;
        bool last;
        char *redirect;
+       bool metacopy;
 };
 
 static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,

@@ -253,19 +254,29 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
                goto put_and_out;
        }
        if (!d_can_lookup(this)) {
-               d->stop = true;
-               if (d->is_dir)
+               if (d->is_dir) {
+                       d->stop = true;
                        goto put_and_out;
-
+               }
                /*
                 * NB: handle failure to lookup non-last element when non-dir
                 * redirects become possible
                 */
                WARN_ON(!last_element);
+               err = ovl_check_metacopy_xattr(this);
+               if (err < 0)
+                       goto out_err;
+               d->stop = !err;
+               d->metacopy = !!err;
                goto out;
        }
-       if (last_element)
+       if (last_element) {
+               if (d->metacopy) {
+                       err = -ESTALE;
+                       goto out_err;
+               }
                d->is_dir = true;
+       }
        if (d->last)
                goto out;
 

@@ -823,7 +834,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
        struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
        struct ovl_entry *poe = dentry->d_parent->d_fsdata;
        struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
-       struct ovl_path *stack = NULL;
+       struct ovl_path *stack = NULL, *origin_path = NULL;
        struct dentry *upperdir, *upperdentry = NULL;
        struct dentry *origin = NULL;
        struct dentry *index = NULL;

@@ -834,6 +845,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,

@@ -841,6 +853,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                .stop = false,
                .last = ofs->config.redirect_follow ? false : !poe->numlower,
                .redirect = NULL,
+               .metacopy = false,
        };
 
        if (dentry->d_name.len > ofs->namelen)

@@ -859,7 +872,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                        goto out;
                }
                if (upperdentry && !d.is_dir) {
-                       BUG_ON(!d.stop || d.redirect);
+                       unsigned int origin_ctr = 0;
+                       BUG_ON(d.redirect);
                        /*
                         * Lookup copy up origin by decoding origin file handle.
                         * We may get a disconnected dentry, which is fine,

@@ -870,9 +884,13 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                         * number - it's the same as if we held a reference
                         * to a dentry in lower layer that was moved under us.
                         */
-                       err = ovl_check_origin(ofs, upperdentry, &stack, &ctr);
+                       err = ovl_check_origin(ofs, upperdentry, &origin_path,
+                                              &origin_ctr);
                        if (err)
                                goto out_put_upper;
+
+                       if (d.metacopy)
+                               metacopy = true;
                }
 
                if (d.redirect) {

@@ -913,7 +931,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                 * If no origin fh is stored in upper of a merge dir, store fh
                 * of lower dir and set upper parent "impure".
                 */
-               if (upperdentry && !ctr && !ofs->noxattr) {
+               if (upperdentry && !ctr && !ofs->noxattr && d.is_dir) {
                        err = ovl_fix_origin(dentry, this, upperdentry);
                        if (err) {
                                dput(this);

@@ -925,18 +943,36 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                 * When "verify_lower" feature is enabled, do not merge with a
                 * lower dir that does not match a stored origin xattr. In any
                 * case, only verified origin is used for index lookup.
+                *
+                * For non-dir dentry, make sure dentry found by lookup
+                * matches the origin stored in upper. Otherwise its an
+                * error.
                 */
-               if (upperdentry && !ctr && ovl_verify_lower(dentry->d_sb)) {
+               if (upperdentry && !ctr &&
+                   ((d.is_dir && ovl_verify_lower(dentry->d_sb)) ||
+                    (!d.is_dir && origin_path))) {
                        err = ovl_verify_origin(upperdentry, this, false);
                        if (err) {
                                dput(this);
-                               break;
+                               if (d.is_dir)
+                                       break;
+                               goto out_put;
                        }
-
-                       /* Bless lower dir as verified origin */
+                       /* Bless lower as verified origin */
                        origin = this;
                }
 
+               if (d.metacopy)
+                       metacopy = true;
+               /*
+                * Do not store intermediate metacopy dentries in chain,
+                * except top most lower metacopy dentry
+                */
+               if (d.metacopy && ctr) {
+                       dput(this);
+                       continue;
+               }
+
                stack[ctr].dentry = this;
                stack[ctr].layer = lower.layer;
                ctr++;

@@ -968,13 +1004,49 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                }
        }
 
+       if (metacopy) {
+               /*
+                * Found a metacopy dentry but did not find corresponding
+                * data dentry
+                */
+               if (d.metacopy) {
+                       err = -ESTALE;
+                       goto out_put;
+               }
+
+               err = -EPERM;
+               if (!ofs->config.metacopy) {
+                       pr_warn_ratelimited("overlay: refusing to follow"
+                                           " metacopy origin for (%pd2)\n",
+                                           dentry);
+                       goto out_put;
+               }
+       } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
+               if (WARN_ON(stack != NULL)) {
+                       err = -EIO;
+                       goto out_put;
+               }
+               stack = origin_path;
+               ctr = 1;
+               origin_path = NULL;
+       }
+
        /*
         * Lookup index by lower inode and verify it matches upper inode.
         * We only trust dir index if we verified that lower dir matches
         * origin, otherwise dir index entries may be inconsistent and we
-        * ignore them. Always lookup index of non-dir and non-upper.
+        * ignore them.
+        *
+        * For non-dir upper metacopy dentry, we already set "origin" if we
+        * verified that lower matched upper origin. If upper origin was
+        * not present (because lower layer did not support fh encode/decode),
+        * do not set "origin" and skip looking up index. This case should
+        * be handled in same way as a non-dir upper without ORIGIN is
+        * handled.
+        *
+        * Always lookup index of non-dir non-metacopy and non-upper.
         */
-       if (ctr && (!upperdentry || !d.is_dir))
+       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
                origin = stack[0].dentry;
 
        if (origin && ovl_indexdir(dentry->d_sb) &&

@@ -1015,6 +1087,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
        }
 
        revert_creds(old_cred);
+       if (origin_path) {
+               dput(origin_path->dentry);
+               kfree(origin_path);
+       }
        dput(index);
        kfree(stack);
        kfree(d.redirect);

@@ -1029,6 +1105,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                dput(stack[i].dentry);
        kfree(stack);
 out_put_upper:
+       if (origin_path) {
+               dput(origin_path->dentry);
+               kfree(origin_path);
+       }
        dput(upperdentry);
        kfree(upperredirect);
 out:

fs/overlayfs/overlayfs.h +1 -0

index 2daea529b7eb..e8954fff1c45 100644
@@ -274,6 +274,7 @@ bool ovl_need_index(struct dentry *dentry);
 int ovl_nlink_start(struct dentry *dentry, bool *locked);
 void ovl_nlink_end(struct dentry *dentry, bool locked);
 int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
+int ovl_check_metacopy_xattr(struct dentry *dentry);
 
 static inline bool ovl_is_impuredir(struct dentry *dentry)
 {

fs/overlayfs/util.c +22 -1

index f8e3c95711b8..ab9a8fae0f99 100644
@@ -778,3 +778,25 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
        pr_err("overlayfs: failed to lock workdir+upperdir\n");
        return -EIO;
 }
+
+/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
+int ovl_check_metacopy_xattr(struct dentry *dentry)
+{
+       int res;
+
+       /* Only regular files can have metacopy xattr */
+       if (!S_ISREG(d_inode(dentry)->i_mode))
+               return 0;
+
+       res = vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
+       if (res < 0) {
+               if (res == -ENODATA || res == -EOPNOTSUPP)
+                       return 0;
+               goto out;
+       }
+
+       return 1;
+out:
+       pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
+       return res;
+}
--

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

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

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

We don't support metacopy feature with nfs_export. So in nfs_export code, we set OVL_UPPERDATA flag set unconditionally if upper inode exists.

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

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

---
 fs/overlayfs/export.c    |   3 ++
 fs/overlayfs/inode.c     |  11 ++++-
 fs/overlayfs/namei.c     | 108 +++++++++++++++++++++++++++++++++++++++++------
 fs/overlayfs/overlayfs.h |   1 +
 fs/overlayfs/util.c      |  22 ++++++++++
 5 files changed, 130 insertions(+), 15 deletions(-)

fs/overlayfs/export.c +3 -0

index 0549286cc55e..52a09a9f74b7 100644
@@ -314,6 +314,9 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
                return ERR_CAST(inode);
        }
 
+       if (upper)
+               ovl_set_flag(OVL_UPPERDATA, inode);
+
        dentry = d_find_any_alias(inode);
        if (!dentry) {
                dentry = d_alloc_anon(inode->i_sb);

fs/overlayfs/inode.c +10 -1

index c128d5d54d0f..83b276ce0240 100644
@@ -770,7 +770,7 @@ struct inode *ovl_get_inode(struct ovl_inode_params *oip)
        bool bylower = ovl_hash_bylower(oip->sb, upperdentry, lowerdentry,
                                        oip->index);
        int fsid = bylower ? oip->lowerpath->layer->fsid : 0;
-       bool is_dir;
+       bool is_dir, metacopy = false;
        unsigned long ino = 0;
        int err = -ENOMEM;
 

@@ -830,6 +830,15 @@ struct inode *ovl_get_inode(struct ovl_inode_params *oip)
        if (oip->index)
                ovl_set_flag(OVL_INDEX, inode);
 
+       if (upperdentry) {
+               err = ovl_check_metacopy_xattr(upperdentry);
+               if (err < 0)
+                       goto out_err;
+               metacopy = err;
+               if (!metacopy)
+                       ovl_set_flag(OVL_UPPERDATA, inode);
+       }
+
        OVL_I(inode)->redirect = oip->redirect;
 
        /* Check for non-merge dir that may have whiteouts */

fs/overlayfs/namei.c +1 -2

index 8fd817bf5529..b2ff08985e29 100644
@@ -24,6 +24,7 @@ struct ovl_lookup_data {
        bool stop;
        bool last;
        char *redirect;
+       bool metacopy;
 };
 
 static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,

@@ -253,19 +254,29 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
                goto put_and_out;
        }
        if (!d_can_lookup(this)) {
-               d->stop = true;
-               if (d->is_dir)

fs/overlayfs/namei.c +14 -4

+                       goto out_err;
+               }
                d->is_dir = true;
+       }
        if (d->last)
                goto out;
 

@@ -823,7 +834,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
        struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
        struct ovl_entry *poe = dentry->d_parent->d_fsdata;
        struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
-       struct ovl_path *stack = NULL;
+       struct ovl_path *stack = NULL, *origin_path = NULL;
        struct dentry *upperdir, *upperdentry = NULL;
        struct dentry *origin = NULL;
        struct dentry *index = NULL;

@@ -834,6 +845,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,

@@ -841,6 +853,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                .stop = false,
                .last = ofs->config.redirect_follow ? false : !poe->numlower,
                .redirect = NULL,
+               .metacopy = false,
        };
 
        if (dentry->d_name.len > ofs->namelen)

@@ -859,7 +872,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                        goto out;
                }
                if (upperdentry && !d.is_dir) {
-                       BUG_ON(!d.stop || d.redirect);
+                       unsigned int origin_ctr = 0;
+                       BUG_ON(d.redirect);
                        /*
                         * Lookup copy up origin by decoding origin file handle.
                         * We may get a disconnected dentry, which is fine,

@@ -870,9 +884,13 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                         * number - it's the same as if we held a reference
                         * to a dentry in lower layer that was moved under us.
                         */
-                       err = ovl_check_origin(ofs, upperdentry, &stack, &ctr);
+                       err = ovl_check_origin(ofs, upperdentry, &origin_path,
+                                              &origin_ctr);
                        if (err)
                                goto out_put_upper;
+
+                       if (d.metacopy)
+                               metacopy = true;
                }
 
                if (d.redirect) {

@@ -913,7 +931,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                 * If no origin fh is stored in upper of a merge dir, store fh
                 * of lower dir and set upper parent "impure".
                 */
-               if (upperdentry && !ctr && !ofs->noxattr) {
+               if (upperdentry && !ctr && !ofs->noxattr && d.is_dir) {
                        err = ovl_fix_origin(dentry, this, upperdentry);
                        if (err) {
                                dput(this);

fs/overlayfs/namei.c +6 -3

                 */
-               if (upperdentry && !ctr && ovl_verify_lower(dentry->d_sb)) {
+               if (upperdentry && !ctr &&
+                   ((d.is_dir && ovl_verify_lower(dentry->d_sb)) ||
+                    (!d.is_dir && origin_path))) {
                        err = ovl_verify_origin(upperdentry, this, false);
                        if (err) {
                                dput(this);
-                               break;
+                               if (d.is_dir)
+                                       break;
+                               goto out_put;
                        }
-

fs/overlayfs/namei.c +6 -0

+                */
+               if (d.metacopy && ctr) {
+                       dput(this);
+                       continue;
+               }
+
                stack[ctr].dentry = this;
                stack[ctr].layer = lower.layer;
                ctr++;

fs/overlayfs/namei.c +39 -2

+                       goto out_put;
+               }
+
+               err = -EPERM;
+               if (!ofs->config.metacopy) {
+                       pr_warn_ratelimited("overlay: refusing to follow"
+                                           " metacopy origin for (%pd2)\n",
+                                           dentry);
+                       goto out_put;
+               }
+       } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
+               if (WARN_ON(stack != NULL)) {
+                       err = -EIO;
+                       goto out_put;
+               }
+               stack = origin_path;
+               ctr = 1;
+               origin_path = NULL;
+       }
+
        /*
         * Lookup index by lower inode and verify it matches upper inode.
         * We only trust dir index if we verified that lower dir matches
         * origin, otherwise dir index entries may be inconsistent and we
-        * ignore them. Always lookup index of non-dir and non-upper.
+        * ignore them.
+        *
+        * For non-dir upper metacopy dentry, we already set "origin" if we
+        * verified that lower matched upper origin. If upper origin was
+        * not present (because lower layer did not support fh encode/decode),
+        * do not set "origin" and skip looking up index. This case should
+        * be handled in same way as a non-dir upper without ORIGIN is
+        * handled.
+        *
+        * Always lookup index of non-dir non-metacopy and non-upper.
         */
-       if (ctr && (!upperdentry || !d.is_dir))
+       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
                origin = stack[0].dentry;
 
        if (origin && ovl_indexdir(dentry->d_sb) &&

@@ -1015,6 +1087,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
        }
 
        revert_creds(old_cred);
+       if (origin_path) {
+               dput(origin_path->dentry);
+               kfree(origin_path);
+       }
        dput(index);
        kfree(stack);
        kfree(d.redirect);

@@ -1029,6 +1105,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                dput(stack[i].dentry);
        kfree(stack);
 out_put_upper:
+       if (origin_path) {
+               dput(origin_path->dentry);
+               kfree(origin_path);
+       }
        dput(upperdentry);
        kfree(upperredirect);
 out:

fs/overlayfs/overlayfs.h +1 -0

index 2daea529b7eb..e8954fff1c45 100644
@@ -274,6 +274,7 @@ bool ovl_need_index(struct dentry *dentry);
 int ovl_nlink_start(struct dentry *dentry, bool *locked);
 void ovl_nlink_end(struct dentry *dentry, bool locked);
 int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir);
+int ovl_check_metacopy_xattr(struct dentry *dentry);
 
 static inline bool ovl_is_impuredir(struct dentry *dentry)
 {

fs/overlayfs/util.c +22 -1

index f8e3c95711b8..ab9a8fae0f99 100644
@@ -778,3 +778,25 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir)
        pr_err("overlayfs: failed to lock workdir+upperdir\n");
        return -EIO;
 }
+
+/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
+int ovl_check_metacopy_xattr(struct dentry *dentry)
+{
+       int res;
+
+       /* Only regular files can have metacopy xattr */
+       if (!S_ISREG(d_inode(dentry)->i_mode))
+               return 0;
+
+       res = vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
+       if (res < 0) {
+               if (res == -ENODATA || res == -EOPNOTSUPP)
+                       return 0;
+               goto out;
+       }
+
+       return 1;
+out:
+       pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
+       return res;
+}
--

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

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 969

Quoted 6 lines · Vivek Goyal

fs/overlayfs/namei.c +5 -0

+               if (d.metacopy)
+                       metacopy = true;
+               /*
+                * Do not store intermediate metacopy dentries in chain,
+                * except top most lower metacopy dentry

I don't get it. We need the bottom most metacopy dentry, not the topmost. Am I missing something?

We also need to check file type here, only regular file makes sense as metacopy, so if it's something else, then get out with EIO.

Quoted 8 lines · Miklos Szeredi

Quoted 6 lines · Vivek Goyal

fs/overlayfs/namei.c +5 -0

+               if (d.metacopy)
+                       metacopy = true;
+               /*
+                * Do not store intermediate metacopy dentries in chain,
+                * except top most lower metacopy dentry

Quoted 2 lines

I don't get it. We need the bottom most metacopy dentry, not the topmost. Am I missing something?

Okay, it's more complicated.

1) there is an upper metacopy dentry:

- store origin (pointed to by ORIGIN or topmost lower dentry) in stack[0]

Quoted 10 lines · Miklos Szeredi

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 969

Quoted 6 lines · Vivek Goyal

fs/overlayfs/namei.c +5 -0

+               if (d.metacopy)
+                       metacopy = true;
+               /*
+                * Do not store intermediate metacopy dentries in chain,
+                * except top most lower metacopy dentry

I don't get it. We need the bottom most metacopy dentry, not the topmost. Am I missing something?

Okay, it's more complicated.

1) there is an upper metacopy dentry:

- store origin (pointed to by ORIGIN or topmost lower dentry) in stack[0]
- store data dentry (lowest in metacopy chain) in stack[1], unless
it's the same as origin

2) there is no upper dentry, but a lower metacopy dentry

- store metacopy dentry in stack[0]
- store data dentry in stack[1]

Does that make more sense?

Quoted 3 lines

We also need to check file type here, only regular file makes sense as metacopy, so if it's something else, then get out with EIO.

I meant file type of *data* inode. Type of metacopy inode is already checked by ovl_check_metacopy_xattr().

Quoted 2 lines

We also need to check file type here, only regular file makes sense as metacopy, so if it's something else, then get out with EIO.

I meant file type of *data* inode. Type of metacopy inode is already checked by ovl_check_metacopy_xattr().

On Thu, May 10, 2018 at 11:36:16AM +0200, Miklos Szeredi wrote:
[..]

Quoted 5 lines

Quoted 2 lines

We also need to check file type here, only regular file makes sense as metacopy, so if it's something else, then get out with EIO.

I meant file type of *data* inode. Type of metacopy inode is already checked by ovl_check_metacopy_xattr().

Hi Miklos,

IIUC, ovl_lookup_single() will make sure we don't return directory dentry for a metacopy upper (After your suggested changes).

        if (last_element) {
                if (d->metacopy) {
                        d->stop = true;
                        goto put_and_out;
                }
                d->is_dir = true;
        }

If that's the case, then we probably don't need additional check in ovl_lookup().

Quoted 92 lines · Vivek Goyal

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

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

We don't support metacopy feature with nfs_export. So in nfs_export code, we set OVL_UPPERDATA flag set unconditionally if upper inode exists.

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

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

---
 fs/overlayfs/export.c    |   3 ++
 fs/overlayfs/inode.c     |  11 ++++-
 fs/overlayfs/namei.c     | 108 +++++++++++++++++++++++++++++++++++++++++------
 fs/overlayfs/overlayfs.h |   1 +
 fs/overlayfs/util.c      |  22 ++++++++++
 5 files changed, 130 insertions(+), 15 deletions(-)

fs/overlayfs/export.c +3 -0

index 0549286cc55e..52a09a9f74b7 100644
@@ -314,6 +314,9 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
                return ERR_CAST(inode);
        }
 
+       if (upper)
+               ovl_set_flag(OVL_UPPERDATA, inode);
+
        dentry = d_find_any_alias(inode);
        if (!dentry) {
                dentry = d_alloc_anon(inode->i_sb);

fs/overlayfs/inode.c +10 -1

index c128d5d54d0f..83b276ce0240 100644
@@ -770,7 +770,7 @@ struct inode *ovl_get_inode(struct ovl_inode_params *oip)
        bool bylower = ovl_hash_bylower(oip->sb, upperdentry, lowerdentry,
                                        oip->index);
        int fsid = bylower ? oip->lowerpath->layer->fsid : 0;
-       bool is_dir;
+       bool is_dir, metacopy = false;
        unsigned long ino = 0;
        int err = -ENOMEM;
 

@@ -830,6 +830,15 @@ struct inode *ovl_get_inode(struct ovl_inode_params *oip)
        if (oip->index)
                ovl_set_flag(OVL_INDEX, inode);
 
+       if (upperdentry) {
+               err = ovl_check_metacopy_xattr(upperdentry);
+               if (err < 0)
+                       goto out_err;
+               metacopy = err;
+               if (!metacopy)
+                       ovl_set_flag(OVL_UPPERDATA, inode);
+       }
+
        OVL_I(inode)->redirect = oip->redirect;
 
        /* Check for non-merge dir that may have whiteouts */

fs/overlayfs/namei.c +4 -3

index 8fd817bf5529..b2ff08985e29 100644
@@ -24,6 +24,7 @@ struct ovl_lookup_data {
        bool stop;
        bool last;
        char *redirect;
+       bool metacopy;
 };
 
 static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,

@@ -253,19 +254,29 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
                goto put_and_out;
        }
        if (!d_can_lookup(this)) {
-               d->stop = true;
-               if (d->is_dir)
+               if (d->is_dir) {
+                       d->stop = true;
                        goto put_and_out;
-
+               }
                /*
                 * NB: handle failure to lookup non-last element when non-dir
                 * redirects become possible

fs/overlayfs/namei.c static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d, · line 265

Quoted 2 lines

fs/overlayfs/namei.c +0 -0

                 */
                WARN_ON(!last_element);

This warning now triggers if we have a metacopy inode on upper (d->is_dir == false) but lookup on lower layer fails in the middle because a path element is not a directory (contrary to the comment, this is not related to redirects, but to non-dir "merge" objects, which we call metacopy).

I think we should handle this case together with the d->is_dir case
above: stop and get out. Lookup should handle the case where we
failed to find the data dentry.

BTW, I think EIO is more appropriate
for this than ESTALE. The former means (among other things) filesystem image is corrupted. The latter means some inconsistencies
were found when performing an operation. They are similar, but while
EIO is permanent (until the source of the error is corrected) ESTALE can go away (e.g. if redoing the lookup or mounting the filesystem fresh).

fs/overlayfs/namei.c static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d, · line 275

Quoted 12 lines

fs/overlayfs/namei.c +8 -1

+               err = ovl_check_metacopy_xattr(this);
+               if (err < 0)
+                       goto out_err;
+               d->stop = !err;
+               d->metacopy = !!err;
                goto out;
        }
-       if (last_element)
+       if (last_element) {
+               if (d->metacopy) {
+                       err = -ESTALE;

Again, just stop and get out and caller will detect the error.

Quoted 72 lines

fs/overlayfs/namei.c +15 -4

+                       goto out_err;
+               }
                d->is_dir = true;
+       }
        if (d->last)
                goto out;
 

@@ -823,7 +834,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
        struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
        struct ovl_entry *poe = dentry->d_parent->d_fsdata;
        struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
-       struct ovl_path *stack = NULL;
+       struct ovl_path *stack = NULL, *origin_path = NULL;
        struct dentry *upperdir, *upperdentry = NULL;
        struct dentry *origin = NULL;
        struct dentry *index = NULL;

@@ -834,6 +845,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,

@@ -841,6 +853,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                .stop = false,
                .last = ofs->config.redirect_follow ? false : !poe->numlower,
                .redirect = NULL,
+               .metacopy = false,
        };
 
        if (dentry->d_name.len > ofs->namelen)

@@ -859,7 +872,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                        goto out;
                }
                if (upperdentry && !d.is_dir) {
-                       BUG_ON(!d.stop || d.redirect);
+                       unsigned int origin_ctr = 0;
+                       BUG_ON(d.redirect);
                        /*
                         * Lookup copy up origin by decoding origin file handle.
                         * We may get a disconnected dentry, which is fine,

@@ -870,9 +884,13 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                         * number - it's the same as if we held a reference
                         * to a dentry in lower layer that was moved under us.
                         */
-                       err = ovl_check_origin(ofs, upperdentry, &stack, &ctr);
+                       err = ovl_check_origin(ofs, upperdentry, &origin_path,
+                                              &origin_ctr);
                        if (err)
                                goto out_put_upper;
+
+                       if (d.metacopy)
+                               metacopy = true;
                }
 
                if (d.redirect) {

@@ -913,7 +931,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                 * If no origin fh is stored in upper of a merge dir, store fh
                 * of lower dir and set upper parent "impure".
                 */
-               if (upperdentry && !ctr && !ofs->noxattr) {
+               if (upperdentry && !ctr && !ofs->noxattr && d.is_dir) {
                        err = ovl_fix_origin(dentry, this, upperdentry);
                        if (err) {
                                dput(this);

@@ -925,18 +943,36 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                 * When "verify_lower" feature is enabled, do not merge with a
                 * lower dir that does not match a stored origin xattr. In any
                 * case, only verified origin is used for index lookup.
+                *

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 949

Quoted 3 lines

fs/overlayfs/namei.c +3 -0

+                * For non-dir dentry, make sure dentry found by lookup
+                * matches the origin stored in upper. Otherwise its an
+                * error.

Umm, why we need to be so strict? This would break the case where the layers are copied with xattr intact, but the origin pointer will obviously be "wrong", which shouldn't be a problem, since that's only needed to get a unique st_ino, nothing else.

Quoted 20 lines

fs/overlayfs/namei.c +7 -4

                 */
-               if (upperdentry && !ctr && ovl_verify_lower(dentry->d_sb)) {
+               if (upperdentry && !ctr &&
+                   ((d.is_dir && ovl_verify_lower(dentry->d_sb)) ||
+                    (!d.is_dir && origin_path))) {
                        err = ovl_verify_origin(upperdentry, this, false);
                        if (err) {
                                dput(this);
-                               break;
+                               if (d.is_dir)
+                                       break;
+                               goto out_put;
                        }
-
-                       /* Bless lower dir as verified origin */
+                       /* Bless lower as verified origin */
                        origin = this;
                }
 

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 969

Quoted 5 lines

fs/overlayfs/namei.c +5 -0

+               if (d.metacopy)
+                       metacopy = true;
+               /*
+                * Do not store intermediate metacopy dentries in chain,
+                * except top most lower metacopy dentry

I don't get it. We need the bottom most metacopy dentry, not the topmost. Am I missing something?

We also need to check file type here, only regular file makes sense as metacopy, so if it's something else, then get out with EIO.

Quoted 14 lines

fs/overlayfs/namei.c +6 -0

+                */
+               if (d.metacopy && ctr) {
+                       dput(this);
+                       continue;
+               }
+
                stack[ctr].dentry = this;
                stack[ctr].layer = lower.layer;
                ctr++;

@@ -968,13 +1004,49 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                }
        }
 

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 1013

Quoted 7 lines

fs/overlayfs/namei.c +7 -0

+       if (metacopy) {
+               /*
+                * Found a metacopy dentry but did not find corresponding
+                * data dentry
+                */
+               if (d.metacopy) {
+                       err = -ESTALE;

-EIO.

Quoted 17 lines · Miklos Szeredi

Quoted 16 lines · Vivek Goyal

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

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

We don't support metacopy feature with nfs_export. So in nfs_export code, we set OVL_UPPERDATA flag set unconditionally if upper inode exists.

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

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

---

Quoted 5 lines

Quoted 5 lines

fs/overlayfs/namei.c +1 -0

@@ -925,18 +943,36 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                 * When "verify_lower" feature is enabled, do not merge with a
                 * lower dir that does not match a stored origin xattr. In any
                 * case, only verified origin is used for index lookup.
+                *

Quoted 8 lines

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 949

Quoted 3 lines

fs/overlayfs/namei.c +3 -0

+                * For non-dir dentry, make sure dentry found by lookup
+                * matches the origin stored in upper. Otherwise its an
+                * error.

Umm, why we need to be so strict? This would break the case where the layers are copied with xattr intact, but the origin pointer will obviously be "wrong", which shouldn't be a problem, since that's only needed to get a unique st_ino, nothing else.

Hmm...., right this breaks the case of copied up layer. The very reason we moved to using path based lookup for metacopy data dentry.

So if we have a origin on upper for metacopy file which does not match lower dentry found using path based lookup, we can ignore the origin information and don't lookup for index either. That also means that inode will be reported of upper. Given we will not use index, that probably will mean broken hardlinks and some strange corner cases. I will make this change and run the tests on copied layers and see what breaks.

Quoted 12 lines

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 958

Quoted 12 lines

fs/overlayfs/namei.c +5 -2

                 */
-               if (upperdentry && !ctr && ovl_verify_lower(dentry->d_sb)) {
+               if (upperdentry && !ctr &&
+                   ((d.is_dir && ovl_verify_lower(dentry->d_sb)) ||
+                    (!d.is_dir && origin_path))) {
                        err = ovl_verify_origin(upperdentry, this, false);
                        if (err) {
                                dput(this);
-                               break;
+                               if (d.is_dir)
+                                       break;

Quoted 1 line

Review struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 959

Quoted 1 line

(quoted diff) +0 -0

                        }

Quoted 4 lines

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 962

Quoted 4 lines

fs/overlayfs/namei.c +1 -2

-
-                       /* Bless lower dir as verified origin */
+                       /* Bless lower as verified origin */
                        origin = this;

Quoted 1 line

Review struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 962

Quoted 1 line

(quoted diff) +0 -0

                }

Quoted 18 lines · Vivek Goyal

Quoted 17 lines · Miklos Szeredi

Quoted 16 lines · Vivek Goyal

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

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

We don't support metacopy feature with nfs_export. So in nfs_export code, we set OVL_UPPERDATA flag set unconditionally if upper inode exists.

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

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

---

[...]

Quoted 17 lines

Quoted 5 lines

Quoted 5 lines

fs/overlayfs/namei.c +1 -0

@@ -925,18 +943,36 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                 * When "verify_lower" feature is enabled, do not merge with a
                 * lower dir that does not match a stored origin xattr. In any
                 * case, only verified origin is used for index lookup.
+                *

Quoted 8 lines

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 949

Quoted 3 lines

fs/overlayfs/namei.c +3 -0

+                * For non-dir dentry, make sure dentry found by lookup
+                * matches the origin stored in upper. Otherwise its an
+                * error.

Umm, why we need to be so strict? This would break the case where the layers are copied with xattr intact, but the origin pointer will obviously be "wrong", which shouldn't be a problem, since that's only needed to get a unique st_ino, nothing else.

Hmm...., right this breaks the case of copied up layer. The very reason we moved to using path based lookup for metacopy data dentry.

Quoted 8 lines

So if we have a origin on upper for metacopy file which does not match lower dentry found using path based lookup, we can ignore the origin information and don't lookup for index either. That also means that inode will be reported of upper. Given we will not use index, that probably will mean broken hardlinks and some strange corner cases. I will make this change and run the tests on copied layers and see what breaks.

OK, so maybe just relax below to:

Quoted 12 lines

Quoted 12 lines

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 958

Quoted 12 lines

fs/overlayfs/namei.c +5 -2

                 */
-               if (upperdentry && !ctr && ovl_verify_lower(dentry->d_sb)) {
+               if (upperdentry && !ctr &&
+                   ((d.is_dir && ovl_verify_lower(dentry->d_sb)) ||
+                    (!d.is_dir && origin_path))) {
                        err = ovl_verify_origin(upperdentry, this, false);
                        if (err) {
                                dput(this);
-                               break;
+                               if (d.is_dir)
+                                       break;

+ else if (ovl_verify_lower(dentry->d_sb))

Quoted 5 lines

fs/overlayfs/namei.c +1 -0

@@ -925,18 +943,36 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                 * When "verify_lower" feature is enabled, do not merge with a
                 * lower dir that does not match a stored origin xattr. In any
                 * case, only verified origin is used for index lookup.
+                *

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 949

Quoted 3 lines

fs/overlayfs/namei.c +3 -0

+                * For non-dir dentry, make sure dentry found by lookup
+                * matches the origin stored in upper. Otherwise its an
+                * error.

Umm, why we need to be so strict? This would break the case where the layers are copied with xattr intact, but the origin pointer will obviously be "wrong", which shouldn't be a problem, since that's only needed to get a unique st_ino, nothing else.

On Thu, May 10, 2018 at 11:19:23AM +0200, Miklos Szeredi wrote:

[..]

Quoted 5 lines

Quoted 5 lines

fs/overlayfs/namei.c +1 -0

@@ -925,18 +943,36 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                 * When "verify_lower" feature is enabled, do not merge with a
                 * lower dir that does not match a stored origin xattr. In any
                 * case, only verified origin is used for index lookup.
+                *

Quoted 8 lines

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 949

Quoted 3 lines

fs/overlayfs/namei.c +3 -0

+                * For non-dir dentry, make sure dentry found by lookup
+                * matches the origin stored in upper. Otherwise its an
+                * error.

Umm, why we need to be so strict? This would break the case where the layers are copied with xattr intact, but the origin pointer will obviously be "wrong", which shouldn't be a problem, since that's only needed to get a unique st_ino, nothing else.

So I have few questions at this point of time, I am not clear about what should be the behavior.

- Layer copying does not work with index=on. So if index is on, it is fair to assume that layer copying will not be allowed and that configuration is not supported. If yes, then we could enforce this check with index=on and not in other cases.

- If we do not enforce ORIGIN verification for non-dir metacopy and overlay is mounted again after layer copy, then before data copy up we will report inode number of lowerstack[0]. And after data copy up it could be inode number of any of the following.

A. lower inode (if previous overlay dentry has not been flushed, it will
have CONST_INO set.).
B. upper inode (if previous overlay dentry got flushed out, and new
lookup could not decode ORIGIN).

C. older lower inode (if previous overlay dentry got flushed out, and
new lookup decoded previous inode before layer copy up.)

I guess both B and C will happen with existing code even with normal copy up. So this behavior with metacopy files might not be a huge concern.

- if upper has REDIRECT, then use that to determine origin (just like
directories)
- else if upper has METACOPY, then go down one level to determine
origin (just like directories)
- else if upper has ORIGIN, then use that as origin (not applicable
to directories (*))
- else no origin (just like directories)

Quoted 7 lines · Miklos Szeredi

- if upper has REDIRECT, then use that to determine origin (just like
directories)
- else if upper has METACOPY, then go down one level to determine
origin (just like directories)
- else if upper has ORIGIN, then use that as origin (not applicable
to directories (*))
- else no origin (just like directories)

Desirable way to find origin summarized in a table:

lookup by dir reg special
--------------------------------------------

redirect    REDIRECT       REDIRECT  n/a
next layer  default        METACOPY  n/a
origin fh   OPAQUE+ORIGIN  ORIGIN    ORIGIN
stop        OPAQUE         default   default

Quoted 10 lines · Miklos Szeredi

Quoted 7 lines · Miklos Szeredi

- if upper has REDIRECT, then use that to determine origin (just like
directories)
- else if upper has METACOPY, then go down one level to determine
origin (just like directories)
- else if upper has ORIGIN, then use that as origin (not applicable
to directories (*))
- else no origin (just like directories)

Quoted 9 lines

Desirable way to find origin summarized in a table:

lookup by dir reg special
--------------------------------------------

redirect    REDIRECT       REDIRECT  n/a
next layer  default        METACOPY  n/a
origin fh   OPAQUE+ORIGIN  ORIGIN    ORIGIN
stop        OPAQUE         default   default

OK, but how to determine the value of indexed_by and hashed_by when next_layer and origin_fh do not agree?

Between the lines I read that your answer is next_layer overrules origin_fh (as we have for directories).

What I am concerned about is inconsistency of the form that index entry name, does not match the ORIGIN xattr of that index entry.

First of all this will fail index verification on mount and may need to be fixed.

I am also concerned that there are bugs lurking in the form of hardlink inode that can be hashed in two different ways depending on how is is looked up.

I am not saying this is not solvable, just that it is more complex than the table above and I wouldn't want Vivek to have to untangle all this mess with current patch series.

So for compromise, I suggest to follow Vivek's suggestion to enforce next_layer == origin_fh if index=on,metacopy=on.

This does not break the copied layer use case, because with copied layers, lower layer origin_fh verification on mount will fail and won't allow to set index=on.

This is similar to the compromise that we have made with nfs_export so we won't need to deal with ovl_fh generations when index name does not match origin fh.

Quoted 11 lines

I am not saying this is not solvable, just that it is more complex than the table above and I wouldn't want Vivek to have to untangle all this mess with current patch series.

So for compromise, I suggest to follow Vivek's suggestion to enforce next_layer == origin_fh if index=on,metacopy=on.

This does not break the copied layer use case, because with copied layers, lower layer origin_fh verification on mount will fail and won't allow to set index=on.

Quoted 3 lines

This is similar to the compromise that we have made with nfs_export so we won't need to deal with ovl_fh generations when index name does not match origin fh.

Okay, lets do that for now.

Hi Miklos, Amir,

Please find attached V2 of the patch. I have taken care of comments. I have again pushed latest patches on metacopy-next branch.

https://github.com/rhvgoyal/linux/commits/metacopy-next

Changes:
- Use EIO instead of ESTALE at few places.
- Make sure metacopy dentry or data dentry is a regular file.
- Modified ovl_lookup_single() for better error handling of bad redirects (Miklos's feedback).
- In ovl_lookup(), for non-dir, if lower does not match against ORIGIN, then we error out only if index=on. Otherwise we continue to use the dentry found by path based lookup.

Thanks
Vivek

Subject: ovl: Modify ovl_lookup() and friends to lookup metacopy dentry

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

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

We don't support metacopy feature with nfs_export. So in nfs_export code, we set OVL_UPPERDATA flag set unconditionally if upper inode exists.

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

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

Reviewed-by Amir Goldstein <[email protected]>

---
 fs/overlayfs/export.c    |    3 +
 fs/overlayfs/inode.c     |   11 +++-
 fs/overlayfs/namei.c     |  125 ++++++++++++++++++++++++++++++++++++++++-------
 fs/overlayfs/overlayfs.h |    1
 fs/overlayfs/util.c      |   22 ++++++++
 5 files changed, 143 insertions(+), 19 deletions(-)

Index: rhvgoyal-linux/fs/overlayfs/namei.c
===================================================================

fs/overlayfs/namei.c 2018-05-11 09:17:38.751646662 -0400 renamed from fs/overlayfs/namei.c 2018-05-10 15:53:00.858878145 -0400 +44 -14

@@ -24,6 +24,7 @@ struct ovl_lookup_data {
        bool stop;
        bool last;
        char *redirect;
+       bool metacopy;
 };
 
 static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,

@@ -253,19 +254,33 @@ static int ovl_lookup_single(struct dent
                goto put_and_out;
        }
        if (!d_can_lookup(this)) {
-               d->stop = true;
-               if (d->is_dir)
+               if (d->is_dir || !last_element) {
+                       d->stop = true;
                        goto put_and_out;
-
+               }
+               err = ovl_check_metacopy_xattr(this);
+               if (err < 0)
+                       goto out_err;
                /*
-                * NB: handle failure to lookup non-last element when non-dir
-                * redirects become possible
+                * This dentry should be a regular file if this is
+                * a metacopy dentry or previous layer lookup found
+                * a metacopy dentry.
                 */
-               WARN_ON(!last_element);
+               if ((err || d->metacopy) && !d_is_reg(this)) {
+                       d->stop = true;
+                       goto put_and_out;
+               }
+               d->stop = !err;
+               d->metacopy = !!err;
                goto out;
        }
-       if (last_element)
+       if (last_element) {
+               if (d->metacopy) {
+                       d->stop = true;
+                       goto put_and_out;
+               }
                d->is_dir = true;
+       }
        if (d->last)
                goto out;
 

@@ -823,7 +838,7 @@ struct dentry *ovl_lookup(struct inode *
        struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
        struct ovl_entry *poe = dentry->d_parent->d_fsdata;
        struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
-       struct ovl_path *stack = NULL;
+       struct ovl_path *stack = NULL, *origin_path = NULL;
        struct dentry *upperdir, *upperdentry = NULL;
        struct dentry *origin = NULL;
        struct dentry *index = NULL;

@@ -834,6 +849,7 @@ struct dentry *ovl_lookup(struct inode *
        struct dentry *this;
        unsigned int i;
        int err;
+       bool metacopy = false;
        struct ovl_lookup_data d = {
                .name = dentry->d_name,
                .is_dir = false,

@@ -841,6 +857,7 @@ struct dentry *ovl_lookup(struct inode *
                .stop = false,
                .last = ofs->config.redirect_follow ? false : !poe->numlower,
                .redirect = NULL,
+               .metacopy = false,
        };
 
        if (dentry->d_name.len > ofs->namelen)

@@ -859,7 +876,8 @@ struct dentry *ovl_lookup(struct inode *
                        goto out;
                }
                if (upperdentry && !d.is_dir) {
-                       BUG_ON(!d.stop || d.redirect);
+                       unsigned int origin_ctr = 0;
+                       BUG_ON(d.redirect);
                        /*
                         * Lookup copy up origin by decoding origin file handle.
                         * We may get a disconnected dentry, which is fine,

@@ -870,9 +888,13 @@ struct dentry *ovl_lookup(struct inode *
                         * number - it's the same as if we held a reference
                         * to a dentry in lower layer that was moved under us.
                         */
-                       err = ovl_check_origin(ofs, upperdentry, &stack, &ctr);
+                       err = ovl_check_origin(ofs, upperdentry, &origin_path,
+                                              &origin_ctr);
                        if (err)
                                goto out_put_upper;
+
+                       if (d.metacopy)
+                               metacopy = true;
                }
 
                if (d.redirect) {

@@ -913,7 +935,7 @@ struct dentry *ovl_lookup(struct inode *
                 * If no origin fh is stored in upper of a merge dir, store fh
                 * of lower dir and set upper parent "impure".
                 */
-               if (upperdentry && !ctr && !ofs->noxattr) {
+               if (upperdentry && !ctr && !ofs->noxattr && d.is_dir) {
                        err = ovl_fix_origin(dentry, this, upperdentry);
                        if (err) {
                                dput(this);

@@ -925,16 +947,39 @@ struct dentry *ovl_lookup(struct inode *
                 * When "verify_lower" feature is enabled, do not merge with a
                 * lower dir that does not match a stored origin xattr. In any
                 * case, only verified origin is used for index lookup.
+                *
+                * For non-dir dentry, if index=on, then ensure origin
+                * matches the dentry found using path based lookup,
+                * otherwise error out. If index=off, then do not error
+                * out if origin does not match.
                 */
-               if (upperdentry && !ctr && ovl_verify_lower(dentry->d_sb)) {
+               if (upperdentry && !ctr &&
+                   ((d.is_dir && ovl_verify_lower(dentry->d_sb)) ||
+                    (!d.is_dir && origin_path))) {
                        err = ovl_verify_origin(upperdentry, this, false);
                        if (err) {
-                               dput(this);
-                               break;
+                               if (d.is_dir) {
+                                       dput(this);
+                                       break;

Quoted 17 lines

fs/overlayfs/namei.c +10 -3

+                *
+                * For non-dir dentry, if index=on, then ensure origin
+                * matches the dentry found using path based lookup,
+                * otherwise error out. If index=off, then do not error
+                * out if origin does not match.
                 */
-               if (upperdentry && !ctr && ovl_verify_lower(dentry->d_sb)) {
+               if (upperdentry && !ctr &&
+                   ((d.is_dir && ovl_verify_lower(dentry->d_sb)) ||
+                    (!d.is_dir && origin_path))) {
                        err = ovl_verify_origin(upperdentry, this, false);
                        if (err) {
-                               dput(this);
-                               break;
+                               if (d.is_dir) {
+                                       dput(this);

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode * · line 972

Quoted 10 lines

fs/overlayfs/namei.c +9 -0

+                                       break;
+                               } else if (ofs->config.index) {
+                                       dput(this);
+                                       goto out_put;
+                               }
+                       } else {
+                               /* Bless lower as verified origin */
+                               origin = this;
                        }
+               }

Logically, this looks correct, but if we don't need the blessing for index=off and we won't fail lookup, then there is no need to ovl_verify_origin() at all if index=off, so this shrinks a few lines of code and saves the extra check.

Thanks,
Amir.