overlayfs: Delayed copy up of data

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

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

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

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

Reviewed-by Amir Goldstein <[email protected]>

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

---
 Documentation/filesystems/overlayfs.txt | 30 ++++++++++++++++++++++++-
 fs/overlayfs/Kconfig                    | 17 ++++++++++++++
 fs/overlayfs/ovl_entry.h                |  1 +
 fs/overlayfs/super.c                    | 40 ++++++++++++++++++++++++++++++++-
 4 files changed, 86 insertions(+), 2 deletions(-)

Documentation/filesystems/overlayfs.txt +29 -1

index 6ea1e64d1464..b7720e61973c 100644
@@ -249,6 +249,30 @@ rightmost one and going left.  In the above example lower1 will be the
 top, lower2 the middle and lower3 the bottom layer.
 
 
+Metadata only copyup
+--------------------
+
+When metadata only copy up feature is enabled, overlayfs will only copy
+up metadata (as opposed to whole file), when a metadata specific operation
+like chown/chmod is performed. Full file will be copied up later when
+file is opened for WRITE operation.
+
+IOW, this is delayed data copy up operation and data is copied up when
+there is a need to actually modify data.
+
+There are multiple ways to enable/disable this feature. A config option
+CONFIG_OVERLAY_FS_METACOPY can be set/unset to enable/disable this feature
+by default. Or one can enable/disable it at module load time with module
+parameter metacopy=on/off. Lastly, there is also a per mount option
+metacopy=on/off to enable/disable this feature per mount.
+
+Do not use metacopy=on with untrusted upper/lower directories. Otherwise
+it is possible that an attacker can create an handcrafted file with
+appropriate REDIRECT and METACOPY xattrs, and gain access to file on lower
+pointed by REDIRECT. This should not be possible on local system as setting
+"trusted." xattrs will require CAP_SYS_ADMIN. But it should be possible
+for untrusted layers like from a pen drive.
+
 Sharing and copying layers
 --------------------------
 

@@ -267,7 +291,7 @@ though it will not result in a crash or deadlock.
 Mounting an overlay using an upper layer path, where the upper layer path
 was previously used by another mounted overlay in combination with a
 different lower layer path, is allowed, unless the "inodes index" feature
-is enabled.
+or "metadata only copyup" feature is enabled.
 
 With the "inodes index" feature, on the first time mount, an NFS file
 handle of the lower layer root directory, along with the UUID of the lower

@@ -280,6 +304,10 @@ lower root origin, mount will fail with ESTALE.  An overlayfs mount with
 does not support NFS export, lower filesystem does not have a valid UUID or
 if the upper filesystem does not support extended attributes.
 
+For "metadata only copyup" feature there is no verification mechanism at
+mount time. So if same upper is mouted with different set of lower, mount
+probably will succeed but expect the unexpected later on. So don't do it.
+
 It is quite a common practice to copy overlay layers to a different
 directory tree on the same or different underlying filesystem, and even
 to a different machine.  With the "inodes index" feature, trying to mount

fs/overlayfs/Kconfig +17 -0

index 406e72de88f6..e1974db486a9 100644
@@ -72,3 +72,20 @@ config OVERLAY_FS_NFS_EXPORT
          Note, that the NFS export feature is not backward compatible.
          That is, mounting an overlay which has a full index on a kernel
          that doesn't support this feature will have unexpected results.
+
+config OVERLAY_FS_METACOPY
+       bool "Overlayfs: turn on metadata only copy up feature by default"
+       depends on OVERLAY_FS
+       depends on !OVERLAY_FS_NFS_EXPORT
+       select OVERLAY_FS_REDIRECT_DIR
+       help
+         If this config option is enabled then overlay filesystems will
+         copy up only metadata where appropriate and data copy up will
+         happen when a file is opended for WRITE operation. It is still
+         possible to turn off this feature globally with the "metacopy=off"
+         module option or on a filesystem instance basis with the
+         "metacopy=off" mount option.
+
+         Note, that this feature is not backward compatible.  That is,
+         mounting an overlay which has metacopy only inodes on a kernel
+         that doesn't support this feature will have unexpected results.

fs/overlayfs/ovl_entry.h +1 -0

index bfef6edcc111..7dc55628080d 100644
@@ -18,6 +18,7 @@ struct ovl_config {
        const char *redirect_mode;
        bool index;
        bool nfs_export;
+       bool metacopy;
 };
 
 struct ovl_layer {

fs/overlayfs/super.c +34 -1

index 9ee37c76091d..0e124b9d902d 100644
@@ -58,6 +58,11 @@ static void ovl_entry_stack_free(struct ovl_entry *oe)
                dput(oe->lowerstack[i].dentry);
 }
 
+static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
+module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
+MODULE_PARM_DESC(ovl_metacopy_def,
+                "Default to on or off for the metadata only copy up feature");
+
 static void ovl_dentry_release(struct dentry *dentry)
 {
        struct ovl_entry *oe = dentry->d_fsdata;

@@ -350,6 +355,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
        if (ofs->config.nfs_export != ovl_nfs_export_def)
                seq_printf(m, ",nfs_export=%s", ofs->config.nfs_export ?
                                                "on" : "off");
+       if (ofs->config.metacopy != ovl_metacopy_def)
+               seq_printf(m, ",metacopy=%s",
+                          ofs->config.metacopy ? "on" : "off");
        return 0;
 }
 

@@ -384,6 +392,8 @@ enum {
        OPT_INDEX_OFF,
        OPT_NFS_EXPORT_ON,
        OPT_NFS_EXPORT_OFF,
+       OPT_METACOPY_ON,
+       OPT_METACOPY_OFF,
        OPT_ERR,
 };
 

@@ -397,6 +407,8 @@ static const match_table_t ovl_tokens = {
        {OPT_INDEX_OFF,                 "index=off"},
        {OPT_NFS_EXPORT_ON,             "nfs_export=on"},
        {OPT_NFS_EXPORT_OFF,            "nfs_export=off"},
+       {OPT_METACOPY_ON,               "metacopy=on"},
+       {OPT_METACOPY_OFF,              "metacopy=off"},
        {OPT_ERR,                       NULL}
 };
 

