| Message ID | 20220421184040.173802-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: make test_dummy_encryption require the encrypt feature | expand |
On Thu, Apr 21, 2022 at 11:40:40AM -0700, 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. > > The motivation for this 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. One of the test scripts involved is xfstests's ext4/053, as the zero-day test rebot has remarked upon. Eric, could you look into submitting a patch to xfstests's ext4/053. Thanks! - Ted
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 81749eaddf4c1..420eb29183eb4 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -2817,17 +2817,24 @@ static int ext4_check_opt_consistency(struct fs_context *fc, } #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; + if (ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION) { + 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 (is_remount && !sbi->s_dummy_enc_policy.policy) { + ext4_msg(NULL, KERN_WARNING, + "Can't set test_dummy_encryption on remount"); + return -1; + } } #endif @@ -5272,12 +5279,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.