[Please use reply-all, not reply!]
On Thu, May 26, 2022 at 10:55:07AM +0200, Lukas Czerner wrote:
> On Wed, May 25, 2022 at 09:04:12PM -0700, 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>
> > ---
>
> Hi, thanks for the patch it looks good, exept maybe small consideration
> below...
>
> > @@ -2673,11 +2656,11 @@ static int ext4_check_quota_consistency(struct fs_context *fc,
> > static int ext4_check_test_dummy_encryption(const struct fs_context *fc,
> > struct super_block *sb)
> > {
> > -#ifdef CONFIG_FS_ENCRYPTION
> > const struct ext4_fs_context *ctx = fc->fs_private;
> > const struct ext4_sb_info *sbi = EXT4_SB(sb);
> > + int err;
> >
> > - if (!(ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION))
> > + if (!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy))
>
> how about
>
> if (!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy) ||
> fscrypt_dummy_policies_equal(&sbi->s_dummy_enc_policy,
> &ctx->dummy_enc_policy))
> return 0;
>
> and remove the two fscrypt_dummy_policies_equal checks below?
>
> But regardless whether you want to change it, you can add
>
> Reviewed-by: Lukas Czerner <lczerner@xxxxxxxxxx>
>
That would work, but I think the code I've proposed makes it a little more
explicit what's going on.
- Eric