@@ -511,6 +523,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
                        config->nfs_export = false;
                        break;
 
+               case OPT_METACOPY_ON:
+                       config->metacopy = true;
+                       break;
+
+               case OPT_METACOPY_OFF:
+                       config->metacopy = false;
+                       break;
+
                default:
                        pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
                        return -EINVAL;

@@ -993,7 +1013,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
        if (err) {
                ofs->noxattr = true;
                ofs->config.index = false;
-               pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off.\n");
+               ofs->config.metacopy = false;
+               pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off and metacopy=off.\n");
                err = 0;
        } else {
                vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);

@@ -1012,6 +1033,11 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
                ofs->config.nfs_export = false;
        }
 
+       /* metacopy feature with upper requires redirect_dir=on */
+       if (ofs->config.metacopy && !ofs->config.redirect_dir) {
+               pr_warn("overlayfs: metadata only copyup requires \"redirect_dir=on\", falling back to metacopy=off.\n");
+               ofs->config.metacopy = false;
+       }
 out:
        mnt_drop_write(mnt);
        return err;

@@ -1188,6 +1214,12 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
                ofs->config.nfs_export = false;
        }
 
+       if (!ofs->config.upperdir && ofs->config.metacopy &&
+           !ofs->config.redirect_follow) {
+               ofs->config.metacopy = false;
+               pr_warn("overlayfs: metadata only copyup requires \"redirect_dir=follow\" on non-upper mount, falling back to metacopy=off.\n");
+       }
+
        err = -ENOMEM;
        stack = kcalloc(stacklen, sizeof(struct path), GFP_KERNEL);
        if (!stack)

@@ -1263,6 +1295,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
        ofs->config.index = ovl_index_def;
        ofs->config.nfs_export = ovl_nfs_export_def;
+       ofs->config.metacopy = ovl_metacopy_def;
        err = ovl_parse_opt((char *) data, &ofs->config);
        if (err)
                goto out_err;

Quoted 219 lines · Vivek Goyal

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

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

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

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

Reviewed-by Amir Goldstein <[email protected]>

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

---
 Documentation/filesystems/overlayfs.txt | 30 ++++++++++++++++++++++++-
 fs/overlayfs/Kconfig                    | 17 ++++++++++++++
 fs/overlayfs/ovl_entry.h                |  1 +
 fs/overlayfs/super.c                    | 40 ++++++++++++++++++++++++++++++++-
 4 files changed, 86 insertions(+), 2 deletions(-)

Documentation/filesystems/overlayfs.txt +29 -1

index 6ea1e64d1464..b7720e61973c 100644
@@ -249,6 +249,30 @@ rightmost one and going left.  In the above example lower1 will be the
 top, lower2 the middle and lower3 the bottom layer.
 
 
+Metadata only copyup
+--------------------
+
+When metadata only copy up feature is enabled, overlayfs will only copy
+up metadata (as opposed to whole file), when a metadata specific operation
+like chown/chmod is performed. Full file will be copied up later when
+file is opened for WRITE operation.
+
+IOW, this is delayed data copy up operation and data is copied up when
+there is a need to actually modify data.
+
+There are multiple ways to enable/disable this feature. A config option
+CONFIG_OVERLAY_FS_METACOPY can be set/unset to enable/disable this feature
+by default. Or one can enable/disable it at module load time with module
+parameter metacopy=on/off. Lastly, there is also a per mount option
+metacopy=on/off to enable/disable this feature per mount.
+
+Do not use metacopy=on with untrusted upper/lower directories. Otherwise
+it is possible that an attacker can create an handcrafted file with
+appropriate REDIRECT and METACOPY xattrs, and gain access to file on lower
+pointed by REDIRECT. This should not be possible on local system as setting
+"trusted." xattrs will require CAP_SYS_ADMIN. But it should be possible
+for untrusted layers like from a pen drive.
+
 Sharing and copying layers
 --------------------------
 

@@ -267,7 +291,7 @@ though it will not result in a crash or deadlock.
 Mounting an overlay using an upper layer path, where the upper layer path
 was previously used by another mounted overlay in combination with a
 different lower layer path, is allowed, unless the "inodes index" feature
-is enabled.
+or "metadata only copyup" feature is enabled.
 
 With the "inodes index" feature, on the first time mount, an NFS file
 handle of the lower layer root directory, along with the UUID of the lower

@@ -280,6 +304,10 @@ lower root origin, mount will fail with ESTALE.  An overlayfs mount with
 does not support NFS export, lower filesystem does not have a valid UUID or
 if the upper filesystem does not support extended attributes.
 
+For "metadata only copyup" feature there is no verification mechanism at
+mount time. So if same upper is mouted with different set of lower, mount
+probably will succeed but expect the unexpected later on. So don't do it.
+
 It is quite a common practice to copy overlay layers to a different
 directory tree on the same or different underlying filesystem, and even
 to a different machine.  With the "inodes index" feature, trying to mount

fs/overlayfs/Kconfig +17 -0

index 406e72de88f6..e1974db486a9 100644
@@ -72,3 +72,20 @@ config OVERLAY_FS_NFS_EXPORT
          Note, that the NFS export feature is not backward compatible.
          That is, mounting an overlay which has a full index on a kernel
          that doesn't support this feature will have unexpected results.
+
+config OVERLAY_FS_METACOPY
+       bool "Overlayfs: turn on metadata only copy up feature by default"
+       depends on OVERLAY_FS
+       depends on !OVERLAY_FS_NFS_EXPORT
+       select OVERLAY_FS_REDIRECT_DIR
+       help
+         If this config option is enabled then overlay filesystems will
+         copy up only metadata where appropriate and data copy up will
+         happen when a file is opended for WRITE operation. It is still
+         possible to turn off this feature globally with the "metacopy=off"
+         module option or on a filesystem instance basis with the
+         "metacopy=off" mount option.
+
+         Note, that this feature is not backward compatible.  That is,
+         mounting an overlay which has metacopy only inodes on a kernel
+         that doesn't support this feature will have unexpected results.

