* [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature
@ 2020-09-23 15:23 Pavel Tikhomirov
2020-09-23 16:09 ` Amir Goldstein
2020-09-23 19:47 ` Vivek Goyal
0 siblings, 2 replies; 10+ messages in thread
From: Pavel Tikhomirov @ 2020-09-23 15:23 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Pavel Tikhomirov, Amir Goldstein, Vivek Goyal, linux-unionfs,
linux-kernel
This relaxes uuid checks for overlay index feature. It is only possible
in case there is only one filesystem for all the work/upper/lower
directories and bare file handles from this backing filesystem are uniq.
In case we have multiple filesystems here just fall back to normal
"index=on".
This is needed when overlayfs is/was mounted in a container with
index enabled (e.g.: to be able to resolve inotify watch file handles on
it to paths in CRIU), and this container is copied and started alongside
with the original one. This way the "copy" container can't have the same
uuid on the superblock and mounting the overlayfs from it later would
fail.
That is an example of the problem on top of loop+ext4:
dd if=/dev/zero of=loopbackfile.img bs=100M count=10
losetup -fP loopbackfile.img
losetup -a
#/dev/loop0: [64768]:35 (/loop-test/loopbackfile.img)
mkfs.ext4 loopbackfile.img
mkdir loop-mp
mount -o loop /dev/loop0 loop-mp
mkdir loop-mp/{lower,upper,work,merged}
mount -t overlay overlay -oindex=on,lowerdir=loop-mp/lower,\
upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
umount loop-mp/merged
umount loop-mp
e2fsck -f /dev/loop0
tune2fs -U random /dev/loop0
mount -o loop /dev/loop0 loop-mp
mount -t overlay overlay -oindex=on,lowerdir=loop-mp/lower,\
upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
#mount: /loop-test/loop-mp/merged:
#mount(2) system call failed: Stale file handle.
mount -t overlay overlay -oindex=nouuid,lowerdir=loop-mp/lower,\
upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
If you just change the uuid of the backing filesystem, overlay is not
mounting any more. In Virtuozzo we copy container disks (ploops) when
crate the copy of container and we require fs uuid to be uniq for a new
container.
v2: in v1 I missed actual uuid check skip - add it
CC: Amir Goldstein <amir73il@gmail.com>
CC: Vivek Goyal <vgoyal@redhat.com>
CC: Miklos Szeredi <miklos@szeredi.hu>
CC: linux-unionfs@vger.kernel.org
CC: linux-kernel@vger.kernel.org
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
fs/overlayfs/Kconfig | 16 +++++++++++
fs/overlayfs/export.c | 6 ++--
fs/overlayfs/namei.c | 35 +++++++++++++++--------
fs/overlayfs/overlayfs.h | 23 +++++++++++----
fs/overlayfs/ovl_entry.h | 2 +-
fs/overlayfs/super.c | 61 +++++++++++++++++++++++++++++-----------
6 files changed, 106 insertions(+), 37 deletions(-)
diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index dd188c7996b3..b00fd44006f9 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -61,6 +61,22 @@ config OVERLAY_FS_INDEX
If unsure, say N.
+config OVERLAY_FS_INDEX_NOUUID
+ bool "Overlayfs: relax uuid checks of inodes index feature"
+ depends on OVERLAY_FS
+ depends on OVERLAY_FS_INDEX
+ help
+ If this config option is enabled then overlay will skip uuid checks
+ for index lower to upper inode map, this only can be done if all
+ upper and lower directories are on the same filesystem where basic
+ fhandles are uniq.
+
+ It is needed to overcome possible change of uuid on superblock of the
+ backing filesystem, e.g. when you copied the virtual disk and mount
+ both the copy of the disk and the original one at the same time.
+
+ If unsure, say N.
+
config OVERLAY_FS_NFS_EXPORT
bool "Overlayfs: turn on NFS export feature by default"
depends on OVERLAY_FS
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 0e696f72cf65..d53feb7547d9 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -676,11 +676,12 @@ static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
struct ovl_fs *ofs = sb->s_fs_info;
struct dentry *dentry;
struct dentry *upper;
+ bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
if (!ovl_upper_mnt(ofs))
return ERR_PTR(-EACCES);
- upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true);
+ upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true, nouuid);
if (IS_ERR_OR_NULL(upper))
return upper;
@@ -700,6 +701,7 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
struct dentry *index = NULL;
struct inode *inode;
int err;
+ bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
/* First lookup overlay inode in inode cache by origin fh */
err = ovl_check_origin_fh(ofs, fh, false, NULL, &stack);
@@ -752,7 +754,7 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
goto out_err;
}
if (index) {
- err = ovl_verify_origin(index, origin.dentry, false);
+ err = ovl_verify_origin(index, origin.dentry, false, nouuid);
if (err)
goto out_err;
}
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index f7d4358db637..676170d719f8 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -149,7 +149,7 @@ static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
}
struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
- bool connected)
+ bool connected, bool nouuid)
{
struct dentry *real;
int bytes;
@@ -158,7 +158,7 @@ struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
* Make sure that the stored uuid matches the uuid of the lower
* layer where file handle will be decoded.
*/
- if (!uuid_equal(&fh->fb.uuid, &mnt->mnt_sb->s_uuid))
+ if (!nouuid && !uuid_equal(&fh->fb.uuid, &mnt->mnt_sb->s_uuid))
return NULL;
bytes = (fh->fb.len - offsetof(struct ovl_fb, fid));
@@ -342,6 +342,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
{
struct dentry *origin = NULL;
int i;
+ bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
for (i = 1; i < ofs->numlayer; i++) {
/*
@@ -353,7 +354,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
continue;
origin = ovl_decode_real_fh(fh, ofs->layers[i].mnt,
- connected);
+ connected, nouuid);
if (origin)
break;
}
@@ -414,7 +415,7 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
* Return 0 on match, -ESTALE on mismatch, < 0 on error.
*/
static int ovl_verify_fh(struct dentry *dentry, const char *name,
- const struct ovl_fh *fh)
+ const struct ovl_fh *fh, bool nouuid)
{
struct ovl_fh *ofh = ovl_get_fh(dentry, name);
int err = 0;
@@ -425,8 +426,14 @@ static int ovl_verify_fh(struct dentry *dentry, const char *name,
if (IS_ERR(ofh))
return PTR_ERR(ofh);
- if (fh->fb.len != ofh->fb.len || memcmp(&fh->fb, &ofh->fb, fh->fb.len))
+ if (fh->fb.len != ofh->fb.len) {
err = -ESTALE;
+ } else {
+ if (nouuid && !uuid_equal(&fh->fb.uuid, &ofh->fb.uuid))
+ ofh->fb.uuid = fh->fb.uuid;
+ if (memcmp(&fh->fb, &ofh->fb, fh->fb.len))
+ err = -ESTALE;
+ }
kfree(ofh);
return err;
@@ -441,7 +448,8 @@ static int ovl_verify_fh(struct dentry *dentry, const char *name,
* Return 0 on match, -ESTALE on mismatch, -ENODATA on no xattr, < 0 on error.
*/
int ovl_verify_set_fh(struct dentry *dentry, const char *name,
- struct dentry *real, bool is_upper, bool set)
+ struct dentry *real, bool is_upper, bool set,
+ bool nouuid)
{
struct inode *inode;
struct ovl_fh *fh;
@@ -454,7 +462,7 @@ int ovl_verify_set_fh(struct dentry *dentry, const char *name,
goto fail;
}
- err = ovl_verify_fh(dentry, name, fh);
+ err = ovl_verify_fh(dentry, name, fh, nouuid);
if (set && err == -ENODATA)
err = ovl_do_setxattr(dentry, name, fh->buf, fh->fb.len, 0);
if (err)
@@ -477,6 +485,7 @@ struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
{
struct ovl_fh *fh;
struct dentry *upper;
+ bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
if (!d_is_dir(index))
return dget(index);
@@ -485,7 +494,7 @@ struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
if (IS_ERR_OR_NULL(fh))
return ERR_CAST(fh);
- upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true);
+ upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true, nouuid);
kfree(fh);
if (IS_ERR_OR_NULL(upper))
@@ -514,6 +523,7 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
struct ovl_path *stack = &origin;
struct dentry *upper = NULL;
int err;
+ bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
if (!d_inode(index))
return 0;
@@ -574,7 +584,7 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
goto fail;
}
- err = ovl_verify_fh(upper, OVL_XATTR_ORIGIN, fh);
+ err = ovl_verify_fh(upper, OVL_XATTR_ORIGIN, fh, nouuid);
dput(upper);
if (err)
goto fail;
@@ -690,6 +700,7 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
struct qstr name;
bool is_dir = d_is_dir(origin);
int err;
+ bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
err = ovl_get_index_name(origin, &name);
if (err)
@@ -741,7 +752,7 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
}
/* Verify that dir index 'upper' xattr points to upper dir */
- err = ovl_verify_upper(index, upper, false);
+ err = ovl_verify_upper(index, upper, false, nouuid);
if (err) {
if (err == -ESTALE) {
pr_warn_ratelimited("suspected multiply redirected dir found (upper=%pd2, origin=%pd2, index=%pd2).\n",
@@ -840,6 +851,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
.redirect = NULL,
.metacopy = false,
};
+ bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
if (dentry->d_name.len > ofs->namelen)
return ERR_PTR(-ENAMETOOLONG);
@@ -939,7 +951,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
if (upperdentry && !ctr &&
((d.is_dir && ovl_verify_lower(dentry->d_sb)) ||
(!d.is_dir && ofs->config.index && origin_path))) {
- err = ovl_verify_origin(upperdentry, this, false);
+ err = ovl_verify_origin(upperdentry, this, false,
+ nouuid);
if (err) {
dput(this);
if (d.is_dir)
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 29bc1ec699e7..2aacfa42bb12 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -18,6 +18,12 @@ enum ovl_path_type {
__OVL_PATH_ORIGIN = (1 << 2),
};
+enum ovl_index_type {
+ OVL_INDEX_OFF = 0,
+ OVL_INDEX_ON,
+ OVL_INDEX_NOUUID,
+};
+
#define OVL_TYPE_UPPER(type) ((type) & __OVL_PATH_UPPER)
#define OVL_TYPE_MERGE(type) ((type) & __OVL_PATH_MERGE)
#define OVL_TYPE_ORIGIN(type) ((type) & __OVL_PATH_ORIGIN)
@@ -362,11 +368,12 @@ static inline int ovl_check_fh_len(struct ovl_fh *fh, int fh_len)
}
struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
- bool connected);
+ bool connected, bool nouuid);
int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
struct dentry *upperdentry, struct ovl_path **stackp);
int ovl_verify_set_fh(struct dentry *dentry, const char *name,
- struct dentry *real, bool is_upper, bool set);
+ struct dentry *real, bool is_upper, bool set,
+ bool nouuid);
struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index);
int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index);
int ovl_get_index_name(struct dentry *origin, struct qstr *name);
@@ -379,15 +386,19 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
bool ovl_lower_positive(struct dentry *dentry);
static inline int ovl_verify_origin(struct dentry *upper,
- struct dentry *origin, bool set)
+ struct dentry *origin, bool set,
+ bool nouuid)
{
- return ovl_verify_set_fh(upper, OVL_XATTR_ORIGIN, origin, false, set);
+ return ovl_verify_set_fh(upper, OVL_XATTR_ORIGIN, origin, false, set,
+ nouuid);
}
static inline int ovl_verify_upper(struct dentry *index,
- struct dentry *upper, bool set)
+ struct dentry *upper, bool set,
+ bool nouuid)
{
- return ovl_verify_set_fh(index, OVL_XATTR_UPPER, upper, true, set);
+ return ovl_verify_set_fh(index, OVL_XATTR_UPPER, upper, true, set,
+ nouuid);
}
/* readdir.c */
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index b429c80879ee..2fd2cc515ad2 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -13,7 +13,7 @@ struct ovl_config {
bool redirect_dir;
bool redirect_follow;
const char *redirect_mode;
- bool index;
+ int index;
bool nfs_export;
int xino;
bool metacopy;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 4b38141c2985..d1ed580f24b7 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -38,10 +38,12 @@ module_param_named(redirect_always_follow, ovl_redirect_always_follow,
MODULE_PARM_DESC(redirect_always_follow,
"Follow redirects even if redirect_dir feature is turned off");
-static bool ovl_index_def = IS_ENABLED(CONFIG_OVERLAY_FS_INDEX);
-module_param_named(index, ovl_index_def, bool, 0644);
+static int ovl_index_def = IS_ENABLED(CONFIG_OVERLAY_FS_INDEX_NOUUID) ?
+ OVL_INDEX_NOUUID :
+ IS_ENABLED(CONFIG_OVERLAY_FS_INDEX);
+module_param_named(index, ovl_index_def, int, 0644);
MODULE_PARM_DESC(index,
- "Default to on or off for the inodes index feature");
+ "Default to on, off or nouuid for the inodes index feature");
static bool ovl_nfs_export_def = IS_ENABLED(CONFIG_OVERLAY_FS_NFS_EXPORT);
module_param_named(nfs_export, ovl_nfs_export_def, bool, 0644);
@@ -352,8 +354,18 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
seq_puts(m, ",default_permissions");
if (strcmp(ofs->config.redirect_mode, ovl_redirect_mode_def()) != 0)
seq_printf(m, ",redirect_dir=%s", ofs->config.redirect_mode);
- if (ofs->config.index != ovl_index_def)
- seq_printf(m, ",index=%s", ofs->config.index ? "on" : "off");
+ if (ofs->config.index != ovl_index_def) {
+ switch (ofs->config.index) {
+ case OVL_INDEX_OFF:
+ seq_puts(m, ",index=off");
+ break;
+ case OVL_INDEX_NOUUID:
+ seq_puts(m, ",index=nouuid");
+ break;
+ default:
+ seq_puts(m, ",index=on");
+ }
+ }
if (ofs->config.nfs_export != ovl_nfs_export_def)
seq_printf(m, ",nfs_export=%s", ofs->config.nfs_export ?
"on" : "off");
@@ -404,6 +416,7 @@ enum {
OPT_REDIRECT_DIR,
OPT_INDEX_ON,
OPT_INDEX_OFF,
+ OPT_INDEX_NOUUID,
OPT_NFS_EXPORT_ON,
OPT_NFS_EXPORT_OFF,
OPT_XINO_ON,
@@ -422,6 +435,7 @@ static const match_table_t ovl_tokens = {
{OPT_REDIRECT_DIR, "redirect_dir=%s"},
{OPT_INDEX_ON, "index=on"},
{OPT_INDEX_OFF, "index=off"},
+ {OPT_INDEX_NOUUID, "index=nouuid"},
{OPT_NFS_EXPORT_ON, "nfs_export=on"},
{OPT_NFS_EXPORT_OFF, "nfs_export=off"},
{OPT_XINO_ON, "xino=on"},
@@ -532,12 +546,17 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
break;
case OPT_INDEX_ON:
- config->index = true;
+ config->index = OVL_INDEX_ON;
index_opt = true;
break;
case OPT_INDEX_OFF:
- config->index = false;
+ config->index = OVL_INDEX_OFF;
+ index_opt = true;
+ break;
+
+ case OPT_INDEX_NOUUID:
+ config->index = OVL_INDEX_NOUUID;
index_opt = true;
break;
@@ -592,7 +611,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
pr_info("option \"index=on\" is useless in a non-upper mount, ignore\n");
index_opt = false;
}
- config->index = false;
+ config->index = OVL_INDEX_OFF;
}
err = ovl_parse_redirect_mode(config, config->redirect_mode);
@@ -644,7 +663,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
config->nfs_export = false;
} else {
/* Automatically enable index otherwise. */
- config->index = true;
+ config->index = OVL_INDEX_ON;
}
}
@@ -859,7 +878,7 @@ static int ovl_lower_dir(const char *name, struct path *path,
fh_type = ovl_can_decode_fh(path->dentry->d_sb);
if ((ofs->config.nfs_export ||
(ofs->config.index && ofs->config.upperdir)) && !fh_type) {
- ofs->config.index = false;
+ ofs->config.index = OVL_INDEX_OFF;
ofs->config.nfs_export = false;
pr_warn("fs on '%s' does not support file handles, falling back to index=off,nfs_export=off.\n",
name);
@@ -1259,7 +1278,7 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
err = ovl_do_setxattr(ofs->workdir, OVL_XATTR_OPAQUE, "0", 1, 0);
if (err) {
ofs->noxattr = true;
- ofs->config.index = false;
+ ofs->config.index = OVL_INDEX_OFF;
ofs->config.metacopy = false;
pr_warn("upper fs does not support xattr, falling back to index=off and metacopy=off.\n");
err = 0;
@@ -1282,7 +1301,7 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
/* Check if upper/work fs supports file handles */
fh_type = ovl_can_decode_fh(ofs->workdir->d_sb);
if (ofs->config.index && !fh_type) {
- ofs->config.index = false;
+ ofs->config.index = OVL_INDEX_OFF;
pr_warn("upper fs does not support file handles, falling back to index=off.\n");
}
@@ -1348,6 +1367,7 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
{
struct vfsmount *mnt = ovl_upper_mnt(ofs);
int err;
+ bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
err = mnt_want_write(mnt);
if (err)
@@ -1355,7 +1375,7 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
/* Verify lower root is upper root origin */
err = ovl_verify_origin(upperpath->dentry, oe->lowerstack[0].dentry,
- true);
+ true, nouuid);
if (err) {
pr_err("failed to verify upper root origin\n");
goto out;
@@ -1385,11 +1405,13 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
*/
if (ovl_check_origin_xattr(ofs->indexdir)) {
err = ovl_verify_set_fh(ofs->indexdir, OVL_XATTR_ORIGIN,
- upperpath->dentry, true, false);
+ upperpath->dentry, true, false,
+ nouuid);
if (err)
pr_err("failed to verify index dir 'origin' xattr\n");
}
- err = ovl_verify_upper(ofs->indexdir, upperpath->dentry, true);
+ err = ovl_verify_upper(ofs->indexdir, upperpath->dentry, true,
+ nouuid);
if (err)
pr_err("failed to verify index dir 'upper' xattr\n");
@@ -1458,7 +1480,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
if (!ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
bad_uuid = true;
if (ofs->config.index || ofs->config.nfs_export) {
- ofs->config.index = false;
+ ofs->config.index = OVL_INDEX_OFF;
ofs->config.nfs_export = false;
pr_warn("%s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
uuid_is_null(&sb->s_uuid) ? "null" :
@@ -1889,9 +1911,14 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
if (err)
goto out_free_oe;
+ if (ofs->config.index == OVL_INDEX_NOUUID && ofs->numfs > 1) {
+ pr_warn("The index=nouuid requires a single fs for lower and upper, falling back to index=on.\n");
+ ofs->config.index = OVL_INDEX_ON;
+ }
+
/* Show index=off in /proc/mounts for forced r/o mount */
if (!ofs->indexdir) {
- ofs->config.index = false;
+ ofs->config.index = OVL_INDEX_OFF;
if (ovl_upper_mnt(ofs) && ofs->config.nfs_export) {
pr_warn("NFS export requires an index dir, falling back to nfs_export=off.\n");
ofs->config.nfs_export = false;
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature
2020-09-23 15:23 [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature Pavel Tikhomirov
@ 2020-09-23 16:09 ` Amir Goldstein
2020-09-23 16:36 ` Amir Goldstein
2020-09-24 8:40 ` Pavel Tikhomirov
2020-09-23 19:47 ` Vivek Goyal
1 sibling, 2 replies; 10+ messages in thread
From: Amir Goldstein @ 2020-09-23 16:09 UTC (permalink / raw)
To: Pavel Tikhomirov; +Cc: Miklos Szeredi, Vivek Goyal, overlayfs, linux-kernel
On Wed, Sep 23, 2020 at 6:23 PM Pavel Tikhomirov
<ptikhomirov@virtuozzo.com> wrote:
>
> This relaxes uuid checks for overlay index feature. It is only possible
> in case there is only one filesystem for all the work/upper/lower
> directories and bare file handles from this backing filesystem are uniq.
> In case we have multiple filesystems here just fall back to normal
> "index=on".
>
> This is needed when overlayfs is/was mounted in a container with
> index enabled (e.g.: to be able to resolve inotify watch file handles on
> it to paths in CRIU), and this container is copied and started alongside
> with the original one. This way the "copy" container can't have the same
> uuid on the superblock and mounting the overlayfs from it later would
> fail.
>
> That is an example of the problem on top of loop+ext4:
>
> dd if=/dev/zero of=loopbackfile.img bs=100M count=10
> losetup -fP loopbackfile.img
> losetup -a
> #/dev/loop0: [64768]:35 (/loop-test/loopbackfile.img)
> mkfs.ext4 loopbackfile.img
> mkdir loop-mp
> mount -o loop /dev/loop0 loop-mp
> mkdir loop-mp/{lower,upper,work,merged}
> mount -t overlay overlay -oindex=on,lowerdir=loop-mp/lower,\
> upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
> umount loop-mp/merged
> umount loop-mp
> e2fsck -f /dev/loop0
> tune2fs -U random /dev/loop0
>
> mount -o loop /dev/loop0 loop-mp
> mount -t overlay overlay -oindex=on,lowerdir=loop-mp/lower,\
> upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
> #mount: /loop-test/loop-mp/merged:
> #mount(2) system call failed: Stale file handle.
>
> mount -t overlay overlay -oindex=nouuid,lowerdir=loop-mp/lower,\
> upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
>
> If you just change the uuid of the backing filesystem, overlay is not
> mounting any more. In Virtuozzo we copy container disks (ploops) when
> crate the copy of container and we require fs uuid to be uniq for a new
> container.
>
> v2: in v1 I missed actual uuid check skip - add it
>
> CC: Amir Goldstein <amir73il@gmail.com>
> CC: Vivek Goyal <vgoyal@redhat.com>
> CC: Miklos Szeredi <miklos@szeredi.hu>
> CC: linux-unionfs@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ---
Look reasonable, but you need to rebase over
git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
Which makes a lot of your work unneeded.
ofs is propagated to most of the relevant helpers
and you should propagate it down to ovl_decode_real_fh().
Some minor comments below...
> fs/overlayfs/Kconfig | 16 +++++++++++
> fs/overlayfs/export.c | 6 ++--
> fs/overlayfs/namei.c | 35 +++++++++++++++--------
> fs/overlayfs/overlayfs.h | 23 +++++++++++----
> fs/overlayfs/ovl_entry.h | 2 +-
> fs/overlayfs/super.c | 61 +++++++++++++++++++++++++++++-----------
> 6 files changed, 106 insertions(+), 37 deletions(-)
>
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index dd188c7996b3..b00fd44006f9 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -61,6 +61,22 @@ config OVERLAY_FS_INDEX
>
> If unsure, say N.
>
> +config OVERLAY_FS_INDEX_NOUUID
> + bool "Overlayfs: relax uuid checks of inodes index feature"
> + depends on OVERLAY_FS
> + depends on OVERLAY_FS_INDEX
> + help
> + If this config option is enabled then overlay will skip uuid checks
> + for index lower to upper inode map, this only can be done if all
> + upper and lower directories are on the same filesystem where basic
> + fhandles are uniq.
> +
> + It is needed to overcome possible change of uuid on superblock of the
> + backing filesystem, e.g. when you copied the virtual disk and mount
> + both the copy of the disk and the original one at the same time.
> +
> + If unsure, say N.
> +
Please do not add a config option for this.
Isn't a mount option sufficient for your needs?
> config OVERLAY_FS_NFS_EXPORT
> bool "Overlayfs: turn on NFS export feature by default"
> depends on OVERLAY_FS
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 0e696f72cf65..d53feb7547d9 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -676,11 +676,12 @@ static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
> struct ovl_fs *ofs = sb->s_fs_info;
> struct dentry *dentry;
> struct dentry *upper;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> if (!ovl_upper_mnt(ofs))
> return ERR_PTR(-EACCES);
>
> - upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true);
> + upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true, nouuid);
> if (IS_ERR_OR_NULL(upper))
> return upper;
>
> @@ -700,6 +701,7 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
> struct dentry *index = NULL;
> struct inode *inode;
> int err;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> /* First lookup overlay inode in inode cache by origin fh */
> err = ovl_check_origin_fh(ofs, fh, false, NULL, &stack);
> @@ -752,7 +754,7 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
> goto out_err;
> }
> if (index) {
> - err = ovl_verify_origin(index, origin.dentry, false);
> + err = ovl_verify_origin(index, origin.dentry, false, nouuid);
> if (err)
> goto out_err;
> }
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index f7d4358db637..676170d719f8 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -149,7 +149,7 @@ static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
> }
>
> struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
> - bool connected)
> + bool connected, bool nouuid)
> {
> struct dentry *real;
> int bytes;
> @@ -158,7 +158,7 @@ struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
> * Make sure that the stored uuid matches the uuid of the lower
> * layer where file handle will be decoded.
> */
> - if (!uuid_equal(&fh->fb.uuid, &mnt->mnt_sb->s_uuid))
> + if (!nouuid && !uuid_equal(&fh->fb.uuid, &mnt->mnt_sb->s_uuid))
> return NULL;
>
> bytes = (fh->fb.len - offsetof(struct ovl_fb, fid));
> @@ -342,6 +342,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
> {
> struct dentry *origin = NULL;
> int i;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> for (i = 1; i < ofs->numlayer; i++) {
> /*
> @@ -353,7 +354,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
> continue;
>
> origin = ovl_decode_real_fh(fh, ofs->layers[i].mnt,
> - connected);
> + connected, nouuid);
> if (origin)
> break;
> }
> @@ -414,7 +415,7 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
> * Return 0 on match, -ESTALE on mismatch, < 0 on error.
> */
> static int ovl_verify_fh(struct dentry *dentry, const char *name,
> - const struct ovl_fh *fh)
> + const struct ovl_fh *fh, bool nouuid)
> {
> struct ovl_fh *ofh = ovl_get_fh(dentry, name);
> int err = 0;
> @@ -425,8 +426,14 @@ static int ovl_verify_fh(struct dentry *dentry, const char *name,
> if (IS_ERR(ofh))
> return PTR_ERR(ofh);
>
> - if (fh->fb.len != ofh->fb.len || memcmp(&fh->fb, &ofh->fb, fh->fb.len))
> + if (fh->fb.len != ofh->fb.len) {
> err = -ESTALE;
> + } else {
> + if (nouuid && !uuid_equal(&fh->fb.uuid, &ofh->fb.uuid))
> + ofh->fb.uuid = fh->fb.uuid;
> + if (memcmp(&fh->fb, &ofh->fb, fh->fb.len))
> + err = -ESTALE;
> + }
>
> kfree(ofh);
> return err;
> @@ -441,7 +448,8 @@ static int ovl_verify_fh(struct dentry *dentry, const char *name,
> * Return 0 on match, -ESTALE on mismatch, -ENODATA on no xattr, < 0 on error.
> */
> int ovl_verify_set_fh(struct dentry *dentry, const char *name,
> - struct dentry *real, bool is_upper, bool set)
> + struct dentry *real, bool is_upper, bool set,
> + bool nouuid)
> {
> struct inode *inode;
> struct ovl_fh *fh;
> @@ -454,7 +462,7 @@ int ovl_verify_set_fh(struct dentry *dentry, const char *name,
> goto fail;
> }
>
> - err = ovl_verify_fh(dentry, name, fh);
> + err = ovl_verify_fh(dentry, name, fh, nouuid);
> if (set && err == -ENODATA)
> err = ovl_do_setxattr(dentry, name, fh->buf, fh->fb.len, 0);
> if (err)
> @@ -477,6 +485,7 @@ struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
> {
> struct ovl_fh *fh;
> struct dentry *upper;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> if (!d_is_dir(index))
> return dget(index);
> @@ -485,7 +494,7 @@ struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
> if (IS_ERR_OR_NULL(fh))
> return ERR_CAST(fh);
>
> - upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true);
> + upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true, nouuid);
> kfree(fh);
>
> if (IS_ERR_OR_NULL(upper))
> @@ -514,6 +523,7 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
> struct ovl_path *stack = &origin;
> struct dentry *upper = NULL;
> int err;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> if (!d_inode(index))
> return 0;
> @@ -574,7 +584,7 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
> goto fail;
> }
>
> - err = ovl_verify_fh(upper, OVL_XATTR_ORIGIN, fh);
> + err = ovl_verify_fh(upper, OVL_XATTR_ORIGIN, fh, nouuid);
> dput(upper);
> if (err)
> goto fail;
> @@ -690,6 +700,7 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
> struct qstr name;
> bool is_dir = d_is_dir(origin);
> int err;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> err = ovl_get_index_name(origin, &name);
> if (err)
> @@ -741,7 +752,7 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
> }
>
> /* Verify that dir index 'upper' xattr points to upper dir */
> - err = ovl_verify_upper(index, upper, false);
> + err = ovl_verify_upper(index, upper, false, nouuid);
> if (err) {
> if (err == -ESTALE) {
> pr_warn_ratelimited("suspected multiply redirected dir found (upper=%pd2, origin=%pd2, index=%pd2).\n",
> @@ -840,6 +851,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> .redirect = NULL,
> .metacopy = false,
> };
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> if (dentry->d_name.len > ofs->namelen)
> return ERR_PTR(-ENAMETOOLONG);
> @@ -939,7 +951,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> if (upperdentry && !ctr &&
> ((d.is_dir && ovl_verify_lower(dentry->d_sb)) ||
> (!d.is_dir && ofs->config.index && origin_path))) {
> - err = ovl_verify_origin(upperdentry, this, false);
> + err = ovl_verify_origin(upperdentry, this, false,
> + nouuid);
> if (err) {
> dput(this);
> if (d.is_dir)
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 29bc1ec699e7..2aacfa42bb12 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -18,6 +18,12 @@ enum ovl_path_type {
> __OVL_PATH_ORIGIN = (1 << 2),
> };
>
> +enum ovl_index_type {
> + OVL_INDEX_OFF = 0,
> + OVL_INDEX_ON,
> + OVL_INDEX_NOUUID,
> +};
> +
> #define OVL_TYPE_UPPER(type) ((type) & __OVL_PATH_UPPER)
> #define OVL_TYPE_MERGE(type) ((type) & __OVL_PATH_MERGE)
> #define OVL_TYPE_ORIGIN(type) ((type) & __OVL_PATH_ORIGIN)
> @@ -362,11 +368,12 @@ static inline int ovl_check_fh_len(struct ovl_fh *fh, int fh_len)
> }
>
> struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
> - bool connected);
> + bool connected, bool nouuid);
> int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
> struct dentry *upperdentry, struct ovl_path **stackp);
> int ovl_verify_set_fh(struct dentry *dentry, const char *name,
> - struct dentry *real, bool is_upper, bool set);
> + struct dentry *real, bool is_upper, bool set,
> + bool nouuid);
> struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index);
> int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index);
> int ovl_get_index_name(struct dentry *origin, struct qstr *name);
> @@ -379,15 +386,19 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> bool ovl_lower_positive(struct dentry *dentry);
>
> static inline int ovl_verify_origin(struct dentry *upper,
> - struct dentry *origin, bool set)
> + struct dentry *origin, bool set,
> + bool nouuid)
> {
> - return ovl_verify_set_fh(upper, OVL_XATTR_ORIGIN, origin, false, set);
> + return ovl_verify_set_fh(upper, OVL_XATTR_ORIGIN, origin, false, set,
> + nouuid);
> }
>
> static inline int ovl_verify_upper(struct dentry *index,
> - struct dentry *upper, bool set)
> + struct dentry *upper, bool set,
> + bool nouuid)
> {
> - return ovl_verify_set_fh(index, OVL_XATTR_UPPER, upper, true, set);
> + return ovl_verify_set_fh(index, OVL_XATTR_UPPER, upper, true, set,
> + nouuid);
> }
>
> /* readdir.c */
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index b429c80879ee..2fd2cc515ad2 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -13,7 +13,7 @@ struct ovl_config {
> bool redirect_dir;
> bool redirect_follow;
> const char *redirect_mode;
> - bool index;
> + int index;
> bool nfs_export;
> int xino;
> bool metacopy;
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 4b38141c2985..d1ed580f24b7 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -38,10 +38,12 @@ module_param_named(redirect_always_follow, ovl_redirect_always_follow,
> MODULE_PARM_DESC(redirect_always_follow,
> "Follow redirects even if redirect_dir feature is turned off");
>
> -static bool ovl_index_def = IS_ENABLED(CONFIG_OVERLAY_FS_INDEX);
> -module_param_named(index, ovl_index_def, bool, 0644);
> +static int ovl_index_def = IS_ENABLED(CONFIG_OVERLAY_FS_INDEX_NOUUID) ?
> + OVL_INDEX_NOUUID :
> + IS_ENABLED(CONFIG_OVERLAY_FS_INDEX);
> +module_param_named(index, ovl_index_def, int, 0644);
> MODULE_PARM_DESC(index,
> - "Default to on or off for the inodes index feature");
> + "Default to on, off or nouuid for the inodes index feature");
No, we don't want to change the module param from bool to int.
Scripts are expecting N/Y module param value (unionmount tests and xfstests).
And I don't see the reason to define nouuid in module param at all.
>
> static bool ovl_nfs_export_def = IS_ENABLED(CONFIG_OVERLAY_FS_NFS_EXPORT);
> module_param_named(nfs_export, ovl_nfs_export_def, bool, 0644);
> @@ -352,8 +354,18 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
> seq_puts(m, ",default_permissions");
> if (strcmp(ofs->config.redirect_mode, ovl_redirect_mode_def()) != 0)
> seq_printf(m, ",redirect_dir=%s", ofs->config.redirect_mode);
> - if (ofs->config.index != ovl_index_def)
> - seq_printf(m, ",index=%s", ofs->config.index ? "on" : "off");
> + if (ofs->config.index != ovl_index_def) {
> + switch (ofs->config.index) {
> + case OVL_INDEX_OFF:
> + seq_puts(m, ",index=off");
> + break;
> + case OVL_INDEX_NOUUID:
> + seq_puts(m, ",index=nouuid");
> + break;
> + default:
> + seq_puts(m, ",index=on");
> + }
> + }
See ovl_xino_str[] please.
> if (ofs->coovl_xino_strnfig.nfs_export != ovl_nfs_export_def)
> seq_printf(m, ",nfs_export=%s", ofs->config.nfs_export ?
> "on" : "off");
> @@ -404,6 +416,7 @@ enum {
> OPT_REDIRECT_DIR,
> OPT_INDEX_ON,
> OPT_INDEX_OFF,
> + OPT_INDEX_NOUUID,
> OPT_NFS_EXPORT_ON,
> OPT_NFS_EXPORT_OFF,
> OPT_XINO_ON,
> @@ -422,6 +435,7 @@ static const match_table_t ovl_tokens = {
> {OPT_REDIRECT_DIR, "redirect_dir=%s"},
> {OPT_INDEX_ON, "index=on"},
> {OPT_INDEX_OFF, "index=off"},
> + {OPT_INDEX_NOUUID, "index=nouuid"},
> {OPT_NFS_EXPORT_ON, "nfs_export=on"},
> {OPT_NFS_EXPORT_OFF, "nfs_export=off"},
> {OPT_XINO_ON, "xino=on"},
> @@ -532,12 +546,17 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> break;
>
> case OPT_INDEX_ON:
> - config->index = true;
> + config->index = OVL_INDEX_ON;
> index_opt = true;
> break;
>
> case OPT_INDEX_OFF:
> - config->index = false;
> + config->index = OVL_INDEX_OFF;
> + index_opt = true;
> + break;
> +
> + case OPT_INDEX_NOUUID:
> + config->index = OVL_INDEX_NOUUID;
> index_opt = true;
> break;
>
> @@ -592,7 +611,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> pr_info("option \"index=on\" is useless in a non-upper mount, ignore\n");
> index_opt = false;
> }
> - config->index = false;
> + config->index = OVL_INDEX_OFF;
> }
>
> err = ovl_parse_redirect_mode(config, config->redirect_mode);
> @@ -644,7 +663,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> config->nfs_export = false;
> } else {
> /* Automatically enable index otherwise. */
> - config->index = true;
> + config->index = OVL_INDEX_ON;
> }
> }
>
> @@ -859,7 +878,7 @@ static int ovl_lower_dir(const char *name, struct path *path,
> fh_type = ovl_can_decode_fh(path->dentry->d_sb);
> if ((ofs->config.nfs_export ||
> (ofs->config.index && ofs->config.upperdir)) && !fh_type) {
> - ofs->config.index = false;
> + ofs->config.index = OVL_INDEX_OFF;
> ofs->config.nfs_export = false;
> pr_warn("fs on '%s' does not support file handles, falling back to index=off,nfs_export=off.\n",
> name);
> @@ -1259,7 +1278,7 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
> err = ovl_do_setxattr(ofs->workdir, OVL_XATTR_OPAQUE, "0", 1, 0);
> if (err) {
> ofs->noxattr = true;
> - ofs->config.index = false;
> + ofs->config.index = OVL_INDEX_OFF;
> ofs->config.metacopy = false;
> pr_warn("upper fs does not support xattr, falling back to index=off and metacopy=off.\n");
> err = 0;
> @@ -1282,7 +1301,7 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
> /* Check if upper/work fs supports file handles */
> fh_type = ovl_can_decode_fh(ofs->workdir->d_sb);
> if (ofs->config.index && !fh_type) {
> - ofs->config.index = false;
> + ofs->config.index = OVL_INDEX_OFF;
> pr_warn("upper fs does not support file handles, falling back to index=off.\n");
> }
>
> @@ -1348,6 +1367,7 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
> {
> struct vfsmount *mnt = ovl_upper_mnt(ofs);
> int err;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> err = mnt_want_write(mnt);
> if (err)
> @@ -1355,7 +1375,7 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
>
> /* Verify lower root is upper root origin */
> err = ovl_verify_origin(upperpath->dentry, oe->lowerstack[0].dentry,
> - true);
> + true, nouuid);
> if (err) {
> pr_err("failed to verify upper root origin\n");
> goto out;
> @@ -1385,11 +1405,13 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
> */
> if (ovl_check_origin_xattr(ofs->indexdir)) {
> err = ovl_verify_set_fh(ofs->indexdir, OVL_XATTR_ORIGIN,
> - upperpath->dentry, true, false);
> + upperpath->dentry, true, false,
> + nouuid);
> if (err)
> pr_err("failed to verify index dir 'origin' xattr\n");
> }
> - err = ovl_verify_upper(ofs->indexdir, upperpath->dentry, true);
> + err = ovl_verify_upper(ofs->indexdir, upperpath->dentry, true,
> + nouuid);
> if (err)
> pr_err("failed to verify index dir 'upper' xattr\n");
>
> @@ -1458,7 +1480,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
> if (!ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
> bad_uuid = true;
> if (ofs->config.index || ofs->config.nfs_export) {
> - ofs->config.index = false;
> + ofs->config.index = OVL_INDEX_OFF;
> ofs->config.nfs_export = false;
> pr_warn("%s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
> uuid_is_null(&sb->s_uuid) ? "null" :
> @@ -1889,9 +1911,14 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> if (err)
> goto out_free_oe;
>
> + if (ofs->config.index == OVL_INDEX_NOUUID && ofs->numfs > 1) {
> + pr_warn("The index=nouuid requires a single fs for lower and upper, falling back to index=on.\n");
> + ofs->config.index = OVL_INDEX_ON;
> + }
> +
It's too late for this fallback now, you already did relaxed ovl_verify_origin()
and now we will continue as if all is ok.
Please fail the mount in this case.
I don't think that users that specifically requested index=nouuid would care to
fallback to index=on.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature
2020-09-23 16:09 ` Amir Goldstein
@ 2020-09-23 16:36 ` Amir Goldstein
2020-09-24 7:40 ` Pavel Tikhomirov
2020-09-24 8:40 ` Pavel Tikhomirov
1 sibling, 1 reply; 10+ messages in thread
From: Amir Goldstein @ 2020-09-23 16:36 UTC (permalink / raw)
To: Pavel Tikhomirov; +Cc: Miklos Szeredi, Vivek Goyal, overlayfs, linux-kernel
> > @@ -414,7 +415,7 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
> > * Return 0 on match, -ESTALE on mismatch, < 0 on error.
> > */
> > static int ovl_verify_fh(struct dentry *dentry, const char *name,
> > - const struct ovl_fh *fh)
> > + const struct ovl_fh *fh, bool nouuid)
> > {
> > struct ovl_fh *ofh = ovl_get_fh(dentry, name);
> > int err = 0;
> > @@ -425,8 +426,14 @@ static int ovl_verify_fh(struct dentry *dentry, const char *name,
> > if (IS_ERR(ofh))
> > return PTR_ERR(ofh);
> >
> > - if (fh->fb.len != ofh->fb.len || memcmp(&fh->fb, &ofh->fb, fh->fb.len))
> > + if (fh->fb.len != ofh->fb.len) {
> > err = -ESTALE;
> > + } else {
> > + if (nouuid && !uuid_equal(&fh->fb.uuid, &ofh->fb.uuid))
> > + ofh->fb.uuid = fh->fb.uuid;
> > + if (memcmp(&fh->fb, &ofh->fb, fh->fb.len))
> > + err = -ESTALE;
> > + }
> >
On second thought I am wondering if we should do that differently.
If users want to work with index=nouuid, they need to work with it from day 1.
index=nouuid should export null uuid in NFS handles and write null uuid
in trusted.overlay.origin xattr.
So in ovl_encode_real_fh() you set null uuid and
instead of relaxing uuid_equal() in ovl_decode_real_fh()
you change it to uuid_is_null().
Do you have a problem with that for Virtuozzo use case?
Thanks,
Amir.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature
2020-09-23 15:23 [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature Pavel Tikhomirov
2020-09-23 16:09 ` Amir Goldstein
@ 2020-09-23 19:47 ` Vivek Goyal
2020-09-24 2:44 ` Amir Goldstein
1 sibling, 1 reply; 10+ messages in thread
From: Vivek Goyal @ 2020-09-23 19:47 UTC (permalink / raw)
To: Pavel Tikhomirov
Cc: Miklos Szeredi, Amir Goldstein, linux-unionfs, linux-kernel
On Wed, Sep 23, 2020 at 06:23:08PM +0300, Pavel Tikhomirov wrote:
> This relaxes uuid checks for overlay index feature. It is only possible
> in case there is only one filesystem for all the work/upper/lower
> directories and bare file handles from this backing filesystem are uniq.
Hi Pavel,
Wondering why upper/work has to be on same filesystem as lower for this to
work?
> In case we have multiple filesystems here just fall back to normal
> "index=on".
>
> This is needed when overlayfs is/was mounted in a container with
> index enabled (e.g.: to be able to resolve inotify watch file handles on
> it to paths in CRIU), and this container is copied and started alongside
> with the original one. This way the "copy" container can't have the same
> uuid on the superblock and mounting the overlayfs from it later would
> fail.
>
> That is an example of the problem on top of loop+ext4:
>
> dd if=/dev/zero of=loopbackfile.img bs=100M count=10
> losetup -fP loopbackfile.img
> losetup -a
> #/dev/loop0: [64768]:35 (/loop-test/loopbackfile.img)
> mkfs.ext4 loopbackfile.img
> mkdir loop-mp
> mount -o loop /dev/loop0 loop-mp
> mkdir loop-mp/{lower,upper,work,merged}
> mount -t overlay overlay -oindex=on,lowerdir=loop-mp/lower,\
> upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
> umount loop-mp/merged
> umount loop-mp
> e2fsck -f /dev/loop0
> tune2fs -U random /dev/loop0
>
> mount -o loop /dev/loop0 loop-mp
> mount -t overlay overlay -oindex=on,lowerdir=loop-mp/lower,\
> upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
> #mount: /loop-test/loop-mp/merged:
> #mount(2) system call failed: Stale file handle.
>
> mount -t overlay overlay -oindex=nouuid,lowerdir=loop-mp/lower,\
> upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
>
> If you just change the uuid of the backing filesystem, overlay is not
> mounting any more. In Virtuozzo we copy container disks (ploops) when
> crate the copy of container and we require fs uuid to be uniq for a new
> container.
>
> v2: in v1 I missed actual uuid check skip - add it
>
> CC: Amir Goldstein <amir73il@gmail.com>
> CC: Vivek Goyal <vgoyal@redhat.com>
> CC: Miklos Szeredi <miklos@szeredi.hu>
> CC: linux-unionfs@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ---
> fs/overlayfs/Kconfig | 16 +++++++++++
> fs/overlayfs/export.c | 6 ++--
> fs/overlayfs/namei.c | 35 +++++++++++++++--------
> fs/overlayfs/overlayfs.h | 23 +++++++++++----
> fs/overlayfs/ovl_entry.h | 2 +-
> fs/overlayfs/super.c | 61 +++++++++++++++++++++++++++++-----------
> 6 files changed, 106 insertions(+), 37 deletions(-)
Please put something in Documentation file to explain this option. Also
explain where it is safe to use and where it is not. I am concerned
about the case where recreating lower can result in file handle resolving
to a different file (as discusse in other thread). I think it would
be nice if we mention it explicitly.
Thanks
Vivek
>
> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> index dd188c7996b3..b00fd44006f9 100644
> --- a/fs/overlayfs/Kconfig
> +++ b/fs/overlayfs/Kconfig
> @@ -61,6 +61,22 @@ config OVERLAY_FS_INDEX
>
> If unsure, say N.
>
> +config OVERLAY_FS_INDEX_NOUUID
> + bool "Overlayfs: relax uuid checks of inodes index feature"
> + depends on OVERLAY_FS
> + depends on OVERLAY_FS_INDEX
> + help
> + If this config option is enabled then overlay will skip uuid checks
> + for index lower to upper inode map, this only can be done if all
> + upper and lower directories are on the same filesystem where basic
> + fhandles are uniq.
> +
> + It is needed to overcome possible change of uuid on superblock of the
> + backing filesystem, e.g. when you copied the virtual disk and mount
> + both the copy of the disk and the original one at the same time.
> +
> + If unsure, say N.
> +
> config OVERLAY_FS_NFS_EXPORT
> bool "Overlayfs: turn on NFS export feature by default"
> depends on OVERLAY_FS
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 0e696f72cf65..d53feb7547d9 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -676,11 +676,12 @@ static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
> struct ovl_fs *ofs = sb->s_fs_info;
> struct dentry *dentry;
> struct dentry *upper;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> if (!ovl_upper_mnt(ofs))
> return ERR_PTR(-EACCES);
>
> - upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true);
> + upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true, nouuid);
> if (IS_ERR_OR_NULL(upper))
> return upper;
>
> @@ -700,6 +701,7 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
> struct dentry *index = NULL;
> struct inode *inode;
> int err;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> /* First lookup overlay inode in inode cache by origin fh */
> err = ovl_check_origin_fh(ofs, fh, false, NULL, &stack);
> @@ -752,7 +754,7 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
> goto out_err;
> }
> if (index) {
> - err = ovl_verify_origin(index, origin.dentry, false);
> + err = ovl_verify_origin(index, origin.dentry, false, nouuid);
> if (err)
> goto out_err;
> }
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index f7d4358db637..676170d719f8 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -149,7 +149,7 @@ static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
> }
>
> struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
> - bool connected)
> + bool connected, bool nouuid)
> {
> struct dentry *real;
> int bytes;
> @@ -158,7 +158,7 @@ struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
> * Make sure that the stored uuid matches the uuid of the lower
> * layer where file handle will be decoded.
> */
> - if (!uuid_equal(&fh->fb.uuid, &mnt->mnt_sb->s_uuid))
> + if (!nouuid && !uuid_equal(&fh->fb.uuid, &mnt->mnt_sb->s_uuid))
> return NULL;
>
> bytes = (fh->fb.len - offsetof(struct ovl_fb, fid));
> @@ -342,6 +342,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
> {
> struct dentry *origin = NULL;
> int i;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> for (i = 1; i < ofs->numlayer; i++) {
> /*
> @@ -353,7 +354,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
> continue;
>
> origin = ovl_decode_real_fh(fh, ofs->layers[i].mnt,
> - connected);
> + connected, nouuid);
> if (origin)
> break;
> }
> @@ -414,7 +415,7 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
> * Return 0 on match, -ESTALE on mismatch, < 0 on error.
> */
> static int ovl_verify_fh(struct dentry *dentry, const char *name,
> - const struct ovl_fh *fh)
> + const struct ovl_fh *fh, bool nouuid)
> {
> struct ovl_fh *ofh = ovl_get_fh(dentry, name);
> int err = 0;
> @@ -425,8 +426,14 @@ static int ovl_verify_fh(struct dentry *dentry, const char *name,
> if (IS_ERR(ofh))
> return PTR_ERR(ofh);
>
> - if (fh->fb.len != ofh->fb.len || memcmp(&fh->fb, &ofh->fb, fh->fb.len))
> + if (fh->fb.len != ofh->fb.len) {
> err = -ESTALE;
> + } else {
> + if (nouuid && !uuid_equal(&fh->fb.uuid, &ofh->fb.uuid))
> + ofh->fb.uuid = fh->fb.uuid;
> + if (memcmp(&fh->fb, &ofh->fb, fh->fb.len))
> + err = -ESTALE;
> + }
>
> kfree(ofh);
> return err;
> @@ -441,7 +448,8 @@ static int ovl_verify_fh(struct dentry *dentry, const char *name,
> * Return 0 on match, -ESTALE on mismatch, -ENODATA on no xattr, < 0 on error.
> */
> int ovl_verify_set_fh(struct dentry *dentry, const char *name,
> - struct dentry *real, bool is_upper, bool set)
> + struct dentry *real, bool is_upper, bool set,
> + bool nouuid)
> {
> struct inode *inode;
> struct ovl_fh *fh;
> @@ -454,7 +462,7 @@ int ovl_verify_set_fh(struct dentry *dentry, const char *name,
> goto fail;
> }
>
> - err = ovl_verify_fh(dentry, name, fh);
> + err = ovl_verify_fh(dentry, name, fh, nouuid);
> if (set && err == -ENODATA)
> err = ovl_do_setxattr(dentry, name, fh->buf, fh->fb.len, 0);
> if (err)
> @@ -477,6 +485,7 @@ struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
> {
> struct ovl_fh *fh;
> struct dentry *upper;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> if (!d_is_dir(index))
> return dget(index);
> @@ -485,7 +494,7 @@ struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
> if (IS_ERR_OR_NULL(fh))
> return ERR_CAST(fh);
>
> - upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true);
> + upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true, nouuid);
> kfree(fh);
>
> if (IS_ERR_OR_NULL(upper))
> @@ -514,6 +523,7 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
> struct ovl_path *stack = &origin;
> struct dentry *upper = NULL;
> int err;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> if (!d_inode(index))
> return 0;
> @@ -574,7 +584,7 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
> goto fail;
> }
>
> - err = ovl_verify_fh(upper, OVL_XATTR_ORIGIN, fh);
> + err = ovl_verify_fh(upper, OVL_XATTR_ORIGIN, fh, nouuid);
> dput(upper);
> if (err)
> goto fail;
> @@ -690,6 +700,7 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
> struct qstr name;
> bool is_dir = d_is_dir(origin);
> int err;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> err = ovl_get_index_name(origin, &name);
> if (err)
> @@ -741,7 +752,7 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
> }
>
> /* Verify that dir index 'upper' xattr points to upper dir */
> - err = ovl_verify_upper(index, upper, false);
> + err = ovl_verify_upper(index, upper, false, nouuid);
> if (err) {
> if (err == -ESTALE) {
> pr_warn_ratelimited("suspected multiply redirected dir found (upper=%pd2, origin=%pd2, index=%pd2).\n",
> @@ -840,6 +851,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> .redirect = NULL,
> .metacopy = false,
> };
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> if (dentry->d_name.len > ofs->namelen)
> return ERR_PTR(-ENAMETOOLONG);
> @@ -939,7 +951,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> if (upperdentry && !ctr &&
> ((d.is_dir && ovl_verify_lower(dentry->d_sb)) ||
> (!d.is_dir && ofs->config.index && origin_path))) {
> - err = ovl_verify_origin(upperdentry, this, false);
> + err = ovl_verify_origin(upperdentry, this, false,
> + nouuid);
> if (err) {
> dput(this);
> if (d.is_dir)
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 29bc1ec699e7..2aacfa42bb12 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -18,6 +18,12 @@ enum ovl_path_type {
> __OVL_PATH_ORIGIN = (1 << 2),
> };
>
> +enum ovl_index_type {
> + OVL_INDEX_OFF = 0,
> + OVL_INDEX_ON,
> + OVL_INDEX_NOUUID,
> +};
> +
> #define OVL_TYPE_UPPER(type) ((type) & __OVL_PATH_UPPER)
> #define OVL_TYPE_MERGE(type) ((type) & __OVL_PATH_MERGE)
> #define OVL_TYPE_ORIGIN(type) ((type) & __OVL_PATH_ORIGIN)
> @@ -362,11 +368,12 @@ static inline int ovl_check_fh_len(struct ovl_fh *fh, int fh_len)
> }
>
> struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
> - bool connected);
> + bool connected, bool nouuid);
> int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
> struct dentry *upperdentry, struct ovl_path **stackp);
> int ovl_verify_set_fh(struct dentry *dentry, const char *name,
> - struct dentry *real, bool is_upper, bool set);
> + struct dentry *real, bool is_upper, bool set,
> + bool nouuid);
> struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index);
> int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index);
> int ovl_get_index_name(struct dentry *origin, struct qstr *name);
> @@ -379,15 +386,19 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> bool ovl_lower_positive(struct dentry *dentry);
>
> static inline int ovl_verify_origin(struct dentry *upper,
> - struct dentry *origin, bool set)
> + struct dentry *origin, bool set,
> + bool nouuid)
> {
> - return ovl_verify_set_fh(upper, OVL_XATTR_ORIGIN, origin, false, set);
> + return ovl_verify_set_fh(upper, OVL_XATTR_ORIGIN, origin, false, set,
> + nouuid);
> }
>
> static inline int ovl_verify_upper(struct dentry *index,
> - struct dentry *upper, bool set)
> + struct dentry *upper, bool set,
> + bool nouuid)
> {
> - return ovl_verify_set_fh(index, OVL_XATTR_UPPER, upper, true, set);
> + return ovl_verify_set_fh(index, OVL_XATTR_UPPER, upper, true, set,
> + nouuid);
> }
>
> /* readdir.c */
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index b429c80879ee..2fd2cc515ad2 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -13,7 +13,7 @@ struct ovl_config {
> bool redirect_dir;
> bool redirect_follow;
> const char *redirect_mode;
> - bool index;
> + int index;
> bool nfs_export;
> int xino;
> bool metacopy;
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 4b38141c2985..d1ed580f24b7 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -38,10 +38,12 @@ module_param_named(redirect_always_follow, ovl_redirect_always_follow,
> MODULE_PARM_DESC(redirect_always_follow,
> "Follow redirects even if redirect_dir feature is turned off");
>
> -static bool ovl_index_def = IS_ENABLED(CONFIG_OVERLAY_FS_INDEX);
> -module_param_named(index, ovl_index_def, bool, 0644);
> +static int ovl_index_def = IS_ENABLED(CONFIG_OVERLAY_FS_INDEX_NOUUID) ?
> + OVL_INDEX_NOUUID :
> + IS_ENABLED(CONFIG_OVERLAY_FS_INDEX);
> +module_param_named(index, ovl_index_def, int, 0644);
> MODULE_PARM_DESC(index,
> - "Default to on or off for the inodes index feature");
> + "Default to on, off or nouuid for the inodes index feature");
>
> static bool ovl_nfs_export_def = IS_ENABLED(CONFIG_OVERLAY_FS_NFS_EXPORT);
> module_param_named(nfs_export, ovl_nfs_export_def, bool, 0644);
> @@ -352,8 +354,18 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
> seq_puts(m, ",default_permissions");
> if (strcmp(ofs->config.redirect_mode, ovl_redirect_mode_def()) != 0)
> seq_printf(m, ",redirect_dir=%s", ofs->config.redirect_mode);
> - if (ofs->config.index != ovl_index_def)
> - seq_printf(m, ",index=%s", ofs->config.index ? "on" : "off");
> + if (ofs->config.index != ovl_index_def) {
> + switch (ofs->config.index) {
> + case OVL_INDEX_OFF:
> + seq_puts(m, ",index=off");
> + break;
> + case OVL_INDEX_NOUUID:
> + seq_puts(m, ",index=nouuid");
> + break;
> + default:
> + seq_puts(m, ",index=on");
> + }
> + }
> if (ofs->config.nfs_export != ovl_nfs_export_def)
> seq_printf(m, ",nfs_export=%s", ofs->config.nfs_export ?
> "on" : "off");
> @@ -404,6 +416,7 @@ enum {
> OPT_REDIRECT_DIR,
> OPT_INDEX_ON,
> OPT_INDEX_OFF,
> + OPT_INDEX_NOUUID,
> OPT_NFS_EXPORT_ON,
> OPT_NFS_EXPORT_OFF,
> OPT_XINO_ON,
> @@ -422,6 +435,7 @@ static const match_table_t ovl_tokens = {
> {OPT_REDIRECT_DIR, "redirect_dir=%s"},
> {OPT_INDEX_ON, "index=on"},
> {OPT_INDEX_OFF, "index=off"},
> + {OPT_INDEX_NOUUID, "index=nouuid"},
> {OPT_NFS_EXPORT_ON, "nfs_export=on"},
> {OPT_NFS_EXPORT_OFF, "nfs_export=off"},
> {OPT_XINO_ON, "xino=on"},
> @@ -532,12 +546,17 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> break;
>
> case OPT_INDEX_ON:
> - config->index = true;
> + config->index = OVL_INDEX_ON;
> index_opt = true;
> break;
>
> case OPT_INDEX_OFF:
> - config->index = false;
> + config->index = OVL_INDEX_OFF;
> + index_opt = true;
> + break;
> +
> + case OPT_INDEX_NOUUID:
> + config->index = OVL_INDEX_NOUUID;
> index_opt = true;
> break;
>
> @@ -592,7 +611,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> pr_info("option \"index=on\" is useless in a non-upper mount, ignore\n");
> index_opt = false;
> }
> - config->index = false;
> + config->index = OVL_INDEX_OFF;
> }
>
> err = ovl_parse_redirect_mode(config, config->redirect_mode);
> @@ -644,7 +663,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> config->nfs_export = false;
> } else {
> /* Automatically enable index otherwise. */
> - config->index = true;
> + config->index = OVL_INDEX_ON;
> }
> }
>
> @@ -859,7 +878,7 @@ static int ovl_lower_dir(const char *name, struct path *path,
> fh_type = ovl_can_decode_fh(path->dentry->d_sb);
> if ((ofs->config.nfs_export ||
> (ofs->config.index && ofs->config.upperdir)) && !fh_type) {
> - ofs->config.index = false;
> + ofs->config.index = OVL_INDEX_OFF;
> ofs->config.nfs_export = false;
> pr_warn("fs on '%s' does not support file handles, falling back to index=off,nfs_export=off.\n",
> name);
> @@ -1259,7 +1278,7 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
> err = ovl_do_setxattr(ofs->workdir, OVL_XATTR_OPAQUE, "0", 1, 0);
> if (err) {
> ofs->noxattr = true;
> - ofs->config.index = false;
> + ofs->config.index = OVL_INDEX_OFF;
> ofs->config.metacopy = false;
> pr_warn("upper fs does not support xattr, falling back to index=off and metacopy=off.\n");
> err = 0;
> @@ -1282,7 +1301,7 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
> /* Check if upper/work fs supports file handles */
> fh_type = ovl_can_decode_fh(ofs->workdir->d_sb);
> if (ofs->config.index && !fh_type) {
> - ofs->config.index = false;
> + ofs->config.index = OVL_INDEX_OFF;
> pr_warn("upper fs does not support file handles, falling back to index=off.\n");
> }
>
> @@ -1348,6 +1367,7 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
> {
> struct vfsmount *mnt = ovl_upper_mnt(ofs);
> int err;
> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>
> err = mnt_want_write(mnt);
> if (err)
> @@ -1355,7 +1375,7 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
>
> /* Verify lower root is upper root origin */
> err = ovl_verify_origin(upperpath->dentry, oe->lowerstack[0].dentry,
> - true);
> + true, nouuid);
> if (err) {
> pr_err("failed to verify upper root origin\n");
> goto out;
> @@ -1385,11 +1405,13 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
> */
> if (ovl_check_origin_xattr(ofs->indexdir)) {
> err = ovl_verify_set_fh(ofs->indexdir, OVL_XATTR_ORIGIN,
> - upperpath->dentry, true, false);
> + upperpath->dentry, true, false,
> + nouuid);
> if (err)
> pr_err("failed to verify index dir 'origin' xattr\n");
> }
> - err = ovl_verify_upper(ofs->indexdir, upperpath->dentry, true);
> + err = ovl_verify_upper(ofs->indexdir, upperpath->dentry, true,
> + nouuid);
> if (err)
> pr_err("failed to verify index dir 'upper' xattr\n");
>
> @@ -1458,7 +1480,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
> if (!ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
> bad_uuid = true;
> if (ofs->config.index || ofs->config.nfs_export) {
> - ofs->config.index = false;
> + ofs->config.index = OVL_INDEX_OFF;
> ofs->config.nfs_export = false;
> pr_warn("%s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
> uuid_is_null(&sb->s_uuid) ? "null" :
> @@ -1889,9 +1911,14 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> if (err)
> goto out_free_oe;
>
> + if (ofs->config.index == OVL_INDEX_NOUUID && ofs->numfs > 1) {
> + pr_warn("The index=nouuid requires a single fs for lower and upper, falling back to index=on.\n");
> + ofs->config.index = OVL_INDEX_ON;
> + }
> +
> /* Show index=off in /proc/mounts for forced r/o mount */
> if (!ofs->indexdir) {
> - ofs->config.index = false;
> + ofs->config.index = OVL_INDEX_OFF;
> if (ovl_upper_mnt(ofs) && ofs->config.nfs_export) {
> pr_warn("NFS export requires an index dir, falling back to nfs_export=off.\n");
> ofs->config.nfs_export = false;
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature
2020-09-23 19:47 ` Vivek Goyal
@ 2020-09-24 2:44 ` Amir Goldstein
2020-09-24 13:18 ` Vivek Goyal
0 siblings, 1 reply; 10+ messages in thread
From: Amir Goldstein @ 2020-09-24 2:44 UTC (permalink / raw)
To: Vivek Goyal; +Cc: Pavel Tikhomirov, Miklos Szeredi, overlayfs, linux-kernel
On Wed, Sep 23, 2020 at 10:47 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, Sep 23, 2020 at 06:23:08PM +0300, Pavel Tikhomirov wrote:
> > This relaxes uuid checks for overlay index feature. It is only possible
> > in case there is only one filesystem for all the work/upper/lower
> > directories and bare file handles from this backing filesystem are uniq.
>
> Hi Pavel,
>
> Wondering why upper/work has to be on same filesystem as lower for this to
> work?
>
I reckon that's because I asked for this constraint, so I will answer.
You are right that the important thing is that all lower layers are
on the same fs, but because of
a888db310195 ovl: fix regression with re-formatted lower squashfs
I preferred to keep the rules simpler.
Pavel's use case is clone of disk and change of its UUID.
This is a real use case and I don't think it is unique to Virtuozzo,
so I wanted index=nouuid to address that use case only and
I prefer that it is documented that way too.
Ironically, one of the justifications for index=nouuid is virtiofs -
because fuse is now allowed as upper (or as same fs),
one can already use fuse passthough (or one could use fuse
passthrough when nfs export works correctly) as a "uuid anonymizer"
for any fs, so in practice, index=nouuid cannot do any more harm
then one can already do when enabling index over virtiofs.
That is why I prefer the interpretation that index=nouuid means
"use null uuid instead of s_uuid for ovl_fh" over the interpretation
"relax comparison of uuid in ovl_fh".
Thanks,
Amir.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature
2020-09-23 16:36 ` Amir Goldstein
@ 2020-09-24 7:40 ` Pavel Tikhomirov
0 siblings, 0 replies; 10+ messages in thread
From: Pavel Tikhomirov @ 2020-09-24 7:40 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Miklos Szeredi, Vivek Goyal, overlayfs, linux-kernel
On 9/23/20 7:36 PM, Amir Goldstein wrote:
>>> @@ -414,7 +415,7 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
>>> * Return 0 on match, -ESTALE on mismatch, < 0 on error.
>>> */
>>> static int ovl_verify_fh(struct dentry *dentry, const char *name,
>>> - const struct ovl_fh *fh)
>>> + const struct ovl_fh *fh, bool nouuid)
>>> {
>>> struct ovl_fh *ofh = ovl_get_fh(dentry, name);
>>> int err = 0;
>>> @@ -425,8 +426,14 @@ static int ovl_verify_fh(struct dentry *dentry, const char *name,
>>> if (IS_ERR(ofh))
>>> return PTR_ERR(ofh);
>>>
>>> - if (fh->fb.len != ofh->fb.len || memcmp(&fh->fb, &ofh->fb, fh->fb.len))
>>> + if (fh->fb.len != ofh->fb.len) {
>>> err = -ESTALE;
>>> + } else {
>>> + if (nouuid && !uuid_equal(&fh->fb.uuid, &ofh->fb.uuid))
>>> + ofh->fb.uuid = fh->fb.uuid;
>>> + if (memcmp(&fh->fb, &ofh->fb, fh->fb.len))
>>> + err = -ESTALE;
>>> + }
>>>
>
> On second thought I am wondering if we should do that differently.
> If users want to work with index=nouuid, they need to work with it from day 1.
> index=nouuid should export null uuid in NFS handles and write null uuid
> in trusted.overlay.origin xattr.
>
> So in ovl_encode_real_fh() you set null uuid and
> instead of relaxing uuid_equal() in ovl_decode_real_fh()
> you change it to uuid_is_null().
>
> Do you have a problem with that for Virtuozzo use case?
Actually we've enabled index=on by default in kernel config in Virtuozzo
only in the new update which is not yet released. So probably we can
switch to index=nouuid with null uuid in fh.
>
> Thanks,
> Amir.
>
--
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature
2020-09-23 16:09 ` Amir Goldstein
2020-09-23 16:36 ` Amir Goldstein
@ 2020-09-24 8:40 ` Pavel Tikhomirov
2020-09-24 10:19 ` Amir Goldstein
1 sibling, 1 reply; 10+ messages in thread
From: Pavel Tikhomirov @ 2020-09-24 8:40 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Miklos Szeredi, Vivek Goyal, overlayfs, linux-kernel
On 9/23/20 7:09 PM, Amir Goldstein wrote:
> On Wed, Sep 23, 2020 at 6:23 PM Pavel Tikhomirov
> <ptikhomirov@virtuozzo.com> wrote:
>>
>> This relaxes uuid checks for overlay index feature. It is only possible
>> in case there is only one filesystem for all the work/upper/lower
>> directories and bare file handles from this backing filesystem are uniq.
>> In case we have multiple filesystems here just fall back to normal
>> "index=on".
>>
>> This is needed when overlayfs is/was mounted in a container with
>> index enabled (e.g.: to be able to resolve inotify watch file handles on
>> it to paths in CRIU), and this container is copied and started alongside
>> with the original one. This way the "copy" container can't have the same
>> uuid on the superblock and mounting the overlayfs from it later would
>> fail.
>>
>> That is an example of the problem on top of loop+ext4:
>>
>> dd if=/dev/zero of=loopbackfile.img bs=100M count=10
>> losetup -fP loopbackfile.img
>> losetup -a
>> #/dev/loop0: [64768]:35 (/loop-test/loopbackfile.img)
>> mkfs.ext4 loopbackfile.img
>> mkdir loop-mp
>> mount -o loop /dev/loop0 loop-mp
>> mkdir loop-mp/{lower,upper,work,merged}
>> mount -t overlay overlay -oindex=on,lowerdir=loop-mp/lower,\
>> upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
>> umount loop-mp/merged
>> umount loop-mp
>> e2fsck -f /dev/loop0
>> tune2fs -U random /dev/loop0
>>
>> mount -o loop /dev/loop0 loop-mp
>> mount -t overlay overlay -oindex=on,lowerdir=loop-mp/lower,\
>> upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
>> #mount: /loop-test/loop-mp/merged:
>> #mount(2) system call failed: Stale file handle.
>>
>> mount -t overlay overlay -oindex=nouuid,lowerdir=loop-mp/lower,\
>> upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
>>
>> If you just change the uuid of the backing filesystem, overlay is not
>> mounting any more. In Virtuozzo we copy container disks (ploops) when
>> crate the copy of container and we require fs uuid to be uniq for a new
>> container.
>>
>> v2: in v1 I missed actual uuid check skip - add it
>>
>> CC: Amir Goldstein <amir73il@gmail.com>
>> CC: Vivek Goyal <vgoyal@redhat.com>
>> CC: Miklos Szeredi <miklos@szeredi.hu>
>> CC: linux-unionfs@vger.kernel.org
>> CC: linux-kernel@vger.kernel.org
>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>> ---
>
> Look reasonable, but you need to rebase over
> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
>
> Which makes a lot of your work unneeded.
> ofs is propagated to most of the relevant helpers
> and you should propagate it down to ovl_decode_real_fh().
Thanks! Will do.
>
> Some minor comments below...
>
>> fs/overlayfs/Kconfig | 16 +++++++++++
>> fs/overlayfs/export.c | 6 ++--
>> fs/overlayfs/namei.c | 35 +++++++++++++++--------
>> fs/overlayfs/overlayfs.h | 23 +++++++++++----
>> fs/overlayfs/ovl_entry.h | 2 +-
>> fs/overlayfs/super.c | 61 +++++++++++++++++++++++++++++-----------
>> 6 files changed, 106 insertions(+), 37 deletions(-)
>>
>> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
>> index dd188c7996b3..b00fd44006f9 100644
>> --- a/fs/overlayfs/Kconfig
>> +++ b/fs/overlayfs/Kconfig
>> @@ -61,6 +61,22 @@ config OVERLAY_FS_INDEX
>>
>> If unsure, say N.
>>
>> +config OVERLAY_FS_INDEX_NOUUID
>> + bool "Overlayfs: relax uuid checks of inodes index feature"
>> + depends on OVERLAY_FS
>> + depends on OVERLAY_FS_INDEX
>> + help
>> + If this config option is enabled then overlay will skip uuid checks
>> + for index lower to upper inode map, this only can be done if all
>> + upper and lower directories are on the same filesystem where basic
>> + fhandles are uniq.
>> +
>> + It is needed to overcome possible change of uuid on superblock of the
>> + backing filesystem, e.g. when you copied the virtual disk and mount
>> + both the copy of the disk and the original one at the same time.
>> +
>> + If unsure, say N.
>> +
>
> Please do not add a config option for this.
> Isn't a mount option sufficient for your needs?
Users inside Virtuozzo container can mount overlayfs inside the CT (we
assume that they do "regular" mounts without any "index=" option as
docker does) so we wan't to setup the default in kernel config, so that
all "regular" mounts of the user become "index=nouuid" automatically,
and thus we would be able to both migrate (CRIU inotify resolution by
fhandle on dump) and copy (copy disk with uuid change) the container
without problem.
>
>> config OVERLAY_FS_NFS_EXPORT
>> bool "Overlayfs: turn on NFS export feature by default"
>> depends on OVERLAY_FS
>> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
>> index 0e696f72cf65..d53feb7547d9 100644
>> --- a/fs/overlayfs/export.c
>> +++ b/fs/overlayfs/export.c
>> @@ -676,11 +676,12 @@ static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
>> struct ovl_fs *ofs = sb->s_fs_info;
>> struct dentry *dentry;
>> struct dentry *upper;
>> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>>
>> if (!ovl_upper_mnt(ofs))
>> return ERR_PTR(-EACCES);
>>
>> - upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true);
>> + upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true, nouuid);
>> if (IS_ERR_OR_NULL(upper))
>> return upper;
>>
>> @@ -700,6 +701,7 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
>> struct dentry *index = NULL;
>> struct inode *inode;
>> int err;
>> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>>
>> /* First lookup overlay inode in inode cache by origin fh */
>> err = ovl_check_origin_fh(ofs, fh, false, NULL, &stack);
>> @@ -752,7 +754,7 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
>> goto out_err;
>> }
>> if (index) {
>> - err = ovl_verify_origin(index, origin.dentry, false);
>> + err = ovl_verify_origin(index, origin.dentry, false, nouuid);
>> if (err)
>> goto out_err;
>> }
>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> index f7d4358db637..676170d719f8 100644
>> --- a/fs/overlayfs/namei.c
>> +++ b/fs/overlayfs/namei.c
>> @@ -149,7 +149,7 @@ static struct ovl_fh *ovl_get_fh(struct dentry *dentry, const char *name)
>> }
>>
>> struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
>> - bool connected)
>> + bool connected, bool nouuid)
>> {
>> struct dentry *real;
>> int bytes;
>> @@ -158,7 +158,7 @@ struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
>> * Make sure that the stored uuid matches the uuid of the lower
>> * layer where file handle will be decoded.
>> */
>> - if (!uuid_equal(&fh->fb.uuid, &mnt->mnt_sb->s_uuid))
>> + if (!nouuid && !uuid_equal(&fh->fb.uuid, &mnt->mnt_sb->s_uuid))
>> return NULL;
>>
>> bytes = (fh->fb.len - offsetof(struct ovl_fb, fid));
>> @@ -342,6 +342,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
>> {
>> struct dentry *origin = NULL;
>> int i;
>> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>>
>> for (i = 1; i < ofs->numlayer; i++) {
>> /*
>> @@ -353,7 +354,7 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
>> continue;
>>
>> origin = ovl_decode_real_fh(fh, ofs->layers[i].mnt,
>> - connected);
>> + connected, nouuid);
>> if (origin)
>> break;
>> }
>> @@ -414,7 +415,7 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry,
>> * Return 0 on match, -ESTALE on mismatch, < 0 on error.
>> */
>> static int ovl_verify_fh(struct dentry *dentry, const char *name,
>> - const struct ovl_fh *fh)
>> + const struct ovl_fh *fh, bool nouuid)
>> {
>> struct ovl_fh *ofh = ovl_get_fh(dentry, name);
>> int err = 0;
>> @@ -425,8 +426,14 @@ static int ovl_verify_fh(struct dentry *dentry, const char *name,
>> if (IS_ERR(ofh))
>> return PTR_ERR(ofh);
>>
>> - if (fh->fb.len != ofh->fb.len || memcmp(&fh->fb, &ofh->fb, fh->fb.len))
>> + if (fh->fb.len != ofh->fb.len) {
>> err = -ESTALE;
>> + } else {
>> + if (nouuid && !uuid_equal(&fh->fb.uuid, &ofh->fb.uuid))
>> + ofh->fb.uuid = fh->fb.uuid;
>> + if (memcmp(&fh->fb, &ofh->fb, fh->fb.len))
>> + err = -ESTALE;
>> + }
>>
>> kfree(ofh);
>> return err;
>> @@ -441,7 +448,8 @@ static int ovl_verify_fh(struct dentry *dentry, const char *name,
>> * Return 0 on match, -ESTALE on mismatch, -ENODATA on no xattr, < 0 on error.
>> */
>> int ovl_verify_set_fh(struct dentry *dentry, const char *name,
>> - struct dentry *real, bool is_upper, bool set)
>> + struct dentry *real, bool is_upper, bool set,
>> + bool nouuid)
>> {
>> struct inode *inode;
>> struct ovl_fh *fh;
>> @@ -454,7 +462,7 @@ int ovl_verify_set_fh(struct dentry *dentry, const char *name,
>> goto fail;
>> }
>>
>> - err = ovl_verify_fh(dentry, name, fh);
>> + err = ovl_verify_fh(dentry, name, fh, nouuid);
>> if (set && err == -ENODATA)
>> err = ovl_do_setxattr(dentry, name, fh->buf, fh->fb.len, 0);
>> if (err)
>> @@ -477,6 +485,7 @@ struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
>> {
>> struct ovl_fh *fh;
>> struct dentry *upper;
>> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>>
>> if (!d_is_dir(index))
>> return dget(index);
>> @@ -485,7 +494,7 @@ struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index)
>> if (IS_ERR_OR_NULL(fh))
>> return ERR_CAST(fh);
>>
>> - upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true);
>> + upper = ovl_decode_real_fh(fh, ovl_upper_mnt(ofs), true, nouuid);
>> kfree(fh);
>>
>> if (IS_ERR_OR_NULL(upper))
>> @@ -514,6 +523,7 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
>> struct ovl_path *stack = &origin;
>> struct dentry *upper = NULL;
>> int err;
>> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>>
>> if (!d_inode(index))
>> return 0;
>> @@ -574,7 +584,7 @@ int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index)
>> goto fail;
>> }
>>
>> - err = ovl_verify_fh(upper, OVL_XATTR_ORIGIN, fh);
>> + err = ovl_verify_fh(upper, OVL_XATTR_ORIGIN, fh, nouuid);
>> dput(upper);
>> if (err)
>> goto fail;
>> @@ -690,6 +700,7 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
>> struct qstr name;
>> bool is_dir = d_is_dir(origin);
>> int err;
>> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>>
>> err = ovl_get_index_name(origin, &name);
>> if (err)
>> @@ -741,7 +752,7 @@ struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry *upper,
>> }
>>
>> /* Verify that dir index 'upper' xattr points to upper dir */
>> - err = ovl_verify_upper(index, upper, false);
>> + err = ovl_verify_upper(index, upper, false, nouuid);
>> if (err) {
>> if (err == -ESTALE) {
>> pr_warn_ratelimited("suspected multiply redirected dir found (upper=%pd2, origin=%pd2, index=%pd2).\n",
>> @@ -840,6 +851,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>> .redirect = NULL,
>> .metacopy = false,
>> };
>> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>>
>> if (dentry->d_name.len > ofs->namelen)
>> return ERR_PTR(-ENAMETOOLONG);
>> @@ -939,7 +951,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>> if (upperdentry && !ctr &&
>> ((d.is_dir && ovl_verify_lower(dentry->d_sb)) ||
>> (!d.is_dir && ofs->config.index && origin_path))) {
>> - err = ovl_verify_origin(upperdentry, this, false);
>> + err = ovl_verify_origin(upperdentry, this, false,
>> + nouuid);
>> if (err) {
>> dput(this);
>> if (d.is_dir)
>> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>> index 29bc1ec699e7..2aacfa42bb12 100644
>> --- a/fs/overlayfs/overlayfs.h
>> +++ b/fs/overlayfs/overlayfs.h
>> @@ -18,6 +18,12 @@ enum ovl_path_type {
>> __OVL_PATH_ORIGIN = (1 << 2),
>> };
>>
>> +enum ovl_index_type {
>> + OVL_INDEX_OFF = 0,
>> + OVL_INDEX_ON,
>> + OVL_INDEX_NOUUID,
>> +};
>> +
>> #define OVL_TYPE_UPPER(type) ((type) & __OVL_PATH_UPPER)
>> #define OVL_TYPE_MERGE(type) ((type) & __OVL_PATH_MERGE)
>> #define OVL_TYPE_ORIGIN(type) ((type) & __OVL_PATH_ORIGIN)
>> @@ -362,11 +368,12 @@ static inline int ovl_check_fh_len(struct ovl_fh *fh, int fh_len)
>> }
>>
>> struct dentry *ovl_decode_real_fh(struct ovl_fh *fh, struct vfsmount *mnt,
>> - bool connected);
>> + bool connected, bool nouuid);
>> int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
>> struct dentry *upperdentry, struct ovl_path **stackp);
>> int ovl_verify_set_fh(struct dentry *dentry, const char *name,
>> - struct dentry *real, bool is_upper, bool set);
>> + struct dentry *real, bool is_upper, bool set,
>> + bool nouuid);
>> struct dentry *ovl_index_upper(struct ovl_fs *ofs, struct dentry *index);
>> int ovl_verify_index(struct ovl_fs *ofs, struct dentry *index);
>> int ovl_get_index_name(struct dentry *origin, struct qstr *name);
>> @@ -379,15 +386,19 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>> bool ovl_lower_positive(struct dentry *dentry);
>>
>> static inline int ovl_verify_origin(struct dentry *upper,
>> - struct dentry *origin, bool set)
>> + struct dentry *origin, bool set,
>> + bool nouuid)
>> {
>> - return ovl_verify_set_fh(upper, OVL_XATTR_ORIGIN, origin, false, set);
>> + return ovl_verify_set_fh(upper, OVL_XATTR_ORIGIN, origin, false, set,
>> + nouuid);
>> }
>>
>> static inline int ovl_verify_upper(struct dentry *index,
>> - struct dentry *upper, bool set)
>> + struct dentry *upper, bool set,
>> + bool nouuid)
>> {
>> - return ovl_verify_set_fh(index, OVL_XATTR_UPPER, upper, true, set);
>> + return ovl_verify_set_fh(index, OVL_XATTR_UPPER, upper, true, set,
>> + nouuid);
>> }
>>
>> /* readdir.c */
>> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
>> index b429c80879ee..2fd2cc515ad2 100644
>> --- a/fs/overlayfs/ovl_entry.h
>> +++ b/fs/overlayfs/ovl_entry.h
>> @@ -13,7 +13,7 @@ struct ovl_config {
>> bool redirect_dir;
>> bool redirect_follow;
>> const char *redirect_mode;
>> - bool index;
>> + int index;
>> bool nfs_export;
>> int xino;
>> bool metacopy;
>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> index 4b38141c2985..d1ed580f24b7 100644
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>> @@ -38,10 +38,12 @@ module_param_named(redirect_always_follow, ovl_redirect_always_follow,
>> MODULE_PARM_DESC(redirect_always_follow,
>> "Follow redirects even if redirect_dir feature is turned off");
>>
>> -static bool ovl_index_def = IS_ENABLED(CONFIG_OVERLAY_FS_INDEX);
>> -module_param_named(index, ovl_index_def, bool, 0644);
>> +static int ovl_index_def = IS_ENABLED(CONFIG_OVERLAY_FS_INDEX_NOUUID) ?
>> + OVL_INDEX_NOUUID :
>> + IS_ENABLED(CONFIG_OVERLAY_FS_INDEX);
>> +module_param_named(index, ovl_index_def, int, 0644);
>> MODULE_PARM_DESC(index,
>> - "Default to on or off for the inodes index feature");
>> + "Default to on, off or nouuid for the inodes index feature");
>
> No, we don't want to change the module param from bool to int.
> Scripts are expecting N/Y module param value (unionmount tests and xfstests).
> And I don't see the reason to define nouuid in module param at all.
Looks like it would be enough for us to have only kernel config option,
we don't use module param... But it looked more selfconsistent to also
be able to change default for module too.
Probably I need to add separate ovl_index_nouuid option without making
ovl_index_def int though. - Will do.
>
>>
>> static bool ovl_nfs_export_def = IS_ENABLED(CONFIG_OVERLAY_FS_NFS_EXPORT);
>> module_param_named(nfs_export, ovl_nfs_export_def, bool, 0644);
>> @@ -352,8 +354,18 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>> seq_puts(m, ",default_permissions");
>> if (strcmp(ofs->config.redirect_mode, ovl_redirect_mode_def()) != 0)
>> seq_printf(m, ",redirect_dir=%s", ofs->config.redirect_mode);
>> - if (ofs->config.index != ovl_index_def)
>> - seq_printf(m, ",index=%s", ofs->config.index ? "on" : "off");
>> + if (ofs->config.index != ovl_index_def) {
>> + switch (ofs->config.index) {
>> + case OVL_INDEX_OFF:
>> + seq_puts(m, ",index=off");
>> + break;
>> + case OVL_INDEX_NOUUID:
>> + seq_puts(m, ",index=nouuid");
>> + break;
>> + default:
>> + seq_puts(m, ",index=on");
>> + }
>> + }
>
> See ovl_xino_str[] please.
Nice hint, will rework.
>
>> if (ofs->coovl_xino_strnfig.nfs_export != ovl_nfs_export_def)
>> seq_printf(m, ",nfs_export=%s", ofs->config.nfs_export ?
>> "on" : "off");
>> @@ -404,6 +416,7 @@ enum {
>> OPT_REDIRECT_DIR,
>> OPT_INDEX_ON,
>> OPT_INDEX_OFF,
>> + OPT_INDEX_NOUUID,
>> OPT_NFS_EXPORT_ON,
>> OPT_NFS_EXPORT_OFF,
>> OPT_XINO_ON,
>> @@ -422,6 +435,7 @@ static const match_table_t ovl_tokens = {
>> {OPT_REDIRECT_DIR, "redirect_dir=%s"},
>> {OPT_INDEX_ON, "index=on"},
>> {OPT_INDEX_OFF, "index=off"},
>> + {OPT_INDEX_NOUUID, "index=nouuid"},
>> {OPT_NFS_EXPORT_ON, "nfs_export=on"},
>> {OPT_NFS_EXPORT_OFF, "nfs_export=off"},
>> {OPT_XINO_ON, "xino=on"},
>> @@ -532,12 +546,17 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>> break;
>>
>> case OPT_INDEX_ON:
>> - config->index = true;
>> + config->index = OVL_INDEX_ON;
>> index_opt = true;
>> break;
>>
>> case OPT_INDEX_OFF:
>> - config->index = false;
>> + config->index = OVL_INDEX_OFF;
>> + index_opt = true;
>> + break;
>> +
>> + case OPT_INDEX_NOUUID:
>> + config->index = OVL_INDEX_NOUUID;
>> index_opt = true;
>> break;
>>
>> @@ -592,7 +611,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>> pr_info("option \"index=on\" is useless in a non-upper mount, ignore\n");
>> index_opt = false;
>> }
>> - config->index = false;
>> + config->index = OVL_INDEX_OFF;
>> }
>>
>> err = ovl_parse_redirect_mode(config, config->redirect_mode);
>> @@ -644,7 +663,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>> config->nfs_export = false;
>> } else {
>> /* Automatically enable index otherwise. */
>> - config->index = true;
>> + config->index = OVL_INDEX_ON;
>> }
>> }
>>
>> @@ -859,7 +878,7 @@ static int ovl_lower_dir(const char *name, struct path *path,
>> fh_type = ovl_can_decode_fh(path->dentry->d_sb);
>> if ((ofs->config.nfs_export ||
>> (ofs->config.index && ofs->config.upperdir)) && !fh_type) {
>> - ofs->config.index = false;
>> + ofs->config.index = OVL_INDEX_OFF;
>> ofs->config.nfs_export = false;
>> pr_warn("fs on '%s' does not support file handles, falling back to index=off,nfs_export=off.\n",
>> name);
>> @@ -1259,7 +1278,7 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
>> err = ovl_do_setxattr(ofs->workdir, OVL_XATTR_OPAQUE, "0", 1, 0);
>> if (err) {
>> ofs->noxattr = true;
>> - ofs->config.index = false;
>> + ofs->config.index = OVL_INDEX_OFF;
>> ofs->config.metacopy = false;
>> pr_warn("upper fs does not support xattr, falling back to index=off and metacopy=off.\n");
>> err = 0;
>> @@ -1282,7 +1301,7 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
>> /* Check if upper/work fs supports file handles */
>> fh_type = ovl_can_decode_fh(ofs->workdir->d_sb);
>> if (ofs->config.index && !fh_type) {
>> - ofs->config.index = false;
>> + ofs->config.index = OVL_INDEX_OFF;
>> pr_warn("upper fs does not support file handles, falling back to index=off.\n");
>> }
>>
>> @@ -1348,6 +1367,7 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
>> {
>> struct vfsmount *mnt = ovl_upper_mnt(ofs);
>> int err;
>> + bool nouuid = ofs->config.index == OVL_INDEX_NOUUID;
>>
>> err = mnt_want_write(mnt);
>> if (err)
>> @@ -1355,7 +1375,7 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
>>
>> /* Verify lower root is upper root origin */
>> err = ovl_verify_origin(upperpath->dentry, oe->lowerstack[0].dentry,
>> - true);
>> + true, nouuid);
>> if (err) {
>> pr_err("failed to verify upper root origin\n");
>> goto out;
>> @@ -1385,11 +1405,13 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs,
>> */
>> if (ovl_check_origin_xattr(ofs->indexdir)) {
>> err = ovl_verify_set_fh(ofs->indexdir, OVL_XATTR_ORIGIN,
>> - upperpath->dentry, true, false);
>> + upperpath->dentry, true, false,
>> + nouuid);
>> if (err)
>> pr_err("failed to verify index dir 'origin' xattr\n");
>> }
>> - err = ovl_verify_upper(ofs->indexdir, upperpath->dentry, true);
>> + err = ovl_verify_upper(ofs->indexdir, upperpath->dentry, true,
>> + nouuid);
>> if (err)
>> pr_err("failed to verify index dir 'upper' xattr\n");
>>
>> @@ -1458,7 +1480,7 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)
>> if (!ovl_lower_uuid_ok(ofs, &sb->s_uuid)) {
>> bad_uuid = true;
>> if (ofs->config.index || ofs->config.nfs_export) {
>> - ofs->config.index = false;
>> + ofs->config.index = OVL_INDEX_OFF;
>> ofs->config.nfs_export = false;
>> pr_warn("%s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n",
>> uuid_is_null(&sb->s_uuid) ? "null" :
>> @@ -1889,9 +1911,14 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>> if (err)
>> goto out_free_oe;
>>
>> + if (ofs->config.index == OVL_INDEX_NOUUID && ofs->numfs > 1) {
>> + pr_warn("The index=nouuid requires a single fs for lower and upper, falling back to index=on.\n");
>> + ofs->config.index = OVL_INDEX_ON;
>> + }
>> +
>
> It's too late for this fallback now, you already did relaxed ovl_verify_origin()
> and now we will continue as if all is ok.
> Please fail the mount in this case.
>
> I don't think that users that specifically requested index=nouuid would care to
> fallback to index=on.
No, it's we who will force users to switch to index=nouuid in our
Virtuozzo case through kernel config default, so probably having a
fallback is a good thing, as users will be able to use their overlay at
least until we "copy" their container.
Maybe I can just move my check before ovl_get_indexdir (as far as I can
see it is the only place from ovl_fill_super where we reach
ovl_verify_fh or ovl_decode_real_fh) and after ovl_get_lowerstack where
numfs is calculated. - Will do.
>
> Thanks,
> Amir.
>
--
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature
2020-09-24 8:40 ` Pavel Tikhomirov
@ 2020-09-24 10:19 ` Amir Goldstein
0 siblings, 0 replies; 10+ messages in thread
From: Amir Goldstein @ 2020-09-24 10:19 UTC (permalink / raw)
To: Pavel Tikhomirov; +Cc: Miklos Szeredi, Vivek Goyal, overlayfs, linux-kernel
On Thu, Sep 24, 2020 at 11:40 AM Pavel Tikhomirov
<ptikhomirov@virtuozzo.com> wrote:
>
>
>
> On 9/23/20 7:09 PM, Amir Goldstein wrote:
> > On Wed, Sep 23, 2020 at 6:23 PM Pavel Tikhomirov
> > <ptikhomirov@virtuozzo.com> wrote:
> >>
> >> This relaxes uuid checks for overlay index feature. It is only possible
> >> in case there is only one filesystem for all the work/upper/lower
> >> directories and bare file handles from this backing filesystem are uniq.
> >> In case we have multiple filesystems here just fall back to normal
> >> "index=on".
> >>
> >> This is needed when overlayfs is/was mounted in a container with
> >> index enabled (e.g.: to be able to resolve inotify watch file handles on
> >> it to paths in CRIU), and this container is copied and started alongside
> >> with the original one. This way the "copy" container can't have the same
> >> uuid on the superblock and mounting the overlayfs from it later would
> >> fail.
> >>
> >> That is an example of the problem on top of loop+ext4:
> >>
> >> dd if=/dev/zero of=loopbackfile.img bs=100M count=10
> >> losetup -fP loopbackfile.img
> >> losetup -a
> >> #/dev/loop0: [64768]:35 (/loop-test/loopbackfile.img)
> >> mkfs.ext4 loopbackfile.img
> >> mkdir loop-mp
> >> mount -o loop /dev/loop0 loop-mp
> >> mkdir loop-mp/{lower,upper,work,merged}
> >> mount -t overlay overlay -oindex=on,lowerdir=loop-mp/lower,\
> >> upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
> >> umount loop-mp/merged
> >> umount loop-mp
> >> e2fsck -f /dev/loop0
> >> tune2fs -U random /dev/loop0
> >>
> >> mount -o loop /dev/loop0 loop-mp
> >> mount -t overlay overlay -oindex=on,lowerdir=loop-mp/lower,\
> >> upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
> >> #mount: /loop-test/loop-mp/merged:
> >> #mount(2) system call failed: Stale file handle.
> >>
> >> mount -t overlay overlay -oindex=nouuid,lowerdir=loop-mp/lower,\
> >> upperdir=loop-mp/upper,workdir=loop-mp/work loop-mp/merged
> >>
> >> If you just change the uuid of the backing filesystem, overlay is not
> >> mounting any more. In Virtuozzo we copy container disks (ploops) when
> >> crate the copy of container and we require fs uuid to be uniq for a new
> >> container.
> >>
> >> v2: in v1 I missed actual uuid check skip - add it
> >>
> >> CC: Amir Goldstein <amir73il@gmail.com>
> >> CC: Vivek Goyal <vgoyal@redhat.com>
> >> CC: Miklos Szeredi <miklos@szeredi.hu>
> >> CC: linux-unionfs@vger.kernel.org
> >> CC: linux-kernel@vger.kernel.org
> >> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> >> ---
> >
> > Look reasonable, but you need to rebase over
> > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
> >
> > Which makes a lot of your work unneeded.
> > ofs is propagated to most of the relevant helpers
> > and you should propagate it down to ovl_decode_real_fh().
>
> Thanks! Will do.
>
> >
> > Some minor comments below...
> >
> >> fs/overlayfs/Kconfig | 16 +++++++++++
> >> fs/overlayfs/export.c | 6 ++--
> >> fs/overlayfs/namei.c | 35 +++++++++++++++--------
> >> fs/overlayfs/overlayfs.h | 23 +++++++++++----
> >> fs/overlayfs/ovl_entry.h | 2 +-
> >> fs/overlayfs/super.c | 61 +++++++++++++++++++++++++++++-----------
> >> 6 files changed, 106 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> >> index dd188c7996b3..b00fd44006f9 100644
> >> --- a/fs/overlayfs/Kconfig
> >> +++ b/fs/overlayfs/Kconfig
> >> @@ -61,6 +61,22 @@ config OVERLAY_FS_INDEX
> >>
> >> If unsure, say N.
> >>
> >> +config OVERLAY_FS_INDEX_NOUUID
> >> + bool "Overlayfs: relax uuid checks of inodes index feature"
> >> + depends on OVERLAY_FS
> >> + depends on OVERLAY_FS_INDEX
> >> + help
> >> + If this config option is enabled then overlay will skip uuid checks
> >> + for index lower to upper inode map, this only can be done if all
> >> + upper and lower directories are on the same filesystem where basic
> >> + fhandles are uniq.
> >> +
> >> + It is needed to overcome possible change of uuid on superblock of the
> >> + backing filesystem, e.g. when you copied the virtual disk and mount
> >> + both the copy of the disk and the original one at the same time.
> >> +
> >> + If unsure, say N.
> >> +
> >
> > Please do not add a config option for this.
> > Isn't a mount option sufficient for your needs?
>
> Users inside Virtuozzo container can mount overlayfs inside the CT (we
> assume that they do "regular" mounts without any "index=" option as
> docker does) so we wan't to setup the default in kernel config, so that
> all "regular" mounts of the user become "index=nouuid" automatically,
> and thus we would be able to both migrate (CRIU inotify resolution by
> fhandle on dump) and copy (copy disk with uuid change) the container
> without problem.
>
That is a problem.
So far, mount options always override module and kernel config options.
So if mount option says inodex=on explicitly, it should be index=on.
The only way I see for you to tackle this is if nouuid option is independent
of index option, e.g. index=on,anon_uuid (or uuid=off).
But if kernel config has a way to turn this on, then module param and
mount option should have a way to turn it off.
[...]
> >> @@ -1889,9 +1911,14 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> >> if (err)
> >> goto out_free_oe;
> >>
> >> + if (ofs->config.index == OVL_INDEX_NOUUID && ofs->numfs > 1) {
> >> + pr_warn("The index=nouuid requires a single fs for lower and upper, falling back to index=on.\n");
> >> + ofs->config.index = OVL_INDEX_ON;
> >> + }
> >> +
> >
> > It's too late for this fallback now, you already did relaxed ovl_verify_origin()
> > and now we will continue as if all is ok.
> > Please fail the mount in this case.
> >
> > I don't think that users that specifically requested index=nouuid would care to
> > fallback to index=on.
>
> No, it's we who will force users to switch to index=nouuid in our
> Virtuozzo case through kernel config default, so probably having a
> fallback is a good thing, as users will be able to use their overlay at
> least until we "copy" their container.
>
> Maybe I can just move my check before ovl_get_indexdir (as far as I can
> see it is the only place from ovl_fill_super where we reach
> ovl_verify_fh or ovl_decode_real_fh) and after ovl_get_lowerstack where
> numfs is calculated. - Will do.
Yes, that would be fine techicaly.
I am not sure if the fallback is helpful though.
Anyway, you would need to get approval from Miklos
on the option itself and how it behaves.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature
2020-09-24 2:44 ` Amir Goldstein
@ 2020-09-24 13:18 ` Vivek Goyal
2020-09-24 14:33 ` Amir Goldstein
0 siblings, 1 reply; 10+ messages in thread
From: Vivek Goyal @ 2020-09-24 13:18 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Pavel Tikhomirov, Miklos Szeredi, overlayfs, linux-kernel
On Thu, Sep 24, 2020 at 05:44:22AM +0300, Amir Goldstein wrote:
> On Wed, Sep 23, 2020 at 10:47 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Wed, Sep 23, 2020 at 06:23:08PM +0300, Pavel Tikhomirov wrote:
> > > This relaxes uuid checks for overlay index feature. It is only possible
> > > in case there is only one filesystem for all the work/upper/lower
> > > directories and bare file handles from this backing filesystem are uniq.
> >
> > Hi Pavel,
> >
> > Wondering why upper/work has to be on same filesystem as lower for this to
> > work?
> >
>
> I reckon that's because I asked for this constraint, so I will answer.
>
> You are right that the important thing is that all lower layers are
> on the same fs, but because of
> a888db310195 ovl: fix regression with re-formatted lower squashfs
Hi Amir,
So with "upper on same as lower fs" contstraint we are just making it
little harder so that people don't use recreated lower with existing
upper? Is that the intention behind this constraint.
On a side note, I have a question about above commit.
So this is basically the issue of upper stored file handle resolving to
a different file (in recreated lower). And we are considering this to
be a corner case. But the very fact a user was running into it, it
probably is not that hard to reproduce. So with the fix a888db310195,
we avoided the problem for simple configurations (no-index, no-metacopy,
and no xino). But if same user runs with index=on, with recreatd lower,
they can still run into similar issues?
>
> I preferred to keep the rules simpler.
>
> Pavel's use case is clone of disk and change of its UUID.
> This is a real use case and I don't think it is unique to Virtuozzo,
> so I wanted index=nouuid to address that use case only and
> I prefer that it is documented that way too.
Sure. I understand that. I am only harping on this to make sure
we tell people to not use this "recreated lower with existing upper".
In Pavel's use case, it is more of a cloned use case and not
re-created use case.
Otherwise people will re-create lower layers with regular filesystems and
use index=nouuid and then run into squashfs like issue one day.
Or we could document what Miklos had said. Using existing upper
with recreated lower will likely run into issues with advanced
overlay features like (index, metacopy, xino etc).
>
> Ironically, one of the justifications for index=nouuid is virtiofs -
> because fuse is now allowed as upper (or as same fs),
> one can already use fuse passthough (or one could use fuse
> passthrough when nfs export works correctly) as a "uuid anonymizer"
> for any fs, so in practice, index=nouuid cannot do any more harm
> then one can already do when enabling index over virtiofs.
Interesing. Using virtiofs or a fuse passthrough filesystem on top
just to avoid uuid check will be lot of work.
But keeping upper/ on same fs as lower fs constraint does not help with this.
>
> That is why I prefer the interpretation that index=nouuid means
> "use null uuid instead of s_uuid for ovl_fh" over the interpretation
> "relax comparison of uuid in ovl_fh".
So bottom line is that there are many ways where users can recreate
lower layers and run into issues.
- squashfs with index
- use a fuse passthrough filesystem
- use index=nouuid
So to me documenting that don't use existig upper with recreated lower
should help with all.
And putting a constraint of "lower and upper being on same fs" seems fine
for now but I am not sure it helps a lot. Anyway, I am fine with this
constratint. Just wanted to understand the rationale behind it.
Thanks
Vivek
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature
2020-09-24 13:18 ` Vivek Goyal
@ 2020-09-24 14:33 ` Amir Goldstein
0 siblings, 0 replies; 10+ messages in thread
From: Amir Goldstein @ 2020-09-24 14:33 UTC (permalink / raw)
To: Vivek Goyal; +Cc: Pavel Tikhomirov, Miklos Szeredi, overlayfs, linux-kernel
On Thu, Sep 24, 2020 at 4:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Thu, Sep 24, 2020 at 05:44:22AM +0300, Amir Goldstein wrote:
> > On Wed, Sep 23, 2020 at 10:47 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Wed, Sep 23, 2020 at 06:23:08PM +0300, Pavel Tikhomirov wrote:
> > > > This relaxes uuid checks for overlay index feature. It is only possible
> > > > in case there is only one filesystem for all the work/upper/lower
> > > > directories and bare file handles from this backing filesystem are uniq.
> > >
> > > Hi Pavel,
> > >
> > > Wondering why upper/work has to be on same filesystem as lower for this to
> > > work?
> > >
> >
> > I reckon that's because I asked for this constraint, so I will answer.
> >
> > You are right that the important thing is that all lower layers are
> > on the same fs, but because of
> > a888db310195 ovl: fix regression with re-formatted lower squashfs
>
> Hi Amir,
>
> So with "upper on same as lower fs" contstraint we are just making it
> little harder so that people don't use recreated lower with existing
> upper? Is that the intention behind this constraint.
>
> On a side note, I have a question about above commit.
>
> So this is basically the issue of upper stored file handle resolving to
> a different file (in recreated lower). And we are considering this to
> be a corner case. But the very fact a user was running into it, it
> probably is not that hard to reproduce. So with the fix a888db310195,
> we avoided the problem for simple configurations (no-index, no-metacopy,
> and no xino). But if same user runs with index=on, with recreatd lower,
> they can still run into similar issues?
>
> >
> > I preferred to keep the rules simpler.
> >
> > Pavel's use case is clone of disk and change of its UUID.
> > This is a real use case and I don't think it is unique to Virtuozzo,
> > so I wanted index=nouuid to address that use case only and
> > I prefer that it is documented that way too.
>
> Sure. I understand that. I am only harping on this to make sure
> we tell people to not use this "recreated lower with existing upper".
> In Pavel's use case, it is more of a cloned use case and not
> re-created use case.
>
> Otherwise people will re-create lower layers with regular filesystems and
> use index=nouuid and then run into squashfs like issue one day.
>
> Or we could document what Miklos had said. Using existing upper
> with recreated lower will likely run into issues with advanced
> overlay features like (index, metacopy, xino etc).
>
I am perfectly fine with saying that
and with allowing the special case of cloning disk with index=nouuid.
There was a "patch" floating around for improving the doc,
I was assuming you will pick it up add your own proposed changes
and make it into a proper patch.
> >
> > Ironically, one of the justifications for index=nouuid is virtiofs -
> > because fuse is now allowed as upper (or as same fs),
> > one can already use fuse passthough (or one could use fuse
> > passthrough when nfs export works correctly) as a "uuid anonymizer"
> > for any fs, so in practice, index=nouuid cannot do any more harm
> > then one can already do when enabling index over virtiofs.
>
> Interesing. Using virtiofs or a fuse passthrough filesystem on top
> just to avoid uuid check will be lot of work.
>
> But keeping upper/ on same fs as lower fs constraint does not help with this.
>
No, it does not. I am only saying index=nouuid is "just" as bad
as what people can already do with virtiofs. Not much worse.
> >
> > That is why I prefer the interpretation that index=nouuid means
> > "use null uuid instead of s_uuid for ovl_fh" over the interpretation
> > "relax comparison of uuid in ovl_fh".
>
> So bottom line is that there are many ways where users can recreate
> lower layers and run into issues.
>
> - squashfs with index
> - use a fuse passthrough filesystem
> - use index=nouuid
>
> So to me documenting that don't use existig upper with recreated lower
> should help with all.
>
> And putting a constraint of "lower and upper being on same fs" seems fine
> for now but I am not sure it helps a lot. Anyway, I am fine with this
> constratint. Just wanted to understand the rationale behind it.
>
Only rational is - it is intended for cloned disk - don't make it easy
to use this for
anything else.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-09-24 14:33 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-23 15:23 [PATCH v2] ovl: introduce new "index=nouuid" option for inodes index feature Pavel Tikhomirov
2020-09-23 16:09 ` Amir Goldstein
2020-09-23 16:36 ` Amir Goldstein
2020-09-24 7:40 ` Pavel Tikhomirov
2020-09-24 8:40 ` Pavel Tikhomirov
2020-09-24 10:19 ` Amir Goldstein
2020-09-23 19:47 ` Vivek Goyal
2020-09-24 2:44 ` Amir Goldstein
2020-09-24 13:18 ` Vivek Goyal
2020-09-24 14:33 ` Amir Goldstein
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).