overlayfs: Delayed copy up of data

All the inode/ovl_inode initialization should happen in ovl_get_inode(). This is especially useful when multiple dentries are pointing to same inode and inode is already in the cash. In that case, we don't have to

initialize the OVL_INDEX. We don't even take ovl_inode->lock in ovl_lookup() and that can run into issues for the case of shared inode.

So move OVL_INDEX flag setting in ovl_get_inode().

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

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

fs/overlayfs/export.c +0 -3

index 87bd4148f4fb..9868c173068e 100644
@@ -311,9 +311,6 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
                return ERR_CAST(inode);
        }
 
-       if (index)
-               ovl_set_flag(OVL_INDEX, inode);
-
        dentry = d_find_any_alias(inode);
        if (!dentry) {
                dentry = d_alloc_anon(inode->i_sb);

fs/overlayfs/inode.c +3 -0

index 3b1bd469accd..06ef9a7a7d1a 100644
@@ -760,6 +760,9 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        if (upperdentry && ovl_is_impuredir(upperdentry))
                ovl_set_flag(OVL_IMPURE, inode);
 
+       if (index)
+               ovl_set_flag(OVL_INDEX, inode);
+
        /* Check for non-merge dir that may have whiteouts */
        if (is_dir) {
                if (((upperdentry && lowerdentry) || numlower > 1) ||

fs/overlayfs/namei.c +0 -3

index f55c00bf9e17..ec92fa2f7d5f 100644
@@ -1005,8 +1005,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                        goto out_free_oe;
 
                OVL_I(inode)->redirect = upperredirect;
-               if (index)
-                       ovl_set_flag(OVL_INDEX, inode);
        }
 
        revert_creds(old_cred);
--

2.13.6

All the inode/ovl_inode properties ideally should be initialized in ovl_get_inode(). Otherwise this can be a problem for the cases where multiple dentries point to same inode (hardlinks, disconnected dentries).

As of now this is not a problem as redirects are used only for directories which don't share inode. But soon I want to use redirects for regular files also and there it will become an issue.

Hence, move ->redirect initialization in ovl_get_inode().

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

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

fs/overlayfs/export.c +0 -0

index 9868c173068e..e668329f7361 100644

fs/overlayfs/export.c +1 -0

+       inode = ovl_get_inode(sb, dget(upper), lower, index, !!lower, NULL);
        if (IS_ERR(inode)) {
                dput(upper);
                return ERR_CAST(inode);

fs/overlayfs/inode.c +4 -1

index 06ef9a7a7d1a..2e7ee09c7831 100644
@@ -704,7 +704,7 @@ static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper,
 
 struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
                            struct dentry *lowerdentry, struct dentry *index,
-                           unsigned int numlower)
+                           unsigned int numlower, char *redirect)
 {
        struct inode *realinode = upperdentry ? d_inode(upperdentry) : NULL;
        struct inode *inode;

@@ -741,6 +741,7 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
                        }
 
                        dput(upperdentry);
+                       kfree(redirect);
                        goto out;
                }
 

@@ -763,6 +764,8 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        if (index)
                ovl_set_flag(OVL_INDEX, inode);
 
+       OVL_I(inode)->redirect = redirect;
+
        /* Check for non-merge dir that may have whiteouts */
        if (is_dir) {
                if (((upperdentry && lowerdentry) || numlower > 1) ||

fs/overlayfs/namei.c +1 -3

index ec92fa2f7d5f..0b325e65864c 100644
@@ -999,12 +999,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                if (ctr)
                        origin = stack[0].dentry;
                inode = ovl_get_inode(dentry->d_sb, upperdentry, origin, index,
-                                     ctr);
+                                     ctr, upperredirect);
                err = PTR_ERR(inode);
                if (IS_ERR(inode))
                        goto out_free_oe;
-
-               OVL_I(inode)->redirect = upperredirect;
        }
 
        revert_creds(old_cred);

fs/overlayfs/overlayfs.h +1 -2

index 225ff1171147..a65ce7fd1b6e 100644
@@ -330,7 +330,7 @@ struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
                               bool is_upper);
 struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
                            struct dentry *lowerdentry, struct dentry *index,
