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
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
