| Message ID | 20220501050857.538984-2-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test_dummy_encryption fixes and cleanups | expand |
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 >
On Wed, May 11, 2022 at 06:20:23PM +0530, Ritesh Harjani wrote: > > +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> I did consider that, but that name seemed too long, as you mentioned. Thanks for the review! - Eric
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) +{ + 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; - } -#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.