| Message ID | 20220501050857.538984-6-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test_dummy_encryption fixes and cleanups | expand |
A couple corrections I'll include in the next version: On Sat, Apr 30, 2022 at 10:08:55PM -0700, Eric Biggers wrote: > + 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; > } I think this needs to be 'fc->purpose == FS_CONTEXT_FOR_RECONFIGURE || fscrypt_is_dummy_policy_set(&sbi->s_dummy_enc_policy)', since ext4 can parse mount options from both s_mount_opts and the regular mount options. > +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; To handle remounts correctly, this needs to be '!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy) || fscrypt_is_dummy_policy_set(&EXT4_SB(sb)->s_dummy_enc_policy)'. - Eric
On 22/05/09 04:40PM, Eric Biggers wrote: > A couple corrections I'll include in the next version: Need few clarifications. Could you please help explain what am I missing here? > > On Sat, Apr 30, 2022 at 10:08:55PM -0700, Eric Biggers wrote: > > + 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; > > } > > I think this needs to be 'fc->purpose == FS_CONTEXT_FOR_RECONFIGURE || > fscrypt_is_dummy_policy_set(&sbi->s_dummy_enc_policy)', since ext4 can parse > mount options from both s_mount_opts and the regular mount options. Sorry, I am missing something here. Could you please help me understand why do we need the other OR case which you mentioned above i.e. "|| fscrypt_is_dummy_policy_set(&sbi->s_dummy_enc_policy)" So maybe to put it this way, when will it be the case where fscrypt_is_dummy_policy_set(&sbi->s_dummy_enc_policy) is true and it is not a FS_CONTEXT_FOR_RECONFIGURE case? Also just in case if I did miss something that also means the comment after this case will not be valid anymore? i.e. /* * 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; > > To handle remounts correctly, this needs to be > '!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy) || > fscrypt_is_dummy_policy_set(&EXT4_SB(sb)->s_dummy_enc_policy)'. Why? Isn't it true that in remount we should update EXT4_SB(sb)->s_dummy_enc_policy only when ctx->dummy_enc_policy is set. If EXT4_SB(sb)->s_dummy_enc_policy is already set and ctx->dummy_enc_policy is not set, that means it's a remount case with no mount opts in which case ext4 should continue to have the same value of EXT4_SB(sb)->s_dummy_enc_policy? Did I miss any case here? -ritesh > > - Eric > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On Wed, May 11, 2022 at 11:24:33PM +0530, Ritesh Harjani wrote: > On 22/05/09 04:40PM, Eric Biggers wrote: > > A couple corrections I'll include in the next version: > > Need few clarifications. Could you please help explain what am I missing here? > > > > > On Sat, Apr 30, 2022 at 10:08:55PM -0700, Eric Biggers wrote: > > > + 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; > > > } > > > > I think this needs to be 'fc->purpose == FS_CONTEXT_FOR_RECONFIGURE || > > fscrypt_is_dummy_policy_set(&sbi->s_dummy_enc_policy)', since ext4 can parse > > mount options from both s_mount_opts and the regular mount options. > > Sorry, I am missing something here. Could you please help me understand why > do we need the other OR case which you mentioned above i.e. > "|| fscrypt_is_dummy_policy_set(&sbi->s_dummy_enc_policy)" > > So maybe to put it this way, when will it be the case where > fscrypt_is_dummy_policy_set(&sbi->s_dummy_enc_policy) is true and it is not a > FS_CONTEXT_FOR_RECONFIGURE case? The case where test_dummy_encryption is present in both the mount options stored in the superblock and in the regular mount options. See how __ext4_fill_super() parses and applies each source of options separately. > Also just in case if I did miss something that also means the comment after this > case will not be valid anymore? > i.e. > /* > * 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; That comment will still be correct. > > > > > > +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; > > > > To handle remounts correctly, this needs to be > > '!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy) || > > fscrypt_is_dummy_policy_set(&EXT4_SB(sb)->s_dummy_enc_policy)'. > > Why? > Isn't it true that in remount we should update EXT4_SB(sb)->s_dummy_enc_policy > only when ctx->dummy_enc_policy is set. If EXT4_SB(sb)->s_dummy_enc_policy is > already set and ctx->dummy_enc_policy is not set, that means it's a remount case with no mount > opts in which case ext4 should continue to have the same value of EXT4_SB(sb)->s_dummy_enc_policy? struct fscrypt_dummy_policy includes dynamically allocated memory, so overwriting it without first freeing it would be a memory leak. - Eric
On 22/05/11 06:03PM, Eric Biggers wrote: > On Wed, May 11, 2022 at 11:24:33PM +0530, Ritesh Harjani wrote: > > On 22/05/09 04:40PM, Eric Biggers wrote: > > > A couple corrections I'll include in the next version: > > > > Need few clarifications. Could you please help explain what am I missing here? > > > > > > > > On Sat, Apr 30, 2022 at 10:08:55PM -0700, Eric Biggers wrote: > > > > + 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; > > > > } > > > > > > I think this needs to be 'fc->purpose == FS_CONTEXT_FOR_RECONFIGURE || > > > fscrypt_is_dummy_policy_set(&sbi->s_dummy_enc_policy)', since ext4 can parse > > > mount options from both s_mount_opts and the regular mount options. > > > > Sorry, I am missing something here. Could you please help me understand why > > do we need the other OR case which you mentioned above i.e. > > "|| fscrypt_is_dummy_policy_set(&sbi->s_dummy_enc_policy)" > > > > So maybe to put it this way, when will it be the case where > > fscrypt_is_dummy_policy_set(&sbi->s_dummy_enc_policy) is true and it is not a > > FS_CONTEXT_FOR_RECONFIGURE case? > > The case where test_dummy_encryption is present in both the mount options stored > in the superblock and in the regular mount options. See how __ext4_fill_super() > parses and applies each source of options separately. Ok, thanks for clarifying. So this says that 1. in case of mount; if test_dummy_encryption is already set with some policy in the disk superblock and if the user is trying to change the mount option in options string, then that is not allowed. 2. Similarly if while remounting we try to change the mount option from the previous mount option, then again this is not allowed. > > > Also just in case if I did miss something that also means the comment after this > > case will not be valid anymore? > > i.e. > > /* > > * 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; > > That comment will still be correct. > > > > > > > > > > +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; > > > > > > To handle remounts correctly, this needs to be > > > '!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy) || > > > fscrypt_is_dummy_policy_set(&EXT4_SB(sb)->s_dummy_enc_policy)'. > > > > Why? > > Isn't it true that in remount we should update EXT4_SB(sb)->s_dummy_enc_policy > > only when ctx->dummy_enc_policy is set. If EXT4_SB(sb)->s_dummy_enc_policy is > > already set and ctx->dummy_enc_policy is not set, that means it's a remount case with no mount > > opts in which case ext4 should continue to have the same value of EXT4_SB(sb)->s_dummy_enc_policy? > > struct fscrypt_dummy_policy includes dynamically allocated memory, so > overwriting it without first freeing it would be a memory leak. Ok yes. Since this is dynamic memory allocation. Hence I see that ext4_apply_test_dummy_encryption() can be called from parse_apply_sb_mount_options(), __ext4_fill_super() and __ext4_remount(). Case 1: when this mount option is set in superblock 1. So in parse_apply_sb_mount_options(), this mount option will get set the first time if it is also set in superblock field. 2. So if we also have a same mount option set in regular mount, or during remount both will have sbi->s_dummy_enc_policy already set (from step 1 above), so we should do nothing here. Case 2: when this mount option is passed as regular mount 1. parse_apply_sb_mount_options() won't set this. 2. __ext4_fill_super() will set this mount option in sbi and hence __ext4_remount should not set this again. And as I see you are cleverly setting memset &ctx->dummy_enc_policy to 0 in case where we applied the parsed mount option to sbi. So that the actual policy doesn't get free when you call __ext4_fc_free() after ext4_apply_options() in parse_apply_sb_mount_options(). And in other cases where this mount option was not applied to sbi mount opt, in that case we anyway want this policy to get free. This somehow looks very confusing to me. But I guess with parse, check and apply mount APIs and with mount options in superblock, regular and remount path, this couldn't be avoided (although I am no expert in this area). Thanks for explaining. I hope I got this right ;) -ritesh
On 22/04/30 10:08PM, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > 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@google.com> 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 >
On Fri, May 13, 2022 at 04:37:41PM +0530, Ritesh Harjani wrote: > > @@ -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? > Yes, you're right. I might have missed that fc->fs_private was being set above. I was also a little lazy here; the part below 'out_free:' should be a separate patch since it also fixes a memory leak of s_qf_names. I'll fix that up. - Eric
On Fri, May 13, 2022 at 04:28:53PM +0530, Ritesh Harjani wrote: > On 22/05/11 06:03PM, Eric Biggers wrote: > > On Wed, May 11, 2022 at 11:24:33PM +0530, Ritesh Harjani wrote: > > > On 22/05/09 04:40PM, Eric Biggers wrote: > > > > A couple corrections I'll include in the next version: > > > > > > Need few clarifications. Could you please help explain what am I missing here? > > > > > > > > > > > On Sat, Apr 30, 2022 at 10:08:55PM -0700, Eric Biggers wrote: > > > > > + 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; > > > > > } > > > > > > > > I think this needs to be 'fc->purpose == FS_CONTEXT_FOR_RECONFIGURE || > > > > fscrypt_is_dummy_policy_set(&sbi->s_dummy_enc_policy)', since ext4 can parse > > > > mount options from both s_mount_opts and the regular mount options. > > > > > > Sorry, I am missing something here. Could you please help me understand why > > > do we need the other OR case which you mentioned above i.e. > > > "|| fscrypt_is_dummy_policy_set(&sbi->s_dummy_enc_policy)" > > > > > > So maybe to put it this way, when will it be the case where > > > fscrypt_is_dummy_policy_set(&sbi->s_dummy_enc_policy) is true and it is not a > > > FS_CONTEXT_FOR_RECONFIGURE case? > > > > The case where test_dummy_encryption is present in both the mount options stored > > in the superblock and in the regular mount options. See how __ext4_fill_super() > > parses and applies each source of options separately. > > Ok, thanks for clarifying. So this says that > 1. in case of mount; if test_dummy_encryption is already set with some policy in > the disk superblock and if the user is trying to change the mount option in > options string, then that is not allowed. > 2. Similarly if while remounting we try to change the mount option from the > previous mount option, then again this is not allowed. > Yes. I assume that the expected behavior of the on-disk mount options is the same as if they were prepended to the user-specified mount options. So we simply aren't allowing conflicting test_dummy_encryption options in the mount options, regardless of where the mount options came from. > > > > > +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; > > > > > > > > To handle remounts correctly, this needs to be > > > > '!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy) || > > > > fscrypt_is_dummy_policy_set(&EXT4_SB(sb)->s_dummy_enc_policy)'. > > > > > > Why? > > > Isn't it true that in remount we should update EXT4_SB(sb)->s_dummy_enc_policy > > > only when ctx->dummy_enc_policy is set. If EXT4_SB(sb)->s_dummy_enc_policy is > > > already set and ctx->dummy_enc_policy is not set, that means it's a remount case with no mount > > > opts in which case ext4 should continue to have the same value of EXT4_SB(sb)->s_dummy_enc_policy? > > > > struct fscrypt_dummy_policy includes dynamically allocated memory, so > > overwriting it without first freeing it would be a memory leak. > > Ok yes. Since this is dynamic memory allocation. Hence > I see that ext4_apply_test_dummy_encryption() can be called from > parse_apply_sb_mount_options(), __ext4_fill_super() and __ext4_remount(). > > Case 1: when this mount option is set in superblock > 1. So in parse_apply_sb_mount_options(), this mount option will get set the > first time if it is also set in superblock field. > > 2. So if we also have a same mount option set in regular mount, > or during remount both will have sbi->s_dummy_enc_policy already set (from > step 1 above), so we should do nothing here. > > Case 2: when this mount option is passed as regular mount > 1. parse_apply_sb_mount_options() won't set this. > 2. __ext4_fill_super() will set this mount option in sbi and hence __ext4_remount > should not set this again. > > And as I see you are cleverly setting memset &ctx->dummy_enc_policy to 0 > in case where we applied the parsed mount option to sbi. So that the actual > policy doesn't get free when you call __ext4_fc_free() after ext4_apply_options() > in parse_apply_sb_mount_options(). And in other cases where this mount option was > not applied to sbi mount opt, in that case we anyway want this policy to get > free. > > This somehow looks very confusing to me. But I guess with parse, check and apply > mount APIs and with mount options in superblock, regular and remount path, this > couldn't be avoided (although I am no expert in this area). > > Thanks for explaining. I hope I got this right ;) That's all correct. I think you're overthinking it a bit. The important thing is that if the dummy policy is being set, we must move it into the ext4_sb_info. Zeroing the old location is just part of transferring ownership of memory in C. If a dummy policy was already set, we don't support changing it, and we've checked that any "new" value is consistent with it, so we don't do anything. - Eric
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); 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) {