-                           unsigned int numlower);
+                           unsigned int numlower, char *redirect);
 static inline void ovl_copyattr(struct inode *from, struct inode *to)
 {
        to->i_uid = from->i_uid;
--

2.13.6

As "locked" tracks the mutex lock state of "new" dentry inode instead of "old" dentry inode, name the variable "new_locked" instead.

Soon I will need to lock old dentry inode lock as well and will track that state in variable "locked".

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

---
 fs/overlayfs/dir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

fs/overlayfs/dir.c +3 -4

index 839709c7803a..8a38f3136547 100644
@@ -892,7 +892,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
                      unsigned int flags)
 {
        int err;
-       bool locked = false;
+       bool new_locked = false;
        struct dentry *old_upperdir;
        struct dentry *new_upperdir;
        struct dentry *olddentry;

@@ -959,7 +959,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
                if (err)
                        goto out_drop_write;
        } else {
-               err = ovl_nlink_start(new, &locked);
+               err = ovl_nlink_start(new, &new_locked);
                if (err)
                        goto out_drop_write;
        }

@@ -1087,7 +1087,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
        unlock_rename(new_upperdir, old_upperdir);
 out_revert_creds:
        revert_creds(old_cred);
-       ovl_nlink_end(new, locked);
+       ovl_nlink_end(new, new_locked);
 out_drop_write:
        ovl_drop_write(old);
 out:
--

2.13.6

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                    | 19 ++++++++++++++++
 fs/overlayfs/ovl_entry.h                |  1 +
 fs/overlayfs/super.c                    | 40 ++++++++++++++++++++++++++++++++-
 4 files changed, 88 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 +0 -0

index ce6ff5a0a6e4..7d9650c9c075 100644

fs/overlayfs/Kconfig +13 -0

+       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.
+
+         If unsure, say N.

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 +22 -1

index 7c24619ae7fc..ddff54fa9e85 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);

fs/overlayfs/super.c +7 -0

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

Quoted 5 lines

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.

I don't see that in the patch

Quoted 77 lines

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                    | 19 ++++++++++++++++
 fs/overlayfs/ovl_entry.h                |  1 +
 fs/overlayfs/super.c                    | 40 ++++++++++++++++++++++++++++++++-
 4 files changed, 88 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 +0 -0

index ce6ff5a0a6e4..7d9650c9c075 100644
@@ -86,3 +86,22 @@ config OVERLAY_FS_NFS_EXPORT
          case basis with the "nfs_export=on" mount option.
 

fs/overlayfs/Kconfig config OVERLAY_FS_NFS_EXPORT · line 93

Quoted 6 lines

fs/overlayfs/Kconfig +5 -0

          Say N unless you fully understand the consequences.
+
+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

Like the test in the code, the dependency should be OVERLAY_FS_NFS_EXPORT depends on !OVERLAY_FS_METACOPY

fs/overlayfs/Kconfig config OVERLAY_FS_NFS_EXPORT · line 94

Quoted 2 lines

fs/overlayfs/Kconfig +1 -0

+       select OVERLAY_FS_REDIRECT_DIR

At first glance, I thought this should be
depends on OVERLAY_FS_REDIRECT_DIR,
like in the code
But I see why select makes sense in the context of config options. Makes me wonder if NFS_EXPORT should also select INDEX

I know why I didn't do this logic in the code, because we do not distinguish in the code between explicit mount option "index=off" and no mount option at all when default is "index=off". The former should disable nfs_export but the latter should enable index.

Quoted 98 lines

fs/overlayfs/Kconfig +13 -0

+       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.
+
+         If unsure, say N.

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 +22 -1

index 7c24619ae7fc..ddff54fa9e85 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;
        }
 

fs/overlayfs/super.c static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath) · line 1040

Quoted 5 lines

fs/overlayfs/super.c +5 -0

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

Please move all these scattered tests that depend only on parsed config values at the end of ovl_parse_redirect_mode().

Quoted 29 lines

fs/overlayfs/super.c +7 -0

 out:
        mnt_drop_write(mnt);
        return err;

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

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

@@ -1331,6 +1364,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
                }
        }
 

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

Quoted 2 lines

fs/overlayfs/super.c +2 -0

