[PATCH] ovl: resolve more conflicting mount options

[PATCH] ovl: resolve more conflicting mount options

[Amir Goldstein]

Similar to the way that a conflict between metacopy=on,redirect_dir=off
is resolved, also resolve conflicts between nfs_export=on,index=off and
nfs_export=on,metacopy=on.

An explicit mount option wins over a default config value.
Both explicit mount options result in an error.

Without this change the xfstests group overlay/exportfs are skipped if
metacopy is enabled by default.

Reported-by: Chengguang Xu <cgxu519@mykernel.net>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 Documentation/filesystems/overlayfs.rst |  7 ++--
 fs/overlayfs/super.c                    | 48 +++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index c9d2bf96b02d..660dbaf0b9b8 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -365,8 +365,8 @@ pointed by REDIRECT. This should not be possible on local system as setting
 "trusted." xattrs will require CAP_SYS_ADMIN. But it should be possible
 for untrusted layers like from a pen drive.
 
-Note: redirect_dir={off|nofollow|follow[*]} conflicts with metacopy=on, and
-results in an error.
+Note: redirect_dir={off|nofollow|follow[*]} and nfs_export=on mount options
+conflict with metacopy=on, and will result in an error.
 
 [*] redirect_dir=follow only conflicts with metacopy=on if upperdir=... is
 given.
@@ -560,6 +560,9 @@ When the NFS export feature is enabled, all directory index entries are
 verified on mount time to check that upper file handles are not stale.
 This verification may cause significant overhead in some cases.
 
+Note: the mount options index=off,nfs_export=on are conflicting and will
+result in an error.
+
 
 Testsuite
 ---------
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 732ad5495c92..fbd6207acdbf 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -470,6 +470,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 	char *p;
 	int err;
 	bool metacopy_opt = false, redirect_opt = false;
+	bool nfs_export_opt = false, index_opt = false;
 
 	config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
 	if (!config->redirect_mode)
@@ -519,18 +520,22 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 
 		case OPT_INDEX_ON:
 			config->index = true;
+			index_opt = true;
 			break;
 
 		case OPT_INDEX_OFF:
 			config->index = false;
+			index_opt = true;
 			break;
 
 		case OPT_NFS_EXPORT_ON:
 			config->nfs_export = true;
+			nfs_export_opt = true;
 			break;
 
 		case OPT_NFS_EXPORT_OFF:
 			config->nfs_export = false;
+			nfs_export_opt = true;
 			break;
 
 		case OPT_XINO_ON:
@@ -552,6 +557,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 
 		case OPT_METACOPY_OFF:
 			config->metacopy = false;
+			metacopy_opt = true;
 			break;
 
 		default:
@@ -601,6 +607,48 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		}
 	}
 
