Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 11 May 2022 23:24:33 +0530
From:   Ritesh Harjani <ritesh.list@gmail.com>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     linux-fscrypt@vger.kernel.org, linux-ext4@vger.kernel.org,
        linux-f2fs-devel@lists.sourceforge.net,
        Lukas Czerner <lczerner@redhat.com>,
        Jeff Layton <jlayton@kernel.org>,
        Theodore Ts'o <tytso@mit.edu>, Jaegeuk Kim <jaegeuk@kernel.org>
Subject: Re: [f2fs-dev] [PATCH v2 5/7] ext4: fix up test_dummy_encryption
 handling for new mount API
Message-ID: <20220511175433.inua5nj6l7qtlywq@riteshh-domain>
References: <20220501050857.538984-1-ebiggers@kernel.org>
 <20220501050857.538984-6-ebiggers@kernel.org>
 <Ynmma+tkA2myRvz6@sol.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Ynmma+tkA2myRvz6@sol.localdomain>
Precedence: bulk

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