[PATCH v4 0/3] option validation in remount

[PATCH v4 0/3] option validation in remount

[Chengguang Xu]

Some overlayfs mount options cannot be changed via remount,
but current remount does not return error even if we specify
different value to unchangeable options.

This patch series adjust option parsing to support remount
and return proper error message and code when recogonizing
unchangeable options in remount.

Thanks,
Chengguang.

---
Chengguang Xu (3):
  ovl: adjust option parsing for supporting remount
  ovl: move the code of ovl_remount() to behind ovl_parse_opt()
  ovl: return error when specifying unchangeable options in remount

 fs/overlayfs/super.c | 150 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 108 insertions(+), 42 deletions(-)

-- 
1.8.3.1

[PATCH v4 1/3] ovl: adjust option parsing for supporting remount

[Chengguang Xu]

Adjust ovl_parse_opt() and ovl_parse_redirect_mode() to support option
parsing in remount.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
Changes since v3:
- Delete redundant argument of ovl_parse_redirect_mode()

Changes since v2:
- Move default initialization of redirect_mode to fill_super()
just before calling parse_opt().
- Move workdir validation check to fill_super().
- Move remount option validation to ovl_remount().

Changes since v1:
- Add remount option validation to parse_opt()

 fs/overlayfs/super.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index e8551c9..e99864e 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -455,23 +455,26 @@ static char *ovl_next_opt(char **s)
 	return sbegin;
 }
 
-static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode)
+static int ovl_parse_redirect_mode(struct ovl_config *config)
 {
-	if (strcmp(mode, "on") == 0) {
+	if (!config->redirect_mode)
+		return 0;
+
+	if (strcmp(config->redirect_mode, "on") == 0) {
 		config->redirect_dir = true;
 		/*
 		 * Does not make sense to have redirect creation without
 		 * redirect following.
 		 */
 		config->redirect_follow = true;
-	} else if (strcmp(mode, "follow") == 0) {
+	} else if (strcmp(config->redirect_mode, "follow") == 0) {
 		config->redirect_follow = true;
-	} else if (strcmp(mode, "off") == 0) {
+	} else if (strcmp(config->redirect_mode, "off") == 0) {
 		if (ovl_redirect_always_follow)
 			config->redirect_follow = true;
-	} else if (strcmp(mode, "nofollow") != 0) {
+	} else if (strcmp(config->redirect_mode, "nofollow") != 0) {
 		pr_err("overlayfs: bad mount option \"redirect_dir=%s\"\n",
-		       mode);
+		       config->redirect_mode);
 		return -EINVAL;
 	}
 
@@ -482,10 +485,6 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 {
 	char *p;
 
-	config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
-	if (!config->redirect_mode)
-		return -ENOMEM;
-
 	while ((p = ovl_next_opt(&opt)) != NULL) {
 		int token;
 		substring_t args[MAX_OPT_ARGS];
@@ -561,15 +560,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		}
 	}
 
-	/* Workdir is useless in non-upper mount */
-	if (!config->upperdir && config->workdir) {
-		pr_info("overlayfs: option \"workdir=%s\" is useless in a non-upper mount, ignore\n",
-			config->workdir);
-		kfree(config->workdir);
-		config->workdir = NULL;
-	}
-
-	return ovl_parse_redirect_mode(config, config->redirect_mode);
+	return ovl_parse_redirect_mode(config);
 }
 
 #define OVL_WORKDIR_NAME "work"
@@ -1376,10 +1367,24 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	ofs->config.index = ovl_index_def;
 	ofs->config.nfs_export = ovl_nfs_export_def;
 	ofs->config.xino = ovl_xino_def();
+
+	ofs->config.redirect_mode = kstrdup(ovl_redirect_mode_def(),
+							GFP_KERNEL);
+	if (!ofs->config.redirect_mode)
+		return -ENOMEM;
+
 	err = ovl_parse_opt((char *) data, &ofs->config);
 	if (err)
 		goto out_err;
 
