[PATCH v2 0/5] Overlayfs strict feature requirements

[PATCH v2 0/5] Overlayfs strict feature requirements

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

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(-)

-- 
2.17.1

[PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled

[Amir Goldstein]

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.

Reported-by: Daniel Walsh <dwalsh@redhat.com>
Suggested-by: Vivek Goyal <vgoyal@redhat.com>
Fixes: d5791044d2e5 ("ovl: Provide a mount option metacopy=on/off...")
Cc: <stable@vger.kernel.org> # v4.19
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     | 74 +++++++++++++++++++++++++++++++---------
 2 files changed, 59 insertions(+), 16 deletions(-)

diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index ec237035333a..fb819aec46c0 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -20,6 +20,7 @@ struct ovl_config {
 	bool nfs_export;
 	int xino;
 	bool metacopy;
+	bool strict;
 };
 
 struct ovl_sb {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 22ffb23ea44d..d91d2ba70d54 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -56,6 +56,50 @@ module_param_named(xino_auto, ovl_xino_auto_def, bool, 0644);
 MODULE_PARM_DESC(ovl_xino_auto_def,
 		 "Auto enable xino feature");
 
+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 int ovl_feature_requires(struct ovl_config *config, const char *feature,
+				const char *requirement)
+{
+	if (config->strict) {
+		pr_err("overlayfs: %s requires %s.\n", feature, requirement);
+		return -EINVAL;
+	}
+
+	pr_warn("overlayfs: %s requies %s, disabling %s feature.\n", feature,
+		requirement, feature);
+
+	return 0;
+}
+
+/*
+ * 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.
+ */
+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;
+
+	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;
@@ -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;
 			break;
 
 		case OPT_METACOPY_OFF:
@@ -573,15 +613,13 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		return err;
 
 	/* metacopy feature with upper requires redirect_dir=on */
-	if (config->upperdir && config->metacopy && !config->redirect_dir) {
-		pr_warn("overlayfs: metadata only copy up requires \"redirect_dir=on\", falling back to metacopy=off.\n");
-		config->metacopy = false;
-	} else if (config->metacopy && !config->redirect_follow) {
-		pr_warn("overlayfs: metadata only copy up requires \"redirect_dir=follow\" on non-upper mount, falling back to metacopy=off.\n");
-		config->metacopy = false;
-	}
+	err = ovl_feature_check(config, &config->metacopy,
+				config->upperdir ? config->redirect_dir :
+						   config->redirect_follow,
+				"metadata only copy up",
+				"\"redirect_dir=on\" or \"redirect_dir=follow\" on non-upper mount");
 
-	return 0;
+	return err;
 }
 
 #define OVL_WORKDIR_NAME "work"
@@ -1055,9 +1093,13 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 	if (err) {
 		ofs->noxattr = true;
 		ofs->config.index = false;
-		ofs->config.metacopy = false;
-		pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off and metacopy=off.\n");
-		err = 0;
+		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;
 	} else {
 		vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
 	}
-- 
2.17.1

Re: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled

[Vivek Goyal]

On Thu, Nov 01, 2018 at 02:48:09AM +0200, Amir Goldstein wrote:

[..]
> + * 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.
> + */
> +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.

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

[..]
> @@ -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

Re: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled

[Miklos Szeredi]

On Thu, Nov 1, 2018 at 2:03 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Nov 01, 2018 at 02:48:09AM +0200, Amir Goldstein wrote:

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

Re: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled

[Miklos Szeredi]

On Thu, Nov 1, 2018 at 2:11 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Nov 1, 2018 at 2:03 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> On Thu, Nov 01, 2018 at 02:48:09AM +0200, Amir Goldstein wrote:
>
>>>
>>>               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.

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

Re: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled

[Amir Goldstein]

On Thu, Nov 1, 2018 at 10:41 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, Nov 1, 2018 at 2:11 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > On Thu, Nov 1, 2018 at 2:03 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> On Thu, Nov 01, 2018 at 02:48:09AM +0200, Amir Goldstein wrote:
> >
> >>>
> >>>               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.
>
> ...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.

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

Re: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled

[Miklos Szeredi]

On Thu, Nov 1, 2018 at 10:22 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Nov 1, 2018 at 10:41 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>> On Thu, Nov 1, 2018 at 2:11 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> > On Thu, Nov 1, 2018 at 2:03 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> On Thu, Nov 01, 2018 at 02:48:09AM +0200, Amir Goldstein wrote:
>> >
>> >>>
>> >>>               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.
>>
>> ...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).

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

Re: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled

[Amir Goldstein]

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

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

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.

Re: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled

[Miklos Szeredi]

On Mon, Nov 5, 2018 at 1:57 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> > 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.
>
>> >
>> >> 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.
>>
>
> 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.

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_dir
nfs_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