+       if (ofs->config.metacopy && ofs->config.nfs_export) {
+               pr_warn("overlayfs: Metadata copy up requires NFS export disabled, falling back to nfs_export=off.\n");

"NFS export is not supported with metadata only copy up, ..."

Quoted 7 lines · Amir Goldstein

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

Quoted 7 lines

Quoted 5 lines

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.

I don't see that in the patch

Oh.., this is leftover from previous patches. Will remove this comment. I have completely got rid of dealing with ORIGIN when moving to REDIRECT based lookup.

Quoted 23 lines

Quoted 23 lines

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                    | 19 ++++++++++++++++
 fs/overlayfs/ovl_entry.h                |  1 +
 fs/overlayfs/super.c                    | 40 ++++++++++++++++++++++++++++++++-
 4 files changed, 88 insertions(+), 2 deletions(-)

Documentation/filesystems/overlayfs.txt +0 -0

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.
 
 

Quoted 11 lines

Documentation/filesystems/overlayfs.txt rightmost one and going left. In the above example lower1 will be the · line 262

Quoted 11 lines

Documentation/filesystems/overlayfs.txt +11 -0

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

Quoted 43 lines · Vivek Goyal

Quoted 7 lines · Amir Goldstein

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

Quoted 7 lines

Quoted 5 lines

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.

I don't see that in the patch

Oh.., this is leftover from previous patches. Will remove this comment. I have completely got rid of dealing with ORIGIN when moving to REDIRECT based lookup.

Quoted 23 lines

Quoted 23 lines

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                    | 19 ++++++++++++++++
 fs/overlayfs/ovl_entry.h                |  1 +
 fs/overlayfs/super.c                    | 40 ++++++++++++++++++++++++++++++++-
 4 files changed, 88 insertions(+), 2 deletions(-)

Documentation/filesystems/overlayfs.txt +0 -0

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.
 
 

Quoted 11 lines

Quoted 11 lines

Documentation/filesystems/overlayfs.txt rightmost one and going left. In the above example lower1 will be the · line 262

Quoted 11 lines

Documentation/filesystems/overlayfs.txt +11 -0

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

Vivek,

Some nick picking here.
This is document is about the only decent documentation for overlayfs. I always try to live up to the standards set by Neil who started it. Abbreviations (IOW) doesn't seem right to me in this context. Also, you'll notice that the form "copyup" does not appear in this document. It may sound petty, but if people wish to read a coherent document about overlayfs, we would be wise to maintain coherency in the phrases we use throughout the document.

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

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

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

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

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

For example.

CPU1 CPU2
ovl_d_real() acquire(oi->lock)
ovl_open_maybe_copy_up() ovl_copy_up_data()
open_open_need_copy_up() vfs_removexattr()
ovl_already_copied_up()

    ovl_dentry_needs_data_copy_up()      ovl_set_flag(OVL_UPPERDATA)
     ovl_test_flag(OVL_UPPERDATA)       release(oi->lock)

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

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

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

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

Reviewed-by Amir Goldstein <[email protected]>

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

---
 fs/overlayfs/copy_up.c   | 56 ++++++++++++++++++++++++++++++----
 fs/overlayfs/dir.c       |  1 +
 fs/overlayfs/overlayfs.h | 18 +++++++++--
 fs/overlayfs/super.c     |  1 +
 fs/overlayfs/util.c      | 78 +++++++++++++++++++++++++++++++++++++++++++++---
 5 files changed, 143 insertions(+), 11 deletions(-)

fs/overlayfs/copy_up.c +21 -1

index 8d9af7fdc8a4..9801ae7baa5d 100644
@@ -195,6 +195,16 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
        return error;
 }
 