fs/overlayfs/ovl_entry.h +1 -0

index bfef6edcc111..7dc55628080d 100644
@@ -18,6 +18,7 @@ struct ovl_config {
        const char *redirect_mode;
        bool index;
        bool nfs_export;
+       bool metacopy;
 };
 
 struct ovl_layer {

fs/overlayfs/super.c +34 -1

index 9ee37c76091d..0e124b9d902d 100644
@@ -58,6 +58,11 @@ static void ovl_entry_stack_free(struct ovl_entry *oe)
                dput(oe->lowerstack[i].dentry);
 }
 
+static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
+module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
+MODULE_PARM_DESC(ovl_metacopy_def,
+                "Default to on or off for the metadata only copy up feature");
+
 static void ovl_dentry_release(struct dentry *dentry)
 {
        struct ovl_entry *oe = dentry->d_fsdata;

@@ -350,6 +355,9 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
        if (ofs->config.nfs_export != ovl_nfs_export_def)
                seq_printf(m, ",nfs_export=%s", ofs->config.nfs_export ?
                                                "on" : "off");
+       if (ofs->config.metacopy != ovl_metacopy_def)
+               seq_printf(m, ",metacopy=%s",
+                          ofs->config.metacopy ? "on" : "off");
        return 0;
 }
 

@@ -384,6 +392,8 @@ enum {
        OPT_INDEX_OFF,
        OPT_NFS_EXPORT_ON,
        OPT_NFS_EXPORT_OFF,
+       OPT_METACOPY_ON,
+       OPT_METACOPY_OFF,
        OPT_ERR,
 };
 

@@ -397,6 +407,8 @@ static const match_table_t ovl_tokens = {
        {OPT_INDEX_OFF,                 "index=off"},
        {OPT_NFS_EXPORT_ON,             "nfs_export=on"},
        {OPT_NFS_EXPORT_OFF,            "nfs_export=off"},
+       {OPT_METACOPY_ON,               "metacopy=on"},
+       {OPT_METACOPY_OFF,              "metacopy=off"},
        {OPT_ERR,                       NULL}
 };
 

@@ -511,6 +523,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
                        config->nfs_export = false;
                        break;
 
+               case OPT_METACOPY_ON:
+                       config->metacopy = true;
+                       break;
+
+               case OPT_METACOPY_OFF:
+                       config->metacopy = false;
+                       break;
+
                default:
                        pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
                        return -EINVAL;

@@ -993,7 +1013,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
        if (err) {
                ofs->noxattr = true;
                ofs->config.index = false;
-               pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off.\n");
+               ofs->config.metacopy = false;
+               pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off and metacopy=off.\n");
                err = 0;
        } else {
                vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);

@@ -1012,6 +1033,11 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
                ofs->config.nfs_export = false;
        }
 
+       /* metacopy feature with upper requires redirect_dir=on */
+       if (ofs->config.metacopy && !ofs->config.redirect_dir) {
+               pr_warn("overlayfs: metadata only copyup requires \"redirect_dir=on\", falling back to metacopy=off.\n");
+               ofs->config.metacopy = false;
+       }
 out:
        mnt_drop_write(mnt);
        return err;

@@ -1188,6 +1214,12 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
                ofs->config.nfs_export = false;
        }
 
+       if (!ofs->config.upperdir && ofs->config.metacopy &&
+           !ofs->config.redirect_follow) {
+               ofs->config.metacopy = false;
+               pr_warn("overlayfs: metadata only copyup requires \"redirect_dir=follow\" on non-upper mount, falling back to metacopy=off.\n");
+       }
+
        err = -ENOMEM;
        stack = kcalloc(stacklen, sizeof(struct path), GFP_KERNEL);
        if (!stack)

@@ -1263,6 +1295,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 
        ofs->config.index = ovl_index_def;
        ofs->config.nfs_export = ovl_nfs_export_def;
+       ofs->config.metacopy = ovl_metacopy_def;
        err = ovl_parse_opt((char *) data, &ofs->config);
        if (err)
                goto out_err;

fs/overlayfs/super.c static int ovl_fill_super(struct super_block *sb, void *data, int silent) · line 1371

Quoted 8 lines

fs/overlayfs/super.c +5 -0

@@ -1331,6 +1364,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
                }
        }
 
+       if (ofs->config.metacopy && ofs->config.nfs_export) {
+               pr_warn("overlayfs: Metadata copy up requires NFS export disabled, falling back to metacopy=off.\n");
+               ofs->config.metacopy = false;
+       }
+

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

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

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

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

OVL_UPPERDATA flag is set unconditionally if upper inode exists.

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

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

---
 fs/overlayfs/export.c |   3 ++
 fs/overlayfs/namei.c  | 121 ++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 115 insertions(+), 9 deletions(-)

fs/overlayfs/export.c +0 -0

index bb94ce9da5c8..35f2d4eb0d7e 100644

fs/overlayfs/export.c +0 -0

        dentry = d_find_any_alias(inode);
        if (!dentry) {
                dentry = d_alloc_anon(inode->i_sb);

fs/overlayfs/namei.c +23 -0

index 70fcfcc684cc..220e754c974b 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,

@@ -208,6 +209,28 @@ struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt)
        return real;
 }
 
+/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
+static int ovl_check_metacopy_xattr(struct dentry *dentry)
+{
+       int res;
+
+       /* Only regular files can have metacopy xattr */
+       if (!S_ISREG(d_inode(dentry)->i_mode))
+               return 0;
+
+       res = vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
+       if (res < 0) {
+               if (res == -ENODATA || res == -EOPNOTSUPP)
+                       return 0;
+               goto out;
+       }
+
+       return 1;
+out:
+       pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
+       return res;
+}
+
 static bool ovl_is_opaquedir(struct dentry *dentry)
 {
        return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);

fs/overlayfs/namei.c +17 -7

                goto out;
        }
        d->is_dir = true;

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

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

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

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

@@ -846,16 +879,20 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                         * number - it's the same as if we held a reference
                         * to a dentry in lower layer that was moved under us.
                         */
