On 22/04/30 10:08PM, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
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@xxxxxxxxx>
> +{
> + 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
>
