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 18:20:23 +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>,
        Theodore Ts'o <tytso@mit.edu>,
        Jaegeuk Kim <jaegeuk@kernel.org>,
        Jeff Layton <jlayton@kernel.org>
Subject: Re: [PATCH v2 1/7] ext4: only allow test_dummy_encryption when
 supported
Message-ID: <20220511125023.gxfkgft35gkjyhef@riteshh-domain>
References: <20220501050857.538984-1-ebiggers@kernel.org>
 <20220501050857.538984-2-ebiggers@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220501050857.538984-2-ebiggers@kernel.org>
Precedence: bulk

On 22/04/30 10:08PM, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> Make the test_dummy_encryption mount option require that the encrypt
> feature flag be already enabled on the filesystem, rather than
> automatically enabling it.  Practically, this means that "-O encrypt"
> will need to be included in MKFS_OPTIONS when running xfstests with the
> test_dummy_encryption mount option.  (ext4/053 also needs an update.)
>
> Moreover, as long as the preconditions for test_dummy_encryption are
> being tightened anyway, take the opportunity to start rejecting it when
> !CONFIG_FS_ENCRYPTION rather than ignoring it.
>
> The motivation for requiring the encrypt feature flag is that:
>
> - Having the filesystem auto-enable feature flags is problematic, as it
>   bypasses the usual sanity checks.  The specific issue which came up
>   recently is that in kernel versions where ext4 supports casefold but
>   not encrypt+casefold (v5.1 through v5.10), the kernel will happily add
>   the encrypt flag to a filesystem that has the casefold flag, making it
>   unmountable -- but only for subsequent mounts, not the initial one.
>   This confused the casefold support detection in xfstests, causing
>   generic/556 to fail rather than be skipped.
>
> - The xfstests-bld test runners (kvm-xfstests et al.) already use the
>   required mkfs flag, so they will not be affected by this change.  Only
>   users of test_dummy_encryption alone will be affected.  But, this
>   option has always been for testing only, so it should be fine to
>   require that the few users of this option update their test scripts.
>
> - f2fs already requires it (for its equivalent feature flag).
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>

So we are changing user behavior with this patch, but since it is only for
test_dummy_encryption mount option which is used for testing and given it is
nicely documented here, the patch looks good to me with a small nit.


> ---
>  fs/ext4/super.c | 59 +++++++++++++++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 1466fbdbc8e34..64ce17714e193 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2427,11 +2427,12 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  		ctx->spec |= EXT4_SPEC_DUMMY_ENCRYPTION;
>  		ctx->test_dummy_enc_arg = kmemdup_nul(param->string, param->size,
>  						      GFP_KERNEL);
> +		return 0;
>  #else
>  		ext4_msg(NULL, KERN_WARNING,
> -			 "Test dummy encryption mount option ignored");
> +			 "test_dummy_encryption option not supported");
> +		return -EINVAL;
>  #endif
> -		return 0;
>  	case Opt_dax:
>  	case Opt_dax_type:
>  #ifdef CONFIG_FS_DAX
> @@ -2786,12 +2787,43 @@ static int ext4_check_quota_consistency(struct fs_context *fc,
>  #endif
>  }
>
> +static int ext4_check_test_dummy_encryption(const struct fs_context *fc,
> +					    struct super_block *sb)

Maybe the function name should match with other option checking, like
ext4_check_test_dummy_encryption_consistency() similar to
ext4_check_quota_consistency(). This makes it clear that both are residents of
ext4_check_opt_consistency()

One can argue it makes the function name quite long. So I don't have hard
objections anyways.

So either ways, feel free to add -

Reviewed-by: Ritesh Harjani <ritesh.list@gmail.com>


> +{
> +	const struct ext4_fs_context *ctx = fc->fs_private;
> +	const struct ext4_sb_info *sbi = EXT4_SB(sb);
> +
> +	if (!IS_ENABLED(CONFIG_FS_ENCRYPTION) ||
> +	    !(ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION))
> +		return 0;
> +
> +	if (!ext4_has_feature_encrypt(sb)) {
> +		ext4_msg(NULL, KERN_WARNING,
> +			 "test_dummy_encryption requires encrypt feature");
> +		return -EINVAL;
> +	}
> +	/*
> +	 * This mount option is just for testing, and it's not worthwhile to
> +	 * implement the extra complexity (e.g. RCU protection) that would be
> +	 * needed to allow it to be set or changed during remount.  We do allow
> +	 * it to be specified during remount, but only if there is no change.
> +	 */
> +	if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE &&
> +	    !DUMMY_ENCRYPTION_ENABLED(sbi)) {
> +		ext4_msg(NULL, KERN_WARNING,
> +			 "Can't set test_dummy_encryption on remount");
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  static int ext4_check_opt_consistency(struct fs_context *fc,
>  				      struct super_block *sb)
>  {
>  	struct ext4_fs_context *ctx = fc->fs_private;
>  	struct ext4_sb_info *sbi = fc->s_fs_info;
>  	int is_remount = fc->purpose == FS_CONTEXT_FOR_RECONFIGURE;
> +	int err;
>
>  	if ((ctx->opt_flags & MOPT_NO_EXT2) && IS_EXT2_SB(sb)) {
>  		ext4_msg(NULL, KERN_ERR,
> @@ -2821,20 +2853,9 @@ static int ext4_check_opt_consistency(struct fs_context *fc,
>  				 "for blocksize < PAGE_SIZE");
>  	}
>
> -#ifdef CONFIG_FS_ENCRYPTION
> -	/*
> -	 * This mount option is just for testing, and it's not worthwhile to
> -	 * implement the extra complexity (e.g. RCU protection) that would be
> -	 * needed to allow it to be set or changed during remount.  We do allow
> -	 * it to be specified during remount, but only if there is no change.
> -	 */
> -	if ((ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION) &&
> -	    is_remount && !sbi->s_dummy_enc_policy.policy) {
> -		ext4_msg(NULL, KERN_WARNING,
> -			 "Can't set test_dummy_encryption on remount");
> -		return -1;

Nice, we also got rid of -1 return value in this patch which is returned to user.
I think this should have been -EINVAL from the very beginning.


-ritesh

> -	}
> -#endif
> +	err = ext4_check_test_dummy_encryption(fc, sb);
> +	if (err)
> +		return err;
>
>  	if ((ctx->spec & EXT4_SPEC_DATAJ) && is_remount) {
>  		if (!sbi->s_journal) {
> @@ -5279,12 +5300,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  		goto failed_mount_wq;
>  	}
>
> -	if (DUMMY_ENCRYPTION_ENABLED(sbi) && !sb_rdonly(sb) &&
> -	    !ext4_has_feature_encrypt(sb)) {
> -		ext4_set_feature_encrypt(sb);
> -		ext4_commit_super(sb);
> -	}
> -
>  	/*
>  	 * Get the # of file system overhead blocks from the
>  	 * superblock if present.
> --
> 2.36.0
>