On 22/04/30 10:08PM, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> Since ext4 was converted to the new mount API, the test_dummy_encryption
> mount option isn't being handled entirely correctly, because the needed
> fscrypt_set_test_dummy_encryption() helper function combines
> parsing/checking/applying into one function. That doesn't work well
> with the new mount API, which split these into separate steps.
>
> This was sort of okay anyway, due to the parsing logic that was copied
> from fscrypt_set_test_dummy_encryption() into ext4_parse_param(),
> combined with an additional check in ext4_check_test_dummy_encryption().
> However, these overlooked the case of changing the value of
> test_dummy_encryption on remount, which isn't allowed but ext4 wasn't
> detecting until ext4_apply_options() when it's too late to fail.
> Another bug is that if test_dummy_encryption was specified multiple
> times with an argument, memory was leaked.
>
> Fix this up properly by using the new helper functions that allow
> splitting up the parse/check/apply steps for test_dummy_encryption.
>
> Fixes: cebe85d570cf ("ext4: switch to the new mount api")
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
I just had a small observation. Feel free to check it at your end too.
> ---
> fs/ext4/ext4.h | 6 ---
> fs/ext4/super.c | 131 +++++++++++++++++++++++++-----------------------
> 2 files changed, 67 insertions(+), 70 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index a743b1e3b89ec..f6d6661817b63 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1440,12 +1440,6 @@ struct ext4_super_block {
>
> #ifdef __KERNEL__
>
> -#ifdef CONFIG_FS_ENCRYPTION
> -#define DUMMY_ENCRYPTION_ENABLED(sbi) ((sbi)->s_dummy_enc_policy.policy != NULL)
> -#else
> -#define DUMMY_ENCRYPTION_ENABLED(sbi) (0)
> -#endif
> -
> /* Number of quota types we support */
> #define EXT4_MAXQUOTAS 3
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 64ce17714e193..43e4cd358b33b 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -87,7 +87,7 @@ static struct inode *ext4_get_journal_inode(struct super_block *sb,
> static int ext4_validate_options(struct fs_context *fc);
> static int ext4_check_opt_consistency(struct fs_context *fc,
> struct super_block *sb);
> -static int ext4_apply_options(struct fs_context *fc, struct super_block *sb);
> +static void ext4_apply_options(struct fs_context *fc, struct super_block *sb);
> static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param);
> static int ext4_get_tree(struct fs_context *fc);
> static int ext4_reconfigure(struct fs_context *fc);
> @@ -1989,31 +1989,12 @@ ext4_sb_read_encoding(const struct ext4_super_block *es)
> }
> #endif
>
> -static int ext4_set_test_dummy_encryption(struct super_block *sb, char *arg)
> -{
> -#ifdef CONFIG_FS_ENCRYPTION
> - struct ext4_sb_info *sbi = EXT4_SB(sb);
> - int err;
> -
> - err = fscrypt_set_test_dummy_encryption(sb, arg,
> - &sbi->s_dummy_enc_policy);
> - if (err) {
> - ext4_msg(sb, KERN_WARNING,
> - "Error while setting test dummy encryption [%d]", err);
> - return err;
> - }
> - ext4_msg(sb, KERN_WARNING, "Test dummy encryption mode enabled");
> -#endif
> - return 0;
> -}
> -
> #define EXT4_SPEC_JQUOTA (1 << 0)
> #define EXT4_SPEC_JQFMT (1 << 1)
> #define EXT4_SPEC_DATAJ (1 << 2)
> #define EXT4_SPEC_SB_BLOCK (1 << 3)
> #define EXT4_SPEC_JOURNAL_DEV (1 << 4)
> #define EXT4_SPEC_JOURNAL_IOPRIO (1 << 5)
> -#define EXT4_SPEC_DUMMY_ENCRYPTION (1 << 6)
> #define EXT4_SPEC_s_want_extra_isize (1 << 7)
> #define EXT4_SPEC_s_max_batch_time (1 << 8)
> #define EXT4_SPEC_s_min_batch_time (1 << 9)
> @@ -2030,7 +2011,7 @@ static int ext4_set_test_dummy_encryption(struct super_block *sb, char *arg)
>
> struct ext4_fs_context {
> char *s_qf_names[EXT4_MAXQUOTAS];
> - char *test_dummy_enc_arg;
> + struct fscrypt_dummy_policy dummy_enc_policy;
> int s_jquota_fmt; /* Format of quota to use */
> #ifdef CONFIG_EXT4_DEBUG
> int s_fc_debug_max_replay;
> @@ -2061,9 +2042,8 @@ struct ext4_fs_context {
> ext4_fsblk_t s_sb_block;
> };
>
> -static void ext4_fc_free(struct fs_context *fc)
> +static void __ext4_fc_free(struct ext4_fs_context *ctx)
> {
> - struct ext4_fs_context *ctx = fc->fs_private;
> int i;
>
> if (!ctx)
> @@ -2072,10 +2052,15 @@ static void ext4_fc_free(struct fs_context *fc)
> for (i = 0; i < EXT4_MAXQUOTAS; i++)
> kfree(ctx->s_qf_names[i]);
>
> - kfree(ctx->test_dummy_enc_arg);
> + fscrypt_free_dummy_policy(&ctx->dummy_enc_policy);
> kfree(ctx);
> }
>
> +static void ext4_fc_free(struct fs_context *fc)
> +{
> + __ext4_fc_free(fc->fs_private);
> +}
> +
> int ext4_init_fs_context(struct fs_context *fc)
> {
> struct ext4_fs_context *ctx;
> @@ -2148,6 +2133,29 @@ static int unnote_qf_name(struct fs_context *fc, int qtype)
> }
> #endif
>
> +static int ext4_parse_test_dummy_encryption(const struct fs_parameter *param,
> + struct ext4_fs_context *ctx)
> +{
> + int err;
> +
> + if (!IS_ENABLED(CONFIG_FS_ENCRYPTION)) {
> + ext4_msg(NULL, KERN_WARNING,
> + "test_dummy_encryption option not supported");
> + return -EINVAL;
> + }
> + err = fscrypt_parse_test_dummy_encryption(param,
> + &ctx->dummy_enc_policy);
> + if (err == -EINVAL) {
> + ext4_msg(NULL, KERN_WARNING,
> + "Value of option \"%s\" is unrecognized", param->key);
> + } else if (err == -EEXIST) {
> + ext4_msg(NULL, KERN_WARNING,
> + "Conflicting test_dummy_encryption options");
> + return -EINVAL;
> + }
> + return err;
> +}
> +
> #define EXT4_SET_CTX(name) \
> static inline void ctx_set_##name(struct ext4_fs_context *ctx, \
> unsigned long flag) \
> @@ -2410,29 +2418,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
> ctx->spec |= EXT4_SPEC_JOURNAL_IOPRIO;
> return 0;
> case Opt_test_dummy_encryption:
> -#ifdef CONFIG_FS_ENCRYPTION
> - if (param->type == fs_value_is_flag) {
> - ctx->spec |= EXT4_SPEC_DUMMY_ENCRYPTION;
> - ctx->test_dummy_enc_arg = NULL;
> - return 0;
> - }
> - if (*param->string &&
> - !(!strcmp(param->string, "v1") ||
> - !strcmp(param->string, "v2"))) {
> - ext4_msg(NULL, KERN_WARNING,
> - "Value of option \"%s\" is unrecognized",
> - param->key);
> - return -EINVAL;
> - }
> - ctx->spec |= EXT4_SPEC_DUMMY_ENCRYPTION;
> - ctx->test_dummy_enc_arg = kmemdup_nul(param->string, param->size,
> - GFP_KERNEL);
> - return 0;
> -#else
> - ext4_msg(NULL, KERN_WARNING,
> - "test_dummy_encryption option not supported");
> - return -EINVAL;
> -#endif
> + return ext4_parse_test_dummy_encryption(param, ctx);
> case Opt_dax:
> case Opt_dax_type:
> #ifdef CONFIG_FS_DAX
> @@ -2623,10 +2609,11 @@ static int parse_apply_sb_mount_options(struct super_block *sb,
> if (s_ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO)
> m_ctx->journal_ioprio = s_ctx->journal_ioprio;
>
> - ret = ext4_apply_options(fc, sb);
> + ext4_apply_options(fc, sb);
> + ret = 0;
>
> out_free:
> - kfree(s_ctx);
> + __ext4_fc_free(s_ctx);
I think we can still call ext4_fc_free(fc) and we don't need __ext4_fc_free().
Right?
-ritesh
> kfree(fc);
> kfree(s_mount_opts);
> return ret;
> @@ -2792,9 +2779,9 @@ static int ext4_check_test_dummy_encryption(const struct fs_context *fc,
> {
> const struct ext4_fs_context *ctx = fc->fs_private;
> const struct ext4_sb_info *sbi = EXT4_SB(sb);
> + int err;
>
> - if (!IS_ENABLED(CONFIG_FS_ENCRYPTION) ||
> - !(ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION))
> + if (!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy))
> return 0;
>
> if (!ext4_has_feature_encrypt(sb)) {
> @@ -2808,13 +2795,35 @@ static int ext4_check_test_dummy_encryption(const struct fs_context *fc,
> * needed to allow it to be set or changed during remount. We do allow
> * it to be specified during remount, but only if there is no change.
> */
> - if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE &&
> - !DUMMY_ENCRYPTION_ENABLED(sbi)) {
> + if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) {
> + if (fscrypt_dummy_policies_equal(&sbi->s_dummy_enc_policy,
> + &ctx->dummy_enc_policy))
> + return 0;
> ext4_msg(NULL, KERN_WARNING,
> - "Can't set test_dummy_encryption on remount");
> + "Can't set or change test_dummy_encryption on remount");
> return -EINVAL;
> }
> - return 0;
> + /*
> + * fscrypt_add_test_dummy_key() technically changes the super_block, so
> + * it technically should be delayed until ext4_apply_options() like the
> + * other changes. But since we never get here for remounts (see above),
> + * and this is the last chance to report errors, we do it here.
> + */
> + err = fscrypt_add_test_dummy_key(sb, &ctx->dummy_enc_policy);
> + if (err)
> + ext4_msg(NULL, KERN_WARNING,
> + "Error adding test dummy encryption key [%d]", err);
> + return err;
> +}
> +
> +static void ext4_apply_test_dummy_encryption(struct ext4_fs_context *ctx,
> + struct super_block *sb)
> +{
> + if (!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy))
> + return;
> + EXT4_SB(sb)->s_dummy_enc_policy = ctx->dummy_enc_policy;
> + memset(&ctx->dummy_enc_policy, 0, sizeof(ctx->dummy_enc_policy));
> + ext4_msg(sb, KERN_WARNING, "Test dummy encryption mode enabled");
> }
>
> static int ext4_check_opt_consistency(struct fs_context *fc,
> @@ -2901,11 +2910,10 @@ static int ext4_check_opt_consistency(struct fs_context *fc,
> return ext4_check_quota_consistency(fc, sb);
> }
>
> -static int ext4_apply_options(struct fs_context *fc, struct super_block *sb)
> +static void ext4_apply_options(struct fs_context *fc, struct super_block *sb)
> {
> struct ext4_fs_context *ctx = fc->fs_private;
> struct ext4_sb_info *sbi = fc->s_fs_info;
> - int ret = 0;
>
> sbi->s_mount_opt &= ~ctx->mask_s_mount_opt;
> sbi->s_mount_opt |= ctx->vals_s_mount_opt;
> @@ -2942,10 +2950,7 @@ static int ext4_apply_options(struct fs_context *fc, struct super_block *sb)
>
> ext4_apply_quota_options(fc, sb);
>
> - if (ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION)
> - ret = ext4_set_test_dummy_encryption(sb, ctx->test_dummy_enc_arg);
> -
> - return ret;
> + ext4_apply_test_dummy_encryption(ctx, sb);
> }
>
>
> @@ -4667,9 +4672,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> if (err < 0)
> goto failed_mount;
>
> - err = ext4_apply_options(fc, sb);
> - if (err < 0)
> - goto failed_mount;
> + ext4_apply_options(fc, sb);
>
> #if IS_ENABLED(CONFIG_UNICODE)
> if (ext4_has_feature_casefold(sb) && !sb->s_encoding) {
> --
> 2.36.0
>