+	/* Resolve nfs_export -> index dependency */
+	if (config->nfs_export && !config->index) {
+		if (nfs_export_opt && index_opt) {
+			pr_err("conflicting options: nfs_export=on,index=off\n");
+			return -EINVAL;
+		}
+		if (index_opt) {
+			/*
+			 * There was an explicit index=off that resulted
+			 * in this conflict.
+			 */
+			pr_info("disabling nfs_export due to index=off\n");
+			config->nfs_export = false;
+		} else {
+			/* Automatically enable index otherwise. */
+			config->index = true;
+		}
+	}
+
+	/* Resolve nfs_export -> !metacopy dependency */
+	if (config->nfs_export && config->metacopy) {
+		if (nfs_export_opt && metacopy_opt) {
+			pr_err("conflicting options: nfs_export=on,metacopy=on\n");
+			return -EINVAL;
+		}
+		if (metacopy_opt) {
+			/*
+			 * There was an explicit metacopy=on that resulted
+			 * in this conflict.
+			 */
+			pr_info("disabling nfs_export due to metacopy=on\n");
+			config->nfs_export = false;
+		} else {
+			/*
+			 * There was an explicit nfs_export=on that resulted
+			 * in this conflict.
+			 */
+			pr_info("disabling metacopy due to nfs_export=on\n");
+			config->metacopy = false;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.17.1

Re: [PATCH] ovl: resolve more conflicting mount options

[Vivek Goyal]

On Thu, Apr 09, 2020 at 07:39:02PM +0300, Amir Goldstein wrote:
> Similar to the way that a conflict between metacopy=on,redirect_dir=off
> is resolved, also resolve conflicts between nfs_export=on,index=off and
> nfs_export=on,metacopy=on.
> 
> An explicit mount option wins over a default config value.
> Both explicit mount options result in an error.
> 
> Without this change the xfstests group overlay/exportfs are skipped if
> metacopy is enabled by default.
> 
> Reported-by: Chengguang Xu <cgxu519@mykernel.net>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  Documentation/filesystems/overlayfs.rst |  7 ++--
>  fs/overlayfs/super.c                    | 48 +++++++++++++++++++++++++
>  2 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index c9d2bf96b02d..660dbaf0b9b8 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -365,8 +365,8 @@ pointed by REDIRECT. This should not be possible on local system as setting
>  "trusted." xattrs will require CAP_SYS_ADMIN. But it should be possible
>  for untrusted layers like from a pen drive.
>  
> -Note: redirect_dir={off|nofollow|follow[*]} conflicts with metacopy=on, and
> -results in an error.
> +Note: redirect_dir={off|nofollow|follow[*]} and nfs_export=on mount options
> +conflict with metacopy=on, and will result in an error.
>  
>  [*] redirect_dir=follow only conflicts with metacopy=on if upperdir=... is
>  given.
> @@ -560,6 +560,9 @@ When the NFS export feature is enabled, all directory index entries are
>  verified on mount time to check that upper file handles are not stale.
>  This verification may cause significant overhead in some cases.
>  
> +Note: the mount options index=off,nfs_export=on are conflicting and will
> +result in an error.
> +
>  
>  Testsuite
>  ---------
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 732ad5495c92..fbd6207acdbf 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -470,6 +470,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  	char *p;
>  	int err;
>  	bool metacopy_opt = false, redirect_opt = false;
> +	bool nfs_export_opt = false, index_opt = false;
>  
>  	config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
>  	if (!config->redirect_mode)
> @@ -519,18 +520,22 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  
>  		case OPT_INDEX_ON:
>  			config->index = true;
> +			index_opt = true;
>  			break;
>  
>  		case OPT_INDEX_OFF:
>  			config->index = false;
> +			index_opt = true;
>  			break;
>  
>  		case OPT_NFS_EXPORT_ON:
>  			config->nfs_export = true;
> +			nfs_export_opt = true;
>  			break;
>  
>  		case OPT_NFS_EXPORT_OFF:
>  			config->nfs_export = false;
> +			nfs_export_opt = true;
>  			break;
>  
>  		case OPT_XINO_ON:
> @@ -552,6 +557,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  
>  		case OPT_METACOPY_OFF:
>  			config->metacopy = false;
> +			metacopy_opt = true;

Hi Amir,

I am wondering why metacopy_opt needs to be set for OPT_METACOPY_OFF case.
In this case config->metacopy=false and it does not conflict with
config->nfs_export at all. So there is no need to know if metacopy=off
was specified as mount option or not.

Vivek

>  			break;
>  
>  		default:
> @@ -601,6 +607,48 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  		}
>  	}
>  
> +	/* Resolve nfs_export -> index dependency */
> +	if (config->nfs_export && !config->index) {
> +		if (nfs_export_opt && index_opt) {
> +			pr_err("conflicting options: nfs_export=on,index=off\n");
> +			return -EINVAL;
> +		}
> +		if (index_opt) {
> +			/*
> +			 * There was an explicit index=off that resulted
> +			 * in this conflict.
> +			 */
> +			pr_info("disabling nfs_export due to index=off\n");
> +			config->nfs_export = false;
> +		} else {
> +			/* Automatically enable index otherwise. */
> +			config->index = true;
> +		}
> +	}
> +
> +	/* Resolve nfs_export -> !metacopy dependency */
> +	if (config->nfs_export && config->metacopy) {
> +		if (nfs_export_opt && metacopy_opt) {
> +			pr_err("conflicting options: nfs_export=on,metacopy=on\n");
> +			return -EINVAL;
> +		}
> +		if (metacopy_opt) {
> +			/*
> +			 * There was an explicit metacopy=on that resulted
> +			 * in this conflict.
> +			 */
> +			pr_info("disabling nfs_export due to metacopy=on\n");
> +			config->nfs_export = false;
> +		} else {
> +			/*
> +			 * There was an explicit nfs_export=on that resulted
> +			 * in this conflict.
> +			 */
> +			pr_info("disabling metacopy due to nfs_export=on\n");
> +			config->metacopy = false;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.17.1
> 

Re: [PATCH] ovl: resolve more conflicting mount options

[Amir Goldstein]

On Fri, Apr 10, 2020 at 12:49 AM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Thu, Apr 09, 2020 at 07:39:02PM +0300, Amir Goldstein wrote:
> > Similar to the way that a conflict between metacopy=on,redirect_dir=off
> > is resolved, also resolve conflicts between nfs_export=on,index=off and
> > nfs_export=on,metacopy=on.
> >
> > An explicit mount option wins over a default config value.
> > Both explicit mount options result in an error.
> >
> > Without this change the xfstests group overlay/exportfs are skipped if
> > metacopy is enabled by default.
> >
> > Reported-by: Chengguang Xu <cgxu519@mykernel.net>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  Documentation/filesystems/overlayfs.rst |  7 ++--
> >  fs/overlayfs/super.c                    | 48 +++++++++++++++++++++++++
> >  2 files changed, 53 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> > index c9d2bf96b02d..660dbaf0b9b8 100644
> > --- a/Documentation/filesystems/overlayfs.rst
> > +++ b/Documentation/filesystems/overlayfs.rst
> > @@ -365,8 +365,8 @@ pointed by REDIRECT. This should not be possible on local system as setting
> >  "trusted." xattrs will require CAP_SYS_ADMIN. But it should be possible
> >  for untrusted layers like from a pen drive.
> >
> > -Note: redirect_dir={off|nofollow|follow[*]} conflicts with metacopy=on, and
> > -results in an error.
> > +Note: redirect_dir={off|nofollow|follow[*]} and nfs_export=on mount options
> > +conflict with metacopy=on, and will result in an error.
> >
> >  [*] redirect_dir=follow only conflicts with metacopy=on if upperdir=... is
> >  given.
> > @@ -560,6 +560,9 @@ When the NFS export feature is enabled, all directory index entries are
> >  verified on mount time to check that upper file handles are not stale.
> >  This verification may cause significant overhead in some cases.
> >
> > +Note: the mount options index=off,nfs_export=on are conflicting and will
> > +result in an error.
> > +
> >
> >  Testsuite
> >  ---------
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 732ad5495c92..fbd6207acdbf 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -470,6 +470,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> >       char *p;
> >       int err;
> >       bool metacopy_opt = false, redirect_opt = false;
> > +     bool nfs_export_opt = false, index_opt = false;
> >
> >       config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
> >       if (!config->redirect_mode)
> > @@ -519,18 +520,22 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> >
> >               case OPT_INDEX_ON:
> >                       config->index = true;
> > +                     index_opt = true;
> >                       break;
> >
> >               case OPT_INDEX_OFF:
> >                       config->index = false;
> > +                     index_opt = true;
> >                       break;
> >
> >               case OPT_NFS_EXPORT_ON:
> >                       config->nfs_export = true;
> > +                     nfs_export_opt = true;
> >                       break;
> >
> >               case OPT_NFS_EXPORT_OFF:
> >                       config->nfs_export = false;
> > +                     nfs_export_opt = true;
> >                       break;
> >
> >               case OPT_XINO_ON:
> > @@ -552,6 +557,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> >
> >               case OPT_METACOPY_OFF:
> >                       config->metacopy = false;
> > +                     metacopy_opt = true;
>
> Hi Amir,
>
> I am wondering why metacopy_opt needs to be set for OPT_METACOPY_OFF case.
> In this case config->metacopy=false and it does not conflict with
> config->nfs_export at all. So there is no need to know if metacopy=off
> was specified as mount option or not.
>

It's true. We can drop that one.
I just liked it better that the meaning of the _opt vars are
"was this value set explicitly", even before we get to using it.

Thanks,
Amir.