+static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
+{
+       struct iattr attr = {
+               .ia_valid = ATTR_SIZE,
+               .ia_size = stat->size,
+       };
+
+       return notify_change(upperdentry, &attr, NULL);
+}
+
 static int ovl_set_timestamps(struct dentry *upperdentry, struct kstat *stat)
 {
        struct iattr attr = {

@@ -586,8 +596,18 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
                        return err;
        }
 
+       if (c->metacopy) {
+               err = ovl_check_setxattr(c->dentry, temp, OVL_XATTR_METACOPY,
+                                        NULL, 0, -EOPNOTSUPP);
+               if (err)
+                       return err;
+       }
+
        inode_lock(temp->d_inode);
-       err = ovl_set_attr(temp, &c->stat);
+       if (c->metacopy)
+               err = ovl_set_size(temp, &c->stat);
+       if (!err)
+               err = ovl_set_attr(temp, &c->stat);
        inode_unlock(temp->d_inode);
 
        return err;

Quoted 99 lines · Vivek Goyal

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

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

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

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

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

For example.

CPU1 CPU2
ovl_d_real() acquire(oi->lock)
ovl_open_maybe_copy_up() ovl_copy_up_data()
open_open_need_copy_up() vfs_removexattr()
ovl_already_copied_up()

    ovl_dentry_needs_data_copy_up()      ovl_set_flag(OVL_UPPERDATA)
     ovl_test_flag(OVL_UPPERDATA)       release(oi->lock)

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

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

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

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

Reviewed-by Amir Goldstein <[email protected]>

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

---
 fs/overlayfs/copy_up.c   | 56 ++++++++++++++++++++++++++++++----
 fs/overlayfs/dir.c       |  1 +
 fs/overlayfs/overlayfs.h | 18 +++++++++--
 fs/overlayfs/super.c     |  1 +
 fs/overlayfs/util.c      | 78 +++++++++++++++++++++++++++++++++++++++++++++---
 5 files changed, 143 insertions(+), 11 deletions(-)

fs/overlayfs/copy_up.c +21 -1

index 8d9af7fdc8a4..9801ae7baa5d 100644
@@ -195,6 +195,16 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
        return error;
 }
 
+static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
+{
+       struct iattr attr = {
+               .ia_valid = ATTR_SIZE,
+               .ia_size = stat->size,
+       };
+
+       return notify_change(upperdentry, &attr, NULL);
+}
+
 static int ovl_set_timestamps(struct dentry *upperdentry, struct kstat *stat)
 {
        struct iattr attr = {

@@ -586,8 +596,18 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
                        return err;
        }
 
+       if (c->metacopy) {
+               err = ovl_check_setxattr(c->dentry, temp, OVL_XATTR_METACOPY,
+                                        NULL, 0, -EOPNOTSUPP);
+               if (err)
+                       return err;
+       }
+
        inode_lock(temp->d_inode);
-       err = ovl_set_attr(temp, &c->stat);
+       if (c->metacopy)
+               err = ovl_set_size(temp, &c->stat);
+       if (!err)
+               err = ovl_set_attr(temp, &c->stat);
        inode_unlock(temp->d_inode);
 
        return err;

fs/overlayfs/copy_up.c static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c) · line 651

Quoted 7 lines

fs/overlayfs/copy_up.c +2 -0

@@ -625,6 +645,8 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
        if (err)
                goto out_cleanup;
 
+       if (!c->metacopy)
+               ovl_set_upperdata(d_inode(c->dentry));
        inode = d_inode(c->dentry);
        ovl_inode_update(inode, newdentry);

Following discussion on patch 20/28, I think this should be

    if (!c->metacopy)
            ovl_set_flag(OVL_UPPERDATA, inode);

without the memory barrier, because all the places that check ovl_has_upperdata check upperdentry first, so the smp_wmb() in ovl_inode_update() is sufficient and the extra wmb is really only needed in ovl_copy_up_meta_inode_data().

Right?

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     |  6 +++-
 fs/overlayfs/namei.c     | 90 ++++++++++++++++++++++++++++++++++++++++++------
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 22 ++++++++++++
 5 files changed, 110 insertions(+), 12 deletions(-)

fs/overlayfs/export.c +3 -0

index e668329f7361..1c233096e59c 100644
@@ -311,6 +311,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 +1 -1

index 3991a890b464..e1dbfed0c449 100644
@@ -677,7 +677,7 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        struct inode *realinode = upperdentry ? d_inode(upperdentry) : NULL;
        struct inode *inode;
        bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry, index);
-       bool is_dir;
+       bool is_dir, metacopy = false;
 
        if (!realinode)
                realinode = d_inode(lowerdentry);