-                       err = ovl_check_origin(ofs, upperdentry, &stack, &ctr);
+                       err = ovl_check_origin(ofs, upperdentry, &origin_path,
+                                              &origin_ctr);
                        if (err)
                                goto out_put_upper;
+
+                       if (d.metacopy)
+                               metacopy = true;
                }
 
                if (d.redirect) {
                        err = -ENOMEM;
                        upperredirect = kstrdup(d.redirect, GFP_KERNEL);
                        if (!upperredirect)
-                               goto out_put_upper;
+                               goto out_put_origin;
                        if (d.redirect[0] == '/')
                                poe = roe;
                }

@@ -867,7 +904,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                stack = kcalloc(ofs->numlower, sizeof(struct ovl_path),
                                GFP_KERNEL);
                if (!stack)
-                       goto out_put_upper;
+                       goto out_put_origin;
        }
 
        for (i = 0; !d.stop && i < poe->numlower; i++) {

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

@@ -898,7 +935,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                 * lower dir that does not match a stored origin xattr. In any
                 * case, only verified origin is used for index lookup.
                 */
-               if (upperdentry && !ctr && ovl_verify_lower(dentry->d_sb)) {
+               if (upperdentry && !ctr && d.is_dir &&
+                   ovl_verify_lower(dentry->d_sb)) {
                        err = ovl_verify_origin(upperdentry, this, false);
                        if (err) {
                                dput(this);

@@ -909,6 +947,29 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                        origin = this;
                }
 
+               /*
+                * For non-dir dentry, make sure dentry found by lookup

fs/overlayfs/namei.c +11 -0

+               if (d.metacopy)
+                       metacopy = true;
+               /*
+                * Do not store intermediate metacopy dentries in chain,
+                * except top most lower metacopy dentry
+                */
+               if (d.metacopy && ctr) {
+                       dput(this);
+                       continue;
+               }
+
                stack[ctr].dentry = this;
                stack[ctr].layer = lower.layer;
                ctr++;

@@ -940,6 +1001,34 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                }

fs/overlayfs/namei.c +8 -0

+                       goto out_put;
+               }
+
+               err = -EPERM;
+               if (!ofs->config.metacopy) {
+                       pr_warn_ratelimited("overlay: refusing to follow"
+                                           " metacopy origin for (%pd2)\n",
+                                           dentry);

fs/overlayfs/namei.c +2 -0

+       }
+
        /*
         * Lookup index by lower inode and verify it matches upper inode.
         * We only trust dir index if we verified that lower dir matches

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

OVL_UPPERDATA flag is set unconditionally if upper inode exists.

You mean if *only* upper inode exists?

Quoted 18 lines

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/namei.c  | 121 ++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 115 insertions(+), 9 deletions(-)

fs/overlayfs/export.c +0 -0

index bb94ce9da5c8..35f2d4eb0d7e 100644

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

Quoted 6 lines

fs/overlayfs/export.c +3 -0

@@ -192,6 +192,9 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
        if (index)
                ovl_set_flag(OVL_INDEX, inode);
 
+       if (upper)
+               ovl_set_flag(OVL_UPPERDATA, inode);
+

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

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

Quoted 45 lines

fs/overlayfs/export.c +0 -0

        dentry = d_find_any_alias(inode);
        if (!dentry) {
                dentry = d_alloc_anon(inode->i_sb);

fs/overlayfs/namei.c +23 -0

index 70fcfcc684cc..220e754c974b 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,

@@ -208,6 +209,28 @@ struct dentry *ovl_decode_fh(struct ovl_fh *fh, struct vfsmount *mnt)
        return real;
 }
 
+/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */
+static int ovl_check_metacopy_xattr(struct dentry *dentry)
+{
+       int res;
+
+       /* Only regular files can have metacopy xattr */
+       if (!S_ISREG(d_inode(dentry)->i_mode))
+               return 0;
+
+       res = vfs_getxattr(dentry, OVL_XATTR_METACOPY, NULL, 0);
+       if (res < 0) {
+               if (res == -ENODATA || res == -EOPNOTSUPP)
+                       return 0;
+               goto out;
+       }
+
+       return 1;
+out:
+       pr_warn_ratelimited("overlayfs: failed to get metacopy (%i)\n", res);
+       return res;
+}
+
 static bool ovl_is_opaquedir(struct dentry *dentry)
 {
        return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);

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

Quoted 6 lines

fs/overlayfs/namei.c +0 -1

@@ -242,9 +265,16 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
                goto put_and_out;
        }
        if (!d_can_lookup(this)) {
-               d->stop = true;
                if (d->is_dir)

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

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

Quoted 10 lines

fs/overlayfs/namei.c +8 -0

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

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

Quoted 95 lines

fs/overlayfs/namei.c +15 -7

                goto out;
        }
        d->is_dir = true;

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

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

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

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

@@ -846,16 +879,20 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                         * number - it's the same as if we held a reference
                         * to a dentry in lower layer that was moved under us.
                         */
-                       err = ovl_check_origin(ofs, upperdentry, &stack, &ctr);
+                       err = ovl_check_origin(ofs, upperdentry, &origin_path,
+                                              &origin_ctr);
                        if (err)
                                goto out_put_upper;
+
+                       if (d.metacopy)
+                               metacopy = true;
                }
 
                if (d.redirect) {
                        err = -ENOMEM;
                        upperredirect = kstrdup(d.redirect, GFP_KERNEL);
                        if (!upperredirect)
-                               goto out_put_upper;
+                               goto out_put_origin;
                        if (d.redirect[0] == '/')
                                poe = roe;
                }

@@ -867,7 +904,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                stack = kcalloc(ofs->numlower, sizeof(struct ovl_path),
                                GFP_KERNEL);
                if (!stack)