+	/* Workdir is useless in non-upper mount */
+	if (!ofs->config.upperdir && ofs->config.workdir) {
+		pr_info("overlayfs: option \"workdir=%s\" is useless in a non-upper mount, ignore\n",
+			ofs->config.workdir);
+		kfree(ofs->config.workdir);
+		ofs->config.workdir = NULL;
+	}
+
 	err = -EINVAL;
 	if (!ofs->config.lowerdir) {
 		if (!silent)
-- 
1.8.3.1

[PATCH v4 2/3] ovl: move the code of ovl_remount() to behind ovl_parse_opt()

[Chengguang Xu]

Move the code of ovl_remount() around without any functional change,
so that ovl_remount() can call ovl_parse_opt() without explicit
statement.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
 fs/overlayfs/super.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index e99864e..0293635a 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -379,27 +379,6 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 	return 0;
 }
 
-static int ovl_remount(struct super_block *sb, int *flags, char *data)
-{
-	struct ovl_fs *ofs = sb->s_fs_info;
-
-	if (!(*flags & SB_RDONLY) && ovl_force_readonly(ofs))
-		return -EROFS;
-
-	return 0;
-}
-
-static const struct super_operations ovl_super_operations = {
-	.alloc_inode	= ovl_alloc_inode,
-	.destroy_inode	= ovl_destroy_inode,
-	.drop_inode	= generic_delete_inode,
-	.put_super	= ovl_put_super,
-	.sync_fs	= ovl_sync_fs,
-	.statfs		= ovl_statfs,
-	.show_options	= ovl_show_options,
-	.remount_fs	= ovl_remount,
-};
-
 enum {
 	OPT_LOWERDIR,
 	OPT_UPPERDIR,
@@ -563,6 +542,27 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 	return ovl_parse_redirect_mode(config);
 }
 
+static int ovl_remount(struct super_block *sb, int *flags, char *data)
+{
+	struct ovl_fs *ofs = sb->s_fs_info;
+
+	if (!(*flags & SB_RDONLY) && ovl_force_readonly(ofs))
+		return -EROFS;
+
+	return 0;
+}
+
+static const struct super_operations ovl_super_operations = {
+	.alloc_inode	= ovl_alloc_inode,
+	.destroy_inode	= ovl_destroy_inode,
+	.drop_inode	= generic_delete_inode,
+	.put_super	= ovl_put_super,
+	.sync_fs	= ovl_sync_fs,
+	.statfs		= ovl_statfs,
+	.show_options	= ovl_show_options,
+	.remount_fs	= ovl_remount,
+};
+
 #define OVL_WORKDIR_NAME "work"
 #define OVL_INDEXDIR_NAME "index"
 
-- 
1.8.3.1

[PATCH v4 3/3] ovl: return error when specifying unchangeable options in remount

[Chengguang Xu]

Check remount options and return error if there is any unchangeable
option.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
Changes since v3:
- Fix error prone macro helper.
- Make another new patch to only include code around.

Changes since v2:
- Introduce two macro helpers to simplify remount option validation.

Changes since v1:
- Refactor ovl_free_fs(), move config freeing operations to ovl_free_config().

 fs/overlayfs/super.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 0293635a..36587cbf 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -230,6 +230,14 @@ static void ovl_destroy_inode(struct inode *inode)
 	call_rcu(&inode->i_rcu, ovl_i_callback);
 }
 
+static void ovl_free_config(struct ovl_config *config)
+{
+	kfree(config->lowerdir);
+	kfree(config->upperdir);
+	kfree(config->workdir);
+	kfree(config->redirect_mode);
+}
+
 static void ovl_free_fs(struct ovl_fs *ofs)
 {
 	unsigned i;
@@ -249,10 +257,7 @@ static void ovl_free_fs(struct ovl_fs *ofs)
 	kfree(ofs->lower_layers);
 	kfree(ofs->lower_fs);
 
-	kfree(ofs->config.lowerdir);
-	kfree(ofs->config.upperdir);
-	kfree(ofs->config.workdir);
-	kfree(ofs->config.redirect_mode);
+	ovl_free_config(&ofs->config);
 	if (ofs->creator_cred)
 		put_cred(ofs->creator_cred);
 	kfree(ofs);
@@ -542,14 +547,70 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 	return ovl_parse_redirect_mode(config);
 }
 
