Overlayfs strict feature requirements
Amir Goldstein
maintainerv2 cover Overlayfs strict feature requirements
Vivek, Miklos,
This series passes overlay/quick xfstests and I verified manually some expected mount failures with metacopy=on and override with metacopy=on,strict=off.
Still needs very carefull review and the ovl_check_rename_whiteout() helper in patch 3 is broken, so I disabled it for now.
Patches 1-3 are marked for stable apply cleanly on v4.19. Patch 4 doesn't apply to v4.19.
Patch 5 will probably apply, but not sure it is stable material.
I did not change behavior w.r.t enabling of redirect_dir, because it involves many corner cases and I don't think it matters for stable. We can always improve it later and let some mount configurations that used to fail succeed with expected user requested mount options. When we address the metacopy => redirect_dir dependency, we should also address the nfs_export => index dependency in a similar manner.
Thanks,
Amir.
Amir Goldstein (5):
ovl: return error on mount if metacopy cannot be enabled
ovl: enforce 'strict' feature requirements with metacopy=on
ovl: enforce 'strict' upper fs requirements with metacopy=on
ovl: enforce 'strict' unique uuid requirement with metacopy=on
ovl: enforce 'strict' upper fs and feature requirements with strict=on
fs/overlayfs/Kconfig | 23 ++++ fs/overlayfs/ovl_entry.h | 1 + fs/overlayfs/super.c | 235 ++++++++++++++++++++++++++++++--------- 3 files changed, 208 insertions(+), 51 deletions(-)
Signature · 1 line
2.17.1
Amir Goldstein
maintainerPATCH v2 1/5 ovl: return error on mount if metacopy cannot be enabled
Consider a user who wants to enable metadata only copy-up with metacopy=on. If this feature can't be enabled, we disable metacopy=off and just leave a warning in logs. metacopy=on requires redirect_dir=on (for upper dir) or redirect_dir=follow (for non-upper mount) and requires that upper fs supports extended attributes.
As user does not see mount failure, he/she assumes metadata only copy-up has been enabled but that's not the case.
So instead of disabling metacopy, return an error to user and leave a message in logs. That may allow user to fix mount options and retry. This is done only if user specified metacopy=on in mount options. If metacopy is enabled as default either through module command line or kernel Kconfig, that's not enforced and it can be disabled automatically if system configuration does not permit it.
--- fs/overlayfs/ovl_entry.h | 1 + fs/overlayfs/super.c | 74 +++++++++++++++++++++++++++++++--------- 2 files changed, 59 insertions(+), 16 deletions(-)
fs/overlayfs/ovl_entry.h +1 -0
@@ -20,6 +20,7 @@ struct ovl_config {bool nfs_export;int xino;bool metacopy; +bool strict; };struct ovl_sb {
fs/overlayfs/super.c +58 -16
105 lines not on this page · raw message
Signature · 1 line
2.17.1
Re: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled
On Thu, Nov 01, 2018 at 02:48:09AM +0200, Amir Goldstein wrote:
[..]
Quoted 11 lines
+ * Check dependencies between features.
+ *
+ * @enabled is a boolean variable that depends on the boolean @required
+ * variable.
+ *
+ * If !@config->strict, the feature is disabled when requirement is not met.
+ * If @config->strict, return error when requirement is not met.
+ *
+ * @feature is the name of the feature and @requirement is the description of
+ * the requirement for the error/warning message.
+ */
fs/overlayfs/super.c module_param_named(xino_auto, ovl_xino_auto_def, bool, 0644); · line 98
Quoted 9 lines
fs/overlayfs/super.c +9
-0+static int ovl_feature_check (struct ovl_config *config,bool *enabled, +bool required,const char *feature, +const char *requirement) +{ +/* If feature is enabled, required condition should be met */ +if (!*enabled || required) +return 0 ; + + *enabled = false;
So going forward, we will allow disabling a feature if strict is not set? Even for new knobs? IOW, say I introduce a feature foo, it will have two modes it will work in (strict=on, strict=off?)
Do we really need this for newer options. I thought we needed this behavior only for older options due to backward compatibility issues.
Quoted 19 lines
fs/overlayfs/super.c +4
-5+ +return ovl_feature_requires (config, feature, requirement); +} +static void ovl_entry_stack_free (struct ovl_entry *oe) {unsigned int i;@@ -64,11 +108,6 @@ static void ovl_entry_stack_free(struct ovl_entry *oe)dput (oe->lowerstack[i].dentry); } -static bool ovl_metacopy_def =IS_ENABLED (CONFIG_OVERLAY_FS_METACOPY ); -module_param_named (metacopy, ovl_metacopy_def, bool,0644 ); -MODULE_PARM_DESC (ovl_metacopy_def, -"Default to on or off for the metadata only copy up feature" ); -static void ovl_dentry_release (struct dentry *dentry) {struct ovl_entry *oe = dentry->d_fsdata;
[..]
fs/overlayfs/super.c static int ovl_parse_opt(char *opt, struct ovl_config *config) · line 590
Quoted 5 lines
fs/overlayfs/super.c +1
-0@@ -548,6 +587,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)case OPT_METACOPY_ON : config->metacopy = true; + config->strict = true;
I think either ->strict should go in a separate patch or we should have a good description in commit message, explaining why ->strict is there and how it will impact behavior going forward.
Thanks
Vivek
Miklos Szeredi
maintainerRe: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled
Quoted 10 lines · Vivek Goyal
Quoted 4 lines · Amir Goldstein
case OPT_METACOPY_ON: config->metacopy = true;+
config->strict= true;I think either ->strict should go in a separate patch or we should have a good description in commit message, explaining why ->strict is there and how it will impact behavior going forward.
I'm redoing Amir's patches a bit, and at the moment I'm more inclined to leave this after the merge window, since there are so many subtle details to deal with.
Back shortly with an updated set.
Thanks,
Miklos
Miklos Szeredi
maintainerRe: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled
Quoted 12 lines · Miklos Szeredi
Quoted 10 lines · Vivek Goyal
Quoted 4 lines · Amir Goldstein
case OPT_METACOPY_ON: config->metacopy = true;+
config->strict= true;I think either ->strict should go in a separate patch or we should have a good description in commit message, explaining why ->strict is there and how it will impact behavior going forward.
Quoted 5 lines
I'm redoing Amir's patches a bit, and at the moment I'm more inclined to leave this after the merge window, since there are so many subtle details to deal with.
Back shortly with an updated set.
...this is more complicated than I thought.
Anyway, pushed a metacopy fix to overlayfs-next, that I'm pretty happy with.
I don't think erroring out on noxattr is important; falling back is actually quite sane. And it isn't going to be an issue in real life, since xattr is supported on lots of fs, and it's unlikely that lots of people will want to use overlayfs with e.g. minix as the upper layer.
Thanks,
Miklos
Amir Goldstein
maintainerRe: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled
Quoted 14 lines · Miklos Szeredi
Quoted 12 lines · Miklos Szeredi
Quoted 10 lines · Vivek Goyal
Quoted 4 lines · Amir Goldstein
case OPT_METACOPY_ON: config->metacopy = true;+
config->strict= true;I think either ->strict should go in a separate patch or we should have a good description in commit message, explaining why ->strict is there and how it will impact behavior going forward.
Quoted 8 lines
Quoted 5 lines
I'm redoing Amir's patches a bit, and at the moment I'm more inclined to leave this after the merge window, since there are so many subtle details to deal with.
Back shortly with an updated set.
...this is more complicated than I thought.
My reaction as well when I started to dive in...
Are you still going to redo the "strict" series?
With metacopy out of the picture, I can just reorder the patches and drop the metacopy=on mentions.
Let me know if you want me to do that.
Quoted 2 lines
Anyway, pushed a metacopy fix to overlayfs-next, that I'm pretty happy with.
Me too.
We should do the same for nfs_export=on implies index=on. There are probably more users that just want nfs_export=on than users that know what index=on even means. Let me know if you want me to do that.
Thanks,
Amir.
Miklos Szeredi
maintainerRe: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled
Quoted 21 lines · Amir Goldstein
Quoted 20 lines · Miklos Szeredi
Quoted 17 lines · Miklos Szeredi
Quoted 10 lines · Vivek Goyal
Quoted 4 lines · Amir Goldstein
case OPT_METACOPY_ON: config->metacopy = true;+
config->strict= true;I think either ->strict should go in a separate patch or we should have a good description in commit message, explaining why ->strict is there and how it will impact behavior going forward.
I'm redoing Amir's patches a bit, and at the moment I'm more inclined to leave this after the merge window, since there are so many subtle details to deal with.
Back shortly with an updated set.
Quoted 8 lines
Quoted 2 lines
...this is more complicated than I thought.
My reaction as well when I started to dive in...
Are you still going to redo the "strict" series?
With metacopy out of the picture, I can just reorder the patches and drop the metacopy=on mentions.
Let me know if you want me to do that.
I'll post what I have next week (done for this week).
Quoted 9 lines
Quoted 3 lines
Anyway, pushed a metacopy fix to overlayfs-next, that I'm pretty happy with.
Me too.
We should do the same for nfs_export=on implies index=on. There are probably more users that just want nfs_export=on than users that know what index=on even means. Let me know if you want me to do that.
Yeah, probably makes sense.
And there's the annoying fact that nfs_export can't be enabled if metacopy is on by default...
Not sure how much backward compat headache would involve changing these.
Thanks,
Miklos
Amir Goldstein
maintainerRe: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled
Quoted 7 lines
Quoted 4 lines
Are you still going to redo the "strict" series?
With metacopy out of the picture, I can just reorder the patches and drop the metacopy=on mentions.
Let me know if you want me to do that.I'll post what I have next week (done for this week).
FWIW, I forgot we need to enforce ovl_inuse_lock (upperdir/workdir) under the "strict" rules.
We relaxed ovl_inuse_lock() for index=off because of docker mount leaks, but if we have no backward compat restrictions, we should enforce it regardless of index=on.
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
Anyway, pushed a metacopy fix to overlayfs-next, that I'm pretty happy with.
Me too.
We should do the same for nfs_export=on implies index=on. There are probably more users that just want nfs_export=on than users that know what index=on even means. Let me know if you want me to do that.
Quoted 7 lines
Yeah, probably makes sense.
And there's the annoying fact that
nfs_exportcan't be enabled if metacopy is on by default...Not sure how much backward compat headache would involve changing these.
Not sure I am seeing the problem.
Case #1:
INDEX=N
NFS_EXPORT=N
METACOPY=N
nfs_export=on
This will change behavior from "fallback to nfs_export=off" to "implicit enabled of index=on" also implying ovl_inuse_lock(). But we don't have any docker users setting nfs_export=on (What for? it doesn't work. docker sets index=off).
Case #2:
INDEX=N
NFS_EXPORT=N
METACOPY=Y
nfs_export=on
Disables metacopy and enables index.
Which problems will that cause (??)
I have no idea how "strict" affects the cases above (??) Kconfig is rather simple because inter dependencies are already encoded.
mount options should always win over Kconfig.
module param inter dependencies - I have no idea.
Thanks,
Amir.
Miklos Szeredi
maintainerRe: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled
Quoted 51 lines · Amir Goldstein
Quoted 7 lines
Quoted 4 lines
Are you still going to redo the "strict" series?
With metacopy out of the picture, I can just reorder the patches and drop the metacopy=on mentions.
Let me know if you want me to do that.I'll post what I have next week (done for this week).
FWIW, I forgot we need to enforce
ovl_inuse_lock(upperdir/workdir) under the "strict" rules.
We relaxedovl_inuse_lock()for index=off because of docker mount leaks, but if we have no backward compat restrictions, we should enforce it regardless of index=on.Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
Anyway, pushed a metacopy fix to overlayfs-next, that I'm pretty happy with.
Me too.
We should do the same for nfs_export=on implies index=on. There are probably more users that just want nfs_export=on than users that know what index=on even means. Let me know if you want me to do that.Quoted 7 lines
Yeah, probably makes sense.
And there's the annoying fact that
nfs_exportcan't be enabled if metacopy is on by default...Not sure how much backward compat headache would involve changing these.
Not sure I am seeing the problem.
Case #1:
INDEX=N
NFS_EXPORT=N
METACOPY=N
nfs_export=onThis will change behavior from "fallback to nfs_export=off" to "implicit enabled of index=on" also implying
ovl_inuse_lock(). But we don't have any docker users setting nfs_export=on (What for? it doesn't work. docker sets index=off).Case #2:
INDEX=N
NFS_EXPORT=N
METACOPY=Y
nfs_export=on
Quoted 8 lines
Disables metacopy and enables index.
Which problems will that cause (??)I have no idea how "strict" affects the cases above (??)
Kconfigis rather simple because inter dependencies are already encoded.
mount options should always win overKconfig.
module param inter dependencies - I have no idea.
After looking at this issue some more, I've pretty much convinced myself that "strict" should not have any affect on inter-option dependencies. The "strict" option is good for things like xattr and inuse_lock, etc.
And if manage to resolve the inter-option dependencies like the metacopy/redirect_dir one, then we're good, so let's see:
metacopy implies redirect_dirnfs_export implies index
if (!upper)
nfs_export implies !redirect_dir
metacopy implies !nfs_export
Like with metacopy/redirect_dir, conflicting mount options would be rejected, only the default would be overridden by the implication.
Not sure about the direction of the nfs_export/metacopy implication, it just as well could be the other way round and since metacopy is new, we probably can still change behavior if backported to stable.
Note: changing the direction only changes the behavior if neither of the options is given, so that's the "resolve module param interdependencies" case.
Thanks,
Miklos
Amir Goldstein
maintainerRe: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled
Quoted 62 lines · Miklos Szeredi
Quoted 51 lines · Amir Goldstein
Quoted 7 lines
Quoted 4 lines
Are you still going to redo the "strict" series?
With metacopy out of the picture, I can just reorder the patches and drop the metacopy=on mentions.
Let me know if you want me to do that.I'll post what I have next week (done for this week).
FWIW, I forgot we need to enforce
ovl_inuse_lock(upperdir/workdir) under the "strict" rules.
We relaxedovl_inuse_lock()for index=off because of docker mount leaks, but if we have no backward compat restrictions, we should enforce it regardless of index=on.Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
Anyway, pushed a metacopy fix to overlayfs-next, that I'm pretty happy with.
Me too.
We should do the same for nfs_export=on implies index=on. There are probably more users that just want nfs_export=on than users that know what index=on even means. Let me know if you want me to do that.Quoted 7 lines
Yeah, probably makes sense.
And there's the annoying fact that
nfs_exportcan't be enabled if metacopy is on by default...Not sure how much backward compat headache would involve changing these.
Not sure I am seeing the problem.
Case #1:
INDEX=N
NFS_EXPORT=N
METACOPY=N
nfs_export=onThis will change behavior from "fallback to nfs_export=off" to "implicit enabled of index=on" also implying
ovl_inuse_lock(). But we don't have any docker users setting nfs_export=on (What for? it doesn't work. docker sets index=off).Case #2:
INDEX=N
NFS_EXPORT=N
METACOPY=Y
nfs_export=onQuoted 8 lines
Disables metacopy and enables index.
Which problems will that cause (??)I have no idea how "strict" affects the cases above (??)
Kconfigis rather simple because inter dependencies are already encoded.
mount options should always win overKconfig.
module param inter dependencies - I have no idea.
Quoted 5 lines
After looking at this issue some more, I've pretty much convinced myself that "strict" should not have any affect on inter-option dependencies. The "strict" option is good for things like xattr and
inuse_lock, etc.
OK. (including RENAME_WHITEOUT and d_type)
But what about being "strict" about the need for underlying fs to support file handles for index and nfs_export?
Implementation-wise that could be solved by storing the information about which options have been set in mount in a bitmap in ovl_config.
Quoted 12 lines
And if manage to resolve the inter-option dependencies like the metacopy/redirect_dir one, then we're good, so let's see:
metacopy implies
redirect_dirnfs_exportimplies index
if (!upper)
nfs_exportimplies !redirect_dir
metacopy implies !nfs_exportLike with metacopy/redirect_dir, conflicting mount options would be rejected, only the default would be overridden by the implication.
OK.
The "thorny part" is that with your implementation of metacopy/redirect_dir all the decisions can be made at the end of options parsing. With nfs_export, mount option can turn it on and then no file handle support will turn it off.
If at option parsing time we let nfs_export imply !redirect_dir, we are left with !redirect_dir and !nfs_export.
If we let "mount option is stronger than defaults" behavior depend on "strict=on" than this is not a problem, because nfs_export/index won't be turned off by no file handles and therefore it is safe to resolve all interdependencies at the end of mount option parsing.
Quoted 7 lines
Not sure about the direction of the nfs_export/metacopy implication, it just as well could be the other way round and since metacopy is new, we probably can still change behavior if backported to stable.
Note: changing the direction only changes the behavior if neither of the options is given, so that's the "resolve module param interdependencies" case.
and Kconfig will choose metacopy if both are set to Y...
I think current direction makes sense. In the system/distro level it makes sense for the default to be efficient in copy up. In the fs instance level, user/application can opt-in for NFS export feature in favor of efficient copy up.
Thanks,
Amir.
Miklos Szeredi
maintainerRe: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled
Quoted 4 lines · Amir Goldstein
OK. (including
RENAME_WHITEOUTandd_type)But what about being "strict" about the need for underlying fs to support file handles for index and
nfs_export?
Yes, that one as well.
Quoted 6 lines
The "thorny part" is that with your implementation of metacopy/redirect_dir all the decisions can be made at the end of options parsing. With
nfs_export, mount option can turn it on and then no file handle support will turn it off.
If at option parsing time we letnfs_exportimply !redirect_dir, we are left with !redirect_dir and !nfs_export.
We could do the option interdependency resolution after detecting features.
Quoted 6 lines
and
Kconfigwill choose metacopy if both are set to Y...I think current direction makes sense. In the system/distro level it makes sense for the default to be efficient in copy up. In the fs instance level, user/application can opt-in for NFS export feature in favor of efficient copy up.
Makes sense.
Thanks,
Miklos
Re: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled
In reply to Miklos SzerediQuoted 19 lines · Miklos Szeredi
Quoted 12 lines · Miklos Szeredi
Quoted 10 lines · Vivek Goyal
Quoted 4 lines · Amir Goldstein
case OPT_METACOPY_ON: config->metacopy = true;+
config->strict= true;I think either ->strict should go in a separate patch or we should have a good description in commit message, explaining why ->strict is there and how it will impact behavior going forward.
Quoted 5 lines
I'm redoing Amir's patches a bit, and at the moment I'm more inclined to leave this after the merge window, since there are so many subtle details to deal with.
Back shortly with an updated set.
Quoted 8 lines
...this is more complicated than I thought.
Anyway, pushed a metacopy fix to overlayfs-next, that I'm pretty happy with.
I don't think erroring out on noxattr is important; falling back is actually quite sane. And it isn't going to be an issue in real life, since xattr is supported on lots of fs, and it's unlikely that lots of people will want to use overlayfs with e.g. minix as the upper layer.
Hi Miklos,
This looks reasonable to me. I tried metacopy=on and it mounted. Only little issue seems to be that it enables redirect_dir but it does not show in /proc/mounts.
#cat /proc/mounts | grep overlay
none /root/overlayfs-testing/merged overlay rw,seclabel,relatime,lowerdir=lower:lower2,upperdir=upper,workdir=work,metacopy=on 0 0
Is this an issue or it is fine given on same kernel configuration all user has to pass is "metacopy=on" to get similar mount settings. Thought of pointing it out to make sure this is not an issue.
Thanks
Vivek
Miklos Szeredi
maintainerRe: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled
Quoted 29 lines · Vivek Goyal
Quoted 19 lines · Miklos Szeredi
Quoted 12 lines · Miklos Szeredi
Quoted 10 lines · Vivek Goyal
Quoted 4 lines · Amir Goldstein
case OPT_METACOPY_ON: config->metacopy = true;+
config->strict= true;I think either ->strict should go in a separate patch or we should have a good description in commit message, explaining why ->strict is there and how it will impact behavior going forward.
Quoted 5 lines
I'm redoing Amir's patches a bit, and at the moment I'm more inclined to leave this after the merge window, since there are so many subtle details to deal with.
Back shortly with an updated set.
Quoted 8 lines
...this is more complicated than I thought.
Anyway, pushed a metacopy fix to overlayfs-next, that I'm pretty happy with.
I don't think erroring out on noxattr is important; falling back is actually quite sane. And it isn't going to be an issue in real life, since xattr is supported on lots of fs, and it's unlikely that lots of people will want to use overlayfs with e.g. minix as the upper layer.
Quoted 5 lines
Hi Miklos,
This looks reasonable to me. I tried metacopy=on and it mounted. Only little issue seems to be that it enables
redirect_dirbut it does not show in/proc/mounts.
That's intentional.
Quoted 7 lines
#cat
/proc/mounts| grep overlay
none/root/overlayfs-testing/mergedoverlay rw,seclabel,relatime,lowerdir=lower:lower2,upperdir=upper,workdir=work,metacopy=on 0 0Is this an issue or it is fine given on same kernel configuration all user has to pass is "metacopy=on" to get similar mount settings. Thought of pointing it out to make sure this is not an issue.
Right, that's what we promise: same kernel configuration, same module params, options copied from /proc/mounts -> same options enabled.
Thanks,
Miklos
Amir Goldstein
maintainerPATCH v2 2/5 ovl: enforce 'strict' feature requirements with metacopy=on
In reply to Amir GoldsteinSome overlayfs features require file handle support from underlying filesystems and extended attributes support from upper filesystem. We should prefer to fail the mount if those feature requirements are not met, but for backward compatibility, we write a warning to log and disable the feature instead.
When user asks explicitly via mount option to enable the new metacopy feature, we do not need to worry about backward compatibility and we can fail the mount if any of the feature requirements are not met.
--- fs/overlayfs/super.c | 84 +++++++++++++++++++++++++------------------- 1 file changed, 47 insertions(+), 37 deletions(-)
fs/overlayfs/super.c +47 -37
@@ -608,6 +608,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) config->workdir =NULL ; } +/* Show index=off in /proc/mounts for forced r/o mount */ +if (!config->upperdir) + config->index = false; + err =ovl_parse_redirect_mode (config, config->redirect_mode);if (err)return err;
@@ -814,13 +818,17 @@ static int ovl_lower_dir(const char *name, struct path *path,* file handles, so they require that all layers support them. */ 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.nfs_export = false; -pr_warn ("overlayfs: fs on '%s' does not support file handles, falling back to index=off,nfs_export=off.\n" , - name); - } + err =ovl_feature_check (&ofs->config, &ofs->config.nfs_export, + fh_type,"NFS export" , +"lower fs file handles support" ); +if (err) + goto out_put; + + err =ovl_feature_check (&ofs->config, &ofs->config.index, + fh_type,"inodes index" , +"lower fs file handles support" ); +if (err) + goto out_put;/* Check if lower fs has 32bit inode numbers */ if (fh_type !=FILEID_INO32_GEN )
@@ -1092,34 +1100,35 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath) err =ovl_do_setxattr (ofs->workdir,OVL_XATTR_OPAQUE ,"0" ,1 ,0 );if (err) { ofs->noxattr = true; - ofs->config.index = false; -pr_warn ("overlayfs: upper fs does not support xattr, falling back to index=off.\n" ); - err =ovl_feature_check (&ofs->config, &ofs->config.metacopy, - !ofs->noxattr, -"metadata only copy up" , -"upper fs xattr support" ); -if (err) - goto out; +pr_warn ("overlayfs: upper fs does not support xattr.\n" ); }else {vfs_removexattr (ofs->workdir,OVL_XATTR_OPAQUE ); } +/* metacopy depends on redirect_dir which depend on xattr */ + err =ovl_feature_check (&ofs->config, &ofs->config.metacopy, + !ofs->noxattr,"metadata only copy up" , +"upper fs xattr support" ); +if (err) + goto out; +/* 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; -pr_warn ("overlayfs: upper fs does not support file handles, falling back to index=off.\n" ); - } + err =ovl_feature_check (&ofs->config, &ofs->config.index, + !ofs->noxattr && fh_type,"inodes index" , +"upper fs file handles and xattr support" ); +if (err) + goto out;/* Check if upper fs has 32bit inode numbers */ if (fh_type !=FILEID_INO32_GEN ) ofs->xino_bits =0 ;/* NFS export of r/w mount depends on index */ -if (ofs->config.nfs_export && !ofs->config.index) { -pr_warn ("overlayfs: NFS export requires \"index=on\", falling back to nfs_export=off.\n" ); - ofs->config.nfs_export = false; - } + err =ovl_feature_check (&ofs->config, &ofs->config.nfs_export, + ofs->config.index,"NFS export" , +"\"index=on\" and upper fs file handles" ); + out:mnt_drop_write (mnt);return err;
@@ -1377,10 +1386,13 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb, }else if (!ofs->config.upperdir && stacklen ==1 ) {pr_err ("overlayfs: at least 2 lowerdir are needed while upperdir nonexistent\n" ); goto out_err; - }else if (!ofs->config.upperdir && ofs->config.nfs_export && - ofs->config.redirect_follow) { -pr_warn ("overlayfs: NFS export requires \"redirect_dir=nofollow\" on non-upper mount, falling back to nfs_export=off.\n" ); - ofs->config.nfs_export = false; + }else if (!ofs->config.upperdir) { + err =ovl_feature_check (&ofs->config, &ofs->config.nfs_export, + !ofs->config.redirect_follow, +"NFS export" , +"\"redirect_dir=nofollow\" on non-upper mount" ); +if (err) + goto out_err; } err = -ENOMEM ;
@@ -1522,18 +1534,16 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) }/* Show index=off in /proc/mounts for forced r/o mount */ -if (!ofs->indexdir) { +if (!ofs->indexdir) ofs->config.index = false; -if (ofs->upper_mnt && ofs->config.nfs_export) { -pr_warn ("overlayfs: NFS export requires an index dir, falling back to nfs_export=off.\n" ); - ofs->config.nfs_export = false; - } - } -if (ofs->config.metacopy && ofs->config.nfs_export) { -pr_warn ("overlayfs: NFS export is not supported with metadata only copy up, falling back to nfs_export=off.\n" ); - ofs->config.nfs_export = false; - } +/* NFS export is not supported with metacopy or without index */ + err =ovl_feature_check (&ofs->config, &ofs->config.nfs_export, + (ofs->indexdir || !ofs->upper_mnt) && + !ofs->config.metacopy,"NFS export" , +"\"metacopy=off\" and \"index=on\" on an upper mount" ); +if (err) + goto out_free_oe;if (ofs->config.nfs_export) sb->s_export_op = &ovl_export_operations;
Signature · 1 line
2.17.1
Amir Goldstein
maintainerPATCH v2 3/5 ovl: enforce 'strict' upper fs requirements with metacopy=on
In reply to Amir GoldsteinOverlayfs works sub-optimally with upper fs that has no xattr/d_type/O_TMPFILE/RENAME_WHITEOUT support. We should basically deprecate support for those filesystems, but so far, we only issue a warning and don't fail the mount for the sake of backward compat. Some features are already being disabled with no xattr support.
when user asks explicitly via mount option to enable the new metacopy feature, we do not need to worry about backward compatibility and we can fail the mount if upper fs is a sub-optimal filesystem.
--- fs/overlayfs/super.c | 65 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 60 insertions(+), 5 deletions(-)
fs/overlayfs/super.c +60 -5
@@ -1054,10 +1054,50 @@ static int ovl_get_upper(struct ovl_fs *ofs, struct path *upperpath)return err; } +static int ovl_check_rename_whiteout (struct dentry *workdir) +{ +struct inode *dir =d_inode (workdir); +struct dentry *whiteout; +struct dentry *temp; +int err; + +/* FIXME */ +return 1 ; + +inode_lock_nested (dir,I_MUTEX_PARENT ); + + temp =ovl_create_temp (workdir,OVL_CATTR (S_IFREG |0 )); + err =PTR_ERR (temp); +if (IS_ERR (temp)) + goto out_unlock; + + whiteout =ovl_create_temp (workdir,OVL_CATTR (S_IFREG |0 )); + err =PTR_ERR (whiteout); +if (IS_ERR (whiteout)) + goto out_cleanup_temp; + + err =ovl_do_rename (dir, whiteout, dir, temp,RENAME_WHITEOUT ); +if (!err &&ovl_is_whiteout (whiteout)) + err =1 ; + +/* Best effort cleanup of temp files from workdir */ +ovl_cleanup (dir, whiteout); +dput (whiteout); +out_cleanup_temp: +ovl_cleanup (dir, temp); +dput (temp); +out_unlock: +inode_unlock (dir); + +return err; +} +static int ovl_make_workdir (struct ovl_fs *ofs,struct path *workpath) {struct vfsmount *mnt = ofs->upper_mnt;struct dentry *temp; +bool rename_whiteout; +bool d_type;int fh_type;int err;
@@ -1079,11 +1119,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)if (err <0 ) goto out; -/* -* We allowed this configuration and don't want to break users over -* kernel upgrade. So warn instead of erroring out. -*/ -if (!err) + d_type = err; +if (!d_type)pr_warn ("overlayfs: upper fs needs to support d_type.\n" );/* Check if upper/work fs supports O_TMPFILE */
@@ -1094,6 +1131,16 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)else pr_warn ("overlayfs: upper fs does not support tmpfile.\n" ); + +/* Check if upper/work fs supports RENAME_WHITEOUT */ + err =ovl_check_rename_whiteout (ofs->workdir); +if (err <0 ) + goto out; + + rename_whiteout = err; +if (!rename_whiteout) +pr_warn ("overlayfs: upper fs does not support RENAME_WHITEOUT.\n" ); +/* * Check if upper/work fs supports trusted.overlay.* xattr */
@@ -1105,6 +1152,14 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)vfs_removexattr (ofs->workdir,OVL_XATTR_OPAQUE ); } +/* With 'strict' policy, sub-optimal upper fs are not allowed */ +if (ofs->config.strict && + (!d_type || !ofs->tmpfile || !rename_whiteout || ofs->noxattr)) { +pr_err ("overlayfs: upper fs missing required features, mount with '-o strict=off' to override strict features check.\n" ); + err = -EINVAL ; + goto out; + } +/* metacopy depends on redirect_dir which depend on xattr */ err = ovl_feature_check(&ofs->config, &ofs->config.metacopy, !ofs->noxattr, "metadata only copy up",
Signature · 1 line
2.17.1
Amir Goldstein
maintainerPATCH v2 4/5 ovl: enforce 'strict' unique uuid requirement with metacopy=on
In reply to Amir GoldsteinWith metacopy=on, fail mount instead of disabling index feature on detection of non unique uuid among underlying filesystems.
--- fs/overlayfs/super.c | 6 ++++++ 1 file changed, 6 insertions(+)
fs/overlayfs/super.c +6 -0
@@ -1323,6 +1323,12 @@ static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path)pr_warn ("overlayfs: %s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n" ,uuid_is_null (&sb->s_uuid) ?"null" :"conflicting" , path->dentry); + + err =ovl_feature_requires (&ofs->config, +"NFS export and inode index" , +"unique uuid with multiple underlying fs" ); +if (err) +return err; } err =get_anon_bdev (&dev);
Signature · 1 line
2.17.1
Amir Goldstein
maintainerQuoted 9 lines · Amir Goldstein
Vivek, Miklos,
This series passes overlay/quick xfstests and I verified manually some expected mount failures with metacopy=on and override with metacopy=on,strict=off.
Still needs very carefull review and the
ovl_check_rename_whiteout()helper in patch 3 is broken, so I disabled it for now.
Pushed a fix of ovl_check_rename_whiteout() to:
https://github.com/amir73il/linux/commits/overlayfs-devel
For simplicity, it leaves the whiteout behind.
Verified that metacopy=on fails on ext2 and metacopy=on,strict=off works on ext2.
Please test!
Thanks,
Amir.
Quoted 30 lines
Patches 1-3 are marked for stable apply cleanly on v4.19. Patch 4 doesn't apply to v4.19.
Patch 5 will probably apply, but not sure it is stable material.I did not change behavior w.r.t enabling of
redirect_dir, because it involves many corner cases and I don't think it matters for stable. We can always improve it later and let some mount configurations that used to fail succeed with expected user requested mount options. When we address the metacopy =>redirect_dirdependency, we should also address thenfs_export=> index dependency in a similar manner.Thanks,
Amir.Amir Goldstein (5):
ovl: return error on mount if metacopy cannot be enabled
ovl: enforce 'strict' feature requirements with metacopy=on
ovl: enforce 'strict' upper fs requirements with metacopy=on
ovl: enforce 'strict' unique uuid requirement with metacopy=on
ovl: enforce 'strict' upper fs and feature requirements with strict=onfs/overlayfs/Kconfig | 23 ++++ fs/overlayfs/ovl_entry.h | 1 + fs/overlayfs/super.c | 235 ++++++++++++++++++++++++++++++--------- 3 files changed, 208 insertions(+), 51 deletions(-)--
2.17.1
Amir Goldstein
maintainerPATCH v2 5/5 ovl: enforce 'strict' upper fs and feature requirements with strict=on
In reply to Amir GoldsteinOverlayfs works sub-optimally with upper fs that has no xattr/d_type/O_TMPFILE/RENAME_WHITEOUT support. We should basically deprecate support for those filesystems, but so far, we only issue a warning and don't fail the mount for the sake of backward compat. Some features are already being disabled with no xattr support in upper fs and with no file handle support in underlying fs.
when user asks explicitly via Kconfig/module/mount option to enable strict feature requirements, we do not need to worry about backward compatibility and we can fail the mount if upper fs is a sub-optimal filesystem or if any of the feature requirements are not met.
So far, the 'strict' behavior was enabled implicitly for metacopy=on. With that change, the 'strcit' behavior can be enabled regardless of metacopy=on and can be relaxed even with metacopy, with mount options "metacopy=on,strict=off" (order is important).
--- fs/overlayfs/Kconfig | 23 +++++++++++++++++++++++ fs/overlayfs/super.c | 20 ++++++++++++++++++++ 2 files changed, 43 insertions(+)
fs/overlayfs/Kconfig +23 -0
@@ -109,6 +109,7 @@ config OVERLAY_FS_METACOPY bool "Overlayfs: turn on metadata only copy up feature by default" depends on OVERLAY_FS select OVERLAY_FS_REDIRECT_DIR + select OVERLAY_FS_STRICT help If this config option is enabled then overlay filesystems will copy up only metadata where appropriate and data copy up will
@@ -122,3 +123,25 @@ config OVERLAY_FS_METACOPY that doesn't support this feature will have unexpected results. If unsure, say N. + +config OVERLAY_FS_STRICT + bool "Overlayfs: turn on strict requirements by default" + depends on OVERLAY_FS + help + The overlayfs filesystem is fully functional and optimal when the + underlying filesystems support extended attributes, d_type values + in readdir, O_TMPFILE and RENAME_WHITEOUT. In addition, some + overlay filesystem features require that underlying filesystems + support exporting NFS file handles. + + If this config option is enabled, then overlay filesystem mount + will fail if any of the requirements from underlying filesystems + are not met. When strict requirements are enabled, overlay filesystem + mount will also fail if any of the mount options are conflicting, + for example: "nfs_export=on,metacopy=on". + + If this config option is enabled, it is still possible to turn off + strict requirements globally with the "strict=off" module option or + on a filesystem instance basis with the "strict=off" mount option. + + If unsure, say N.
fs/overlayfs/super.c +20 -0
@@ -61,6 +61,11 @@ module_param_named(metacopy, ovl_metacopy_def, bool, 0644);MODULE_PARM_DESC (ovl_metacopy_def,"Default to on or off for the metadata only copy up feature" ); +static bool ovl_strict_def =IS_ENABLED (CONFIG_OVERLAY_FS_STRICT ); +module_param_named (strict, ovl_strict_def, bool,0644 ); +MODULE_PARM_DESC (ovl_strict_def, +"Default to on or off for strict feature requirements" ); +static int ovl_feature_requires (struct ovl_config *config,const char *feature,const char *requirement) {
@@ -401,6 +406,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)if (ofs->config.metacopy != ovl_metacopy_def)seq_printf (m,",metacopy=%s" , ofs->config.metacopy ?"on" :"off" ); +if (ofs->config.strict != ovl_strict_def) +seq_printf (m,",strict=%s" , ofs->config.strict ?"on" :"off" );return 0 ; }
@@ -440,6 +447,8 @@ enum {OPT_XINO_AUTO ,OPT_METACOPY_ON ,OPT_METACOPY_OFF , +OPT_STRICT_ON , +OPT_STRICT_OFF , OPT_ERR, };
@@ -458,6 +467,8 @@ static const match_table_t ovl_tokens = { {OPT_XINO_AUTO, "xino=auto"}, {OPT_METACOPY_ON, "metacopy=on"}, {OPT_METACOPY_OFF, "metacopy=off"}, + {OPT_STRICT_ON, "strict=on"}, + {OPT_STRICT_OFF, "strict=off"}, {OPT_ERR, NULL} };
@@ -594,6 +605,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) config->metacopy = false;break ; +case OPT_STRICT_ON : + config->strict = true; +break ; + +case OPT_STRICT_OFF : + config->strict = false; +break ; +default :pr_err ("overlayfs: unrecognized mount option \"%s\" or missing value\n" , p);return -EINVAL ;
@@ -1533,6 +1552,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) ofs->config.nfs_export = ovl_nfs_export_def; ofs->config.xino =ovl_xino_def (); ofs->config.metacopy = ovl_metacopy_def; + ofs->config.strict = ovl_strict_def; err =ovl_parse_opt ((char *) data, &ofs->config);if (err) goto out_err;
Signature · 1 line
2.17.1
Quoted 13 lines · Amir Goldstein
Vivek, Miklos,
This series passes overlay/quick xfstests and I verified manually some expected mount failures with metacopy=on and override with metacopy=on,strict=off.
Still needs very carefull review and the
ovl_check_rename_whiteout()helper in patch 3 is broken, so I disabled it for now.Patches 1-3 are marked for stable apply cleanly on v4.19. Patch 4 doesn't apply to v4.19.
Patch 5 will probably apply, but not sure it is stable material.
Quoted 6 lines
I did not change behavior w.r.t enabling of
redirect_dir, because it involves many corner cases and I don't think it matters for stable. We can always improve it later and let some mount configurations that used to fail succeed with expected user requested mount options. When we address the metacopy =>redirect_dirdependency, we should also address thenfs_export=> index dependency in a similar manner.
I am wondering why are we rushing in "strict" into 4.19. I understand that going forward we want to do something but what's the rush that it has to be enabled under "metacopy" only.
Given, metacopy has only been shipped in 4.19, why not do it this way.
- In 4.19 and 4.20 just ship the simple behavior where if user passes metacopy=on, it is not disabled and user will instead get -EINVAL.
- Work on proper semantics for ->strict interface and get it included in 4.21. Whenver overlayfs is mounted and ->strict is not on, print a warning in logs saying it is recommended to run with "strict=on".
- Whenever a new knob in overlayfs is introduced, make sure it automatically sets ->strict=on.
IOW, while I see the need of ->strict, I don't see the need of necessarily enabling it on using metacopy. I think we are trying to rush it in.
Its probably better to not couple ->strict and metacopy at this point of time. First get "strict" feature upstream in 4.21 and then couple it with future mount options we introduce.
Thanks
Vivek
Quoted 19 lines
Thanks,
Amir.Amir Goldstein (5):
ovl: return error on mount if metacopy cannot be enabled
ovl: enforce 'strict' feature requirements with metacopy=on
ovl: enforce 'strict' upper fs requirements with metacopy=on
ovl: enforce 'strict' unique uuid requirement with metacopy=on
ovl: enforce 'strict' upper fs and feature requirements with strict=onfs/overlayfs/Kconfig | 23 ++++ fs/overlayfs/ovl_entry.h | 1 + fs/overlayfs/super.c | 235 ++++++++++++++++++++++++++++++--------- 3 files changed, 208 insertions(+), 51 deletions(-)Signature · 1 line
2.17.1
Amir Goldstein
maintainerQuoted 38 lines · Vivek Goyal
Quoted 13 lines · Amir Goldstein
Vivek, Miklos,
This series passes overlay/quick xfstests and I verified manually some expected mount failures with metacopy=on and override with metacopy=on,strict=off.
Still needs very carefull review and the
ovl_check_rename_whiteout()helper in patch 3 is broken, so I disabled it for now.Patches 1-3 are marked for stable apply cleanly on v4.19. Patch 4 doesn't apply to v4.19.
Patch 5 will probably apply, but not sure it is stable material.Quoted 6 lines
I did not change behavior w.r.t enabling of
redirect_dir, because it involves many corner cases and I don't think it matters for stable. We can always improve it later and let some mount configurations that used to fail succeed with expected user requested mount options. When we address the metacopy =>redirect_dirdependency, we should also address thenfs_export=> index dependency in a similar manner.I am wondering why are we rushing in "strict" into 4.19. I understand that going forward we want to do something but what's the rush that it has to be enabled under "metacopy" only.
Given, metacopy has only been shipped in 4.19, why not do it this way.
- In 4.19 and 4.20 just ship the simple behavior where if user passes metacopy=on, it is not disabled and user will instead get
-EINVAL.- Work on proper semantics for ->strict interface and get it included in 4.21. Whenver overlayfs is mounted and ->strict is not on, print a warning in logs saying it is recommended to run with "strict=on".
- Whenever a new knob in overlayfs is introduced, make sure it automatically sets ->strict=on.
Quoted 7 lines
IOW, while I see the need of ->strict, I don't see the need of necessarily enabling it on using metacopy. I think we are trying to rush it in.
Its probably better to not couple ->strict and metacopy at this point of time. First get "strict" feature upstream in 4.21 and then couple it with future mount options we introduce.
I agree that we should not rush "strict" this late in the cycle and any fix to metacopy=on should be after rc1.
My concerns regarding metacopy=on behavior:
- Should it fail with noxattr in v4.20/v4.19?
- Should it still fail with noxattr after upgrade to v4.21 with default STRICT=n?
- Should it still fail with noxattr after upgrade to v4.21 with mount strict=on?
- Same questions for default REDIRECT_DIR=n and mount redirect_dir=off
I have *no concerns* with metacopy=on implying redirect_dir=on for v4.20/v4.19. (a.k.a Miklos' proposal).
I do not understand why we *have to* implement any sort of strict behavior for metacopy in v4.20/v4.19.
User can always work around this issue by checking resulting mount options in /proc/mounts after mount.
IMO "strict" behavior for metacopy=on is just a convenience feature, not a bug fix.
Thanks,
Amir.
Quoted 47 lines · Amir Goldstein
Quoted 38 lines · Vivek Goyal
Quoted 13 lines · Amir Goldstein
Vivek, Miklos,
This series passes overlay/quick xfstests and I verified manually some expected mount failures with metacopy=on and override with metacopy=on,strict=off.
Still needs very carefull review and the
ovl_check_rename_whiteout()helper in patch 3 is broken, so I disabled it for now.Patches 1-3 are marked for stable apply cleanly on v4.19. Patch 4 doesn't apply to v4.19.
Patch 5 will probably apply, but not sure it is stable material.Quoted 6 lines
I did not change behavior w.r.t enabling of
redirect_dir, because it involves many corner cases and I don't think it matters for stable. We can always improve it later and let some mount configurations that used to fail succeed with expected user requested mount options. When we address the metacopy =>redirect_dirdependency, we should also address thenfs_export=> index dependency in a similar manner.I am wondering why are we rushing in "strict" into 4.19. I understand that going forward we want to do something but what's the rush that it has to be enabled under "metacopy" only.
Given, metacopy has only been shipped in 4.19, why not do it this way.
- In 4.19 and 4.20 just ship the simple behavior where if user passes metacopy=on, it is not disabled and user will instead get
-EINVAL.- Work on proper semantics for ->strict interface and get it included in 4.21. Whenver overlayfs is mounted and ->strict is not on, print a warning in logs saying it is recommended to run with "strict=on".
- Whenever a new knob in overlayfs is introduced, make sure it automatically sets ->strict=on.
Quoted 7 lines
IOW, while I see the need of ->strict, I don't see the need of necessarily enabling it on using metacopy. I think we are trying to rush it in.
Its probably better to not couple ->strict and metacopy at this point of time. First get "strict" feature upstream in 4.21 and then couple it with future mount options we introduce.
Quoted 5 lines
I agree that we should not rush "strict" this late in the cycle and any fix to metacopy=on should be after rc1.
My concerns regarding metacopy=on behavior:
- Should it fail with noxattr in v4.20/v4.19?
I would think it should fail if user passed metacopy=on as mount option. Otherwise we can disable metacopy.
Quoted 2 lines
- Should it still fail with noxattr after upgrade to v4.21 with default STRICT=n?
I would prefer that way. That is all the new mount options are enforced if passed as mount option. If there is a strong reason to deviate, then may be we can relax it in some cases with strict=off. For example, discards is an optional feature and both ext4/xfs don't fail mount if block device does not support discard (IIUC).
Quoted 1 line
- Should it still fail with noxattr after upgrade to v4.21 with mount strict=on?
Yes. If user passed it in mount option, it needs to be enforced, IMHO. It should not matter if strict=on/off.
To me strict seems to be there for two things.
- Making sure overlayfs runs as optimal configuration.
- Making sure old knobs now error out (and not be disabled).
I would think strict does not affect the new knobs and also it should not affect the behavior of default values coming from module/Kconfig.
Quoted 4 lines
- Same questions for default REDIRECT_DIR=n and mount redirect_dir=off
I have *no concerns* with metacopy=on implying redirect_dir=on for v4.20/v4.19. (a.k.a Miklos' proposal).
For 4.19, even if metacopy=on does not imply redirect_dir=on, I am fine with it. We can always make this change in 4.21, without breaking backward compatibility.
Quoted 2 lines
I do not understand why we *have to* implement any sort of strict behavior for metacopy in v4.20/v4.19.
Because we agreed that it makes sense that if we can't honor the behavior user asked for as mount option, we should fail mount, instead of disabling silently.
If we don't do it in 4.19 (while we still have opportunity), then we have same backward comatibility issue and will have to rely on strict=on.
I would think my initial patch was much simpler which simply returned error if metacopy=on could not be honored. We should consider taking that in for 4.19 stable and 4.20. And then take time to resolve all issues around automatic enabling of redirect_dir and all the issues around strict and push that in 4.21.
Quoted 2 lines
User can always work around this issue by checking resulting mount options in
/proc/mountsafter mount.
This is extra step users have to do. I think its nicer to let mount fail.
Secondly, if metacopy is enabled in module/Kconfig, then /proc/mounts will not show it (if metacopy=on succeeded), right? So how will a user figure out if metacopy is on or not. I thought /proc/mounts will say metacopy=on only if module/Kconfig did not enable it and it was enabled using mount option.
Quoted 2 lines
IMO "strict" behavior for metacopy=on is just a convenience feature, not a bug fix.
Agreed.
Thanks
Vivek