fs/overlayfs/inode.c +3 -0

+       if (upperdentry && !metacopy)
+               ovl_set_flag(OVL_UPPERDATA, inode);
+
        OVL_I(inode)->redirect = redirect;
 
        /* Check for non-merge dir that may have whiteouts */

fs/overlayfs/namei.c +1 -0

index 0b325e65864c..1dba89e9543f 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,

fs/overlayfs/namei.c +25 -7

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

@@ -815,7 +820,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;

@@ -826,6 +831,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,

@@ -833,6 +839,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)

@@ -851,7 +858,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,

@@ -862,16 +870,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;
                }

@@ -883,7 +895,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++) {

@@ -905,7 +917,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);

@@ -917,16 +929,35 @@ 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);

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;

fs/overlayfs/namei.c +30 -0

+               /*
+                * 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

@@ -1006,6 +1065,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);

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 16 lines

fs/overlayfs/namei.c +7 -1

@@ -917,16 +929,35 @@ 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);

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

Quoted 10 lines

fs/overlayfs/namei.c +5 -3

-                               break;
+                               if (d.is_dir)
+                                       break;
+                               goto out_put;
                        }
-
                        /* Bless lower dir as verified origin */
-                       origin = this;
+                       if (d.is_dir)
+                               origin = this;

It's ok to bless verified non-dir as well.
It is going to be blesses anyway, just above index lookup if ctr > 0.

Quoted 18 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;

@@ -960,6 +991,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 995

Quoted 2 lines

fs/overlayfs/namei.c +2 -0

+       if (metacopy) {
+               BUG_ON(d.is_dir);

Yeh, I think that is really a bug, because you need to detect the case of dir in lower layer under metacopy in upper layer and do something about it.

Quoted 16 lines

fs/overlayfs/namei.c +16 -0

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

Quoted 5 lines

fs/overlayfs/namei.c +5 -0

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

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

Quoted 8 lines

fs/overlayfs/namei.c +5 -0

+               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

Quoted 17 lines · Amir Goldstein

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 49 lines

Quoted 16 lines

fs/overlayfs/namei.c +7 -1

@@ -917,16 +929,35 @@ 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);

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

Quoted 10 lines

fs/overlayfs/namei.c +5 -3

-                               break;
+                               if (d.is_dir)
+                                       break;
+                               goto out_put;
                        }
-
                        /* Bless lower dir as verified origin */
-                       origin = this;
+                       if (d.is_dir)
+                               origin = this;

It's ok to bless verified non-dir as well.
It is going to be blesses anyway, just above index lookup if ctr > 0.

Quoted 18 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;

@@ -960,6 +991,34 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                }
        }
 

Quoted 23 lines

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

Quoted 2 lines

fs/overlayfs/namei.c +2 -0

+       if (metacopy) {
+               BUG_ON(d.is_dir);

Yeh, I think that is really a bug, because you need to detect the case of dir in lower layer under metacopy in upper layer and do something about it.

Quoted 16 lines

fs/overlayfs/namei.c +16 -0

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

in (!origin_path) lower was followed by name/redirect and not verified by origin, we should not lookup index of non-dir below. Right now non-dir index entries assume that origin xattr exists and matches the entry name. We may be able to relax that in the future using non-dir redirect, but we are not there yet.

Quoted 15 lines

Quoted 5 lines

fs/overlayfs/namei.c +3 -0

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

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

Quoted 10 lines

fs/overlayfs/namei.c +7 -0

+                       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.
         */
        if (ctr && (!upperdentry || !d.is_dir))
                origin = stack[0].dentry;

So this condition needs to be fixed.

Quoted 93 lines · Amir Goldstein

Quoted 17 lines · Amir Goldstein

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 49 lines

Quoted 16 lines

fs/overlayfs/namei.c +7 -1

@@ -917,16 +929,35 @@ 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);

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

Quoted 10 lines

fs/overlayfs/namei.c +5 -3

-                               break;
+                               if (d.is_dir)
+                                       break;
+                               goto out_put;
                        }
-
                        /* Bless lower dir as verified origin */
-                       origin = this;
+                       if (d.is_dir)
+                               origin = this;