-                       goto out_put_upper;
+                       goto out_put_origin;
        }
 
        for (i = 0; !d.stop && i < poe->numlower; i++) {

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

@@ -898,7 +935,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                 * lower dir that does not match a stored origin xattr. In any
                 * case, only verified origin is used for index lookup.
                 */
-               if (upperdentry && !ctr && ovl_verify_lower(dentry->d_sb)) {
+               if (upperdentry && !ctr && d.is_dir &&
+                   ovl_verify_lower(dentry->d_sb)) {
                        err = ovl_verify_origin(upperdentry, this, false);
                        if (err) {
                                dput(this);

@@ -909,6 +947,29 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                        origin = this;
                }
 

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

Quoted 12 lines

fs/overlayfs/namei.c +12 -0

+               /*
+                * For non-dir dentry, make sure dentry found by lookup
+                * matches the origin stored in upper
+                */
+               if (!d.is_dir && upperdentry && !ctr && origin_path) {
+                       err = ovl_verify_origin(upperdentry, this, false);
+                       if (err) {
+                               dput(this);
+                               goto out_put;
+                       }
+               }
+

Why is this code duplicated from d.is_dir case?

Quoted 20 lines

fs/overlayfs/namei.c +11 -0

+               if (d.metacopy)
+                       metacopy = true;
+               /*
+                * Do not store intermediate metacopy dentries in chain,
+                * except top most lower metacopy dentry
+                */
+               if (d.metacopy && ctr) {
+                       dput(this);
+                       continue;
+               }
+
                stack[ctr].dentry = this;
                stack[ctr].layer = lower.layer;
                ctr++;

@@ -940,6 +1001,34 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                }
        }
 

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

Quoted 8 lines

fs/overlayfs/namei.c +8 -0

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

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

Quoted 9 lines

fs/overlayfs/namei.c +8 -0

+                       goto out_put;
+               }
+
+               err = -EPERM;
+               if (!ofs->config.metacopy) {
+                       pr_warn_ratelimited("overlay: refusing to follow"
+                                           " metacopy origin for (%pd2)\n",
+                                           dentry);

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

Quoted 10 lines

fs/overlayfs/namei.c +10 -0

+                       goto out_put;
+               }
+       } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
+               if (WARN_ON(stack != NULL)) {
+                       err = -EIO;
+                       goto out_put;
+               }
+               stack = origin_path;
+               ctr = 1;
+               origin_path = NULL;

I am having a hard time understanding why origin_path is needed at all and why not lookup only by path and then ovl_verify_origin().

The only case I can think of where following by origin is required if for decoding a non-indexed upper file handle which is also a metacopy, but that has nothing to do with ovl_lookup().

Please enlighten me.

Quoted 7 lines

fs/overlayfs/namei.c +2 -0

+       }
+
        /*
         * Lookup index by lower inode and verify it matches upper inode.
         * We only trust dir index if we verified that lower dir matches

@@ -972,8 +1061,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 

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

Quoted 7 lines

fs/overlayfs/namei.c +3 -1

        if (upperdentry)
                ovl_dentry_set_upper_alias(dentry);
-       else if (index)
+       else if (index) {
                upperdentry = dget(index);
+               metacopy = ovl_check_metacopy_xattr(index);
+       }

{} should be on both if and else statements.

On Wed, Mar 07, 2018 at 04:42:54PM +0200, Amir Goldstein wrote:

Quoted 11 lines

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

Quoted 9 lines

static bool ovl_is_opaquedir(struct dentry *dentry)

 {
        return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);

fs/overlayfs/namei.c +0 -1

@@ -242,9 +265,16 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
                goto put_and_out;
        }
        if (!d_can_lookup(this)) {
-               d->stop = true;
                if (d->is_dir)

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

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

Quoted 11 lines

Quoted 10 lines

fs/overlayfs/namei.c +8 -0

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

Quoted 3 lines

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

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

Quoted 13 lines

Quoted 12 lines

fs/overlayfs/namei.c +12 -0

+               /*
+                * For non-dir dentry, make sure dentry found by lookup
+                * matches the origin stored in upper
+                */
+               if (!d.is_dir && upperdentry && !ctr && origin_path) {
+                       err = ovl_verify_origin(upperdentry, this, false);
+                       if (err) {
+                               dput(this);
+                               goto out_put;
+                       }
+               }
+

Quoted 1 line

Why is this code duplicated from d.is_dir case?

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

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

Quoted 9 lines

Quoted 8 lines

fs/overlayfs/namei.c +8 -0

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

Quoted 2 lines

I am having a hard time understanding why origin_path is needed at all and why not lookup only by path and then ovl_verify_origin().

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

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

Quoted 1 line · Vivek Goyal

On Wed, Mar 07, 2018 at 04:42:54PM +0200, Amir Goldstein wrote:

[...]

Quoted 12 lines

Quoted 11 lines

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

Quoted 9 lines

static bool ovl_is_opaquedir(struct dentry *dentry)

 {
        return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);

fs/overlayfs/namei.c +0 -1

@@ -242,9 +265,16 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
                goto put_and_out;
        }
        if (!d_can_lookup(this)) {
-               d->stop = true;
                if (d->is_dir)

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

Quoted 5 lines

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

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

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

fs/overlayfs/namei.c +8 -0

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

Quoted 5 lines

Quoted 3 lines

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

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

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

[...]

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

fs/overlayfs/namei.c +12 -0

+               /*
+                * For non-dir dentry, make sure dentry found by lookup
+                * matches the origin stored in upper
+                */
+               if (!d.is_dir && upperdentry && !ctr && origin_path) {
+                       err = ovl_verify_origin(upperdentry, this, false);
+                       if (err) {
+                               dput(this);
+                               goto out_put;
+                       }
+               }
+

Quoted 5 lines

Quoted 1 line

Why is this code duplicated from d.is_dir case?

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

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

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

Quoted 7 lines

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

[...]

Quoted 12 lines

Quoted 11 lines

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

Quoted 8 lines

fs/overlayfs/namei.c +8 -0

+       } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
+               if (WARN_ON(stack != NULL)) {
+                       err = -EIO;
+                       goto out_put;
+               }
+               stack = origin_path;
+               ctr = 1;
+               origin_path = NULL;

I am having a hard time understanding why origin_path is needed at all and why not lookup only by path and then ovl_verify_origin().

Quoted 4 lines

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

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

Quoted 7 lines

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

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

Quoted 22 lines · Amir Goldstein

Quoted 1 line · Vivek Goyal

