On Wed, Apr 4, 2018 at 12:27 PM, Chengguang Xu <cgxu519@xxxxxxx> wrote:
> Some overlayfs mount options cannot be changed via remount,
> but remount operation does not return proper error even if
> we specify different value to unchangeable options.
>
> This patch add option parsing support for remount so we can
> recogonize unchangeable options in remount.
>
> Signed-off-by: Chengguang Xu <cgxu519@xxxxxxx>
> ---
>
> Changes since v1:
> - Add remount option validation to parse_opt()
>
> fs/overlayfs/super.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 127 insertions(+), 6 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 7c24619..4112720 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -425,6 +425,16 @@ static char *ovl_next_opt(char **s)
>
> static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode)
> {
> +
> + if (!mode) {
> + config->redirect_mode = kstrdup(ovl_redirect_mode_def(),
> + GFP_KERNEL);
> + if (!config->redirect_mode)
> + return -ENOMEM;
> +
> + mode = config->redirect_mode;
> + }
This makes @mode argument to function redundant.
> +
> if (strcmp(mode, "on") == 0) {
> config->redirect_dir = true;
> /*
> @@ -446,13 +456,49 @@ static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode)
> return 0;
> }
>
> -static int ovl_parse_opt(char *opt, struct ovl_config *config)
> +static char *ovl_token_to_string(int token)
> {
> - char *p;
> + switch (token) {
> + case OPT_UPPERDIR: return "upperdir";
> + case OPT_LOWERDIR: return "lowerdir";
> + case OPT_WORKDIR: return "workdir";
> + case OPT_DEFAULT_PERMISSIONS: return "default_permissions";
> + case OPT_REDIRECT_DIR: return "redirect_dir";
> + case OPT_INDEX_ON:
> + case OPT_INDEX_OFF: return "index";
> + case OPT_NFS_EXPORT_ON:
> + case OPT_NFS_EXPORT_OFF: return "nfs_export";
> + default: return "???";
> + }
> +}
You don't need this. The option name is available in args[0]
variable when calling the parsing macros and I don't think
you need to bother with stripping the =xxx part.
>
> - config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
> - if (!config->redirect_mode)
> - return -ENOMEM;
> +static int ovl_valid_remount_string_option(int token, const char *new,
> + const char *old)
> +{
> + if (!old || strcmp(new, old)) {
> + pr_err("overlayfs: option %s cannot be changed on remount.\n",
> + ovl_token_to_string(token));
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int ovl_valid_remount_bool_option(int token, const bool new,
> + const bool old)
> +{
> + if (new != old) {
> + pr_err("overlayfs: option %s cannot be changed on remount.\n",
> + ovl_token_to_string(token));
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int ovl_parse_opt(char *opt, struct ovl_config *config,
> + struct ovl_config *old_config)
> +{
> + char *p;
> + int err;
>
> while ((p = ovl_next_opt(&opt)) != NULL) {
> int token;
> @@ -468,6 +514,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> config->upperdir = match_strdup(&args[0]);
> if (!config->upperdir)
> return -ENOMEM;
> + if (old_config) {
> + err = ovl_valid_remount_string_option(
> + token,
> + config->upperdir,
> + old_config->upperdir);
> + if (err)
> + return err;
> + }
Brevity: instead of adding 8 lines to each case, you can squeeze the entire
handling of string option to a single macro, e.g.:
err = (ovl_parse_string_option(upperdir, &args[0], config, oldconfig);
if (err)
return err;
Same for ovl_parse_bool_option.
Later, when you want to introduce an option that could be changed
on remount or only changed from NULL, just add new macro flavors:
ovl_parse_remount_[string|bool]_option
ovl_parse_test_and_remount_[string|bool]_option
Let me know if you are not clear on how.
Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
