Re: [PATCH v4] ext4: fix up test_dummy_encryption handling for new mount API

[Eric Biggers <ebiggers@kernel.org>]

[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@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>
> > ---
> 
> 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@redhat.com>
> 

That would work, but I think the code I've proposed makes it a little more
explicit what's going on.

- Eric