[PATCH] ovl: disable new features for deprecated upper fs

[PATCH] ovl: disable new features for deprecated upper fs

[Amir Goldstein]

Overlayfs works sub-optimally with upper fs that has no
xattr/d_type/O_TMPFILE 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.

Such configurations, if they exist in the wild at all, get very
little to no testing coverage for new features, so disable all new
features for all of these untested configurations.

For example, when upper fs is created with mkfs.xfs -m crc=0 -n ftype=0
and all overlayfs features are enalbed by default, mount emits the
following warnings:

 overlayfs: upper fs needs to support d_type.
 ...upper fs missing required features, falling back to redirect_dir=off.
 ...upper fs missing required features, falling back to index=off.
 ...upper fs missing required features, falling back to xino=off.
 ...upper fs missing required features, falling back to metacopy=off.
 ...NFS export requires "index=on", falling back to nfs_export=off.

[backport hint: The new features started rolling in since kernel v4.10
 so different stable kernels will need different versions of this patch]

Cc: <stable@vger.kernel.org> # v4.10
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
Miklos,

I reccon we want this applied this to old stable kernels.
If you accpet this patch, I will prepare a backport version for LTS
4.14.

Thanks,
Amir.

 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     | 38 ++++++++++++++++++++++++++++++++++----
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index ec237035333a..0ca6bb86eba3 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -60,6 +60,7 @@ struct ovl_fs {
 	struct ovl_config config;
 	/* creds of process who forced instantiation of super block */
 	const struct cred *creator_cred;
+	bool d_type;
 	bool tmpfile;
 	bool noxattr;
 	/* Did we take the inuse lock? */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 333bba83074f..7d7008267ed0 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -468,6 +468,16 @@ static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode)
 	return 0;
 }
 
+static int ovl_set_redirect_mode(struct ovl_config *config, const char *mode)
+{
+	kfree(config->redirect_mode);
+	config->redirect_mode = kstrdup(mode, GFP_KERNEL);
+	if (!config->redirect_mode)
+		return -ENOMEM;
+
+	return ovl_parse_redirect_mode(config, mode);
+}
+
 static int ovl_parse_opt(char *opt, struct ovl_config *config)
 {
 	char *p;
@@ -1037,7 +1047,8 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath)
 	 * We allowed this configuration and don't want to break users over
 	 * kernel upgrade. So warn instead of erroring out.
 	 */
-	if (!err)
+	ofs->d_type = err;
+	if (!ofs->d_type)
 		pr_warn("overlayfs: upper fs needs to support d_type.\n");
 
 	/* Check if upper/work fs supports O_TMPFILE */
@@ -1054,14 +1065,33 @@ 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;
-		ofs->config.metacopy = false;
-		pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off and metacopy=off.\n");
+		pr_warn("overlayfs: upper fs does not support xattr.\n");
 		err = 0;
 	} else {
 		vfs_removexattr(ofs->workdir, OVL_XATTR_OPAQUE);
 	}
 