On Wed, Mar 07, 2018 at 04:42:54PM +0200, Amir Goldstein wrote:

[...]

Quoted 12 lines

Quoted 11 lines

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

Quoted 9 lines

static bool ovl_is_opaquedir(struct dentry *dentry)

 {
        return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);

fs/overlayfs/namei.c +0 -1

@@ -242,9 +265,16 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
                goto put_and_out;
        }
        if (!d_can_lookup(this)) {
-               d->stop = true;
                if (d->is_dir)

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

Quoted 5 lines

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

Quoted 4 lines

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

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

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

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

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

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

---
 fs/overlayfs/util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/overlayfs/util.c +1 -2

index 7929cc872df6..19b936b0a551 100644
@@ -122,7 +122,7 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
                                type |= __OVL_PATH_MERGE;
                }
        } else {
-               if (oe->numlower > 1)
+               if (oe->numlower > 1 && d_is_dir(dentry))
                        type |= __OVL_PATH_MERGE;
        }
        return type;
--

2.13.6

Quoted 4 lines · Vivek Goyal

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

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

Quoted 22 lines

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

---
 fs/overlayfs/util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/overlayfs/util.c +1 -2

index 7929cc872df6..19b936b0a551 100644
@@ -122,7 +122,7 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
                                type |= __OVL_PATH_MERGE;
                }
        } else {
-               if (oe->numlower > 1)
+               if (oe->numlower > 1 && d_is_dir(dentry))
                        type |= __OVL_PATH_MERGE;
        }
        return type;
--

2.13.6

Quoted 8 lines · Amir Goldstein

Quoted 4 lines · Vivek Goyal

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

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

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

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

Quoted 10 lines · Vivek Goyal

Quoted 8 lines · Amir Goldstein

Quoted 4 lines · Vivek Goyal

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

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

Quoted 9 lines

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

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

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

In fact, in the case of ovl_rename(), for directory, the test is
if (ovl_type_merge_or_lower(old))

      err = ovl_set_redirect(old, samedir);

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

Quoted 24 lines · Amir Goldstein

Quoted 10 lines · Vivek Goyal

Quoted 8 lines · Amir Goldstein

Quoted 4 lines · Vivek Goyal

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

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

Quoted 9 lines

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

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

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

Quoted 8 lines

In fact, in the case of ovl_rename(), for directory, the test is
if (ovl_type_merge_or_lower(old))

      err = ovl_set_redirect(old, samedir);

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

Hi Amir,

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

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

What am I missing?

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

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

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

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

---
 fs/overlayfs/copy_up.c   | 14 ++++++++++----
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 14 ++++++++++++++
 3 files changed, 25 insertions(+), 4 deletions(-)

fs/overlayfs/copy_up.c +10 -4

index 9801ae7baa5d..0c8d2755bd25 100644
@@ -585,13 +585,15 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
        }
 
        if (S_ISREG(c->stat.mode) && !c->metacopy) {
-               struct path upperpath;
+               struct path upperpath, datapath;
 
                ovl_path_upper(c->dentry, &upperpath);
                BUG_ON(upperpath.dentry != NULL);
                upperpath.dentry = temp;
 
-               err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
+               ovl_path_lowerdata(c->dentry, &datapath);
+               BUG_ON(datapath.dentry == NULL);
+               err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
                if (err)
                        return err;
        }

@@ -754,14 +756,18 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 /* Copy up data of an inode which was copied up metadata only in the past. */
 static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 {
-       struct path upperpath;
+       struct path upperpath, datapath;
        int err;
 
        ovl_path_upper(c->dentry, &upperpath);
        if (WARN_ON(upperpath.dentry == NULL))
                return -EIO;
 
-       err = ovl_copy_up_data(&c->lowerpath, &upperpath, c->stat.size);
+       ovl_path_lowerdata(c->dentry, &datapath);
+       if (WARN_ON(datapath.dentry == NULL))
+               return -EIO;
+
+       err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
        if (err)
                return err;
 

fs/overlayfs/overlayfs.h +1 -0

index b48bd73786f1..d999813f9aeb 100644
@@ -221,6 +221,7 @@ bool ovl_dentry_weird(struct dentry *dentry);
 enum ovl_path_type ovl_path_type(struct dentry *dentry);
 void ovl_path_upper(struct dentry *dentry, struct path *path);
 void ovl_path_lower(struct dentry *dentry, struct path *path);
+void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
 struct dentry *ovl_dentry_lower(struct dentry *dentry);

fs/overlayfs/util.c +0 -0

index 19b936b0a551..22df74cb1def 100644

fs/overlayfs/util.c +14 -1

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

2.13.6

Quoted 14 lines

fs/overlayfs/overlayfs.h +1 -0

index b48bd73786f1..d999813f9aeb 100644
@@ -221,6 +221,7 @@ bool ovl_dentry_weird(struct dentry *dentry);
 enum ovl_path_type ovl_path_type(struct dentry *dentry);
 void ovl_path_upper(struct dentry *dentry, struct path *path);
 void ovl_path_lower(struct dentry *dentry, struct path *path);
+void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
 struct dentry *ovl_dentry_lower(struct dentry *dentry);

fs/overlayfs/util.c +0 -0

index 19b936b0a551..22df74cb1def 100644

fs/overlayfs/util.c void ovl_path_lower(struct dentry *dentry, struct path *path) · line 150

Quoted 6 lines

fs/overlayfs/util.c +0 -0

@@ -148,6 +148,20 @@ void ovl_path_lower(struct dentry *dentry, struct path *path)
        }
 }
 

A comment here why ovl_path_lowerdata is needed would be nice.

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

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

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

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

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

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

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

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

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

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

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

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

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

Reviewed-by Amir Goldstein <[email protected]>

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

---
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/super.c     | 21 +++++++++++++++++----
 fs/overlayfs/util.c      |  8 ++++++++
 3 files changed, 26 insertions(+), 4 deletions(-)

fs/overlayfs/overlayfs.h +1 -0

index 2d682923252e..24725b6668b9 100644
@@ -225,6 +225,7 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
 struct dentry *ovl_dentry_lower(struct dentry *dentry);
+struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
 struct dentry *ovl_dentry_real(struct dentry *dentry);
 struct dentry *ovl_i_dentry_upper(struct inode *inode);
 struct inode *ovl_inode_upper(struct inode *inode);

fs/overlayfs/super.c +17 -4

index d3dbdd695722..4be4e47cbf57 100644
@@ -96,8 +96,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
        struct dentry *real;
        int err;
 
-       if (flags & D_REAL_UPPER)
-               return ovl_dentry_upper(dentry);
+       if (flags & D_REAL_UPPER) {
+               real = ovl_dentry_upper(dentry);
+               if (!real)
+                       return NULL;
+               if (!ovl_has_upperdata(dentry))
+                       return NULL;
+               return real;
+       }
 
        if (!d_is_reg(dentry)) {
                if (!inode || inode == d_inode(dentry))

@@ -113,15 +119,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 
        real = ovl_dentry_upper(dentry);
        if (real && (!inode || inode == d_inode(real))) {
+               bool metacopy = !ovl_has_upperdata(dentry);
                if (!inode) {
                        err = ovl_check_append_only(d_inode(real), open_flags);
                        if (err)
                                return ERR_PTR(err);
-               }
+
+                       if (unlikely(metacopy))
+                               goto lower;
+               } else if (unlikely(metacopy))
+                       goto bug;
+
                return real;
        }
 
-       real = ovl_dentry_lower(dentry);
+lower:
+       real = ovl_dentry_lowerdata(dentry);
        if (!real)
                goto bug;
 

fs/overlayfs/util.c +0 -0

index 274bbfc855e0..36d41f7001e3 100644
@@ -186,6 +186,14 @@ struct dentry *ovl_dentry_lower(struct dentry *dentry)
        return oe->numlower ? oe->lowerstack[0].dentry : NULL;

Quoted 130 lines · Vivek Goyal

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

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

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

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

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

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

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

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

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

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

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

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

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

Reviewed-by Amir Goldstein <[email protected]>

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

---
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/super.c     | 21 +++++++++++++++++----
 fs/overlayfs/util.c      |  8 ++++++++
 3 files changed, 26 insertions(+), 4 deletions(-)

fs/overlayfs/overlayfs.h +1 -0

index 2d682923252e..24725b6668b9 100644
@@ -225,6 +225,7 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
 struct dentry *ovl_dentry_lower(struct dentry *dentry);
+struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
 struct dentry *ovl_dentry_real(struct dentry *dentry);
 struct dentry *ovl_i_dentry_upper(struct inode *inode);
 struct inode *ovl_inode_upper(struct inode *inode);

fs/overlayfs/super.c +17 -4

index d3dbdd695722..4be4e47cbf57 100644
@@ -96,8 +96,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
        struct dentry *real;
        int err;
 
-       if (flags & D_REAL_UPPER)
-               return ovl_dentry_upper(dentry);
+       if (flags & D_REAL_UPPER) {
+               real = ovl_dentry_upper(dentry);
+               if (!real)
+                       return NULL;
+               if (!ovl_has_upperdata(dentry))
+                       return NULL;
+               return real;
+       }
 
        if (!d_is_reg(dentry)) {
                if (!inode || inode == d_inode(dentry))

@@ -113,15 +119,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 
        real = ovl_dentry_upper(dentry);
        if (real && (!inode || inode == d_inode(real))) {
+               bool metacopy = !ovl_has_upperdata(dentry);
                if (!inode) {
                        err = ovl_check_append_only(d_inode(real), open_flags);
                        if (err)
                                return ERR_PTR(err);
-               }
+
+                       if (unlikely(metacopy))
+                               goto lower;
+               } else if (unlikely(metacopy))
+                       goto bug;
+
                return real;
        }
 
-       real = ovl_dentry_lower(dentry);
+lower:
+       real = ovl_dentry_lowerdata(dentry);
        if (!real)
                goto bug;
 

fs/overlayfs/util.c +0 -0

index 274bbfc855e0..36d41f7001e3 100644
@@ -186,6 +186,14 @@ struct dentry *ovl_dentry_lower(struct dentry *dentry)
        return oe->numlower ? oe->lowerstack[0].dentry : NULL;
 }
 

fs/overlayfs/util.c struct dentry *ovl_dentry_lower(struct dentry *dentry) · line 196

Quoted 8 lines

fs/overlayfs/util.c +8 -0

+struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
+{
+       struct ovl_entry *oe = dentry->d_fsdata;
+       int idx = oe->numlower - 1;
+
+       return oe->lowerstack[idx].dentry;
+}
+

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

Quoted 131 lines · Amir Goldstein

Quoted 130 lines · Vivek Goyal

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

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

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

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

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

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

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

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

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

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

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

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

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

Reviewed-by Amir Goldstein <[email protected]>

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

---
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/super.c     | 21 +++++++++++++++++----
 fs/overlayfs/util.c      |  8 ++++++++
 3 files changed, 26 insertions(+), 4 deletions(-)

fs/overlayfs/overlayfs.h +1 -0

index 2d682923252e..24725b6668b9 100644
@@ -225,6 +225,7 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
 struct dentry *ovl_dentry_lower(struct dentry *dentry);
+struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
 struct dentry *ovl_dentry_real(struct dentry *dentry);
 struct dentry *ovl_i_dentry_upper(struct inode *inode);
 struct inode *ovl_inode_upper(struct inode *inode);

fs/overlayfs/super.c +17 -4

index d3dbdd695722..4be4e47cbf57 100644
@@ -96,8 +96,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
        struct dentry *real;
        int err;
 
