Re: [f2fs-dev] [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API

[Eric Biggers <ebiggers@kernel.org>]

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