It's ok to bless verified non-dir as well.
It is going to be blesses anyway, just above index lookup if ctr > 0.

Quoted 18 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;

@@ -960,6 +991,34 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                }
        }
 

Quoted 23 lines

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

Quoted 2 lines

fs/overlayfs/namei.c +2 -0

+       if (metacopy) {
+               BUG_ON(d.is_dir);

Yeh, I think that is really a bug, because you need to detect the case of dir in lower layer under metacopy in upper layer and do something about it.

Quoted 16 lines

fs/overlayfs/namei.c +16 -0

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

Quoted 5 lines

in (!origin_path) lower was followed by name/redirect and not verified by origin, we should not lookup index of non-dir below. Right now non-dir index entries assume that origin xattr exists and matches the entry name. We may be able to relax that in the future using non-dir redirect, but we are not there yet.

Ok.

Quoted 15 lines

Quoted 15 lines

Quoted 5 lines

fs/overlayfs/namei.c +3 -0

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

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

Quoted 10 lines

fs/overlayfs/namei.c +7 -0

+                       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

Quoted 7 lines

         * origin, otherwise dir index entries may be inconsistent and we
         * ignore them. Always lookup index of non-dir and non-upper.
         */
        if (ctr && (!upperdentry || !d.is_dir))
                origin = stack[0].dentry;

So this condition needs to be fixed.

Ok, So I could make it like the case of directory. That is for metacopy files bless origin only if it was verified and modify above condiiton as follows.

         if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
                 origin = stack[0].dentry;

That way for a metcopy dentry we will search for index only if origin was verified.

Quoted 54 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     |  6 +++-
 fs/overlayfs/namei.c     | 90 ++++++++++++++++++++++++++++++++++++++++++------
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 22 ++++++++++++
 5 files changed, 110 insertions(+), 12 deletions(-)

fs/overlayfs/export.c +3 -0

index e668329f7361..1c233096e59c 100644
@@ -311,6 +311,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 +1 -1

index 3991a890b464..e1dbfed0c449 100644
@@ -677,7 +677,7 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        struct inode *realinode = upperdentry ? d_inode(upperdentry) : NULL;
        struct inode *inode;
        bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry, index);
-       bool is_dir;
+       bool is_dir, metacopy = false;
 
        if (!realinode)
                realinode = d_inode(lowerdentry);

@@ -732,6 +732,10 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        if (index)
                ovl_set_flag(OVL_INDEX, inode);
 

fs/overlayfs/inode.c struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry, · line 735

Quoted 1 line

fs/overlayfs/inode.c +1 -0

+       metacopy = ovl_check_metacopy_xattr(upperdentry ?: lowerdentry);

No reason to check metacopy on lowerdentry.

Quoted 18 lines

fs/overlayfs/inode.c +3 -0

+       if (upperdentry && !metacopy)
+               ovl_set_flag(OVL_UPPERDATA, inode);
+
        OVL_I(inode)->redirect = redirect;
 
        /* Check for non-merge dir that may have whiteouts */

fs/overlayfs/namei.c +1 -0

index 0b325e65864c..1dba89e9543f 100644
@@ -24,6 +24,7 @@ struct ovl_lookup_data {
        bool stop;
        bool last;
        char *redirect;
+       bool metacopy;
 };
 

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

Quoted 6 lines

fs/overlayfs/namei.c +0 -1

 static int ovl_check_redirect(struct dentry *dentry, struct ovl_lookup_data *d,

@@ -252,9 +253,13 @@ 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;

upper was a dir. You look in lower and find a non-dir. you need to stop going to next layer. goto put_and_out won't do that.

Similarly, you need to handle the case where dir is found below non-dir with metacopy.

Quoted 108 lines

fs/overlayfs/namei.c +28 -8

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

@@ -815,7 +820,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;

@@ -826,6 +831,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,

@@ -833,6 +839,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)

@@ -851,7 +858,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,

@@ -862,16 +870,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;
                }

@@ -883,7 +895,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++) {

@@ -905,7 +917,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);

@@ -917,16 +929,35 @@ 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;
                        }

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

Quoted 5 lines

fs/overlayfs/namei.c +2 -2

-
                        /* Bless lower dir as verified origin */