-       if (flags & D_REAL_UPPER)
-               return ovl_dentry_upper(dentry);
+       if (flags & D_REAL_UPPER) {
+               real = ovl_dentry_upper(dentry);
+               if (!real)
+                       return NULL;
+               if (!ovl_has_upperdata(dentry))
+                       return NULL;
+               return real;
+       }
 
        if (!d_is_reg(dentry)) {
                if (!inode || inode == d_inode(dentry))

@@ -113,15 +119,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 
        real = ovl_dentry_upper(dentry);
        if (real && (!inode || inode == d_inode(real))) {
+               bool metacopy = !ovl_has_upperdata(dentry);
                if (!inode) {
                        err = ovl_check_append_only(d_inode(real), open_flags);
                        if (err)
                                return ERR_PTR(err);
-               }
+
+                       if (unlikely(metacopy))
+                               goto lower;
+               } else if (unlikely(metacopy))
+                       goto bug;
+
                return real;
        }
 
-       real = ovl_dentry_lower(dentry);
+lower:
+       real = ovl_dentry_lowerdata(dentry);
        if (!real)
                goto bug;
 

fs/overlayfs/util.c +0 -0

index 274bbfc855e0..36d41f7001e3 100644
@@ -186,6 +186,14 @@ struct dentry *ovl_dentry_lower(struct dentry *dentry)
        return oe->numlower ? oe->lowerstack[0].dentry : NULL;
 }
 

Quoted 13 lines

fs/overlayfs/util.c struct dentry *ovl_dentry_lower(struct dentry *dentry) · line 196

Quoted 8 lines

fs/overlayfs/util.c +8 -0

+struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
+{
+       struct ovl_entry *oe = dentry->d_fsdata;
+       int idx = oe->numlower - 1;
+
+       return oe->lowerstack[idx].dentry;
+}
+

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

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

Quoted 146 lines · Vivek Goyal

Quoted 144 lines · Amir Goldstein

Quoted 128 lines · Vivek Goyal

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

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

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

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

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

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

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

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

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

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

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

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

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

Reviewed-by Amir Goldstein <[email protected]>

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

---
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/super.c     | 21 +++++++++++++++++----
 fs/overlayfs/util.c      |  8 ++++++++
 3 files changed, 26 insertions(+), 4 deletions(-)

fs/overlayfs/overlayfs.h +1 -0

index 2d682923252e..24725b6668b9 100644
@@ -225,6 +225,7 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
 enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
 struct dentry *ovl_dentry_upper(struct dentry *dentry);
 struct dentry *ovl_dentry_lower(struct dentry *dentry);
+struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
 struct dentry *ovl_dentry_real(struct dentry *dentry);
 struct dentry *ovl_i_dentry_upper(struct inode *inode);
 struct inode *ovl_inode_upper(struct inode *inode);

fs/overlayfs/super.c +17 -4

index d3dbdd695722..4be4e47cbf57 100644
@@ -96,8 +96,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
        struct dentry *real;
        int err;
 
-       if (flags & D_REAL_UPPER)
-               return ovl_dentry_upper(dentry);
+       if (flags & D_REAL_UPPER) {
+               real = ovl_dentry_upper(dentry);
+               if (!real)
+                       return NULL;
+               if (!ovl_has_upperdata(dentry))
+                       return NULL;
+               return real;
+       }
 
        if (!d_is_reg(dentry)) {
                if (!inode || inode == d_inode(dentry))

@@ -113,15 +119,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 
        real = ovl_dentry_upper(dentry);
        if (real && (!inode || inode == d_inode(real))) {
+               bool metacopy = !ovl_has_upperdata(dentry);
                if (!inode) {
                        err = ovl_check_append_only(d_inode(real), open_flags);
                        if (err)
                                return ERR_PTR(err);
-               }
+
+                       if (unlikely(metacopy))
+                               goto lower;
+               } else if (unlikely(metacopy))
+                       goto bug;
+
                return real;
        }
 
-       real = ovl_dentry_lower(dentry);
+lower:
+       real = ovl_dentry_lowerdata(dentry);
        if (!real)
                goto bug;
 

fs/overlayfs/util.c +0 -0

index 274bbfc855e0..36d41f7001e3 100644
@@ -186,6 +186,14 @@ struct dentry *ovl_dentry_lower(struct dentry *dentry)
        return oe->numlower ? oe->lowerstack[0].dentry : NULL;

fs/overlayfs/util.c struct dentry *ovl_dentry_lower(struct dentry *dentry) · line 196

Quoted 10 lines

fs/overlayfs/util.c +8 -0

 }
 
+struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
+{
+       struct ovl_entry *oe = dentry->d_fsdata;
+       int idx = oe->numlower - 1;
+
+       return oe->lowerstack[idx].dentry;
+}
+

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

Quoted 2 lines

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

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

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

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

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

---
 fs/overlayfs/namei.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

fs/overlayfs/namei.c +0 -0

index 220e754c974b..a4a5c5f5540d 100644

fs/overlayfs/namei.c +1 -1

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

Quoted 12 lines · Vivek Goyal

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

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

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

---
 fs/overlayfs/namei.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

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

Quoted 11 lines

fs/overlayfs/namei.c +1 -2

index 220e754c974b..a4a5c5f5540d 100644
@@ -265,22 +265,22 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
                goto put_and_out;
        }
        if (!d_can_lookup(this)) {
-               if (d->is_dir)
-                       goto put_and_out;
+               d->is_dir = false;

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

Quoted 10 lines

fs/overlayfs/namei.c +1 -1

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

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

Quoted 10 lines

fs/overlayfs/namei.c +5 -5

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

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

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

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

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

Quoted 4 lines

fs/overlayfs/namei.c +1 -0

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

Thanks,
Amir.

Quoted 13 lines · Amir Goldstein

Quoted 12 lines · Vivek Goyal

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

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

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

---
 fs/overlayfs/namei.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Quoted 15 lines

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

Quoted 11 lines

fs/overlayfs/namei.c +1 -2

index 220e754c974b..a4a5c5f5540d 100644
@@ -265,22 +265,22 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
                goto put_and_out;
        }
        if (!d_can_lookup(this)) {
-               if (d->is_dir)
-                       goto put_and_out;
+               d->is_dir = false;

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

Can you elaborate a bit more. I never understood this check.

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

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

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

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

Quoted 22 lines

Quoted 11 lines

fs/overlayfs/namei.c +1 -1

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

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

Quoted 10 lines

fs/overlayfs/namei.c +5 -5

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

Quoted 8 lines

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

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

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

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

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