Re: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled

[Amir Goldstein]

On Wed, Nov 7, 2018 at 1:26 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, Nov 5, 2018 at 1:57 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> >> > 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.
> >
> >> >
> >> >> 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.
> >>
> >
> > 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.
>
> 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.


> 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_dir
> nfs_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.
>

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.

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

Re: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled

[Miklos Szeredi]

On Wed, Nov 7, 2018 at 12:59 PM, Amir Goldstein <amir73il@gmail.com> wrote:

> 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?

Yes, that one as well.

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

We could do the option interdependency resolution after detecting features.

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

Makes sense.

Thanks,
Miklos

Re: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled

[Vivek Goyal]

On Thu, Nov 01, 2018 at 09:41:08PM +0100, Miklos Szeredi wrote:
> On Thu, Nov 1, 2018 at 2:11 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > On Thu, Nov 1, 2018 at 2:03 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> On Thu, Nov 01, 2018 at 02:48:09AM +0200, Amir Goldstein wrote:
> >
> >>>
> >>>               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.
> 
> ...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

Re: [PATCH v2 1/5] ovl: return error on mount if metacopy cannot be enabled

[Miklos Szeredi]

On Thu, Nov 1, 2018 at 10:25 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Nov 01, 2018 at 09:41:08PM +0100, Miklos Szeredi wrote:
>> On Thu, Nov 1, 2018 at 2:11 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> > On Thu, Nov 1, 2018 at 2:03 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> On Thu, Nov 01, 2018 at 02:48:09AM +0200, Amir Goldstein wrote:
>> >
>> >>>
>> >>>               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.
>>
>> ...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.

That's intentional.

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

Right, that's what we promise: same kernel configuration, same module
params, options copied from /proc/mounts -> same options enabled.

Thanks,
Miklos

[PATCH v2 2/5] ovl: enforce 'strict' feature requirements with metacopy=on

[Amir Goldstein]

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

Fixes: d5791044d2e5 ("ovl: Provide a mount option metacopy=on/off...")
Cc: <stable@vger.kernel.org> # v4.19
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 84 +++++++++++++++++++++++++-------------------
 1 file changed, 47 insertions(+), 37 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index d91d2ba70d54..20135dd28192 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -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;
-- 
2.17.1

[PATCH v2 3/5] ovl: enforce 'strict' upper fs requirements with metacopy=on

[Amir Goldstein]

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

Fixes: d5791044d2e5 ("ovl: Provide a mount option metacopy=on/off...")
Cc: <stable@vger.kernel.org> # v4.19
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 65 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 20135dd28192..4aca0cc67455 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -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",
-- 
2.17.1

[PATCH v2 4/5] ovl: enforce 'strict' unique uuid requirement with metacopy=on

[Amir Goldstein]

With metacopy=on, fail mount instead of disabling index feature on
detection of non unique uuid among underlying filesystems.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 4aca0cc67455..d848112f4fc9 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -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);
-- 
2.17.1

[PATCH v2 5/5] ovl: enforce 'strict' upper fs and feature requirements with strict=on

[Amir Goldstein]

Overlayfs 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).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/Kconfig | 23 +++++++++++++++++++++++
 fs/overlayfs/super.c | 20 ++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index 2ef91be2a04e..620e379959c2 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -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.
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index d848112f4fc9..b6fd86b0a1c8 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -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;
-- 
2.17.1

Re: [PATCH v2 0/5] Overlayfs strict feature requirements

[Amir Goldstein]

On Thu, Nov 1, 2018 at 2:48 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> 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.

>
> 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(-)
>
> --
> 2.17.1
>

Re: [PATCH v2 0/5] Overlayfs strict feature requirements

[Vivek Goyal]

On Thu, Nov 01, 2018 at 02:48:08AM +0200, Amir Goldstein wrote:
> 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.

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


> 
> 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(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH v2 0/5] Overlayfs strict feature requirements

[Amir Goldstein]

On Thu, Nov 1, 2018 at 3:16 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Thu, Nov 01, 2018 at 02:48:08AM +0200, Amir Goldstein wrote:
> > 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.
>
> 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.
>

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.

Re: [PATCH v2 0/5] Overlayfs strict feature requirements

[Vivek Goyal]

On Thu, Nov 01, 2018 at 03:42:46PM +0200, Amir Goldstein wrote:
> On Thu, Nov 1, 2018 at 3:16 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Thu, Nov 01, 2018 at 02:48:08AM +0200, Amir Goldstein wrote:
> > > 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.
> >
> > 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.
> >
> 
> 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.

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

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

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

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

> User can always work around this issue by checking resulting mount
> options in /proc/mounts after 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.

> IMO "strict" behavior for metacopy=on is just a convenience feature,
> not a bug fix.

Agreed.

Thanks
Vivek