+	if (!ofs->d_type || !ofs->tmpfile || ofs->noxattr) {
+		if (ofs->config.redirect_dir) {
+			err = ovl_set_redirect_mode(&ofs->config, "off");
+			if (err)
+				goto out;
+			pr_warn("overlayfs: upper fs missing required features, falling back to redirect_dir=off.\n");
+		}
+		if (ofs->config.index) {
+			ofs->config.index = false;
+			pr_warn("overlayfs: upper fs missing required features, falling back to index=off.\n");
+		}
+		if (ofs->config.xino != OVL_XINO_OFF) {
+			ofs->config.xino = OVL_XINO_OFF;
+			pr_warn("overlayfs: upper fs missing required features, falling back to xino=off.\n");
+		}
+		if (ofs->config.metacopy) {
+			ofs->config.metacopy = false;
+			pr_warn("overlayfs: upper fs missing required features, falling back to metacopy=off.\n");
+		}
+	}
+
 	/* Check if upper/work fs supports file handles */
 	fh_type = ovl_can_decode_fh(ofs->workdir->d_sb);
 	if (ofs->config.index && !fh_type) {
-- 
2.17.1

Re: [PATCH] ovl: disable new features for deprecated upper fs

[Miklos Szeredi]

On Sat, Oct 6, 2018 at 8:10 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> Overlayfs works sub-optimally with upper fs that has no
> xattr/d_type/O_TMPFILE 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.
>
> Such configurations, if they exist in the wild at all, get very
> little to no testing coverage for new features, so disable all new
> features for all of these untested configurations.
>
> For example, when upper fs is created with mkfs.xfs -m crc=0 -n ftype=0
> and all overlayfs features are enalbed by default, mount emits the
> following warnings:
>
>  overlayfs: upper fs needs to support d_type.
>  ...upper fs missing required features, falling back to redirect_dir=off.
>  ...upper fs missing required features, falling back to index=off.
>  ...upper fs missing required features, falling back to xino=off.
>  ...upper fs missing required features, falling back to metacopy=off.
>  ...NFS export requires "index=on", falling back to nfs_export=off.
>
> [backport hint: The new features started rolling in since kernel v4.10
>  so different stable kernels will need different versions of this patch]

I do not have good feelings about this patch.  At least a little more
analysis about what are the requirements of each overlay feature would
be good.

Also, disabling a feature instead of failing the mount is not always
the right answer, as Vivek's recent report pointed out.   Maybe the
best thing would be for metacopy to simply fail if there are no
xattrs.   The others are a bit more risky to change because of
possible backward compatibility failures.

Not getting enough testing is not a valid reason to disable a feature.
We could allow artificially disabling xattr or tmpfile with an overlay
mount option to allow testing these cases (possibly gated with a
kernel config option to not let ordinary users mess with such testing
options).

For now I'll just reduce the scope of this patch to only disable those
features where it obviously makes no sense without xattr.

Not sure if tmpfile is actually required by any feature?

Thanks,
Miklos

Re: [PATCH] ovl: disable new features for deprecated upper fs

[Amir Goldstein]

[adding back CC list, which I accidentally dropped a few emails back..]

On Mon, Oct 29, 2018 at 10:18 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Oct 29, 2018 at 9:16 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Sat, Oct 27, 2018 at 06:50:42AM +0300, Amir Goldstein wrote:
> > [..]
> > > Rule of thumb: if you have to write so much to explain a feature to
> > > yourself it won't fly nicely in documentation and users will not understand
> > > how it works.
> > >
> > > The special casing of metacopy is really bad is that aspect.
> >
> > Hi Amir,
> >
> > I think I am not trying to special case metacopy. I am trying to think
> > a good reasonable behavior for any new parameter introduced.
> >
>
> OK, but having different behavior for metacopy and for nfs_export
> just because they were not introduced at the same time is inconsistent.
>
> > >
> > > Often when we need to think too much on an issue is because we resist to
> > > take the simple solution that we don't like.
> >
> > What do other filesystems do? If user asked for a certain feature and
> > passed that in option in mount option, disabling that silently (with a
> > warning message in logs), feels odd to me.
> >
>
> There is no consistent behavior in that regard AFAIK.
> I believe that is what happens with -o dax on xfs.
> That is what _require_scratch_dax() says.
>
> > >
> > > The simple solution is no implicit behavior everything user gets is
> > > explicitly configurable and explicitly displayed in /proc/mounts.
> > >
> > > Your analysis misses a very important aspect of /proc/mounts reflecting the
> > > nature of this mount and the fact that user can replay the options in
> > > /proc/mounts to get the same resulting mount in the future. It may not
> > > always be the case with filesystems, but some users will be surprised if it
> > > doesn't work.
> >
> > Hmm.., not sure about that. Where did I change the nature of /proc/mounts.
> > If we are enforcing an option and return error, then this point is moot.
> > If we are automatically disabling a feature, then its no different than
> > what we are doing now.
> >
> > Are you saying that things are currently broken and my proposal did not
> > address that? Or things are currently working and my proposal broke
> > that.
> >
>
> I am not really sure. Just let's try to keep that in mind -
> if we replay mount options from /proc/mounts they are expected
> to create a mount with same options at least for the same kernel/module
> config. I guess nothing you proposed breaks that.
>
> > >
> > > So the simple to implement and to document is either an explicit mount
> > > option determine the enforcement behavior for all options or new values to
> > > existing mount options that are different from the value of default
> > > enabled. For example:
> > >
> > > metacopy=require
> >
> > So your idea is that for every option we have two variant. One which
> > can be disabled silently and one which can't be disabled and error will
> > be returned.
> >
> > So metacopy=require means metacopy=on or metacopy=off. Somebody might
> > want to enforce metacopy=off (in case kernel enables it by default).
>
> I did not consider require-off.
> I did propose an alternative solution: -o incompat=[error|disable]
> which applies to *all* options provided in mount command - not
> only to metacopy, because it happens to be easy enough to change
> its behavior at this time.
>
> >
> > But how is much different then my idea. I am saying instead of having
> > two variants, make sure anything passed as mount option, is always
> > enforced.
> >
>
> I think your idea is good... if we only implemented it from the first feature
> (redirect_dir) it would have been awesome.
>
> > >
> > > It's so simple to understand, you don't even need to document it.
> >
> > Well, many of the rules I have mentioned are implicit with
> > metacopy=require. Some rules it does not address like automatically
> > enabling a feature (redirect_dir=on).
> >
>
> I agree that turning redirect_dir on makes sense, however
> redirect_dir=no_follow,metacopy=on should not work.
> We need to somehow make order in all the mess, but sadly, I just don't see it
> happening without a boring new mount/module/Kconfig option.
> I just don't see how we can sanely document all the behavior otherwise.
> I'll be happy to be proven wrong.
>
> > >
> > > It's so simple to implement, you don't even need to change all place that
> > > refer to config.metacopy as boolean when you turn it into an enum.
> >
> > I think at high level there are only two rules different.
> >
> > - I am saying anything passed as mount option is enfroced and returned
> >   error if it can't be enforced.
> >
> > - You are saying that anyting passed as mount option is overridable by
> >   default and create a new option if user wants to change that behavior.
> >
> > I think rest of the rules will more or less apply. Just that these
> > are not documented anywhere.
> >
> > Anyway, I am fine with any reasonable proposal. Just want to make sure
> > Dan Walsh and other users of metacopy are not surprised. Just think of
> > this.
> >
> > - A user passes in metacopy=on and gets -EINVAL (because kernel is old
> >   and does not support the feature).
> >
> > - A new kernel supports metacopy feature and user passes in metacopy=on
> >   and does not get any error, but still metacopy feature is not enabled.
> >
> > I think that should be fixed. A kernel not supporting the feature returns
> > an error if feature can't be enabled but a kernel supporting the feature
> > does not return an error, despite the fact that feature could not be
> > enabled.
> >
>
> The problem is real all right and we need to solve it, yet you did not provide
> any argument against an explicit new config option to opt-in for new sane
> behavior. I don't like it, but I don't see how we can avoid it.
>

Adding another terrible idea to add to the pile of non perfect solutions.
I never liked the 'feature=[on|off]' standard, which is different from the more
common standard of '[feature|nofeature]' mount options.

What if we interpreted mount options 'feature=[on|off]' as advisory and
mount options '[feature|nofeature]' as mandatory?

The mandatory semantics could mark the start of the mkfs.overlay era
(a.k.a overlayfs on-disk format v2).
Meaning that you can only mount with mandatory 'metacopy' if either:
- upperdir AND workdir are empty
OR
- layers where created with mkfs.overlay with mandatory 'metacopy'
OR
- overlay was always mounted in the past with mandatory 'metacopy'
OR
- fsck.overlay is used to check layers and add mandatory 'metacopy'

Zhangyi,

Do you have plans to post another version of overlayfs feature set
patches for kernel/overlayfs-progs anytime soon?

Thanks,
Amir.

Re: [PATCH] ovl: disable new features for deprecated upper fs

[zhangyi (F)]

On 2018/10/30 5:32, Amir Goldstein Wrote:
> [adding back CC list, which I accidentally dropped a few emails back..]
> 
[..]
> 
> Adding another terrible idea to add to the pile of non perfect solutions.
> I never liked the 'feature=[on|off]' standard, which is different from the more
> common standard of '[feature|nofeature]' mount options.
> 
> What if we interpreted mount options 'feature=[on|off]' as advisory and
> mount options '[feature|nofeature]' as mandatory?
> 
> The mandatory semantics could mark the start of the mkfs.overlay era
> (a.k.a overlayfs on-disk format v2).
> Meaning that you can only mount with mandatory 'metacopy' if either:
> - upperdir AND workdir are empty
> OR
> - layers where created with mkfs.overlay with mandatory 'metacopy'
> OR
> - overlay was always mounted in the past with mandatory 'metacopy'
> OR
> - fsck.overlay is used to check layers and add mandatory 'metacopy'
> 
> Zhangyi,
> 
> Do you have plans to post another version of overlayfs feature set
> patches for kernel/overlayfs-progs anytime soon?
> 

Hi Amir,

Yes, I've done about 70% of the work for kernel, but I haven't started
yet for overlayfs-progs. I've been quite busy lately, and have to
postpone this work. Anyway, I will try to post the next version as soon
as posible.

Thanks,
Yi.