> 在 2018年4月1日，下午3:39，Amir Goldstein <amir73il@xxxxxxxxx> 写道：
>
> On Sun, Apr 1, 2018 at 7:57 AM, Chengguang Xu <cgxu519@xxxxxxx> wrote:
>> Check remount options and return error if there is any unchangeable
>> option.
>>
>> Signed-off-by: Chengguang Xu <cgxu519@xxxxxxx>
>> ---
>> fs/overlayfs/super.c | 110 +++++++++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 89 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> index 5c8944b..ba00295 100644
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>> @@ -353,27 +353,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,
>> @@ -534,6 +513,95 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config, bool remount)
>> return ovl_parse_redirect_mode(config, config->redirect_mode);
>> }
>>
>> +static bool ovl_string_option_equal(const char *new, const char *old)
>> +{
>> + if (new) {
>> + if (old) {
>> + if (!strcmp(new, old))
>> + return true;
>> + else
>> + return false;
>> + } else {
>> + return false;
>> + }
>> + } else {
>> + if (old)
>> + return false;
>> + else
>> + return true;
>> + }
>> +}
>> +
>
> Or maybe just:
> { return old && !strcmp(new, old); }
>
> You really only need to check when new is non NULL.
>
> But you don't even need this much if you follow my suggestion below.
Yes, you are right. I just think to make a common string option compare function,
so it is not optimized and it is maybe useful for further case(ro->rw?), in this
case upperdir/wordir are from NULL to non NULL. If there is no other place need
it in the future then we will need to modify it to better fit for existing case.
>
>> +static int ovl_remount(struct super_block *sb, int *flags, char *data)
>> +{
>> + struct ovl_fs *ofs = sb->s_fs_info;
>> + struct ovl_config *old_config = &ofs->config;
>> + struct ovl_config *new_config;
>> + bool remount = true;
>
> No need for helper var.
>
>> + int err;
>> +
>> + if (!(*flags & SB_RDONLY) && ovl_force_readonly(ofs))
>> + return -EROFS;
>> +
>> + new_config = kzalloc(sizeof(struct ovl_config), GFP_KERNEL);
>> + if (!new_config)
>> + return -ENOMEM;
>> +
>> + new_config->index = old_config->index;
>> + new_config->nfs_export = old_config->nfs_export;
>> +
>> + err = ovl_parse_opt((char *)data, new_config, remount);
>> + if (err)
>> + goto out_err;
>> +
>> + err = -EINVAL;
>> + if (new_config->lowerdir &&
>> + !ovl_string_option_equal(new_config->lowerdir,
>> + old_config->lowerdir))
>> + goto out_err;
>> +
>> + if (new_config->upperdir &&
>> + !ovl_string_option_equal(new_config->upperdir,
>> + old_config->upperdir))
>> + goto out_err;
>> +
>> + if (new_config->workdir &&
>> + !ovl_string_option_equal(new_config->workdir,
>> + old_config->workdir))
>> + goto out_err;
>> +
>> + if (new_config->redirect_mode &&
>> + !ovl_string_option_equal(new_config->redirect_mode,
>> + old_config->redirect_mode))
>> + goto out_err;
>> +
>> + if (new_config->index != old_config->index)
>> + goto out_err;
>> +
>> + if (new_config->nfs_export != old_config->nfs_export)
>> + goto out_err;
>> +
>> + return 0;
>> +out_err:
>
> This error really calls for error in dmesg preferably pointing user
> to the offending mount option.
>
> For this reason, I think it might be better to implement those checks
> inside ovl_parse_opt() while parsing each option.
>
> Instead of passing bool remount, you can pass ovl_config *oldconfig
> to signify remount.
>
> For string options, you can replace the following code with a small helper
> or macro to also include the remount logic:
>
> kfree(config->upperdir);
> config->upperdir = match_strdup(&args[0]);
> if (!config->upperdir)
> return -ENOMEM;
> if (oldconfig && old->config->upperdir &&
> !strcmp(config->upperdir, old->config->upperdir)
> goto remount_err; // where you can
> print an error with args[0]
>
Actually, in my initial version I did just like what you wrote here,
but considering some future potential abilities(for example ro -> rw)
that we can add later, I decided to get new value through parsing.
I haven’t got time to check closely, but ro -> rw in remount seems doable
if we carefully handle readdir related things, and what about other options?
Are all of them really unchangeable?
>
> For bool options you just need to add a test at the end
> to compare values with oldconfig if oldconfig is not NULL
> and error, because it is OK to pass options index=on,index=off,..
> as long as the last is same as oldconfig.
>
>
>> + kfree(new_config->lowerdir);
>> + kfree(new_config->upperdir);
>> + kfree(new_config->workdir);
>> + kfree(new_config->redirect_mode);
>> + kfree(new_config);
>
> That's code duplication with ovl_free_fs()
> Please use ovl_free_config() helper.
>
> 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