-                       origin = this;
+                       if (d.is_dir)
+                               origin = this;

It's ok to bless verified non-dir as well.
It is going to be blesses anyway, just above index lookup if ctr > 0.

Quoted 16 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;

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

Quoted 5 lines

fs/overlayfs/namei.c +2 -0

@@ -960,6 +991,34 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                }
        }
 
+       if (metacopy) {
+               BUG_ON(d.is_dir);

Yeh, I think that is really a bug, because you need to detect the case of dir in lower layer under metacopy in upper layer and do something about it.

Quoted 44 lines

fs/overlayfs/namei.c +30 -0

+               /*
+                * 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

@@ -1006,6 +1065,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);

@@ -1019,6 +1082,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
        for (i = 0; i < ctr; i++)
                dput(stack[i].dentry);
        kfree(stack);

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

Quoted 5 lines

fs/overlayfs/namei.c +5 -0

+out_put_origin:
+       if (origin_path) {
+               dput(origin_path->dentry);
+               kfree(origin_path);
+       }

There is no need for the new goto label.
Just add this in existing out_put_upper label.

Thanks,
Amir,

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

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

Reviewed-by Amir Goldstein <[email protected]>

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

---
 fs/overlayfs/inode.c     | 17 ++++++++++++++++-
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 16 ++++++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

fs/overlayfs/inode.c +3 -0

index e1dbfed0c449..cd03f3e642fd 100644
@@ -76,6 +76,9 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
        bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
        bool samefs = ovl_same_sb(dentry->d_sb);
        int err;
+       bool metacopy = false;
+
+       metacopy = ovl_is_metacopy_dentry(dentry);
 
        type = ovl_path_real(dentry, &realpath);
        old_cred = ovl_override_creds(dentry->d_sb);

Quoted 32 lines · Vivek Goyal

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

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

Reviewed-by Amir Goldstein <[email protected]>

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

---
 fs/overlayfs/inode.c     | 17 ++++++++++++++++-
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 16 ++++++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

fs/overlayfs/inode.c +3 -0

index e1dbfed0c449..cd03f3e642fd 100644
@@ -76,6 +76,9 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
        bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
        bool samefs = ovl_same_sb(dentry->d_sb);
        int err;
+       bool metacopy = false;
+
+       metacopy = ovl_is_metacopy_dentry(dentry);
 
        type = ovl_path_real(dentry, &realpath);
        old_cred = ovl_override_creds(dentry->d_sb);

fs/overlayfs/inode.c int ovl_getattr(const struct path *path, struct kstat *stat, · line 100

Quoted 6 lines

fs/overlayfs/inode.c +2 -1

@@ -93,7 +96,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
        if (!is_dir || samefs) {
                if (OVL_TYPE_ORIGIN(type)) {
                        struct kstat lowerstat;
-                       u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
+                       u32 lowermask = STATX_INO | STATX_BLOCKS |
+                                       (!is_dir ? STATX_NLINK : 0);

Leftover.

But I think you could do with one vfs_getattr():

         if (!is_dir || samefs) {
                   u32 lowermask = metacopy ? STATX_BLOCKS : 0;
                   if (OVL_TYPE_ORIGIN(type))
                           lowermask |= STATX_INO | (!is_dir ? STATX_NLINK : 0);
                   if (lowermask) {
                           ovl_path_lower(dentry, &realpath);
                           err = vfs_getattr(&realpath, &lowerstat,

Quoted 44 lines · Amir Goldstein

Quoted 32 lines · Vivek Goyal

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

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

Reviewed-by Amir Goldstein <[email protected]>

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

---
 fs/overlayfs/inode.c     | 17 ++++++++++++++++-
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 16 ++++++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

fs/overlayfs/inode.c +3 -0

index e1dbfed0c449..cd03f3e642fd 100644
@@ -76,6 +76,9 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
        bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
        bool samefs = ovl_same_sb(dentry->d_sb);
        int err;
+       bool metacopy = false;
+
+       metacopy = ovl_is_metacopy_dentry(dentry);
 
        type = ovl_path_real(dentry, &realpath);
        old_cred = ovl_override_creds(dentry->d_sb);

fs/overlayfs/inode.c int ovl_getattr(const struct path *path, struct kstat *stat, · line 100

Quoted 6 lines

fs/overlayfs/inode.c +2 -1

@@ -93,7 +96,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
        if (!is_dir || samefs) {
                if (OVL_TYPE_ORIGIN(type)) {
                        struct kstat lowerstat;
-                       u32 lowermask = STATX_INO | (!is_dir ? STATX_NLINK : 0);
+                       u32 lowermask = STATX_INO | STATX_BLOCKS |
+                                       (!is_dir ? STATX_NLINK : 0);

Leftover.

But I think you could do with one vfs_getattr():

Quoted 7 lines

         if (!is_dir || samefs) {
                   u32 lowermask = metacopy ? STATX_BLOCKS : 0;
                   if (OVL_TYPE_ORIGIN(type))
                           lowermask |= STATX_INO | (!is_dir ? STATX_NLINK : 0);
                   if (lowermask) {
                           ovl_path_lower(dentry, &realpath);
                           err = vfs_getattr(&realpath, &lowerstat,

We now support mid layer metacopy dentries. So we will need to do vfs_getattr() on lowest data dentry for data blocks. While rest of the info (inode number and nlink) comes from top most lower dentry.

So I don't think we can get rid of extra vfs_getattr()?

Now we have the notion of data dentry and metacopy dentry. ovl_dentry_lower() will return lower dentry at idx 0, but it could be either data or metacopy

dentry. Now we support metacopy dentries in lower layers so it is possible that lowerstack[0] is metacopy dentry while lowerstack[1] is actual data dentry.

So add an helper which returns lowest most dentry which is supposed to be data dentry.

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

---
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

fs/overlayfs/overlayfs.h +1 -0

index 4eb4b765887f..214d9f08c574 100644
@@ -226,6 +226,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/util.c +0 -0

index fd98329e820c..394674c4c820 100644
@@ -186,6 +186,20 @@ struct dentry *ovl_dentry_lower(struct dentry *dentry)
        return oe->numlower ? oe->lowerstack[0].dentry : NULL;
 }
 

fs/overlayfs/util.c +4 -1

+
+       return idx >= 0 ? oe->lowerstack[idx].dentry : NULL;
+}
+
 struct dentry *ovl_dentry_real(struct dentry *dentry)
 {
        return ovl_dentry_upper(dentry) ?: ovl_dentry_lower(dentry);
--

2.13.6

Quoted 2 lines · Vivek Goyal

Now we have the notion of data dentry and metacopy dentry. ovl_dentry_lower() will return lower dentry at idx 0, but it could be either data or metacopy

will return the upper most lower dentry. (idx 0 means upper).

Quoted 9 lines

dentry. Now we support metacopy dentries in lower layers so it is possible that lowerstack[0] is metacopy dentry while lowerstack[1] is actual data dentry.

So add an helper which returns lowest most dentry which is supposed to be data dentry.

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

Reviewed-by Amir Goldstein <[email protected]>

Quoted 25 lines

---
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

fs/overlayfs/overlayfs.h +1 -0

index 4eb4b765887f..214d9f08c574 100644
@@ -226,6 +226,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/util.c +0 -0

index fd98329e820c..394674c4c820 100644
@@ -186,6 +186,20 @@ 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 198

Quoted 10 lines

fs/overlayfs/util.c +10 -0

+/*
+ * ovl_dentry_lower() could return either a data dentry or metacopy dentry
+ * dependig on what is stored in lowerstack[0]. At times we need to find
+ * lower dentry which has data (and not metacopy dntry). This helper
+ * returns the lower data dentry.
+ */
+struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
+{
+       struct ovl_entry *oe = dentry->d_fsdata;
+       int idx = oe->numlower - 1;

Please stick with convention of layer->idx that idx 0 is upper.

Quoted 10 lines

fs/overlayfs/util.c +4 -1

+
+       return idx >= 0 ? oe->lowerstack[idx].dentry : NULL;
+}
+
 struct dentry *ovl_dentry_real(struct dentry *dentry)
 {
        return ovl_dentry_upper(dentry) ?: ovl_dentry_lower(dentry);
--

2.13.6