+static bool ovl_opt_bool_neq(bool new, bool old, const char *name)
+{
+	if (new != old) {
+		pr_err("overlayfs: option \"%s\" cannot be %s on remount.\n",
+					name, new ? "enabled" : "disabled");
+		return true;
+	}
+	return false;
+}
+
+static bool ovl_opt_string_neq(const char *new, const char *old,
+						const char *name)
+{
+	if (new && (!old || strcmp(new, old))) {
+		pr_err("overlayfs: option \"%s\" cannot be changed on remount.\n",
+									name);
+		return true;
+	}
+	return false;
+}
+
+#define ovl_config_opt_neq(old, new, name, type) \
+	ovl_opt_##type##_neq((old)->name, (new)->name, #name)
+
 static int ovl_remount(struct super_block *sb, int *flags, char *data)
 {
 	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_config *oc = &ofs->config;
+	struct ovl_config *nc;
+	int err;
 
 	if (!(*flags & SB_RDONLY) && ovl_force_readonly(ofs))
 		return -EROFS;
 
+	nc = kzalloc(sizeof(struct ovl_config), GFP_KERNEL);
+	if (!nc)
+		return -ENOMEM;
+
+	nc->index = oc->index;
+	nc->nfs_export = oc->nfs_export;
+
+	err = ovl_parse_opt((char *)data, nc);
+	if (err)
+		goto out_err;
+
+	err = -EINVAL;
+	if (ovl_config_opt_neq(nc, oc, index, bool))
+		goto out_err;
+	if (ovl_config_opt_neq(nc, oc, nfs_export, bool))
+		goto out_err;
+	if (ovl_config_opt_neq(nc, oc, lowerdir, string))
+		goto out_err;
+	if (ovl_config_opt_neq(nc, oc, upperdir, string))
+		goto out_err;
+	if (ovl_config_opt_neq(nc, oc, workdir, string))
+		goto out_err;
+	if (ovl_config_opt_neq(nc, oc, redirect_mode, string))
+		goto out_err;
+
 	return 0;
+out_err:
+	ovl_free_config(nc);
+	kfree(nc);
+	return err;
 }
 
 static const struct super_operations ovl_super_operations = {
-- 
1.8.3.1

Re: [PATCH v4 0/3] option validation in remount

[Amir Goldstein]

On Fri, Apr 13, 2018 at 3:39 PM, Chengguang Xu <cgxu519@gmx.com> wrote:
> Some overlayfs mount options cannot be changed via remount,
> but current remount does not return error even if we specify
> different value to unchangeable options.
>
> This patch series adjust option parsing to support remount
> and return proper error message and code when recogonizing
> unchangeable options in remount.
>

Surely, you have motivation beyond what is listed here.
Would be nice to list short and long term goals for changing
remount options.

The series looks ok to me.
There is a question whether erroring on remount with changed
options is considered a regression to some applications.
I don't know how to answer that.
Does your use case *require* that remount will fail?
Would it be ok to ignore unchangeable options like today,
but warn about it?

After Miklos approves the expected behavior,
please write xfstests to verify expected behavior.

Thanks,
Amir.

Re: [PATCH v4 0/3] option validation in remount

[Miklos Szeredi]

On Fri, Apr 13, 2018 at 2:56 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Apr 13, 2018 at 3:39 PM, Chengguang Xu <cgxu519@gmx.com> wrote:
>> Some overlayfs mount options cannot be changed via remount,
>> but current remount does not return error even if we specify
>> different value to unchangeable options.
>>
>> This patch series adjust option parsing to support remount
>> and return proper error message and code when recogonizing
>> unchangeable options in remount.
>>
>
> Surely, you have motivation beyond what is listed here.
> Would be nice to list short and long term goals for changing
> remount options.
>
> The series looks ok to me.
> There is a question whether erroring on remount with changed
> options is considered a regression to some applications.

Theoretically: yes.  In real life: maybe.  We need to be careful.

> I don't know how to answer that.
> Does your use case *require* that remount will fail?
> Would it be ok to ignore unchangeable options like today,
> but warn about it?

Adding a warning is definitely okay, since it makes no sense to supply
unchangeable options.  Maybe at some point we can change the warning
to a failure.

It would have been best to error out on unknown/unchangeable options
from the start.  Hopefully the new mount API will make implementing
option parsing and reconfiguration less error prone.

Thanks,
Miklos

Re: [PATCH v4 3/3] ovl: return error when specifying unchangeable options in remount

[Miklos Szeredi]

On Fri, Apr 13, 2018 at 2:39 PM, Chengguang Xu <cgxu519@gmx.com> wrote:
> Check remount options and return error if there is any unchangeable
> option.
>
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> ---
> Changes since v3:
> - Fix error prone macro helper.
> - Make another new patch to only include code around.
>
> Changes since v2:
> - Introduce two macro helpers to simplify remount option validation.
>
> Changes since v1:
> - Refactor ovl_free_fs(), move config freeing operations to ovl_free_config().
>
>  fs/overlayfs/super.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 65 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 0293635a..36587cbf 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -230,6 +230,14 @@ static void ovl_destroy_inode(struct inode *inode)
>         call_rcu(&inode->i_rcu, ovl_i_callback);
>  }
>
> +static void ovl_free_config(struct ovl_config *config)
> +{
> +       kfree(config->lowerdir);
> +       kfree(config->upperdir);
> +       kfree(config->workdir);
> +       kfree(config->redirect_mode);
> +}
> +
>  static void ovl_free_fs(struct ovl_fs *ofs)
>  {
>         unsigned i;
> @@ -249,10 +257,7 @@ static void ovl_free_fs(struct ovl_fs *ofs)
>         kfree(ofs->lower_layers);
>         kfree(ofs->lower_fs);
>
> -       kfree(ofs->config.lowerdir);
> -       kfree(ofs->config.upperdir);
> -       kfree(ofs->config.workdir);
> -       kfree(ofs->config.redirect_mode);
> +       ovl_free_config(&ofs->config);
>         if (ofs->creator_cred)
>                 put_cred(ofs->creator_cred);
>         kfree(ofs);
> @@ -542,14 +547,70 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>         return ovl_parse_redirect_mode(config);
>  }
>
> +static bool ovl_opt_bool_neq(bool new, bool old, const char *name)
> +{
> +       if (new != old) {
> +               pr_err("overlayfs: option \"%s\" cannot be %s on remount.\n",
> +                                       name, new ? "enabled" : "disabled");
> +               return true;
> +       }
> +       return false;
> +}
> +
> +static bool ovl_opt_string_neq(const char *new, const char *old,
> +                                               const char *name)
> +{
> +       if (new && (!old || strcmp(new, old))) {
> +               pr_err("overlayfs: option \"%s\" cannot be changed on remount.\n",
> +                                                                       name);
> +               return true;
> +       }
> +       return false;
> +}
> +
> +#define ovl_config_opt_neq(old, new, name, type) \
> +       ovl_opt_##type##_neq((old)->name, (new)->name, #name)
> +
>  static int ovl_remount(struct super_block *sb, int *flags, char *data)
>  {
>         struct ovl_fs *ofs = sb->s_fs_info;
> +       struct ovl_config *oc = &ofs->config;
> +       struct ovl_config *nc;
> +       int err;
>
>         if (!(*flags & SB_RDONLY) && ovl_force_readonly(ofs))
>                 return -EROFS;
>
> +       nc = kzalloc(sizeof(struct ovl_config), GFP_KERNEL);
> +       if (!nc)
> +               return -ENOMEM;
> +
> +       nc->index = oc->index;
> +       nc->nfs_export = oc->nfs_export;

What about the other options?

We should copy all of the old config to the new config here.

If an option is not given, it should be assumed to stay the same as
the previous value (i.e. no need to supply options to remount that you
don't want to change).

Thanks,
Miklos

Re: [PATCH v4 3/3] ovl: return error when specifying unchangeable options in remount

[cgxu519]

在 2018年4月13日，下午9:50，Miklos Szeredi <miklos@szeredi.hu> 写道：
> 
> On Fri, Apr 13, 2018 at 2:39 PM, Chengguang Xu <cgxu519@gmx.com> wrote:
>> Check remount options and return error if there is any unchangeable
>> option.
>> 
>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
>> ---
>> Changes since v3:
>> - Fix error prone macro helper.
>> - Make another new patch to only include code around.
>> 
>> Changes since v2:
>> - Introduce two macro helpers to simplify remount option validation.
>> 
>> Changes since v1:
>> - Refactor ovl_free_fs(), move config freeing operations to ovl_free_config().
>> 
>> fs/overlayfs/super.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 65 insertions(+), 4 deletions(-)
>> 
>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> index 0293635a..36587cbf 100644
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>> @@ -230,6 +230,14 @@ static void ovl_destroy_inode(struct inode *inode)
>>        call_rcu(&inode->i_rcu, ovl_i_callback);
>> }
>> 
>> +static void ovl_free_config(struct ovl_config *config)
>> +{
>> +       kfree(config->lowerdir);
>> +       kfree(config->upperdir);
>> +       kfree(config->workdir);
>> +       kfree(config->redirect_mode);
>> +}
>> +
>> static void ovl_free_fs(struct ovl_fs *ofs)
>> {
>>        unsigned i;
>> @@ -249,10 +257,7 @@ static void ovl_free_fs(struct ovl_fs *ofs)
>>        kfree(ofs->lower_layers);
>>        kfree(ofs->lower_fs);
>> 
>> -       kfree(ofs->config.lowerdir);
>> -       kfree(ofs->config.upperdir);
>> -       kfree(ofs->config.workdir);
>> -       kfree(ofs->config.redirect_mode);
>> +       ovl_free_config(&ofs->config);
>>        if (ofs->creator_cred)
>>                put_cred(ofs->creator_cred);
>>        kfree(ofs);
>> @@ -542,14 +547,70 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>>        return ovl_parse_redirect_mode(config);
>> }
>> 
>> +static bool ovl_opt_bool_neq(bool new, bool old, const char *name)
>> +{
>> +       if (new != old) {
>> +               pr_err("overlayfs: option \"%s\" cannot be %s on remount.\n",
>> +                                       name, new ? "enabled" : "disabled");
>> +               return true;
>> +       }
>> +       return false;
>> +}
>> +
>> +static bool ovl_opt_string_neq(const char *new, const char *old,
>> +                                               const char *name)
>> +{
>> +       if (new && (!old || strcmp(new, old))) {
>> +               pr_err("overlayfs: option \"%s\" cannot be changed on remount.\n",
>> +                                                                       name);
>> +               return true;
>> +       }
>> +       return false;
>> +}
>> +
>> +#define ovl_config_opt_neq(old, new, name, type) \
>> +       ovl_opt_##type##_neq((old)->name, (new)->name, #name)
>> +
>> static int ovl_remount(struct super_block *sb, int *flags, char *data)
>> {
>>        struct ovl_fs *ofs = sb->s_fs_info;
>> +       struct ovl_config *oc = &ofs->config;
>> +       struct ovl_config *nc;
>> +       int err;
>> 
>>        if (!(*flags & SB_RDONLY) && ovl_force_readonly(ofs))
>>                return -EROFS;
>> 
>> +       nc = kzalloc(sizeof(struct ovl_config), GFP_KERNEL);
>> +       if (!nc)
>> +               return -ENOMEM;
>> +
>> +       nc->index = oc->index;
>> +       nc->nfs_export = oc->nfs_export;
> 
> What about the other options?
> 
> We should copy all of the old config to the new config here.
> 
> If an option is not given, it should be assumed to stay the same as
> the previous value (i.e. no need to supply options to remount that you
> don't want to change).

Yes, need to add ‘default_permissions’ later.

For ‘redirect_dir’ and ‘redirect_follow’, the condition of changing is specifying
new value to ‘redirect_mode’ and we have already checked it.

Thanks，
Chengguang。

Re: [PATCH v4 3/3] ovl: return error when specifying unchangeable options in remount

[Miklos Szeredi]

On Fri, Apr 13, 2018 at 4:03 PM, cgxu519@gmx.com <cgxu519@gmx.com> wrote:
> 在 2018年4月13日，下午9:50，Miklos Szeredi <miklos@szeredi.hu> 写道：
>>
>> On Fri, Apr 13, 2018 at 2:39 PM, Chengguang Xu <cgxu519@gmx.com> wrote:
>>> Check remount options and return error if there is any unchangeable
>>> option.
>>>
>>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
>>> ---
>>> Changes since v3:
>>> - Fix error prone macro helper.
>>> - Make another new patch to only include code around.
>>>
>>> Changes since v2:
>>> - Introduce two macro helpers to simplify remount option validation.
>>>
>>> Changes since v1:
>>> - Refactor ovl_free_fs(), move config freeing operations to ovl_free_config().
>>>
>>> fs/overlayfs/super.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++---
>>> 1 file changed, 65 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>>> index 0293635a..36587cbf 100644
>>> --- a/fs/overlayfs/super.c
>>> +++ b/fs/overlayfs/super.c
>>> @@ -230,6 +230,14 @@ static void ovl_destroy_inode(struct inode *inode)
>>>        call_rcu(&inode->i_rcu, ovl_i_callback);
>>> }
>>>
>>> +static void ovl_free_config(struct ovl_config *config)
>>> +{
>>> +       kfree(config->lowerdir);
>>> +       kfree(config->upperdir);
>>> +       kfree(config->workdir);
>>> +       kfree(config->redirect_mode);
>>> +}
>>> +
>>> static void ovl_free_fs(struct ovl_fs *ofs)
>>> {
>>>        unsigned i;
>>> @@ -249,10 +257,7 @@ static void ovl_free_fs(struct ovl_fs *ofs)
>>>        kfree(ofs->lower_layers);
>>>        kfree(ofs->lower_fs);
>>>
>>> -       kfree(ofs->config.lowerdir);
>>> -       kfree(ofs->config.upperdir);
>>> -       kfree(ofs->config.workdir);
>>> -       kfree(ofs->config.redirect_mode);
>>> +       ovl_free_config(&ofs->config);
>>>        if (ofs->creator_cred)
>>>                put_cred(ofs->creator_cred);
>>>        kfree(ofs);
>>> @@ -542,14 +547,70 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>>>        return ovl_parse_redirect_mode(config);
>>> }
>>>
>>> +static bool ovl_opt_bool_neq(bool new, bool old, const char *name)
>>> +{
>>> +       if (new != old) {
>>> +               pr_err("overlayfs: option \"%s\" cannot be %s on remount.\n",
>>> +                                       name, new ? "enabled" : "disabled");
>>> +               return true;
>>> +       }
>>> +       return false;
>>> +}
>>> +
>>> +static bool ovl_opt_string_neq(const char *new, const char *old,
>>> +                                               const char *name)
>>> +{
>>> +       if (new && (!old || strcmp(new, old))) {
>>> +               pr_err("overlayfs: option \"%s\" cannot be changed on remount.\n",
>>> +                                                                       name);
>>> +               return true;
>>> +       }
>>> +       return false;
>>> +}
>>> +
>>> +#define ovl_config_opt_neq(old, new, name, type) \
>>> +       ovl_opt_##type##_neq((old)->name, (new)->name, #name)
>>> +
>>> static int ovl_remount(struct super_block *sb, int *flags, char *data)
>>> {
>>>        struct ovl_fs *ofs = sb->s_fs_info;
>>> +       struct ovl_config *oc = &ofs->config;
>>> +       struct ovl_config *nc;
>>> +       int err;
>>>
>>>        if (!(*flags & SB_RDONLY) && ovl_force_readonly(ofs))
>>>                return -EROFS;
>>>
>>> +       nc = kzalloc(sizeof(struct ovl_config), GFP_KERNEL);
>>> +       if (!nc)
>>> +               return -ENOMEM;
>>> +
>>> +       nc->index = oc->index;
>>> +       nc->nfs_export = oc->nfs_export;
>>
>> What about the other options?
>>
>> We should copy all of the old config to the new config here.
>>
>> If an option is not given, it should be assumed to stay the same as
>> the previous value (i.e. no need to supply options to remount that you
>> don't want to change).
>
> Yes, need to add ‘default_permissions’ later.
>
> For ‘redirect_dir’ and ‘redirect_follow’, the condition of changing is specifying
> new value to ‘redirect_mode’ and we have already checked it.

Ah, I missed the fact that ovl_opt_string_neq() takes care of the not
specified case.

Thanks,
Miklos

Re: [PATCH v4 0/3] option validation in remount

[cgxu519]

> 在 2018年4月13日，下午9:46，Miklos Szeredi <miklos@szeredi.hu> 写道：
> 
> On Fri, Apr 13, 2018 at 2:56 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Fri, Apr 13, 2018 at 3:39 PM, Chengguang Xu <cgxu519@gmx.com> wrote:
>>> Some overlayfs mount options cannot be changed via remount,
>>> but current remount does not return error even if we specify
>>> different value to unchangeable options.
>>> 
>>> This patch series adjust option parsing to support remount
>>> and return proper error message and code when recogonizing
>>> unchangeable options in remount.
>>> 
>> 
>> Surely, you have motivation beyond what is listed here.
>> Would be nice to list short and long term goals for changing
>> remount options.
>> 
>> The series looks ok to me.
>> There is a question whether erroring on remount with changed
>> options is considered a regression to some applications.
> 
> Theoretically: yes.  In real life: maybe.  We need to be careful.
> 
>> I don't know how to answer that.
>> Does your use case *require* that remount will fail?
>> Would it be ok to ignore unchangeable options like today,
>> but warn about it?
> 
> Adding a warning is definitely okay, since it makes no sense to supply
> unchangeable options.  Maybe at some point we can change the warning
> to a failure.
> 
> It would have been best to error out on unknown/unchangeable options
> from the start.  Hopefully the new mount API will make implementing
> option parsing and reconfiguration less error prone.

I’ll modify the code to only print warning but not return error code
when user trying to change unchangeable options, but if user specifies
unrecognized option or the option is invalid then still return -EINVAL.

Thanks,
Chengguang.

Re: [PATCH v4 0/3] option validation in remount

[cgxu519]

在 2018年4月13日，下午8:56，Amir Goldstein <amir73il@gmail.com> 写道：
> 
> On Fri, Apr 13, 2018 at 3:39 PM, Chengguang Xu <cgxu519@gmx.com> wrote:
>> Some overlayfs mount options cannot be changed via remount,
>> but current remount does not return error even if we specify
>> different value to unchangeable options.
>> 
>> This patch series adjust option parsing to support remount
>> and return proper error message and code when recogonizing
>> unchangeable options in remount.
>> 
> 
> Surely, you have motivation beyond what is listed here.
> Would be nice to list short and long term goals for changing
> remount options.
> 
> The series looks ok to me.
> There is a question whether erroring on remount with changed
> options is considered a regression to some applications.
> I don't know how to answer that.
> Does your use case *require* that remount will fail?
> Would it be ok to ignore unchangeable options like today,
> but warn about it?

hmmm, some of our sysadmins get confused by the phenomenon of
success remount without changing options. It’s obviously different
compare to other filesystems but I think with proper explanation
they can accept this and change their scripts.


> 
> After Miklos approves the expected behavior,
> please write xfstests to verify expected behavior.

Sure, I’ll make a case to verify it.