* [PATCH v2 0/7] test_dummy_encryption fixes and cleanups
@ 2022-05-01 5:08 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Lukas Czerner, Theodore Ts'o, Jaegeuk Kim, Jeff Layton
This series cleans up and fixes the way that ext4 and f2fs handle the
test_dummy_encryption mount option:
- Patches 1-2 make test_dummy_encryption consistently require that the
'encrypt' feature flag already be enabled and that
CONFIG_FS_ENCRYPTION be enabled. Note, this will cause xfstest
ext4/053 to start failing; my xfstests patch "ext4/053: update the
test_dummy_encryption tests" will fix that.
- Patches 3-7 replace the fscrypt_set_test_dummy_encryption() helper
function with new functions that work properly with the new mount API,
by splitting up the parsing, checking, and applying steps. These fix
bugs that were introduced when ext4 started using the new mount API.
We can either take all these patches through the fscrypt tree, or we can
take them in multiple cycles as follows:
1. patch 1 via ext4, patch 2 via f2fs, patch 3-4 via fscrypt
2. patch 5 via ext4, patch 6 via f2fs
3. patch 7 via fscrypt
Ted and Jaegeuk, let me know what you prefer.
Changed v1 => v2:
- Added patches 2-7
- Also reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
Eric Biggers (7):
ext4: only allow test_dummy_encryption when supported
f2fs: reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
fscrypt: factor out fscrypt_policy_to_key_spec()
fscrypt: add new helper functions for test_dummy_encryption
ext4: fix up test_dummy_encryption handling for new mount API
f2fs: use the updated test_dummy_encryption helper functions
fscrypt: remove fscrypt_set_test_dummy_encryption()
fs/crypto/fscrypt_private.h | 6 +-
fs/crypto/keyring.c | 64 +++++++++++---
fs/crypto/keysetup.c | 20 +----
fs/crypto/policy.c | 121 +++++++++++++------------
fs/ext4/ext4.h | 6 --
fs/ext4/super.c | 172 ++++++++++++++++++++----------------
fs/f2fs/super.c | 28 ++++--
include/linux/fscrypt.h | 41 ++++++++-
8 files changed, 280 insertions(+), 178 deletions(-)
base-commit: 8013d1d3d2e33236dee13a133fba49ad55045e79
--
2.36.0
^ permalink raw reply [flat|nested] 42+ messages in thread
* [f2fs-dev] [PATCH v2 0/7] test_dummy_encryption fixes and cleanups
@ 2022-05-01 5:08 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Jeff Layton, Lukas Czerner, Theodore Ts'o, Jaegeuk Kim
This series cleans up and fixes the way that ext4 and f2fs handle the
test_dummy_encryption mount option:
- Patches 1-2 make test_dummy_encryption consistently require that the
'encrypt' feature flag already be enabled and that
CONFIG_FS_ENCRYPTION be enabled. Note, this will cause xfstest
ext4/053 to start failing; my xfstests patch "ext4/053: update the
test_dummy_encryption tests" will fix that.
- Patches 3-7 replace the fscrypt_set_test_dummy_encryption() helper
function with new functions that work properly with the new mount API,
by splitting up the parsing, checking, and applying steps. These fix
bugs that were introduced when ext4 started using the new mount API.
We can either take all these patches through the fscrypt tree, or we can
take them in multiple cycles as follows:
1. patch 1 via ext4, patch 2 via f2fs, patch 3-4 via fscrypt
2. patch 5 via ext4, patch 6 via f2fs
3. patch 7 via fscrypt
Ted and Jaegeuk, let me know what you prefer.
Changed v1 => v2:
- Added patches 2-7
- Also reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
Eric Biggers (7):
ext4: only allow test_dummy_encryption when supported
f2fs: reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
fscrypt: factor out fscrypt_policy_to_key_spec()
fscrypt: add new helper functions for test_dummy_encryption
ext4: fix up test_dummy_encryption handling for new mount API
f2fs: use the updated test_dummy_encryption helper functions
fscrypt: remove fscrypt_set_test_dummy_encryption()
fs/crypto/fscrypt_private.h | 6 +-
fs/crypto/keyring.c | 64 +++++++++++---
fs/crypto/keysetup.c | 20 +----
fs/crypto/policy.c | 121 +++++++++++++------------
fs/ext4/ext4.h | 6 --
fs/ext4/super.c | 172 ++++++++++++++++++++----------------
fs/f2fs/super.c | 28 ++++--
include/linux/fscrypt.h | 41 ++++++++-
8 files changed, 280 insertions(+), 178 deletions(-)
base-commit: 8013d1d3d2e33236dee13a133fba49ad55045e79
--
2.36.0
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 1/7] ext4: only allow test_dummy_encryption when supported
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
@ 2022-05-01 5:08 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Lukas Czerner, Theodore Ts'o, Jaegeuk Kim, Jeff Layton
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>
---
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)
+{
+ 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.
--
2.36.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [f2fs-dev] [PATCH v2 1/7] ext4: only allow test_dummy_encryption when supported
@ 2022-05-01 5:08 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Jeff Layton, Lukas Czerner, Theodore Ts'o, Jaegeuk Kim
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>
---
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)
+{
+ 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.
--
2.36.0
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2 2/7] f2fs: reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
@ 2022-05-01 5:08 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Lukas Czerner, Theodore Ts'o, Jaegeuk Kim, Jeff Layton
From: Eric Biggers <ebiggers@google.com>
There is no good reason to allow this mount option when the kernel isn't
configured with encryption support. Since this option is only for
testing, we can just fix this; we don't really need to worry about
breaking anyone who might be counting on this option being ignored.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
fs/f2fs/super.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 4368f90571bd6..6f69491aa5731 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -525,10 +525,11 @@ static int f2fs_set_test_dummy_encryption(struct super_block *sb,
return -EINVAL;
}
f2fs_warn(sbi, "Test dummy encryption mode enabled");
+ return 0;
#else
- f2fs_warn(sbi, "Test dummy encryption mount option ignored");
+ f2fs_warn(sbi, "test_dummy_encryption option not supported");
+ return -EINVAL;
#endif
- return 0;
}
#ifdef CONFIG_F2FS_FS_COMPRESSION
--
2.36.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [f2fs-dev] [PATCH v2 2/7] f2fs: reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
@ 2022-05-01 5:08 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Jeff Layton, Lukas Czerner, Theodore Ts'o, Jaegeuk Kim
From: Eric Biggers <ebiggers@google.com>
There is no good reason to allow this mount option when the kernel isn't
configured with encryption support. Since this option is only for
testing, we can just fix this; we don't really need to worry about
breaking anyone who might be counting on this option being ignored.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
fs/f2fs/super.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 4368f90571bd6..6f69491aa5731 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -525,10 +525,11 @@ static int f2fs_set_test_dummy_encryption(struct super_block *sb,
return -EINVAL;
}
f2fs_warn(sbi, "Test dummy encryption mode enabled");
+ return 0;
#else
- f2fs_warn(sbi, "Test dummy encryption mount option ignored");
+ f2fs_warn(sbi, "test_dummy_encryption option not supported");
+ return -EINVAL;
#endif
- return 0;
}
#ifdef CONFIG_F2FS_FS_COMPRESSION
--
2.36.0
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2 3/7] fscrypt: factor out fscrypt_policy_to_key_spec()
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
@ 2022-05-01 5:08 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Lukas Czerner, Theodore Ts'o, Jaegeuk Kim, Jeff Layton
From: Eric Biggers <ebiggers@google.com>
Factor out a function that builds the fscrypt_key_specifier for an
fscrypt_policy. Before this was only needed when finding the key for a
file, but now it will also be needed for test_dummy_encryption support.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
fs/crypto/fscrypt_private.h | 2 ++
fs/crypto/keysetup.c | 20 +++-----------------
fs/crypto/policy.c | 20 ++++++++++++++++++++
3 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 5b0a9e6478b5d..f32d0ee91e70e 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -621,6 +621,8 @@ int fscrypt_setup_v1_file_key_via_subscribed_keyrings(struct fscrypt_info *ci);
bool fscrypt_policies_equal(const union fscrypt_policy *policy1,
const union fscrypt_policy *policy2);
+int fscrypt_policy_to_key_spec(const union fscrypt_policy *policy,
+ struct fscrypt_key_specifier *key_spec);
bool fscrypt_supported_policy(const union fscrypt_policy *policy_u,
const struct inode *inode);
int fscrypt_policy_from_context(union fscrypt_policy *policy_u,
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index eede186b04ce3..571d220d6e226 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -425,23 +425,9 @@ static int setup_file_encryption_key(struct fscrypt_info *ci,
if (err)
return err;
- switch (ci->ci_policy.version) {
- case FSCRYPT_POLICY_V1:
- mk_spec.type = FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR;
- memcpy(mk_spec.u.descriptor,
- ci->ci_policy.v1.master_key_descriptor,
- FSCRYPT_KEY_DESCRIPTOR_SIZE);
- break;
- case FSCRYPT_POLICY_V2:
- mk_spec.type = FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER;
- memcpy(mk_spec.u.identifier,
- ci->ci_policy.v2.master_key_identifier,
- FSCRYPT_KEY_IDENTIFIER_SIZE);
- break;
- default:
- WARN_ON(1);
- return -EINVAL;
- }
+ err = fscrypt_policy_to_key_spec(&ci->ci_policy, &mk_spec);
+ if (err)
+ return err;
key = fscrypt_find_master_key(ci->ci_inode->i_sb, &mk_spec);
if (IS_ERR(key)) {
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index ed3d623724cdd..2a11276913a98 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -32,6 +32,26 @@ bool fscrypt_policies_equal(const union fscrypt_policy *policy1,
return !memcmp(policy1, policy2, fscrypt_policy_size(policy1));
}
+int fscrypt_policy_to_key_spec(const union fscrypt_policy *policy,
+ struct fscrypt_key_specifier *key_spec)
+{
+ switch (policy->version) {
+ case FSCRYPT_POLICY_V1:
+ key_spec->type = FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR;
+ memcpy(key_spec->u.descriptor, policy->v1.master_key_descriptor,
+ FSCRYPT_KEY_DESCRIPTOR_SIZE);
+ return 0;
+ case FSCRYPT_POLICY_V2:
+ key_spec->type = FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER;
+ memcpy(key_spec->u.identifier, policy->v2.master_key_identifier,
+ FSCRYPT_KEY_IDENTIFIER_SIZE);
+ return 0;
+ default:
+ WARN_ON(1);
+ return -EINVAL;
+ }
+}
+
static const union fscrypt_policy *
fscrypt_get_dummy_policy(struct super_block *sb)
{
--
2.36.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [f2fs-dev] [PATCH v2 3/7] fscrypt: factor out fscrypt_policy_to_key_spec()
@ 2022-05-01 5:08 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Jeff Layton, Lukas Czerner, Theodore Ts'o, Jaegeuk Kim
From: Eric Biggers <ebiggers@google.com>
Factor out a function that builds the fscrypt_key_specifier for an
fscrypt_policy. Before this was only needed when finding the key for a
file, but now it will also be needed for test_dummy_encryption support.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
fs/crypto/fscrypt_private.h | 2 ++
fs/crypto/keysetup.c | 20 +++-----------------
fs/crypto/policy.c | 20 ++++++++++++++++++++
3 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 5b0a9e6478b5d..f32d0ee91e70e 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -621,6 +621,8 @@ int fscrypt_setup_v1_file_key_via_subscribed_keyrings(struct fscrypt_info *ci);
bool fscrypt_policies_equal(const union fscrypt_policy *policy1,
const union fscrypt_policy *policy2);
+int fscrypt_policy_to_key_spec(const union fscrypt_policy *policy,
+ struct fscrypt_key_specifier *key_spec);
bool fscrypt_supported_policy(const union fscrypt_policy *policy_u,
const struct inode *inode);
int fscrypt_policy_from_context(union fscrypt_policy *policy_u,
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index eede186b04ce3..571d220d6e226 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -425,23 +425,9 @@ static int setup_file_encryption_key(struct fscrypt_info *ci,
if (err)
return err;
- switch (ci->ci_policy.version) {
- case FSCRYPT_POLICY_V1:
- mk_spec.type = FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR;
- memcpy(mk_spec.u.descriptor,
- ci->ci_policy.v1.master_key_descriptor,
- FSCRYPT_KEY_DESCRIPTOR_SIZE);
- break;
- case FSCRYPT_POLICY_V2:
- mk_spec.type = FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER;
- memcpy(mk_spec.u.identifier,
- ci->ci_policy.v2.master_key_identifier,
- FSCRYPT_KEY_IDENTIFIER_SIZE);
- break;
- default:
- WARN_ON(1);
- return -EINVAL;
- }
+ err = fscrypt_policy_to_key_spec(&ci->ci_policy, &mk_spec);
+ if (err)
+ return err;
key = fscrypt_find_master_key(ci->ci_inode->i_sb, &mk_spec);
if (IS_ERR(key)) {
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index ed3d623724cdd..2a11276913a98 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -32,6 +32,26 @@ bool fscrypt_policies_equal(const union fscrypt_policy *policy1,
return !memcmp(policy1, policy2, fscrypt_policy_size(policy1));
}
+int fscrypt_policy_to_key_spec(const union fscrypt_policy *policy,
+ struct fscrypt_key_specifier *key_spec)
+{
+ switch (policy->version) {
+ case FSCRYPT_POLICY_V1:
+ key_spec->type = FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR;
+ memcpy(key_spec->u.descriptor, policy->v1.master_key_descriptor,
+ FSCRYPT_KEY_DESCRIPTOR_SIZE);
+ return 0;
+ case FSCRYPT_POLICY_V2:
+ key_spec->type = FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER;
+ memcpy(key_spec->u.identifier, policy->v2.master_key_identifier,
+ FSCRYPT_KEY_IDENTIFIER_SIZE);
+ return 0;
+ default:
+ WARN_ON(1);
+ return -EINVAL;
+ }
+}
+
static const union fscrypt_policy *
fscrypt_get_dummy_policy(struct super_block *sb)
{
--
2.36.0
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2 4/7] fscrypt: add new helper functions for test_dummy_encryption
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
@ 2022-05-01 5:08 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Lukas Czerner, Theodore Ts'o, Jaegeuk Kim, Jeff Layton
From: Eric Biggers <ebiggers@google.com>
Unfortunately the design of fscrypt_set_test_dummy_encryption() doesn't
work properly for the new mount API, as it combines too many steps into
one function:
- Parse the argument to test_dummy_encryption
- Check the setting against the filesystem instance
- Apply the setting to the filesystem instance
The new mount API has split these into separate steps. ext4 partially
worked around this by duplicating some of the logic, but it still had
some bugs. To address this, add some new helper functions that split up
the steps of fscrypt_set_test_dummy_encryption():
- fscrypt_parse_test_dummy_encryption()
- fscrypt_dummy_policies_equal()
- fscrypt_add_test_dummy_key()
While we're add it, also add a function fscrypt_is_dummy_policy_set()
which will be useful to avoid some #ifdef's.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
fs/crypto/fscrypt_private.h | 4 +-
fs/crypto/keyring.c | 64 +++++++++++++++++----
fs/crypto/policy.c | 112 +++++++++++++++++++-----------------
include/linux/fscrypt.h | 39 +++++++++++++
4 files changed, 151 insertions(+), 68 deletions(-)
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index f32d0ee91e70e..36941d7fd6ad1 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -545,8 +545,8 @@ struct key *
fscrypt_find_master_key(struct super_block *sb,
const struct fscrypt_key_specifier *mk_spec);
-int fscrypt_add_test_dummy_key(struct super_block *sb,
- struct fscrypt_key_specifier *key_spec);
+int fscrypt_get_test_dummy_key_identifier(
+ u8 key_identifier[FSCRYPT_KEY_IDENTIFIER_SIZE]);
int fscrypt_verify_key_added(struct super_block *sb,
const u8 identifier[FSCRYPT_KEY_IDENTIFIER_SIZE]);
diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index 0b3ffbb4faf4a..caee9f8620dd9 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -688,28 +688,68 @@ int fscrypt_ioctl_add_key(struct file *filp, void __user *_uarg)
}
EXPORT_SYMBOL_GPL(fscrypt_ioctl_add_key);
-/*
- * Add the key for '-o test_dummy_encryption' to the filesystem keyring.
- *
- * Use a per-boot random key to prevent people from misusing this option.
- */
-int fscrypt_add_test_dummy_key(struct super_block *sb,
- struct fscrypt_key_specifier *key_spec)
+static void
+fscrypt_get_test_dummy_secret(struct fscrypt_master_key_secret *secret)
{
static u8 test_key[FSCRYPT_MAX_KEY_SIZE];
+
+ get_random_once(test_key, FSCRYPT_MAX_KEY_SIZE);
+
+ memset(secret, 0, sizeof(*secret));
+ secret->size = FSCRYPT_MAX_KEY_SIZE;
+ memcpy(secret->raw, test_key, FSCRYPT_MAX_KEY_SIZE);
+}
+
+int fscrypt_get_test_dummy_key_identifier(
+ u8 key_identifier[FSCRYPT_KEY_IDENTIFIER_SIZE])
+{
struct fscrypt_master_key_secret secret;
int err;
- get_random_once(test_key, FSCRYPT_MAX_KEY_SIZE);
+ fscrypt_get_test_dummy_secret(&secret);
- memset(&secret, 0, sizeof(secret));
- secret.size = FSCRYPT_MAX_KEY_SIZE;
- memcpy(secret.raw, test_key, FSCRYPT_MAX_KEY_SIZE);
+ err = fscrypt_init_hkdf(&secret.hkdf, secret.raw, secret.size);
+ if (err)
+ goto out;
+ err = fscrypt_hkdf_expand(&secret.hkdf, HKDF_CONTEXT_KEY_IDENTIFIER,
+ NULL, 0, key_identifier,
+ FSCRYPT_KEY_IDENTIFIER_SIZE);
+out:
+ wipe_master_key_secret(&secret);
+ return err;
+}
+
+/**
+ * fscrypt_add_test_dummy_key() - add the test dummy encryption key
+ * @sb: the filesystem instance to add the key to
+ * @dummy_policy: the encryption policy for test_dummy_encryption
+ *
+ * If needed, add the key for the test_dummy_encryption mount option to the
+ * filesystem. To prevent misuse of this mount option, a per-boot random key is
+ * used instead of a hardcoded one. This makes it so that any encrypted files
+ * created using this option won't be accessible after a reboot.
+ *
+ * Return: 0 on success, -errno on failure
+ */
+int fscrypt_add_test_dummy_key(struct super_block *sb,
+ const struct fscrypt_dummy_policy *dummy_policy)
+{
+ const union fscrypt_policy *policy = dummy_policy->policy;
+ struct fscrypt_key_specifier key_spec;
+ struct fscrypt_master_key_secret secret;
+ int err;
- err = add_master_key(sb, &secret, key_spec);
+ if (!policy)
+ return 0;
+ err = fscrypt_policy_to_key_spec(policy, &key_spec);
+ if (err)
+ return err;
+ fscrypt_get_test_dummy_secret(&secret);
+ err = add_master_key(sb, &secret, &key_spec);
wipe_master_key_secret(&secret);
return err;
}
+EXPORT_SYMBOL_GPL(fscrypt_add_test_dummy_key);
/*
* Verify that the current user has added a master key with the given identifier
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 2a11276913a98..5f858cee1e3b0 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -10,6 +10,7 @@
* Modified by Eric Biggers, 2019 for v2 policy support.
*/
+#include <linux/fs_context.h>
#include <linux/random.h>
#include <linux/seq_file.h>
#include <linux/string.h>
@@ -724,73 +725,45 @@ int fscrypt_set_context(struct inode *inode, void *fs_data)
EXPORT_SYMBOL_GPL(fscrypt_set_context);
/**
- * fscrypt_set_test_dummy_encryption() - handle '-o test_dummy_encryption'
- * @sb: the filesystem on which test_dummy_encryption is being specified
- * @arg: the argument to the test_dummy_encryption option. May be NULL.
- * @dummy_policy: the filesystem's current dummy policy (input/output, see
- * below)
+ * fscrypt_parse_test_dummy_encryption() - parse the test_dummy_encryption mount option
+ * @param: the mount option
+ * @dummy_policy: (input/output) the place to write the dummy policy that will
+ * result from parsing the option. Zero-initialize this. If a policy is
+ * already set here (due to test_dummy_encryption being given multiple
+ * times), then this function will verify that the policies are the same.
*
- * Handle the test_dummy_encryption mount option by creating a dummy encryption
- * policy, saving it in @dummy_policy, and adding the corresponding dummy
- * encryption key to the filesystem. If the @dummy_policy is already set, then
- * instead validate that it matches @arg. Don't support changing it via
- * remount, as that is difficult to do safely.
- *
- * Return: 0 on success (dummy policy set, or the same policy is already set);
- * -EEXIST if a different dummy policy is already set;
- * or another -errno value.
+ * Return: 0 on success; -EINVAL if the argument is invalid; -EEXIST if the
+ * argument conflicts with one already specified; or -ENOMEM.
*/
-int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
- struct fscrypt_dummy_policy *dummy_policy)
+int fscrypt_parse_test_dummy_encryption(const struct fs_parameter *param,
+ struct fscrypt_dummy_policy *dummy_policy)
{
- struct fscrypt_key_specifier key_spec = { 0 };
- int version;
- union fscrypt_policy *policy = NULL;
+ const char *arg = "v2";
+ union fscrypt_policy *policy;
int err;
- if (!arg)
- arg = "v2";
-
- if (!strcmp(arg, "v1")) {
- version = FSCRYPT_POLICY_V1;
- key_spec.type = FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR;
- memset(key_spec.u.descriptor, 0x42,
- FSCRYPT_KEY_DESCRIPTOR_SIZE);
- } else if (!strcmp(arg, "v2")) {
- version = FSCRYPT_POLICY_V2;
- key_spec.type = FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER;
- /* key_spec.u.identifier gets filled in when adding the key */
- } else {
- err = -EINVAL;
- goto out;
- }
+ if (param->type == fs_value_is_string && *param->string)
+ arg = param->string;
policy = kzalloc(sizeof(*policy), GFP_KERNEL);
- if (!policy) {
- err = -ENOMEM;
- goto out;
- }
-
- err = fscrypt_add_test_dummy_key(sb, &key_spec);
- if (err)
- goto out;
+ if (!policy)
+ return -ENOMEM;
- policy->version = version;
- switch (policy->version) {
- case FSCRYPT_POLICY_V1:
+ if (!strcmp(arg, "v1")) {
+ policy->version = FSCRYPT_POLICY_V1;
policy->v1.contents_encryption_mode = FSCRYPT_MODE_AES_256_XTS;
policy->v1.filenames_encryption_mode = FSCRYPT_MODE_AES_256_CTS;
- memcpy(policy->v1.master_key_descriptor, key_spec.u.descriptor,
+ memset(policy->v1.master_key_descriptor, 0x42,
FSCRYPT_KEY_DESCRIPTOR_SIZE);
- break;
- case FSCRYPT_POLICY_V2:
+ } else if (!strcmp(arg, "v2")) {
+ policy->version = FSCRYPT_POLICY_V2;
policy->v2.contents_encryption_mode = FSCRYPT_MODE_AES_256_XTS;
policy->v2.filenames_encryption_mode = FSCRYPT_MODE_AES_256_CTS;
- memcpy(policy->v2.master_key_identifier, key_spec.u.identifier,
- FSCRYPT_KEY_IDENTIFIER_SIZE);
- break;
- default:
- WARN_ON(1);
+ err = fscrypt_get_test_dummy_key_identifier(
+ policy->v2.master_key_identifier);
+ if (err)
+ goto out;
+ } else {
err = -EINVAL;
goto out;
}
@@ -809,6 +782,37 @@ int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
kfree(policy);
return err;
}
+EXPORT_SYMBOL_GPL(fscrypt_parse_test_dummy_encryption);
+
+/**
+ * fscrypt_dummy_policies_equal() - check whether two dummy policies are equal
+ * @p1: the first test dummy policy (may be unset)
+ * @p2: the second test dummy policy (may be unset)
+ *
+ * Return: %true if the dummy policies are both set and equal, or both unset.
+ */
+bool fscrypt_dummy_policies_equal(const struct fscrypt_dummy_policy *p1,
+ const struct fscrypt_dummy_policy *p2)
+{
+ if (!p1->policy && !p2->policy)
+ return true;
+ if (!p1->policy || !p2->policy)
+ return false;
+ return fscrypt_policies_equal(p1->policy, p2->policy);
+}
+EXPORT_SYMBOL_GPL(fscrypt_dummy_policies_equal);
+
+/* Deprecated, do not use */
+int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
+ struct fscrypt_dummy_policy *dummy_policy)
+{
+ struct fs_parameter param = {
+ .type = fs_value_is_string,
+ .string = arg ? (char *)arg : "",
+ };
+ return fscrypt_parse_test_dummy_encryption(¶m, dummy_policy) ?:
+ fscrypt_add_test_dummy_key(sb, dummy_policy);
+}
EXPORT_SYMBOL_GPL(fscrypt_set_test_dummy_encryption);
/**
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 50d92d805bd8c..099b881e63e49 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -22,6 +22,7 @@
union fscrypt_policy;
struct fscrypt_info;
+struct fs_parameter;
struct seq_file;
struct fscrypt_str {
@@ -279,10 +280,19 @@ struct fscrypt_dummy_policy {
const union fscrypt_policy *policy;
};
+int fscrypt_parse_test_dummy_encryption(const struct fs_parameter *param,
+ struct fscrypt_dummy_policy *dummy_policy);
+bool fscrypt_dummy_policies_equal(const struct fscrypt_dummy_policy *p1,
+ const struct fscrypt_dummy_policy *p2);
int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
struct fscrypt_dummy_policy *dummy_policy);
void fscrypt_show_test_dummy_encryption(struct seq_file *seq, char sep,
struct super_block *sb);
+static inline bool
+fscrypt_is_dummy_policy_set(const struct fscrypt_dummy_policy *dummy_policy)
+{
+ return dummy_policy->policy != NULL;
+}
static inline void
fscrypt_free_dummy_policy(struct fscrypt_dummy_policy *dummy_policy)
{
@@ -293,6 +303,8 @@ fscrypt_free_dummy_policy(struct fscrypt_dummy_policy *dummy_policy)
/* keyring.c */
void fscrypt_sb_free(struct super_block *sb);
int fscrypt_ioctl_add_key(struct file *filp, void __user *arg);
+int fscrypt_add_test_dummy_key(struct super_block *sb,
+ const struct fscrypt_dummy_policy *dummy_policy);
int fscrypt_ioctl_remove_key(struct file *filp, void __user *arg);
int fscrypt_ioctl_remove_key_all_users(struct file *filp, void __user *arg);
int fscrypt_ioctl_get_key_status(struct file *filp, void __user *arg);
@@ -467,12 +479,32 @@ static inline int fscrypt_set_context(struct inode *inode, void *fs_data)
struct fscrypt_dummy_policy {
};
+static inline int
+fscrypt_parse_test_dummy_encryption(const struct fs_parameter *param,
+ struct fscrypt_dummy_policy *dummy_policy)
+{
+ return -EINVAL;
+}
+
+static inline bool
+fscrypt_dummy_policies_equal(const struct fscrypt_dummy_policy *p1,
+ const struct fscrypt_dummy_policy *p2)
+{
+ return true;
+}
+
static inline void fscrypt_show_test_dummy_encryption(struct seq_file *seq,
char sep,
struct super_block *sb)
{
}
+static inline bool
+fscrypt_is_dummy_policy_set(const struct fscrypt_dummy_policy *dummy_policy)
+{
+ return false;
+}
+
static inline void
fscrypt_free_dummy_policy(struct fscrypt_dummy_policy *dummy_policy)
{
@@ -488,6 +520,13 @@ static inline int fscrypt_ioctl_add_key(struct file *filp, void __user *arg)
return -EOPNOTSUPP;
}
+static inline int
+fscrypt_add_test_dummy_key(struct super_block *sb,
+ const struct fscrypt_dummy_policy *dummy_policy)
+{
+ return 0;
+}
+
static inline int fscrypt_ioctl_remove_key(struct file *filp, void __user *arg)
{
return -EOPNOTSUPP;
--
2.36.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [f2fs-dev] [PATCH v2 4/7] fscrypt: add new helper functions for test_dummy_encryption
@ 2022-05-01 5:08 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Jeff Layton, Lukas Czerner, Theodore Ts'o, Jaegeuk Kim
From: Eric Biggers <ebiggers@google.com>
Unfortunately the design of fscrypt_set_test_dummy_encryption() doesn't
work properly for the new mount API, as it combines too many steps into
one function:
- Parse the argument to test_dummy_encryption
- Check the setting against the filesystem instance
- Apply the setting to the filesystem instance
The new mount API has split these into separate steps. ext4 partially
worked around this by duplicating some of the logic, but it still had
some bugs. To address this, add some new helper functions that split up
the steps of fscrypt_set_test_dummy_encryption():
- fscrypt_parse_test_dummy_encryption()
- fscrypt_dummy_policies_equal()
- fscrypt_add_test_dummy_key()
While we're add it, also add a function fscrypt_is_dummy_policy_set()
which will be useful to avoid some #ifdef's.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
fs/crypto/fscrypt_private.h | 4 +-
fs/crypto/keyring.c | 64 +++++++++++++++++----
fs/crypto/policy.c | 112 +++++++++++++++++++-----------------
include/linux/fscrypt.h | 39 +++++++++++++
4 files changed, 151 insertions(+), 68 deletions(-)
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index f32d0ee91e70e..36941d7fd6ad1 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -545,8 +545,8 @@ struct key *
fscrypt_find_master_key(struct super_block *sb,
const struct fscrypt_key_specifier *mk_spec);
-int fscrypt_add_test_dummy_key(struct super_block *sb,
- struct fscrypt_key_specifier *key_spec);
+int fscrypt_get_test_dummy_key_identifier(
+ u8 key_identifier[FSCRYPT_KEY_IDENTIFIER_SIZE]);
int fscrypt_verify_key_added(struct super_block *sb,
const u8 identifier[FSCRYPT_KEY_IDENTIFIER_SIZE]);
diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index 0b3ffbb4faf4a..caee9f8620dd9 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -688,28 +688,68 @@ int fscrypt_ioctl_add_key(struct file *filp, void __user *_uarg)
}
EXPORT_SYMBOL_GPL(fscrypt_ioctl_add_key);
-/*
- * Add the key for '-o test_dummy_encryption' to the filesystem keyring.
- *
- * Use a per-boot random key to prevent people from misusing this option.
- */
-int fscrypt_add_test_dummy_key(struct super_block *sb,
- struct fscrypt_key_specifier *key_spec)
+static void
+fscrypt_get_test_dummy_secret(struct fscrypt_master_key_secret *secret)
{
static u8 test_key[FSCRYPT_MAX_KEY_SIZE];
+
+ get_random_once(test_key, FSCRYPT_MAX_KEY_SIZE);
+
+ memset(secret, 0, sizeof(*secret));
+ secret->size = FSCRYPT_MAX_KEY_SIZE;
+ memcpy(secret->raw, test_key, FSCRYPT_MAX_KEY_SIZE);
+}
+
+int fscrypt_get_test_dummy_key_identifier(
+ u8 key_identifier[FSCRYPT_KEY_IDENTIFIER_SIZE])
+{
struct fscrypt_master_key_secret secret;
int err;
- get_random_once(test_key, FSCRYPT_MAX_KEY_SIZE);
+ fscrypt_get_test_dummy_secret(&secret);
- memset(&secret, 0, sizeof(secret));
- secret.size = FSCRYPT_MAX_KEY_SIZE;
- memcpy(secret.raw, test_key, FSCRYPT_MAX_KEY_SIZE);
+ err = fscrypt_init_hkdf(&secret.hkdf, secret.raw, secret.size);
+ if (err)
+ goto out;
+ err = fscrypt_hkdf_expand(&secret.hkdf, HKDF_CONTEXT_KEY_IDENTIFIER,
+ NULL, 0, key_identifier,
+ FSCRYPT_KEY_IDENTIFIER_SIZE);
+out:
+ wipe_master_key_secret(&secret);
+ return err;
+}
+
+/**
+ * fscrypt_add_test_dummy_key() - add the test dummy encryption key
+ * @sb: the filesystem instance to add the key to
+ * @dummy_policy: the encryption policy for test_dummy_encryption
+ *
+ * If needed, add the key for the test_dummy_encryption mount option to the
+ * filesystem. To prevent misuse of this mount option, a per-boot random key is
+ * used instead of a hardcoded one. This makes it so that any encrypted files
+ * created using this option won't be accessible after a reboot.
+ *
+ * Return: 0 on success, -errno on failure
+ */
+int fscrypt_add_test_dummy_key(struct super_block *sb,
+ const struct fscrypt_dummy_policy *dummy_policy)
+{
+ const union fscrypt_policy *policy = dummy_policy->policy;
+ struct fscrypt_key_specifier key_spec;
+ struct fscrypt_master_key_secret secret;
+ int err;
- err = add_master_key(sb, &secret, key_spec);
+ if (!policy)
+ return 0;
+ err = fscrypt_policy_to_key_spec(policy, &key_spec);
+ if (err)
+ return err;
+ fscrypt_get_test_dummy_secret(&secret);
+ err = add_master_key(sb, &secret, &key_spec);
wipe_master_key_secret(&secret);
return err;
}
+EXPORT_SYMBOL_GPL(fscrypt_add_test_dummy_key);
/*
* Verify that the current user has added a master key with the given identifier
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 2a11276913a98..5f858cee1e3b0 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -10,6 +10,7 @@
* Modified by Eric Biggers, 2019 for v2 policy support.
*/
+#include <linux/fs_context.h>
#include <linux/random.h>
#include <linux/seq_file.h>
#include <linux/string.h>
@@ -724,73 +725,45 @@ int fscrypt_set_context(struct inode *inode, void *fs_data)
EXPORT_SYMBOL_GPL(fscrypt_set_context);
/**
- * fscrypt_set_test_dummy_encryption() - handle '-o test_dummy_encryption'
- * @sb: the filesystem on which test_dummy_encryption is being specified
- * @arg: the argument to the test_dummy_encryption option. May be NULL.
- * @dummy_policy: the filesystem's current dummy policy (input/output, see
- * below)
+ * fscrypt_parse_test_dummy_encryption() - parse the test_dummy_encryption mount option
+ * @param: the mount option
+ * @dummy_policy: (input/output) the place to write the dummy policy that will
+ * result from parsing the option. Zero-initialize this. If a policy is
+ * already set here (due to test_dummy_encryption being given multiple
+ * times), then this function will verify that the policies are the same.
*
- * Handle the test_dummy_encryption mount option by creating a dummy encryption
- * policy, saving it in @dummy_policy, and adding the corresponding dummy
- * encryption key to the filesystem. If the @dummy_policy is already set, then
- * instead validate that it matches @arg. Don't support changing it via
- * remount, as that is difficult to do safely.
- *
- * Return: 0 on success (dummy policy set, or the same policy is already set);
- * -EEXIST if a different dummy policy is already set;
- * or another -errno value.
+ * Return: 0 on success; -EINVAL if the argument is invalid; -EEXIST if the
+ * argument conflicts with one already specified; or -ENOMEM.
*/
-int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
- struct fscrypt_dummy_policy *dummy_policy)
+int fscrypt_parse_test_dummy_encryption(const struct fs_parameter *param,
+ struct fscrypt_dummy_policy *dummy_policy)
{
- struct fscrypt_key_specifier key_spec = { 0 };
- int version;
- union fscrypt_policy *policy = NULL;
+ const char *arg = "v2";
+ union fscrypt_policy *policy;
int err;
- if (!arg)
- arg = "v2";
-
- if (!strcmp(arg, "v1")) {
- version = FSCRYPT_POLICY_V1;
- key_spec.type = FSCRYPT_KEY_SPEC_TYPE_DESCRIPTOR;
- memset(key_spec.u.descriptor, 0x42,
- FSCRYPT_KEY_DESCRIPTOR_SIZE);
- } else if (!strcmp(arg, "v2")) {
- version = FSCRYPT_POLICY_V2;
- key_spec.type = FSCRYPT_KEY_SPEC_TYPE_IDENTIFIER;
- /* key_spec.u.identifier gets filled in when adding the key */
- } else {
- err = -EINVAL;
- goto out;
- }
+ if (param->type == fs_value_is_string && *param->string)
+ arg = param->string;
policy = kzalloc(sizeof(*policy), GFP_KERNEL);
- if (!policy) {
- err = -ENOMEM;
- goto out;
- }
-
- err = fscrypt_add_test_dummy_key(sb, &key_spec);
- if (err)
- goto out;
+ if (!policy)
+ return -ENOMEM;
- policy->version = version;
- switch (policy->version) {
- case FSCRYPT_POLICY_V1:
+ if (!strcmp(arg, "v1")) {
+ policy->version = FSCRYPT_POLICY_V1;
policy->v1.contents_encryption_mode = FSCRYPT_MODE_AES_256_XTS;
policy->v1.filenames_encryption_mode = FSCRYPT_MODE_AES_256_CTS;
- memcpy(policy->v1.master_key_descriptor, key_spec.u.descriptor,
+ memset(policy->v1.master_key_descriptor, 0x42,
FSCRYPT_KEY_DESCRIPTOR_SIZE);
- break;
- case FSCRYPT_POLICY_V2:
+ } else if (!strcmp(arg, "v2")) {
+ policy->version = FSCRYPT_POLICY_V2;
policy->v2.contents_encryption_mode = FSCRYPT_MODE_AES_256_XTS;
policy->v2.filenames_encryption_mode = FSCRYPT_MODE_AES_256_CTS;
- memcpy(policy->v2.master_key_identifier, key_spec.u.identifier,
- FSCRYPT_KEY_IDENTIFIER_SIZE);
- break;
- default:
- WARN_ON(1);
+ err = fscrypt_get_test_dummy_key_identifier(
+ policy->v2.master_key_identifier);
+ if (err)
+ goto out;
+ } else {
err = -EINVAL;
goto out;
}
@@ -809,6 +782,37 @@ int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
kfree(policy);
return err;
}
+EXPORT_SYMBOL_GPL(fscrypt_parse_test_dummy_encryption);
+
+/**
+ * fscrypt_dummy_policies_equal() - check whether two dummy policies are equal
+ * @p1: the first test dummy policy (may be unset)
+ * @p2: the second test dummy policy (may be unset)
+ *
+ * Return: %true if the dummy policies are both set and equal, or both unset.
+ */
+bool fscrypt_dummy_policies_equal(const struct fscrypt_dummy_policy *p1,
+ const struct fscrypt_dummy_policy *p2)
+{
+ if (!p1->policy && !p2->policy)
+ return true;
+ if (!p1->policy || !p2->policy)
+ return false;
+ return fscrypt_policies_equal(p1->policy, p2->policy);
+}
+EXPORT_SYMBOL_GPL(fscrypt_dummy_policies_equal);
+
+/* Deprecated, do not use */
+int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
+ struct fscrypt_dummy_policy *dummy_policy)
+{
+ struct fs_parameter param = {
+ .type = fs_value_is_string,
+ .string = arg ? (char *)arg : "",
+ };
+ return fscrypt_parse_test_dummy_encryption(¶m, dummy_policy) ?:
+ fscrypt_add_test_dummy_key(sb, dummy_policy);
+}
EXPORT_SYMBOL_GPL(fscrypt_set_test_dummy_encryption);
/**
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 50d92d805bd8c..099b881e63e49 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -22,6 +22,7 @@
union fscrypt_policy;
struct fscrypt_info;
+struct fs_parameter;
struct seq_file;
struct fscrypt_str {
@@ -279,10 +280,19 @@ struct fscrypt_dummy_policy {
const union fscrypt_policy *policy;
};
+int fscrypt_parse_test_dummy_encryption(const struct fs_parameter *param,
+ struct fscrypt_dummy_policy *dummy_policy);
+bool fscrypt_dummy_policies_equal(const struct fscrypt_dummy_policy *p1,
+ const struct fscrypt_dummy_policy *p2);
int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
struct fscrypt_dummy_policy *dummy_policy);
void fscrypt_show_test_dummy_encryption(struct seq_file *seq, char sep,
struct super_block *sb);
+static inline bool
+fscrypt_is_dummy_policy_set(const struct fscrypt_dummy_policy *dummy_policy)
+{
+ return dummy_policy->policy != NULL;
+}
static inline void
fscrypt_free_dummy_policy(struct fscrypt_dummy_policy *dummy_policy)
{
@@ -293,6 +303,8 @@ fscrypt_free_dummy_policy(struct fscrypt_dummy_policy *dummy_policy)
/* keyring.c */
void fscrypt_sb_free(struct super_block *sb);
int fscrypt_ioctl_add_key(struct file *filp, void __user *arg);
+int fscrypt_add_test_dummy_key(struct super_block *sb,
+ const struct fscrypt_dummy_policy *dummy_policy);
int fscrypt_ioctl_remove_key(struct file *filp, void __user *arg);
int fscrypt_ioctl_remove_key_all_users(struct file *filp, void __user *arg);
int fscrypt_ioctl_get_key_status(struct file *filp, void __user *arg);
@@ -467,12 +479,32 @@ static inline int fscrypt_set_context(struct inode *inode, void *fs_data)
struct fscrypt_dummy_policy {
};
+static inline int
+fscrypt_parse_test_dummy_encryption(const struct fs_parameter *param,
+ struct fscrypt_dummy_policy *dummy_policy)
+{
+ return -EINVAL;
+}
+
+static inline bool
+fscrypt_dummy_policies_equal(const struct fscrypt_dummy_policy *p1,
+ const struct fscrypt_dummy_policy *p2)
+{
+ return true;
+}
+
static inline void fscrypt_show_test_dummy_encryption(struct seq_file *seq,
char sep,
struct super_block *sb)
{
}
+static inline bool
+fscrypt_is_dummy_policy_set(const struct fscrypt_dummy_policy *dummy_policy)
+{
+ return false;
+}
+
static inline void
fscrypt_free_dummy_policy(struct fscrypt_dummy_policy *dummy_policy)
{
@@ -488,6 +520,13 @@ static inline int fscrypt_ioctl_add_key(struct file *filp, void __user *arg)
return -EOPNOTSUPP;
}
+static inline int
+fscrypt_add_test_dummy_key(struct super_block *sb,
+ const struct fscrypt_dummy_policy *dummy_policy)
+{
+ return 0;
+}
+
static inline int fscrypt_ioctl_remove_key(struct file *filp, void __user *arg)
{
return -EOPNOTSUPP;
--
2.36.0
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
@ 2022-05-01 5:08 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Lukas Czerner, Theodore Ts'o, Jaegeuk Kim, Jeff Layton
From: Eric Biggers <ebiggers@google.com>
Since ext4 was converted to the new mount API, the test_dummy_encryption
mount option isn't being handled entirely correctly, because the needed
fscrypt_set_test_dummy_encryption() helper function combines
parsing/checking/applying into one function. That doesn't work well
with the new mount API, which split these into separate steps.
This was sort of okay anyway, due to the parsing logic that was copied
from fscrypt_set_test_dummy_encryption() into ext4_parse_param(),
combined with an additional check in ext4_check_test_dummy_encryption().
However, these overlooked the case of changing the value of
test_dummy_encryption on remount, which isn't allowed but ext4 wasn't
detecting until ext4_apply_options() when it's too late to fail.
Another bug is that if test_dummy_encryption was specified multiple
times with an argument, memory was leaked.
Fix this up properly by using the new helper functions that allow
splitting up the parse/check/apply steps for test_dummy_encryption.
Fixes: cebe85d570cf ("ext4: switch to the new mount api")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
fs/ext4/ext4.h | 6 ---
fs/ext4/super.c | 131 +++++++++++++++++++++++++-----------------------
2 files changed, 67 insertions(+), 70 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index a743b1e3b89ec..f6d6661817b63 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1440,12 +1440,6 @@ struct ext4_super_block {
#ifdef __KERNEL__
-#ifdef CONFIG_FS_ENCRYPTION
-#define DUMMY_ENCRYPTION_ENABLED(sbi) ((sbi)->s_dummy_enc_policy.policy != NULL)
-#else
-#define DUMMY_ENCRYPTION_ENABLED(sbi) (0)
-#endif
-
/* Number of quota types we support */
#define EXT4_MAXQUOTAS 3
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 64ce17714e193..43e4cd358b33b 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -87,7 +87,7 @@ static struct inode *ext4_get_journal_inode(struct super_block *sb,
static int ext4_validate_options(struct fs_context *fc);
static int ext4_check_opt_consistency(struct fs_context *fc,
struct super_block *sb);
-static int ext4_apply_options(struct fs_context *fc, struct super_block *sb);
+static void ext4_apply_options(struct fs_context *fc, struct super_block *sb);
static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param);
static int ext4_get_tree(struct fs_context *fc);
static int ext4_reconfigure(struct fs_context *fc);
@@ -1989,31 +1989,12 @@ ext4_sb_read_encoding(const struct ext4_super_block *es)
}
#endif
-static int ext4_set_test_dummy_encryption(struct super_block *sb, char *arg)
-{
-#ifdef CONFIG_FS_ENCRYPTION
- struct ext4_sb_info *sbi = EXT4_SB(sb);
- int err;
-
- err = fscrypt_set_test_dummy_encryption(sb, arg,
- &sbi->s_dummy_enc_policy);
- if (err) {
- ext4_msg(sb, KERN_WARNING,
- "Error while setting test dummy encryption [%d]", err);
- return err;
- }
- ext4_msg(sb, KERN_WARNING, "Test dummy encryption mode enabled");
-#endif
- return 0;
-}
-
#define EXT4_SPEC_JQUOTA (1 << 0)
#define EXT4_SPEC_JQFMT (1 << 1)
#define EXT4_SPEC_DATAJ (1 << 2)
#define EXT4_SPEC_SB_BLOCK (1 << 3)
#define EXT4_SPEC_JOURNAL_DEV (1 << 4)
#define EXT4_SPEC_JOURNAL_IOPRIO (1 << 5)
-#define EXT4_SPEC_DUMMY_ENCRYPTION (1 << 6)
#define EXT4_SPEC_s_want_extra_isize (1 << 7)
#define EXT4_SPEC_s_max_batch_time (1 << 8)
#define EXT4_SPEC_s_min_batch_time (1 << 9)
@@ -2030,7 +2011,7 @@ static int ext4_set_test_dummy_encryption(struct super_block *sb, char *arg)
struct ext4_fs_context {
char *s_qf_names[EXT4_MAXQUOTAS];
- char *test_dummy_enc_arg;
+ struct fscrypt_dummy_policy dummy_enc_policy;
int s_jquota_fmt; /* Format of quota to use */
#ifdef CONFIG_EXT4_DEBUG
int s_fc_debug_max_replay;
@@ -2061,9 +2042,8 @@ struct ext4_fs_context {
ext4_fsblk_t s_sb_block;
};
-static void ext4_fc_free(struct fs_context *fc)
+static void __ext4_fc_free(struct ext4_fs_context *ctx)
{
- struct ext4_fs_context *ctx = fc->fs_private;
int i;
if (!ctx)
@@ -2072,10 +2052,15 @@ static void ext4_fc_free(struct fs_context *fc)
for (i = 0; i < EXT4_MAXQUOTAS; i++)
kfree(ctx->s_qf_names[i]);
- kfree(ctx->test_dummy_enc_arg);
+ fscrypt_free_dummy_policy(&ctx->dummy_enc_policy);
kfree(ctx);
}
+static void ext4_fc_free(struct fs_context *fc)
+{
+ __ext4_fc_free(fc->fs_private);
+}
+
int ext4_init_fs_context(struct fs_context *fc)
{
struct ext4_fs_context *ctx;
@@ -2148,6 +2133,29 @@ static int unnote_qf_name(struct fs_context *fc, int qtype)
}
#endif
+static int ext4_parse_test_dummy_encryption(const struct fs_parameter *param,
+ struct ext4_fs_context *ctx)
+{
+ int err;
+
+ if (!IS_ENABLED(CONFIG_FS_ENCRYPTION)) {
+ ext4_msg(NULL, KERN_WARNING,
+ "test_dummy_encryption option not supported");
+ return -EINVAL;
+ }
+ err = fscrypt_parse_test_dummy_encryption(param,
+ &ctx->dummy_enc_policy);
+ if (err == -EINVAL) {
+ ext4_msg(NULL, KERN_WARNING,
+ "Value of option \"%s\" is unrecognized", param->key);
+ } else if (err == -EEXIST) {
+ ext4_msg(NULL, KERN_WARNING,
+ "Conflicting test_dummy_encryption options");
+ return -EINVAL;
+ }
+ return err;
+}
+
#define EXT4_SET_CTX(name) \
static inline void ctx_set_##name(struct ext4_fs_context *ctx, \
unsigned long flag) \
@@ -2410,29 +2418,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
ctx->spec |= EXT4_SPEC_JOURNAL_IOPRIO;
return 0;
case Opt_test_dummy_encryption:
-#ifdef CONFIG_FS_ENCRYPTION
- if (param->type == fs_value_is_flag) {
- ctx->spec |= EXT4_SPEC_DUMMY_ENCRYPTION;
- ctx->test_dummy_enc_arg = NULL;
- return 0;
- }
- if (*param->string &&
- !(!strcmp(param->string, "v1") ||
- !strcmp(param->string, "v2"))) {
- ext4_msg(NULL, KERN_WARNING,
- "Value of option \"%s\" is unrecognized",
- param->key);
- return -EINVAL;
- }
- 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 option not supported");
- return -EINVAL;
-#endif
+ return ext4_parse_test_dummy_encryption(param, ctx);
case Opt_dax:
case Opt_dax_type:
#ifdef CONFIG_FS_DAX
@@ -2623,10 +2609,11 @@ static int parse_apply_sb_mount_options(struct super_block *sb,
if (s_ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO)
m_ctx->journal_ioprio = s_ctx->journal_ioprio;
- ret = ext4_apply_options(fc, sb);
+ ext4_apply_options(fc, sb);
+ ret = 0;
out_free:
- kfree(s_ctx);
+ __ext4_fc_free(s_ctx);
kfree(fc);
kfree(s_mount_opts);
return ret;
@@ -2792,9 +2779,9 @@ static int ext4_check_test_dummy_encryption(const struct fs_context *fc,
{
const struct ext4_fs_context *ctx = fc->fs_private;
const struct ext4_sb_info *sbi = EXT4_SB(sb);
+ int err;
- if (!IS_ENABLED(CONFIG_FS_ENCRYPTION) ||
- !(ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION))
+ if (!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy))
return 0;
if (!ext4_has_feature_encrypt(sb)) {
@@ -2808,13 +2795,35 @@ static int ext4_check_test_dummy_encryption(const struct fs_context *fc,
* 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)) {
+ 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;
}
- return 0;
+ /*
+ * 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;
+ EXT4_SB(sb)->s_dummy_enc_policy = ctx->dummy_enc_policy;
+ memset(&ctx->dummy_enc_policy, 0, sizeof(ctx->dummy_enc_policy));
+ ext4_msg(sb, KERN_WARNING, "Test dummy encryption mode enabled");
}
static int ext4_check_opt_consistency(struct fs_context *fc,
@@ -2901,11 +2910,10 @@ static int ext4_check_opt_consistency(struct fs_context *fc,
return ext4_check_quota_consistency(fc, sb);
}
-static int ext4_apply_options(struct fs_context *fc, struct super_block *sb)
+static void ext4_apply_options(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 ret = 0;
sbi->s_mount_opt &= ~ctx->mask_s_mount_opt;
sbi->s_mount_opt |= ctx->vals_s_mount_opt;
@@ -2942,10 +2950,7 @@ static int ext4_apply_options(struct fs_context *fc, struct super_block *sb)
ext4_apply_quota_options(fc, sb);
- if (ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION)
- ret = ext4_set_test_dummy_encryption(sb, ctx->test_dummy_enc_arg);
-
- return ret;
+ ext4_apply_test_dummy_encryption(ctx, sb);
}
@@ -4667,9 +4672,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
if (err < 0)
goto failed_mount;
- err = ext4_apply_options(fc, sb);
- if (err < 0)
- goto failed_mount;
+ ext4_apply_options(fc, sb);
#if IS_ENABLED(CONFIG_UNICODE)
if (ext4_has_feature_casefold(sb) && !sb->s_encoding) {
--
2.36.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [f2fs-dev] [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
@ 2022-05-01 5:08 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Jeff Layton, Lukas Czerner, Theodore Ts'o, Jaegeuk Kim
From: Eric Biggers <ebiggers@google.com>
Since ext4 was converted to the new mount API, the test_dummy_encryption
mount option isn't being handled entirely correctly, because the needed
fscrypt_set_test_dummy_encryption() helper function combines
parsing/checking/applying into one function. That doesn't work well
with the new mount API, which split these into separate steps.
This was sort of okay anyway, due to the parsing logic that was copied
from fscrypt_set_test_dummy_encryption() into ext4_parse_param(),
combined with an additional check in ext4_check_test_dummy_encryption().
However, these overlooked the case of changing the value of
test_dummy_encryption on remount, which isn't allowed but ext4 wasn't
detecting until ext4_apply_options() when it's too late to fail.
Another bug is that if test_dummy_encryption was specified multiple
times with an argument, memory was leaked.
Fix this up properly by using the new helper functions that allow
splitting up the parse/check/apply steps for test_dummy_encryption.
Fixes: cebe85d570cf ("ext4: switch to the new mount api")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
fs/ext4/ext4.h | 6 ---
fs/ext4/super.c | 131 +++++++++++++++++++++++++-----------------------
2 files changed, 67 insertions(+), 70 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index a743b1e3b89ec..f6d6661817b63 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1440,12 +1440,6 @@ struct ext4_super_block {
#ifdef __KERNEL__
-#ifdef CONFIG_FS_ENCRYPTION
-#define DUMMY_ENCRYPTION_ENABLED(sbi) ((sbi)->s_dummy_enc_policy.policy != NULL)
-#else
-#define DUMMY_ENCRYPTION_ENABLED(sbi) (0)
-#endif
-
/* Number of quota types we support */
#define EXT4_MAXQUOTAS 3
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 64ce17714e193..43e4cd358b33b 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -87,7 +87,7 @@ static struct inode *ext4_get_journal_inode(struct super_block *sb,
static int ext4_validate_options(struct fs_context *fc);
static int ext4_check_opt_consistency(struct fs_context *fc,
struct super_block *sb);
-static int ext4_apply_options(struct fs_context *fc, struct super_block *sb);
+static void ext4_apply_options(struct fs_context *fc, struct super_block *sb);
static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param);
static int ext4_get_tree(struct fs_context *fc);
static int ext4_reconfigure(struct fs_context *fc);
@@ -1989,31 +1989,12 @@ ext4_sb_read_encoding(const struct ext4_super_block *es)
}
#endif
-static int ext4_set_test_dummy_encryption(struct super_block *sb, char *arg)
-{
-#ifdef CONFIG_FS_ENCRYPTION
- struct ext4_sb_info *sbi = EXT4_SB(sb);
- int err;
-
- err = fscrypt_set_test_dummy_encryption(sb, arg,
- &sbi->s_dummy_enc_policy);
- if (err) {
- ext4_msg(sb, KERN_WARNING,
- "Error while setting test dummy encryption [%d]", err);
- return err;
- }
- ext4_msg(sb, KERN_WARNING, "Test dummy encryption mode enabled");
-#endif
- return 0;
-}
-
#define EXT4_SPEC_JQUOTA (1 << 0)
#define EXT4_SPEC_JQFMT (1 << 1)
#define EXT4_SPEC_DATAJ (1 << 2)
#define EXT4_SPEC_SB_BLOCK (1 << 3)
#define EXT4_SPEC_JOURNAL_DEV (1 << 4)
#define EXT4_SPEC_JOURNAL_IOPRIO (1 << 5)
-#define EXT4_SPEC_DUMMY_ENCRYPTION (1 << 6)
#define EXT4_SPEC_s_want_extra_isize (1 << 7)
#define EXT4_SPEC_s_max_batch_time (1 << 8)
#define EXT4_SPEC_s_min_batch_time (1 << 9)
@@ -2030,7 +2011,7 @@ static int ext4_set_test_dummy_encryption(struct super_block *sb, char *arg)
struct ext4_fs_context {
char *s_qf_names[EXT4_MAXQUOTAS];
- char *test_dummy_enc_arg;
+ struct fscrypt_dummy_policy dummy_enc_policy;
int s_jquota_fmt; /* Format of quota to use */
#ifdef CONFIG_EXT4_DEBUG
int s_fc_debug_max_replay;
@@ -2061,9 +2042,8 @@ struct ext4_fs_context {
ext4_fsblk_t s_sb_block;
};
-static void ext4_fc_free(struct fs_context *fc)
+static void __ext4_fc_free(struct ext4_fs_context *ctx)
{
- struct ext4_fs_context *ctx = fc->fs_private;
int i;
if (!ctx)
@@ -2072,10 +2052,15 @@ static void ext4_fc_free(struct fs_context *fc)
for (i = 0; i < EXT4_MAXQUOTAS; i++)
kfree(ctx->s_qf_names[i]);
- kfree(ctx->test_dummy_enc_arg);
+ fscrypt_free_dummy_policy(&ctx->dummy_enc_policy);
kfree(ctx);
}
+static void ext4_fc_free(struct fs_context *fc)
+{
+ __ext4_fc_free(fc->fs_private);
+}
+
int ext4_init_fs_context(struct fs_context *fc)
{
struct ext4_fs_context *ctx;
@@ -2148,6 +2133,29 @@ static int unnote_qf_name(struct fs_context *fc, int qtype)
}
#endif
+static int ext4_parse_test_dummy_encryption(const struct fs_parameter *param,
+ struct ext4_fs_context *ctx)
+{
+ int err;
+
+ if (!IS_ENABLED(CONFIG_FS_ENCRYPTION)) {
+ ext4_msg(NULL, KERN_WARNING,
+ "test_dummy_encryption option not supported");
+ return -EINVAL;
+ }
+ err = fscrypt_parse_test_dummy_encryption(param,
+ &ctx->dummy_enc_policy);
+ if (err == -EINVAL) {
+ ext4_msg(NULL, KERN_WARNING,
+ "Value of option \"%s\" is unrecognized", param->key);
+ } else if (err == -EEXIST) {
+ ext4_msg(NULL, KERN_WARNING,
+ "Conflicting test_dummy_encryption options");
+ return -EINVAL;
+ }
+ return err;
+}
+
#define EXT4_SET_CTX(name) \
static inline void ctx_set_##name(struct ext4_fs_context *ctx, \
unsigned long flag) \
@@ -2410,29 +2418,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
ctx->spec |= EXT4_SPEC_JOURNAL_IOPRIO;
return 0;
case Opt_test_dummy_encryption:
-#ifdef CONFIG_FS_ENCRYPTION
- if (param->type == fs_value_is_flag) {
- ctx->spec |= EXT4_SPEC_DUMMY_ENCRYPTION;
- ctx->test_dummy_enc_arg = NULL;
- return 0;
- }
- if (*param->string &&
- !(!strcmp(param->string, "v1") ||
- !strcmp(param->string, "v2"))) {
- ext4_msg(NULL, KERN_WARNING,
- "Value of option \"%s\" is unrecognized",
- param->key);
- return -EINVAL;
- }
- 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 option not supported");
- return -EINVAL;
-#endif
+ return ext4_parse_test_dummy_encryption(param, ctx);
case Opt_dax:
case Opt_dax_type:
#ifdef CONFIG_FS_DAX
@@ -2623,10 +2609,11 @@ static int parse_apply_sb_mount_options(struct super_block *sb,
if (s_ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO)
m_ctx->journal_ioprio = s_ctx->journal_ioprio;
- ret = ext4_apply_options(fc, sb);
+ ext4_apply_options(fc, sb);
+ ret = 0;
out_free:
- kfree(s_ctx);
+ __ext4_fc_free(s_ctx);
kfree(fc);
kfree(s_mount_opts);
return ret;
@@ -2792,9 +2779,9 @@ static int ext4_check_test_dummy_encryption(const struct fs_context *fc,
{
const struct ext4_fs_context *ctx = fc->fs_private;
const struct ext4_sb_info *sbi = EXT4_SB(sb);
+ int err;
- if (!IS_ENABLED(CONFIG_FS_ENCRYPTION) ||
- !(ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION))
+ if (!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy))
return 0;
if (!ext4_has_feature_encrypt(sb)) {
@@ -2808,13 +2795,35 @@ static int ext4_check_test_dummy_encryption(const struct fs_context *fc,
* 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)) {
+ 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;
}
- return 0;
+ /*
+ * 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;
+ EXT4_SB(sb)->s_dummy_enc_policy = ctx->dummy_enc_policy;
+ memset(&ctx->dummy_enc_policy, 0, sizeof(ctx->dummy_enc_policy));
+ ext4_msg(sb, KERN_WARNING, "Test dummy encryption mode enabled");
}
static int ext4_check_opt_consistency(struct fs_context *fc,
@@ -2901,11 +2910,10 @@ static int ext4_check_opt_consistency(struct fs_context *fc,
return ext4_check_quota_consistency(fc, sb);
}
-static int ext4_apply_options(struct fs_context *fc, struct super_block *sb)
+static void ext4_apply_options(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 ret = 0;
sbi->s_mount_opt &= ~ctx->mask_s_mount_opt;
sbi->s_mount_opt |= ctx->vals_s_mount_opt;
@@ -2942,10 +2950,7 @@ static int ext4_apply_options(struct fs_context *fc, struct super_block *sb)
ext4_apply_quota_options(fc, sb);
- if (ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION)
- ret = ext4_set_test_dummy_encryption(sb, ctx->test_dummy_enc_arg);
-
- return ret;
+ ext4_apply_test_dummy_encryption(ctx, sb);
}
@@ -4667,9 +4672,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
if (err < 0)
goto failed_mount;
- err = ext4_apply_options(fc, sb);
- if (err < 0)
- goto failed_mount;
+ ext4_apply_options(fc, sb);
#if IS_ENABLED(CONFIG_UNICODE)
if (ext4_has_feature_casefold(sb) && !sb->s_encoding) {
--
2.36.0
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2 6/7] f2fs: use the updated test_dummy_encryption helper functions
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
@ 2022-05-01 5:08 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Lukas Czerner, Theodore Ts'o, Jaegeuk Kim, Jeff Layton
From: Eric Biggers <ebiggers@google.com>
Switch f2fs over to the functions that are replacing
fscrypt_set_test_dummy_encryption(). Since f2fs hasn't been converted
to the new mount API yet, this doesn't really provide a benefit for
f2fs. But it allows fscrypt_set_test_dummy_encryption() to be removed.
Also take the opportunity to eliminate an #ifdef.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
fs/f2fs/super.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 6f69491aa5731..c08cbe0dfcd85 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -8,6 +8,7 @@
#include <linux/module.h>
#include <linux/init.h>
#include <linux/fs.h>
+#include <linux/fs_context.h>
#include <linux/sched/mm.h>
#include <linux/statfs.h>
#include <linux/buffer_head.h>
@@ -492,9 +493,19 @@ static int f2fs_set_test_dummy_encryption(struct super_block *sb,
bool is_remount)
{
struct f2fs_sb_info *sbi = F2FS_SB(sb);
-#ifdef CONFIG_FS_ENCRYPTION
+ struct fs_parameter param = {
+ .type = fs_value_is_string,
+ .string = arg->from ? arg->from : "",
+ };
+ struct fscrypt_dummy_policy *policy =
+ &F2FS_OPTION(sbi).dummy_enc_policy;
int err;
+ if (!IS_ENABLED(CONFIG_FS_ENCRYPTION)) {
+ f2fs_warn(sbi, "test_dummy_encryption option not supported");
+ return -EINVAL;
+ }
+
if (!f2fs_sb_has_encrypt(sbi)) {
f2fs_err(sbi, "Encrypt feature is off");
return -EINVAL;
@@ -506,12 +517,12 @@ static int f2fs_set_test_dummy_encryption(struct super_block *sb,
* 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 && !F2FS_OPTION(sbi).dummy_enc_policy.policy) {
+ if (is_remount && !fscrypt_is_dummy_policy_set(policy)) {
f2fs_warn(sbi, "Can't set test_dummy_encryption on remount");
return -EINVAL;
}
- err = fscrypt_set_test_dummy_encryption(
- sb, arg->from, &F2FS_OPTION(sbi).dummy_enc_policy);
+
+ err = fscrypt_parse_test_dummy_encryption(¶m, policy);
if (err) {
if (err == -EEXIST)
f2fs_warn(sbi,
@@ -524,12 +535,14 @@ static int f2fs_set_test_dummy_encryption(struct super_block *sb,
opt, err);
return -EINVAL;
}
+ err = fscrypt_add_test_dummy_key(sb, policy);
+ if (err) {
+ f2fs_warn(sbi, "Error adding test dummy encryption key [%d]",
+ err);
+ return err;
+ }
f2fs_warn(sbi, "Test dummy encryption mode enabled");
return 0;
-#else
- f2fs_warn(sbi, "test_dummy_encryption option not supported");
- return -EINVAL;
-#endif
}
#ifdef CONFIG_F2FS_FS_COMPRESSION
--
2.36.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [f2fs-dev] [PATCH v2 6/7] f2fs: use the updated test_dummy_encryption helper functions
@ 2022-05-01 5:08 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Jeff Layton, Lukas Czerner, Theodore Ts'o, Jaegeuk Kim
From: Eric Biggers <ebiggers@google.com>
Switch f2fs over to the functions that are replacing
fscrypt_set_test_dummy_encryption(). Since f2fs hasn't been converted
to the new mount API yet, this doesn't really provide a benefit for
f2fs. But it allows fscrypt_set_test_dummy_encryption() to be removed.
Also take the opportunity to eliminate an #ifdef.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
fs/f2fs/super.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 6f69491aa5731..c08cbe0dfcd85 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -8,6 +8,7 @@
#include <linux/module.h>
#include <linux/init.h>
#include <linux/fs.h>
+#include <linux/fs_context.h>
#include <linux/sched/mm.h>
#include <linux/statfs.h>
#include <linux/buffer_head.h>
@@ -492,9 +493,19 @@ static int f2fs_set_test_dummy_encryption(struct super_block *sb,
bool is_remount)
{
struct f2fs_sb_info *sbi = F2FS_SB(sb);
-#ifdef CONFIG_FS_ENCRYPTION
+ struct fs_parameter param = {
+ .type = fs_value_is_string,
+ .string = arg->from ? arg->from : "",
+ };
+ struct fscrypt_dummy_policy *policy =
+ &F2FS_OPTION(sbi).dummy_enc_policy;
int err;
+ if (!IS_ENABLED(CONFIG_FS_ENCRYPTION)) {
+ f2fs_warn(sbi, "test_dummy_encryption option not supported");
+ return -EINVAL;
+ }
+
if (!f2fs_sb_has_encrypt(sbi)) {
f2fs_err(sbi, "Encrypt feature is off");
return -EINVAL;
@@ -506,12 +517,12 @@ static int f2fs_set_test_dummy_encryption(struct super_block *sb,
* 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 && !F2FS_OPTION(sbi).dummy_enc_policy.policy) {
+ if (is_remount && !fscrypt_is_dummy_policy_set(policy)) {
f2fs_warn(sbi, "Can't set test_dummy_encryption on remount");
return -EINVAL;
}
- err = fscrypt_set_test_dummy_encryption(
- sb, arg->from, &F2FS_OPTION(sbi).dummy_enc_policy);
+
+ err = fscrypt_parse_test_dummy_encryption(¶m, policy);
if (err) {
if (err == -EEXIST)
f2fs_warn(sbi,
@@ -524,12 +535,14 @@ static int f2fs_set_test_dummy_encryption(struct super_block *sb,
opt, err);
return -EINVAL;
}
+ err = fscrypt_add_test_dummy_key(sb, policy);
+ if (err) {
+ f2fs_warn(sbi, "Error adding test dummy encryption key [%d]",
+ err);
+ return err;
+ }
f2fs_warn(sbi, "Test dummy encryption mode enabled");
return 0;
-#else
- f2fs_warn(sbi, "test_dummy_encryption option not supported");
- return -EINVAL;
-#endif
}
#ifdef CONFIG_F2FS_FS_COMPRESSION
--
2.36.0
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2 7/7] fscrypt: remove fscrypt_set_test_dummy_encryption()
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
@ 2022-05-01 5:08 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Lukas Czerner, Theodore Ts'o, Jaegeuk Kim, Jeff Layton
From: Eric Biggers <ebiggers@google.com>
Now that all its callers have been converted to
fscrypt_parse_test_dummy_encryption() and fscrypt_add_test_dummy_key()
instead, fscrypt_set_test_dummy_encryption() can be removed.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
fs/crypto/policy.c | 13 -------------
include/linux/fscrypt.h | 2 --
2 files changed, 15 deletions(-)
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 5f858cee1e3b0..d0a8921577def 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -802,19 +802,6 @@ bool fscrypt_dummy_policies_equal(const struct fscrypt_dummy_policy *p1,
}
EXPORT_SYMBOL_GPL(fscrypt_dummy_policies_equal);
-/* Deprecated, do not use */
-int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
- struct fscrypt_dummy_policy *dummy_policy)
-{
- struct fs_parameter param = {
- .type = fs_value_is_string,
- .string = arg ? (char *)arg : "",
- };
- return fscrypt_parse_test_dummy_encryption(¶m, dummy_policy) ?:
- fscrypt_add_test_dummy_key(sb, dummy_policy);
-}
-EXPORT_SYMBOL_GPL(fscrypt_set_test_dummy_encryption);
-
/**
* fscrypt_show_test_dummy_encryption() - show '-o test_dummy_encryption'
* @seq: the seq_file to print the option to
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 099b881e63e49..11db6d61d4244 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -284,8 +284,6 @@ int fscrypt_parse_test_dummy_encryption(const struct fs_parameter *param,
struct fscrypt_dummy_policy *dummy_policy);
bool fscrypt_dummy_policies_equal(const struct fscrypt_dummy_policy *p1,
const struct fscrypt_dummy_policy *p2);
-int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
- struct fscrypt_dummy_policy *dummy_policy);
void fscrypt_show_test_dummy_encryption(struct seq_file *seq, char sep,
struct super_block *sb);
static inline bool
--
2.36.0
^ permalink raw reply related [flat|nested] 42+ messages in thread
* [f2fs-dev] [PATCH v2 7/7] fscrypt: remove fscrypt_set_test_dummy_encryption()
@ 2022-05-01 5:08 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-01 5:08 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Jeff Layton, Lukas Czerner, Theodore Ts'o, Jaegeuk Kim
From: Eric Biggers <ebiggers@google.com>
Now that all its callers have been converted to
fscrypt_parse_test_dummy_encryption() and fscrypt_add_test_dummy_key()
instead, fscrypt_set_test_dummy_encryption() can be removed.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
fs/crypto/policy.c | 13 -------------
include/linux/fscrypt.h | 2 --
2 files changed, 15 deletions(-)
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 5f858cee1e3b0..d0a8921577def 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -802,19 +802,6 @@ bool fscrypt_dummy_policies_equal(const struct fscrypt_dummy_policy *p1,
}
EXPORT_SYMBOL_GPL(fscrypt_dummy_policies_equal);
-/* Deprecated, do not use */
-int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
- struct fscrypt_dummy_policy *dummy_policy)
-{
- struct fs_parameter param = {
- .type = fs_value_is_string,
- .string = arg ? (char *)arg : "",
- };
- return fscrypt_parse_test_dummy_encryption(¶m, dummy_policy) ?:
- fscrypt_add_test_dummy_key(sb, dummy_policy);
-}
-EXPORT_SYMBOL_GPL(fscrypt_set_test_dummy_encryption);
-
/**
* fscrypt_show_test_dummy_encryption() - show '-o test_dummy_encryption'
* @seq: the seq_file to print the option to
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 099b881e63e49..11db6d61d4244 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -284,8 +284,6 @@ int fscrypt_parse_test_dummy_encryption(const struct fs_parameter *param,
struct fscrypt_dummy_policy *dummy_policy);
bool fscrypt_dummy_policies_equal(const struct fscrypt_dummy_policy *p1,
const struct fscrypt_dummy_policy *p2);
-int fscrypt_set_test_dummy_encryption(struct super_block *sb, const char *arg,
- struct fscrypt_dummy_policy *dummy_policy);
void fscrypt_show_test_dummy_encryption(struct seq_file *seq, char sep,
struct super_block *sb);
static inline bool
--
2.36.0
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 0/7] test_dummy_encryption fixes and cleanups
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
@ 2022-05-09 23:36 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-09 23:36 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Lukas Czerner, Jeff Layton, Theodore Ts'o, Jaegeuk Kim
On Sat, Apr 30, 2022 at 10:08:50PM -0700, Eric Biggers wrote:
> This series cleans up and fixes the way that ext4 and f2fs handle the
> test_dummy_encryption mount option:
>
> - Patches 1-2 make test_dummy_encryption consistently require that the
> 'encrypt' feature flag already be enabled and that
> CONFIG_FS_ENCRYPTION be enabled. Note, this will cause xfstest
> ext4/053 to start failing; my xfstests patch "ext4/053: update the
> test_dummy_encryption tests" will fix that.
>
> - Patches 3-7 replace the fscrypt_set_test_dummy_encryption() helper
> function with new functions that work properly with the new mount API,
> by splitting up the parsing, checking, and applying steps. These fix
> bugs that were introduced when ext4 started using the new mount API.
>
> We can either take all these patches through the fscrypt tree, or we can
> take them in multiple cycles as follows:
>
> 1. patch 1 via ext4, patch 2 via f2fs, patch 3-4 via fscrypt
> 2. patch 5 via ext4, patch 6 via f2fs
> 3. patch 7 via fscrypt
>
> Ted and Jaegeuk, let me know what you prefer.
>
> Changed v1 => v2:
> - Added patches 2-7
> - Also reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
>
> Eric Biggers (7):
> ext4: only allow test_dummy_encryption when supported
> f2fs: reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
> fscrypt: factor out fscrypt_policy_to_key_spec()
> fscrypt: add new helper functions for test_dummy_encryption
> ext4: fix up test_dummy_encryption handling for new mount API
> f2fs: use the updated test_dummy_encryption helper functions
> fscrypt: remove fscrypt_set_test_dummy_encryption()
Since I haven't heard from anyone, I've gone ahead and applied patches 3-4 to
fscrypt#master for 5.19, so that the filesystem-specific patches can be taken in
5.20. But patches 1-2 could still be applied now.
Any feedback on this series would be greatly appreciated!
- Eric
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 0/7] test_dummy_encryption fixes and cleanups
@ 2022-05-09 23:36 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-09 23:36 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Jeff Layton, Lukas Czerner, Theodore Ts'o, Jaegeuk Kim
On Sat, Apr 30, 2022 at 10:08:50PM -0700, Eric Biggers wrote:
> This series cleans up and fixes the way that ext4 and f2fs handle the
> test_dummy_encryption mount option:
>
> - Patches 1-2 make test_dummy_encryption consistently require that the
> 'encrypt' feature flag already be enabled and that
> CONFIG_FS_ENCRYPTION be enabled. Note, this will cause xfstest
> ext4/053 to start failing; my xfstests patch "ext4/053: update the
> test_dummy_encryption tests" will fix that.
>
> - Patches 3-7 replace the fscrypt_set_test_dummy_encryption() helper
> function with new functions that work properly with the new mount API,
> by splitting up the parsing, checking, and applying steps. These fix
> bugs that were introduced when ext4 started using the new mount API.
>
> We can either take all these patches through the fscrypt tree, or we can
> take them in multiple cycles as follows:
>
> 1. patch 1 via ext4, patch 2 via f2fs, patch 3-4 via fscrypt
> 2. patch 5 via ext4, patch 6 via f2fs
> 3. patch 7 via fscrypt
>
> Ted and Jaegeuk, let me know what you prefer.
>
> Changed v1 => v2:
> - Added patches 2-7
> - Also reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
>
> Eric Biggers (7):
> ext4: only allow test_dummy_encryption when supported
> f2fs: reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
> fscrypt: factor out fscrypt_policy_to_key_spec()
> fscrypt: add new helper functions for test_dummy_encryption
> ext4: fix up test_dummy_encryption handling for new mount API
> f2fs: use the updated test_dummy_encryption helper functions
> fscrypt: remove fscrypt_set_test_dummy_encryption()
Since I haven't heard from anyone, I've gone ahead and applied patches 3-4 to
fscrypt#master for 5.19, so that the filesystem-specific patches can be taken in
5.20. But patches 1-2 could still be applied now.
Any feedback on this series would be greatly appreciated!
- Eric
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
@ 2022-05-09 23:40 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-09 23:40 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Lukas Czerner, Jeff Layton, Theodore Ts'o, Jaegeuk Kim
A couple corrections I'll include in the next version:
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.
> +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)'.
- Eric
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
@ 2022-05-09 23:40 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-09 23:40 UTC (permalink / raw)
To: linux-fscrypt, linux-ext4, linux-f2fs-devel
Cc: Jeff Layton, Lukas Czerner, Theodore Ts'o, Jaegeuk Kim
A couple corrections I'll include in the next version:
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.
> +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)'.
- Eric
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 0/7] test_dummy_encryption fixes and cleanups
2022-05-09 23:36 ` Eric Biggers
@ 2022-05-10 23:23 ` Jaegeuk Kim
-1 siblings, 0 replies; 42+ messages in thread
From: Jaegeuk Kim @ 2022-05-10 23:23 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, Jeff Layton,
Lukas Czerner, Theodore Ts'o
On 05/09, Eric Biggers wrote:
> On Sat, Apr 30, 2022 at 10:08:50PM -0700, Eric Biggers wrote:
> > This series cleans up and fixes the way that ext4 and f2fs handle the
> > test_dummy_encryption mount option:
> >
> > - Patches 1-2 make test_dummy_encryption consistently require that the
> > 'encrypt' feature flag already be enabled and that
> > CONFIG_FS_ENCRYPTION be enabled. Note, this will cause xfstest
> > ext4/053 to start failing; my xfstests patch "ext4/053: update the
> > test_dummy_encryption tests" will fix that.
> >
> > - Patches 3-7 replace the fscrypt_set_test_dummy_encryption() helper
> > function with new functions that work properly with the new mount API,
> > by splitting up the parsing, checking, and applying steps. These fix
> > bugs that were introduced when ext4 started using the new mount API.
> >
> > We can either take all these patches through the fscrypt tree, or we can
> > take them in multiple cycles as follows:
> >
> > 1. patch 1 via ext4, patch 2 via f2fs, patch 3-4 via fscrypt
> > 2. patch 5 via ext4, patch 6 via f2fs
> > 3. patch 7 via fscrypt
> >
> > Ted and Jaegeuk, let me know what you prefer.
> >
> > Changed v1 => v2:
> > - Added patches 2-7
> > - Also reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
> >
> > Eric Biggers (7):
> > ext4: only allow test_dummy_encryption when supported
> > f2fs: reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
> > fscrypt: factor out fscrypt_policy_to_key_spec()
> > fscrypt: add new helper functions for test_dummy_encryption
> > ext4: fix up test_dummy_encryption handling for new mount API
> > f2fs: use the updated test_dummy_encryption helper functions
> > fscrypt: remove fscrypt_set_test_dummy_encryption()
>
> Since I haven't heard from anyone, I've gone ahead and applied patches 3-4 to
> fscrypt#master for 5.19, so that the filesystem-specific patches can be taken in
> 5.20. But patches 1-2 could still be applied now.
Hi Eric,
Let me apply #2 in the f2fs tree first.
You can put "Acked-by: Jaegeuk Kim <jaegeuk@kernel.org>" in #6.
Thanks,
>
> Any feedback on this series would be greatly appreciated!
>
> - Eric
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 0/7] test_dummy_encryption fixes and cleanups
@ 2022-05-10 23:23 ` Jaegeuk Kim
0 siblings, 0 replies; 42+ messages in thread
From: Jaegeuk Kim @ 2022-05-10 23:23 UTC (permalink / raw)
To: Eric Biggers
Cc: Theodore Ts'o, Jeff Layton, linux-f2fs-devel, linux-fscrypt,
Lukas Czerner, linux-ext4
On 05/09, Eric Biggers wrote:
> On Sat, Apr 30, 2022 at 10:08:50PM -0700, Eric Biggers wrote:
> > This series cleans up and fixes the way that ext4 and f2fs handle the
> > test_dummy_encryption mount option:
> >
> > - Patches 1-2 make test_dummy_encryption consistently require that the
> > 'encrypt' feature flag already be enabled and that
> > CONFIG_FS_ENCRYPTION be enabled. Note, this will cause xfstest
> > ext4/053 to start failing; my xfstests patch "ext4/053: update the
> > test_dummy_encryption tests" will fix that.
> >
> > - Patches 3-7 replace the fscrypt_set_test_dummy_encryption() helper
> > function with new functions that work properly with the new mount API,
> > by splitting up the parsing, checking, and applying steps. These fix
> > bugs that were introduced when ext4 started using the new mount API.
> >
> > We can either take all these patches through the fscrypt tree, or we can
> > take them in multiple cycles as follows:
> >
> > 1. patch 1 via ext4, patch 2 via f2fs, patch 3-4 via fscrypt
> > 2. patch 5 via ext4, patch 6 via f2fs
> > 3. patch 7 via fscrypt
> >
> > Ted and Jaegeuk, let me know what you prefer.
> >
> > Changed v1 => v2:
> > - Added patches 2-7
> > - Also reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
> >
> > Eric Biggers (7):
> > ext4: only allow test_dummy_encryption when supported
> > f2fs: reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION
> > fscrypt: factor out fscrypt_policy_to_key_spec()
> > fscrypt: add new helper functions for test_dummy_encryption
> > ext4: fix up test_dummy_encryption handling for new mount API
> > f2fs: use the updated test_dummy_encryption helper functions
> > fscrypt: remove fscrypt_set_test_dummy_encryption()
>
> Since I haven't heard from anyone, I've gone ahead and applied patches 3-4 to
> fscrypt#master for 5.19, so that the filesystem-specific patches can be taken in
> 5.20. But patches 1-2 could still be applied now.
Hi Eric,
Let me apply #2 in the f2fs tree first.
You can put "Acked-by: Jaegeuk Kim <jaegeuk@kernel.org>" in #6.
Thanks,
>
> Any feedback on this series would be greatly appreciated!
>
> - Eric
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/7] ext4: only allow test_dummy_encryption when supported
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
@ 2022-05-11 12:50 ` Ritesh Harjani
-1 siblings, 0 replies; 42+ messages in thread
From: Ritesh Harjani @ 2022-05-11 12:50 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, Lukas Czerner,
Theodore Ts'o, Jaegeuk Kim, Jeff Layton
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
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 1/7] ext4: only allow test_dummy_encryption when supported
@ 2022-05-11 12:50 ` Ritesh Harjani
0 siblings, 0 replies; 42+ messages in thread
From: Ritesh Harjani @ 2022-05-11 12:50 UTC (permalink / raw)
To: Eric Biggers
Cc: Theodore Ts'o, Jeff Layton, linux-f2fs-devel, linux-fscrypt,
Lukas Czerner, Jaegeuk Kim, linux-ext4
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
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/7] ext4: only allow test_dummy_encryption when supported
2022-05-11 12:50 ` [f2fs-dev] " Ritesh Harjani
@ 2022-05-11 17:18 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-11 17:18 UTC (permalink / raw)
To: Ritesh Harjani
Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, Lukas Czerner,
Theodore Ts'o, Jaegeuk Kim, Jeff Layton
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 1/7] ext4: only allow test_dummy_encryption when supported
@ 2022-05-11 17:18 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-11 17:18 UTC (permalink / raw)
To: Ritesh Harjani
Cc: Theodore Ts'o, Jeff Layton, linux-f2fs-devel, linux-fscrypt,
Lukas Czerner, Jaegeuk Kim, linux-ext4
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
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
2022-05-09 23:40 ` Eric Biggers
@ 2022-05-11 17:54 ` Ritesh Harjani
-1 siblings, 0 replies; 42+ messages in thread
From: Ritesh Harjani @ 2022-05-11 17:54 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, Lukas Czerner,
Jeff Layton, Theodore Ts'o, Jaegeuk Kim
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
@ 2022-05-11 17:54 ` Ritesh Harjani
0 siblings, 0 replies; 42+ messages in thread
From: Ritesh Harjani @ 2022-05-11 17:54 UTC (permalink / raw)
To: Eric Biggers
Cc: Theodore Ts'o, Jeff Layton, linux-f2fs-devel, linux-fscrypt,
Jaegeuk Kim, Lukas Czerner, linux-ext4
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
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
2022-05-11 17:54 ` Ritesh Harjani
@ 2022-05-11 18:03 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-11 18:03 UTC (permalink / raw)
To: Ritesh Harjani
Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, Lukas Czerner,
Jeff Layton, Theodore Ts'o, Jaegeuk Kim
On Wed, May 11, 2022 at 11:24:33PM +0530, Ritesh Harjani wrote:
> 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?
The case where test_dummy_encryption is present in both the mount options stored
in the superblock and in the regular mount options. See how __ext4_fill_super()
parses and applies each source of options separately.
> 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;
That comment will still be correct.
>
> >
> > > +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?
struct fscrypt_dummy_policy includes dynamically allocated memory, so
overwriting it without first freeing it would be a memory leak.
- Eric
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
@ 2022-05-11 18:03 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-11 18:03 UTC (permalink / raw)
To: Ritesh Harjani
Cc: Theodore Ts'o, Jeff Layton, linux-f2fs-devel, linux-fscrypt,
Jaegeuk Kim, Lukas Czerner, linux-ext4
On Wed, May 11, 2022 at 11:24:33PM +0530, Ritesh Harjani wrote:
> 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?
The case where test_dummy_encryption is present in both the mount options stored
in the superblock and in the regular mount options. See how __ext4_fill_super()
parses and applies each source of options separately.
> 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;
That comment will still be correct.
>
> >
> > > +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?
struct fscrypt_dummy_policy includes dynamically allocated memory, so
overwriting it without first freeing it would be a memory leak.
- Eric
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
2022-05-11 18:03 ` Eric Biggers
@ 2022-05-13 10:58 ` Ritesh Harjani
-1 siblings, 0 replies; 42+ messages in thread
From: Ritesh Harjani @ 2022-05-13 10:58 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, Lukas Czerner,
Jeff Layton, Theodore Ts'o, Jaegeuk Kim
On 22/05/11 06:03PM, Eric Biggers wrote:
> On Wed, May 11, 2022 at 11:24:33PM +0530, Ritesh Harjani wrote:
> > 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?
>
> The case where test_dummy_encryption is present in both the mount options stored
> in the superblock and in the regular mount options. See how __ext4_fill_super()
> parses and applies each source of options separately.
Ok, thanks for clarifying. So this says that
1. in case of mount; if test_dummy_encryption is already set with some policy in
the disk superblock and if the user is trying to change the mount option in
options string, then that is not allowed.
2. Similarly if while remounting we try to change the mount option from the
previous mount option, then again this is not allowed.
>
> > 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;
>
> That comment will still be correct.
>
> >
> > >
> > > > +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?
>
> struct fscrypt_dummy_policy includes dynamically allocated memory, so
> overwriting it without first freeing it would be a memory leak.
Ok yes. Since this is dynamic memory allocation. Hence
I see that ext4_apply_test_dummy_encryption() can be called from
parse_apply_sb_mount_options(), __ext4_fill_super() and __ext4_remount().
Case 1: when this mount option is set in superblock
1. So in parse_apply_sb_mount_options(), this mount option will get set the
first time if it is also set in superblock field.
2. So if we also have a same mount option set in regular mount,
or during remount both will have sbi->s_dummy_enc_policy already set (from
step 1 above), so we should do nothing here.
Case 2: when this mount option is passed as regular mount
1. parse_apply_sb_mount_options() won't set this.
2. __ext4_fill_super() will set this mount option in sbi and hence __ext4_remount
should not set this again.
And as I see you are cleverly setting memset &ctx->dummy_enc_policy to 0
in case where we applied the parsed mount option to sbi. So that the actual
policy doesn't get free when you call __ext4_fc_free() after ext4_apply_options()
in parse_apply_sb_mount_options(). And in other cases where this mount option was
not applied to sbi mount opt, in that case we anyway want this policy to get
free.
This somehow looks very confusing to me. But I guess with parse, check and apply
mount APIs and with mount options in superblock, regular and remount path, this
couldn't be avoided (although I am no expert in this area).
Thanks for explaining. I hope I got this right ;)
-ritesh
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
@ 2022-05-13 10:58 ` Ritesh Harjani
0 siblings, 0 replies; 42+ messages in thread
From: Ritesh Harjani @ 2022-05-13 10:58 UTC (permalink / raw)
To: Eric Biggers
Cc: Theodore Ts'o, Jeff Layton, linux-f2fs-devel, linux-fscrypt,
Jaegeuk Kim, Lukas Czerner, linux-ext4
On 22/05/11 06:03PM, Eric Biggers wrote:
> On Wed, May 11, 2022 at 11:24:33PM +0530, Ritesh Harjani wrote:
> > 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?
>
> The case where test_dummy_encryption is present in both the mount options stored
> in the superblock and in the regular mount options. See how __ext4_fill_super()
> parses and applies each source of options separately.
Ok, thanks for clarifying. So this says that
1. in case of mount; if test_dummy_encryption is already set with some policy in
the disk superblock and if the user is trying to change the mount option in
options string, then that is not allowed.
2. Similarly if while remounting we try to change the mount option from the
previous mount option, then again this is not allowed.
>
> > 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;
>
> That comment will still be correct.
>
> >
> > >
> > > > +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?
>
> struct fscrypt_dummy_policy includes dynamically allocated memory, so
> overwriting it without first freeing it would be a memory leak.
Ok yes. Since this is dynamic memory allocation. Hence
I see that ext4_apply_test_dummy_encryption() can be called from
parse_apply_sb_mount_options(), __ext4_fill_super() and __ext4_remount().
Case 1: when this mount option is set in superblock
1. So in parse_apply_sb_mount_options(), this mount option will get set the
first time if it is also set in superblock field.
2. So if we also have a same mount option set in regular mount,
or during remount both will have sbi->s_dummy_enc_policy already set (from
step 1 above), so we should do nothing here.
Case 2: when this mount option is passed as regular mount
1. parse_apply_sb_mount_options() won't set this.
2. __ext4_fill_super() will set this mount option in sbi and hence __ext4_remount
should not set this again.
And as I see you are cleverly setting memset &ctx->dummy_enc_policy to 0
in case where we applied the parsed mount option to sbi. So that the actual
policy doesn't get free when you call __ext4_fc_free() after ext4_apply_options()
in parse_apply_sb_mount_options(). And in other cases where this mount option was
not applied to sbi mount opt, in that case we anyway want this policy to get
free.
This somehow looks very confusing to me. But I guess with parse, check and apply
mount APIs and with mount options in superblock, regular and remount path, this
couldn't be avoided (although I am no expert in this area).
Thanks for explaining. I hope I got this right ;)
-ritesh
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
@ 2022-05-13 11:07 ` Ritesh Harjani
-1 siblings, 0 replies; 42+ messages in thread
From: Ritesh Harjani @ 2022-05-13 11:07 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, Lukas Czerner,
Theodore Ts'o, Jaegeuk Kim, Jeff Layton
On 22/04/30 10:08PM, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> Since ext4 was converted to the new mount API, the test_dummy_encryption
> mount option isn't being handled entirely correctly, because the needed
> fscrypt_set_test_dummy_encryption() helper function combines
> parsing/checking/applying into one function. That doesn't work well
> with the new mount API, which split these into separate steps.
>
> This was sort of okay anyway, due to the parsing logic that was copied
> from fscrypt_set_test_dummy_encryption() into ext4_parse_param(),
> combined with an additional check in ext4_check_test_dummy_encryption().
> However, these overlooked the case of changing the value of
> test_dummy_encryption on remount, which isn't allowed but ext4 wasn't
> detecting until ext4_apply_options() when it's too late to fail.
> Another bug is that if test_dummy_encryption was specified multiple
> times with an argument, memory was leaked.
>
> Fix this up properly by using the new helper functions that allow
> splitting up the parse/check/apply steps for test_dummy_encryption.
>
> Fixes: cebe85d570cf ("ext4: switch to the new mount api")
> Signed-off-by: Eric Biggers <ebiggers@google.com>
I just had a small observation. Feel free to check it at your end too.
> ---
> fs/ext4/ext4.h | 6 ---
> fs/ext4/super.c | 131 +++++++++++++++++++++++++-----------------------
> 2 files changed, 67 insertions(+), 70 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index a743b1e3b89ec..f6d6661817b63 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1440,12 +1440,6 @@ struct ext4_super_block {
>
> #ifdef __KERNEL__
>
> -#ifdef CONFIG_FS_ENCRYPTION
> -#define DUMMY_ENCRYPTION_ENABLED(sbi) ((sbi)->s_dummy_enc_policy.policy != NULL)
> -#else
> -#define DUMMY_ENCRYPTION_ENABLED(sbi) (0)
> -#endif
> -
> /* Number of quota types we support */
> #define EXT4_MAXQUOTAS 3
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 64ce17714e193..43e4cd358b33b 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -87,7 +87,7 @@ static struct inode *ext4_get_journal_inode(struct super_block *sb,
> static int ext4_validate_options(struct fs_context *fc);
> static int ext4_check_opt_consistency(struct fs_context *fc,
> struct super_block *sb);
> -static int ext4_apply_options(struct fs_context *fc, struct super_block *sb);
> +static void ext4_apply_options(struct fs_context *fc, struct super_block *sb);
> static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param);
> static int ext4_get_tree(struct fs_context *fc);
> static int ext4_reconfigure(struct fs_context *fc);
> @@ -1989,31 +1989,12 @@ ext4_sb_read_encoding(const struct ext4_super_block *es)
> }
> #endif
>
> -static int ext4_set_test_dummy_encryption(struct super_block *sb, char *arg)
> -{
> -#ifdef CONFIG_FS_ENCRYPTION
> - struct ext4_sb_info *sbi = EXT4_SB(sb);
> - int err;
> -
> - err = fscrypt_set_test_dummy_encryption(sb, arg,
> - &sbi->s_dummy_enc_policy);
> - if (err) {
> - ext4_msg(sb, KERN_WARNING,
> - "Error while setting test dummy encryption [%d]", err);
> - return err;
> - }
> - ext4_msg(sb, KERN_WARNING, "Test dummy encryption mode enabled");
> -#endif
> - return 0;
> -}
> -
> #define EXT4_SPEC_JQUOTA (1 << 0)
> #define EXT4_SPEC_JQFMT (1 << 1)
> #define EXT4_SPEC_DATAJ (1 << 2)
> #define EXT4_SPEC_SB_BLOCK (1 << 3)
> #define EXT4_SPEC_JOURNAL_DEV (1 << 4)
> #define EXT4_SPEC_JOURNAL_IOPRIO (1 << 5)
> -#define EXT4_SPEC_DUMMY_ENCRYPTION (1 << 6)
> #define EXT4_SPEC_s_want_extra_isize (1 << 7)
> #define EXT4_SPEC_s_max_batch_time (1 << 8)
> #define EXT4_SPEC_s_min_batch_time (1 << 9)
> @@ -2030,7 +2011,7 @@ static int ext4_set_test_dummy_encryption(struct super_block *sb, char *arg)
>
> struct ext4_fs_context {
> char *s_qf_names[EXT4_MAXQUOTAS];
> - char *test_dummy_enc_arg;
> + struct fscrypt_dummy_policy dummy_enc_policy;
> int s_jquota_fmt; /* Format of quota to use */
> #ifdef CONFIG_EXT4_DEBUG
> int s_fc_debug_max_replay;
> @@ -2061,9 +2042,8 @@ struct ext4_fs_context {
> ext4_fsblk_t s_sb_block;
> };
>
> -static void ext4_fc_free(struct fs_context *fc)
> +static void __ext4_fc_free(struct ext4_fs_context *ctx)
> {
> - struct ext4_fs_context *ctx = fc->fs_private;
> int i;
>
> if (!ctx)
> @@ -2072,10 +2052,15 @@ static void ext4_fc_free(struct fs_context *fc)
> for (i = 0; i < EXT4_MAXQUOTAS; i++)
> kfree(ctx->s_qf_names[i]);
>
> - kfree(ctx->test_dummy_enc_arg);
> + fscrypt_free_dummy_policy(&ctx->dummy_enc_policy);
> kfree(ctx);
> }
>
> +static void ext4_fc_free(struct fs_context *fc)
> +{
> + __ext4_fc_free(fc->fs_private);
> +}
> +
> int ext4_init_fs_context(struct fs_context *fc)
> {
> struct ext4_fs_context *ctx;
> @@ -2148,6 +2133,29 @@ static int unnote_qf_name(struct fs_context *fc, int qtype)
> }
> #endif
>
> +static int ext4_parse_test_dummy_encryption(const struct fs_parameter *param,
> + struct ext4_fs_context *ctx)
> +{
> + int err;
> +
> + if (!IS_ENABLED(CONFIG_FS_ENCRYPTION)) {
> + ext4_msg(NULL, KERN_WARNING,
> + "test_dummy_encryption option not supported");
> + return -EINVAL;
> + }
> + err = fscrypt_parse_test_dummy_encryption(param,
> + &ctx->dummy_enc_policy);
> + if (err == -EINVAL) {
> + ext4_msg(NULL, KERN_WARNING,
> + "Value of option \"%s\" is unrecognized", param->key);
> + } else if (err == -EEXIST) {
> + ext4_msg(NULL, KERN_WARNING,
> + "Conflicting test_dummy_encryption options");
> + return -EINVAL;
> + }
> + return err;
> +}
> +
> #define EXT4_SET_CTX(name) \
> static inline void ctx_set_##name(struct ext4_fs_context *ctx, \
> unsigned long flag) \
> @@ -2410,29 +2418,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
> ctx->spec |= EXT4_SPEC_JOURNAL_IOPRIO;
> return 0;
> case Opt_test_dummy_encryption:
> -#ifdef CONFIG_FS_ENCRYPTION
> - if (param->type == fs_value_is_flag) {
> - ctx->spec |= EXT4_SPEC_DUMMY_ENCRYPTION;
> - ctx->test_dummy_enc_arg = NULL;
> - return 0;
> - }
> - if (*param->string &&
> - !(!strcmp(param->string, "v1") ||
> - !strcmp(param->string, "v2"))) {
> - ext4_msg(NULL, KERN_WARNING,
> - "Value of option \"%s\" is unrecognized",
> - param->key);
> - return -EINVAL;
> - }
> - 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 option not supported");
> - return -EINVAL;
> -#endif
> + return ext4_parse_test_dummy_encryption(param, ctx);
> case Opt_dax:
> case Opt_dax_type:
> #ifdef CONFIG_FS_DAX
> @@ -2623,10 +2609,11 @@ static int parse_apply_sb_mount_options(struct super_block *sb,
> if (s_ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO)
> m_ctx->journal_ioprio = s_ctx->journal_ioprio;
>
> - ret = ext4_apply_options(fc, sb);
> + ext4_apply_options(fc, sb);
> + ret = 0;
>
> out_free:
> - kfree(s_ctx);
> + __ext4_fc_free(s_ctx);
I think we can still call ext4_fc_free(fc) and we don't need __ext4_fc_free().
Right?
-ritesh
> kfree(fc);
> kfree(s_mount_opts);
> return ret;
> @@ -2792,9 +2779,9 @@ static int ext4_check_test_dummy_encryption(const struct fs_context *fc,
> {
> const struct ext4_fs_context *ctx = fc->fs_private;
> const struct ext4_sb_info *sbi = EXT4_SB(sb);
> + int err;
>
> - if (!IS_ENABLED(CONFIG_FS_ENCRYPTION) ||
> - !(ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION))
> + if (!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy))
> return 0;
>
> if (!ext4_has_feature_encrypt(sb)) {
> @@ -2808,13 +2795,35 @@ static int ext4_check_test_dummy_encryption(const struct fs_context *fc,
> * 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)) {
> + 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;
> }
> - return 0;
> + /*
> + * 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;
> + EXT4_SB(sb)->s_dummy_enc_policy = ctx->dummy_enc_policy;
> + memset(&ctx->dummy_enc_policy, 0, sizeof(ctx->dummy_enc_policy));
> + ext4_msg(sb, KERN_WARNING, "Test dummy encryption mode enabled");
> }
>
> static int ext4_check_opt_consistency(struct fs_context *fc,
> @@ -2901,11 +2910,10 @@ static int ext4_check_opt_consistency(struct fs_context *fc,
> return ext4_check_quota_consistency(fc, sb);
> }
>
> -static int ext4_apply_options(struct fs_context *fc, struct super_block *sb)
> +static void ext4_apply_options(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 ret = 0;
>
> sbi->s_mount_opt &= ~ctx->mask_s_mount_opt;
> sbi->s_mount_opt |= ctx->vals_s_mount_opt;
> @@ -2942,10 +2950,7 @@ static int ext4_apply_options(struct fs_context *fc, struct super_block *sb)
>
> ext4_apply_quota_options(fc, sb);
>
> - if (ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION)
> - ret = ext4_set_test_dummy_encryption(sb, ctx->test_dummy_enc_arg);
> -
> - return ret;
> + ext4_apply_test_dummy_encryption(ctx, sb);
> }
>
>
> @@ -4667,9 +4672,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> if (err < 0)
> goto failed_mount;
>
> - err = ext4_apply_options(fc, sb);
> - if (err < 0)
> - goto failed_mount;
> + ext4_apply_options(fc, sb);
>
> #if IS_ENABLED(CONFIG_UNICODE)
> if (ext4_has_feature_casefold(sb) && !sb->s_encoding) {
> --
> 2.36.0
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
@ 2022-05-13 11:07 ` Ritesh Harjani
0 siblings, 0 replies; 42+ messages in thread
From: Ritesh Harjani @ 2022-05-13 11:07 UTC (permalink / raw)
To: Eric Biggers
Cc: Theodore Ts'o, Jeff Layton, linux-f2fs-devel, linux-fscrypt,
Lukas Czerner, Jaegeuk Kim, linux-ext4
On 22/04/30 10:08PM, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> Since ext4 was converted to the new mount API, the test_dummy_encryption
> mount option isn't being handled entirely correctly, because the needed
> fscrypt_set_test_dummy_encryption() helper function combines
> parsing/checking/applying into one function. That doesn't work well
> with the new mount API, which split these into separate steps.
>
> This was sort of okay anyway, due to the parsing logic that was copied
> from fscrypt_set_test_dummy_encryption() into ext4_parse_param(),
> combined with an additional check in ext4_check_test_dummy_encryption().
> However, these overlooked the case of changing the value of
> test_dummy_encryption on remount, which isn't allowed but ext4 wasn't
> detecting until ext4_apply_options() when it's too late to fail.
> Another bug is that if test_dummy_encryption was specified multiple
> times with an argument, memory was leaked.
>
> Fix this up properly by using the new helper functions that allow
> splitting up the parse/check/apply steps for test_dummy_encryption.
>
> Fixes: cebe85d570cf ("ext4: switch to the new mount api")
> Signed-off-by: Eric Biggers <ebiggers@google.com>
I just had a small observation. Feel free to check it at your end too.
> ---
> fs/ext4/ext4.h | 6 ---
> fs/ext4/super.c | 131 +++++++++++++++++++++++++-----------------------
> 2 files changed, 67 insertions(+), 70 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index a743b1e3b89ec..f6d6661817b63 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1440,12 +1440,6 @@ struct ext4_super_block {
>
> #ifdef __KERNEL__
>
> -#ifdef CONFIG_FS_ENCRYPTION
> -#define DUMMY_ENCRYPTION_ENABLED(sbi) ((sbi)->s_dummy_enc_policy.policy != NULL)
> -#else
> -#define DUMMY_ENCRYPTION_ENABLED(sbi) (0)
> -#endif
> -
> /* Number of quota types we support */
> #define EXT4_MAXQUOTAS 3
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 64ce17714e193..43e4cd358b33b 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -87,7 +87,7 @@ static struct inode *ext4_get_journal_inode(struct super_block *sb,
> static int ext4_validate_options(struct fs_context *fc);
> static int ext4_check_opt_consistency(struct fs_context *fc,
> struct super_block *sb);
> -static int ext4_apply_options(struct fs_context *fc, struct super_block *sb);
> +static void ext4_apply_options(struct fs_context *fc, struct super_block *sb);
> static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param);
> static int ext4_get_tree(struct fs_context *fc);
> static int ext4_reconfigure(struct fs_context *fc);
> @@ -1989,31 +1989,12 @@ ext4_sb_read_encoding(const struct ext4_super_block *es)
> }
> #endif
>
> -static int ext4_set_test_dummy_encryption(struct super_block *sb, char *arg)
> -{
> -#ifdef CONFIG_FS_ENCRYPTION
> - struct ext4_sb_info *sbi = EXT4_SB(sb);
> - int err;
> -
> - err = fscrypt_set_test_dummy_encryption(sb, arg,
> - &sbi->s_dummy_enc_policy);
> - if (err) {
> - ext4_msg(sb, KERN_WARNING,
> - "Error while setting test dummy encryption [%d]", err);
> - return err;
> - }
> - ext4_msg(sb, KERN_WARNING, "Test dummy encryption mode enabled");
> -#endif
> - return 0;
> -}
> -
> #define EXT4_SPEC_JQUOTA (1 << 0)
> #define EXT4_SPEC_JQFMT (1 << 1)
> #define EXT4_SPEC_DATAJ (1 << 2)
> #define EXT4_SPEC_SB_BLOCK (1 << 3)
> #define EXT4_SPEC_JOURNAL_DEV (1 << 4)
> #define EXT4_SPEC_JOURNAL_IOPRIO (1 << 5)
> -#define EXT4_SPEC_DUMMY_ENCRYPTION (1 << 6)
> #define EXT4_SPEC_s_want_extra_isize (1 << 7)
> #define EXT4_SPEC_s_max_batch_time (1 << 8)
> #define EXT4_SPEC_s_min_batch_time (1 << 9)
> @@ -2030,7 +2011,7 @@ static int ext4_set_test_dummy_encryption(struct super_block *sb, char *arg)
>
> struct ext4_fs_context {
> char *s_qf_names[EXT4_MAXQUOTAS];
> - char *test_dummy_enc_arg;
> + struct fscrypt_dummy_policy dummy_enc_policy;
> int s_jquota_fmt; /* Format of quota to use */
> #ifdef CONFIG_EXT4_DEBUG
> int s_fc_debug_max_replay;
> @@ -2061,9 +2042,8 @@ struct ext4_fs_context {
> ext4_fsblk_t s_sb_block;
> };
>
> -static void ext4_fc_free(struct fs_context *fc)
> +static void __ext4_fc_free(struct ext4_fs_context *ctx)
> {
> - struct ext4_fs_context *ctx = fc->fs_private;
> int i;
>
> if (!ctx)
> @@ -2072,10 +2052,15 @@ static void ext4_fc_free(struct fs_context *fc)
> for (i = 0; i < EXT4_MAXQUOTAS; i++)
> kfree(ctx->s_qf_names[i]);
>
> - kfree(ctx->test_dummy_enc_arg);
> + fscrypt_free_dummy_policy(&ctx->dummy_enc_policy);
> kfree(ctx);
> }
>
> +static void ext4_fc_free(struct fs_context *fc)
> +{
> + __ext4_fc_free(fc->fs_private);
> +}
> +
> int ext4_init_fs_context(struct fs_context *fc)
> {
> struct ext4_fs_context *ctx;
> @@ -2148,6 +2133,29 @@ static int unnote_qf_name(struct fs_context *fc, int qtype)
> }
> #endif
>
> +static int ext4_parse_test_dummy_encryption(const struct fs_parameter *param,
> + struct ext4_fs_context *ctx)
> +{
> + int err;
> +
> + if (!IS_ENABLED(CONFIG_FS_ENCRYPTION)) {
> + ext4_msg(NULL, KERN_WARNING,
> + "test_dummy_encryption option not supported");
> + return -EINVAL;
> + }
> + err = fscrypt_parse_test_dummy_encryption(param,
> + &ctx->dummy_enc_policy);
> + if (err == -EINVAL) {
> + ext4_msg(NULL, KERN_WARNING,
> + "Value of option \"%s\" is unrecognized", param->key);
> + } else if (err == -EEXIST) {
> + ext4_msg(NULL, KERN_WARNING,
> + "Conflicting test_dummy_encryption options");
> + return -EINVAL;
> + }
> + return err;
> +}
> +
> #define EXT4_SET_CTX(name) \
> static inline void ctx_set_##name(struct ext4_fs_context *ctx, \
> unsigned long flag) \
> @@ -2410,29 +2418,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
> ctx->spec |= EXT4_SPEC_JOURNAL_IOPRIO;
> return 0;
> case Opt_test_dummy_encryption:
> -#ifdef CONFIG_FS_ENCRYPTION
> - if (param->type == fs_value_is_flag) {
> - ctx->spec |= EXT4_SPEC_DUMMY_ENCRYPTION;
> - ctx->test_dummy_enc_arg = NULL;
> - return 0;
> - }
> - if (*param->string &&
> - !(!strcmp(param->string, "v1") ||
> - !strcmp(param->string, "v2"))) {
> - ext4_msg(NULL, KERN_WARNING,
> - "Value of option \"%s\" is unrecognized",
> - param->key);
> - return -EINVAL;
> - }
> - 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 option not supported");
> - return -EINVAL;
> -#endif
> + return ext4_parse_test_dummy_encryption(param, ctx);
> case Opt_dax:
> case Opt_dax_type:
> #ifdef CONFIG_FS_DAX
> @@ -2623,10 +2609,11 @@ static int parse_apply_sb_mount_options(struct super_block *sb,
> if (s_ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO)
> m_ctx->journal_ioprio = s_ctx->journal_ioprio;
>
> - ret = ext4_apply_options(fc, sb);
> + ext4_apply_options(fc, sb);
> + ret = 0;
>
> out_free:
> - kfree(s_ctx);
> + __ext4_fc_free(s_ctx);
I think we can still call ext4_fc_free(fc) and we don't need __ext4_fc_free().
Right?
-ritesh
> kfree(fc);
> kfree(s_mount_opts);
> return ret;
> @@ -2792,9 +2779,9 @@ static int ext4_check_test_dummy_encryption(const struct fs_context *fc,
> {
> const struct ext4_fs_context *ctx = fc->fs_private;
> const struct ext4_sb_info *sbi = EXT4_SB(sb);
> + int err;
>
> - if (!IS_ENABLED(CONFIG_FS_ENCRYPTION) ||
> - !(ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION))
> + if (!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy))
> return 0;
>
> if (!ext4_has_feature_encrypt(sb)) {
> @@ -2808,13 +2795,35 @@ static int ext4_check_test_dummy_encryption(const struct fs_context *fc,
> * 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)) {
> + 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;
> }
> - return 0;
> + /*
> + * 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;
> + EXT4_SB(sb)->s_dummy_enc_policy = ctx->dummy_enc_policy;
> + memset(&ctx->dummy_enc_policy, 0, sizeof(ctx->dummy_enc_policy));
> + ext4_msg(sb, KERN_WARNING, "Test dummy encryption mode enabled");
> }
>
> static int ext4_check_opt_consistency(struct fs_context *fc,
> @@ -2901,11 +2910,10 @@ static int ext4_check_opt_consistency(struct fs_context *fc,
> return ext4_check_quota_consistency(fc, sb);
> }
>
> -static int ext4_apply_options(struct fs_context *fc, struct super_block *sb)
> +static void ext4_apply_options(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 ret = 0;
>
> sbi->s_mount_opt &= ~ctx->mask_s_mount_opt;
> sbi->s_mount_opt |= ctx->vals_s_mount_opt;
> @@ -2942,10 +2950,7 @@ static int ext4_apply_options(struct fs_context *fc, struct super_block *sb)
>
> ext4_apply_quota_options(fc, sb);
>
> - if (ctx->spec & EXT4_SPEC_DUMMY_ENCRYPTION)
> - ret = ext4_set_test_dummy_encryption(sb, ctx->test_dummy_enc_arg);
> -
> - return ret;
> + ext4_apply_test_dummy_encryption(ctx, sb);
> }
>
>
> @@ -4667,9 +4672,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> if (err < 0)
> goto failed_mount;
>
> - err = ext4_apply_options(fc, sb);
> - if (err < 0)
> - goto failed_mount;
> + ext4_apply_options(fc, sb);
>
> #if IS_ENABLED(CONFIG_UNICODE)
> if (ext4_has_feature_casefold(sb) && !sb->s_encoding) {
> --
> 2.36.0
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 0/7] test_dummy_encryption fixes and cleanups
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
@ 2022-05-13 19:36 ` Theodore Ts'o
-1 siblings, 0 replies; 42+ messages in thread
From: Theodore Ts'o @ 2022-05-13 19:36 UTC (permalink / raw)
To: Eric Biggers
Cc: Jeff Layton, linux-f2fs-devel, linux-fscrypt, Lukas Czerner,
Jaegeuk Kim, linux-ext4
On Sat, Apr 30, 2022 at 10:08:50PM -0700, Eric Biggers wrote:
> We can either take all these patches through the fscrypt tree, or we can
> take them in multiple cycles as follows:
>
> 1. patch 1 via ext4, patch 2 via f2fs, patch 3-4 via fscrypt
> 2. patch 5 via ext4, patch 6 via f2fs
> 3. patch 7 via fscrypt
>
> Ted and Jaegeuk, let me know what you prefer.
In order to avoid patch conflicts with other patch series, what I'd
prefer is to take them in multiple cycles. I can take patch #1 in my
initial pull request to Linus, and then do a second pull request to
Linus with patch #5 post -rc1 or -rc2 (depending on when patches #3
and #4 hit Linus's tree).
Does that sound good?
- Ted
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 0/7] test_dummy_encryption fixes and cleanups
@ 2022-05-13 19:36 ` Theodore Ts'o
0 siblings, 0 replies; 42+ messages in thread
From: Theodore Ts'o @ 2022-05-13 19:36 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, Lukas Czerner,
Jaegeuk Kim, Jeff Layton
On Sat, Apr 30, 2022 at 10:08:50PM -0700, Eric Biggers wrote:
> We can either take all these patches through the fscrypt tree, or we can
> take them in multiple cycles as follows:
>
> 1. patch 1 via ext4, patch 2 via f2fs, patch 3-4 via fscrypt
> 2. patch 5 via ext4, patch 6 via f2fs
> 3. patch 7 via fscrypt
>
> Ted and Jaegeuk, let me know what you prefer.
In order to avoid patch conflicts with other patch series, what I'd
prefer is to take them in multiple cycles. I can take patch #1 in my
initial pull request to Linus, and then do a second pull request to
Linus with patch #5 post -rc1 or -rc2 (depending on when patches #3
and #4 hit Linus's tree).
Does that sound good?
- Ted
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
2022-05-13 11:07 ` [f2fs-dev] " Ritesh Harjani
@ 2022-05-13 21:59 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-13 21:59 UTC (permalink / raw)
To: Ritesh Harjani
Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, Lukas Czerner,
Theodore Ts'o, Jaegeuk Kim, Jeff Layton
On Fri, May 13, 2022 at 04:37:41PM +0530, Ritesh Harjani wrote:
> > @@ -2623,10 +2609,11 @@ static int parse_apply_sb_mount_options(struct super_block *sb,
> > if (s_ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO)
> > m_ctx->journal_ioprio = s_ctx->journal_ioprio;
> >
> > - ret = ext4_apply_options(fc, sb);
> > + ext4_apply_options(fc, sb);
> > + ret = 0;
> >
> > out_free:
> > - kfree(s_ctx);
> > + __ext4_fc_free(s_ctx);
>
> I think we can still call ext4_fc_free(fc) and we don't need __ext4_fc_free().
> Right?
>
Yes, you're right. I might have missed that fc->fs_private was being set above.
I was also a little lazy here; the part below 'out_free:' should be a separate
patch since it also fixes a memory leak of s_qf_names. I'll fix that up.
- Eric
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
@ 2022-05-13 21:59 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-13 21:59 UTC (permalink / raw)
To: Ritesh Harjani
Cc: Theodore Ts'o, Jeff Layton, linux-f2fs-devel, linux-fscrypt,
Lukas Czerner, Jaegeuk Kim, linux-ext4
On Fri, May 13, 2022 at 04:37:41PM +0530, Ritesh Harjani wrote:
> > @@ -2623,10 +2609,11 @@ static int parse_apply_sb_mount_options(struct super_block *sb,
> > if (s_ctx->spec & EXT4_SPEC_JOURNAL_IOPRIO)
> > m_ctx->journal_ioprio = s_ctx->journal_ioprio;
> >
> > - ret = ext4_apply_options(fc, sb);
> > + ext4_apply_options(fc, sb);
> > + ret = 0;
> >
> > out_free:
> > - kfree(s_ctx);
> > + __ext4_fc_free(s_ctx);
>
> I think we can still call ext4_fc_free(fc) and we don't need __ext4_fc_free().
> Right?
>
Yes, you're right. I might have missed that fc->fs_private was being set above.
I was also a little lazy here; the part below 'out_free:' should be a separate
patch since it also fixes a memory leak of s_qf_names. I'll fix that up.
- Eric
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
2022-05-13 10:58 ` Ritesh Harjani
@ 2022-05-13 22:24 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-13 22:24 UTC (permalink / raw)
To: Ritesh Harjani
Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, Lukas Czerner,
Jeff Layton, Theodore Ts'o, Jaegeuk Kim
On Fri, May 13, 2022 at 04:28:53PM +0530, Ritesh Harjani wrote:
> On 22/05/11 06:03PM, Eric Biggers wrote:
> > On Wed, May 11, 2022 at 11:24:33PM +0530, Ritesh Harjani wrote:
> > > 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?
> >
> > The case where test_dummy_encryption is present in both the mount options stored
> > in the superblock and in the regular mount options. See how __ext4_fill_super()
> > parses and applies each source of options separately.
>
> Ok, thanks for clarifying. So this says that
> 1. in case of mount; if test_dummy_encryption is already set with some policy in
> the disk superblock and if the user is trying to change the mount option in
> options string, then that is not allowed.
> 2. Similarly if while remounting we try to change the mount option from the
> previous mount option, then again this is not allowed.
>
Yes. I assume that the expected behavior of the on-disk mount options is the
same as if they were prepended to the user-specified mount options. So we
simply aren't allowing conflicting test_dummy_encryption options in the mount
options, regardless of where the mount options came from.
> > > > > +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?
> >
> > struct fscrypt_dummy_policy includes dynamically allocated memory, so
> > overwriting it without first freeing it would be a memory leak.
>
> Ok yes. Since this is dynamic memory allocation. Hence
> I see that ext4_apply_test_dummy_encryption() can be called from
> parse_apply_sb_mount_options(), __ext4_fill_super() and __ext4_remount().
>
> Case 1: when this mount option is set in superblock
> 1. So in parse_apply_sb_mount_options(), this mount option will get set the
> first time if it is also set in superblock field.
>
> 2. So if we also have a same mount option set in regular mount,
> or during remount both will have sbi->s_dummy_enc_policy already set (from
> step 1 above), so we should do nothing here.
>
> Case 2: when this mount option is passed as regular mount
> 1. parse_apply_sb_mount_options() won't set this.
> 2. __ext4_fill_super() will set this mount option in sbi and hence __ext4_remount
> should not set this again.
>
> And as I see you are cleverly setting memset &ctx->dummy_enc_policy to 0
> in case where we applied the parsed mount option to sbi. So that the actual
> policy doesn't get free when you call __ext4_fc_free() after ext4_apply_options()
> in parse_apply_sb_mount_options(). And in other cases where this mount option was
> not applied to sbi mount opt, in that case we anyway want this policy to get
> free.
>
> This somehow looks very confusing to me. But I guess with parse, check and apply
> mount APIs and with mount options in superblock, regular and remount path, this
> couldn't be avoided (although I am no expert in this area).
>
> Thanks for explaining. I hope I got this right ;)
That's all correct. I think you're overthinking it a bit. The important thing
is that if the dummy policy is being set, we must move it into the ext4_sb_info.
Zeroing the old location is just part of transferring ownership of memory in C.
If a dummy policy was already set, we don't support changing it, and we've
checked that any "new" value is consistent with it, so we don't do anything.
- Eric
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API
@ 2022-05-13 22:24 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-13 22:24 UTC (permalink / raw)
To: Ritesh Harjani
Cc: Theodore Ts'o, Jeff Layton, linux-f2fs-devel, linux-fscrypt,
Jaegeuk Kim, Lukas Czerner, linux-ext4
On Fri, May 13, 2022 at 04:28:53PM +0530, Ritesh Harjani wrote:
> On 22/05/11 06:03PM, Eric Biggers wrote:
> > On Wed, May 11, 2022 at 11:24:33PM +0530, Ritesh Harjani wrote:
> > > 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?
> >
> > The case where test_dummy_encryption is present in both the mount options stored
> > in the superblock and in the regular mount options. See how __ext4_fill_super()
> > parses and applies each source of options separately.
>
> Ok, thanks for clarifying. So this says that
> 1. in case of mount; if test_dummy_encryption is already set with some policy in
> the disk superblock and if the user is trying to change the mount option in
> options string, then that is not allowed.
> 2. Similarly if while remounting we try to change the mount option from the
> previous mount option, then again this is not allowed.
>
Yes. I assume that the expected behavior of the on-disk mount options is the
same as if they were prepended to the user-specified mount options. So we
simply aren't allowing conflicting test_dummy_encryption options in the mount
options, regardless of where the mount options came from.
> > > > > +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?
> >
> > struct fscrypt_dummy_policy includes dynamically allocated memory, so
> > overwriting it without first freeing it would be a memory leak.
>
> Ok yes. Since this is dynamic memory allocation. Hence
> I see that ext4_apply_test_dummy_encryption() can be called from
> parse_apply_sb_mount_options(), __ext4_fill_super() and __ext4_remount().
>
> Case 1: when this mount option is set in superblock
> 1. So in parse_apply_sb_mount_options(), this mount option will get set the
> first time if it is also set in superblock field.
>
> 2. So if we also have a same mount option set in regular mount,
> or during remount both will have sbi->s_dummy_enc_policy already set (from
> step 1 above), so we should do nothing here.
>
> Case 2: when this mount option is passed as regular mount
> 1. parse_apply_sb_mount_options() won't set this.
> 2. __ext4_fill_super() will set this mount option in sbi and hence __ext4_remount
> should not set this again.
>
> And as I see you are cleverly setting memset &ctx->dummy_enc_policy to 0
> in case where we applied the parsed mount option to sbi. So that the actual
> policy doesn't get free when you call __ext4_fc_free() after ext4_apply_options()
> in parse_apply_sb_mount_options(). And in other cases where this mount option was
> not applied to sbi mount opt, in that case we anyway want this policy to get
> free.
>
> This somehow looks very confusing to me. But I guess with parse, check and apply
> mount APIs and with mount options in superblock, regular and remount path, this
> couldn't be avoided (although I am no expert in this area).
>
> Thanks for explaining. I hope I got this right ;)
That's all correct. I think you're overthinking it a bit. The important thing
is that if the dummy policy is being set, we must move it into the ext4_sb_info.
Zeroing the old location is just part of transferring ownership of memory in C.
If a dummy policy was already set, we don't support changing it, and we've
checked that any "new" value is consistent with it, so we don't do anything.
- Eric
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [f2fs-dev] [PATCH v2 0/7] test_dummy_encryption fixes and cleanups
2022-05-13 19:36 ` Theodore Ts'o
@ 2022-05-13 23:26 ` Eric Biggers
-1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-13 23:26 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Jeff Layton, linux-f2fs-devel, linux-fscrypt, Lukas Czerner,
Jaegeuk Kim, linux-ext4
On Fri, May 13, 2022 at 03:36:05PM -0400, Theodore Ts'o wrote:
> On Sat, Apr 30, 2022 at 10:08:50PM -0700, Eric Biggers wrote:
> > We can either take all these patches through the fscrypt tree, or we can
> > take them in multiple cycles as follows:
> >
> > 1. patch 1 via ext4, patch 2 via f2fs, patch 3-4 via fscrypt
> > 2. patch 5 via ext4, patch 6 via f2fs
> > 3. patch 7 via fscrypt
> >
> > Ted and Jaegeuk, let me know what you prefer.
>
> In order to avoid patch conflicts with other patch series, what I'd
> prefer is to take them in multiple cycles. I can take patch #1 in my
> initial pull request to Linus, and then do a second pull request to
> Linus with patch #5 post -rc1 or -rc2 (depending on when patches #3
> and #4 hit Linus's tree).
>
> Does that sound good?
That basically sounds fine. I've just sent out v3 of this series, with the fix
for the memory leak in parse_apply_sb_mount_options() as its own patch. That
patch can be applied now too, so you can take patches 1-2 of the v3 series in
your initial pull request.
- Eric
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 0/7] test_dummy_encryption fixes and cleanups
@ 2022-05-13 23:26 ` Eric Biggers
0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2022-05-13 23:26 UTC (permalink / raw)
To: Theodore Ts'o
Cc: linux-fscrypt, linux-ext4, linux-f2fs-devel, Lukas Czerner,
Jaegeuk Kim, Jeff Layton
On Fri, May 13, 2022 at 03:36:05PM -0400, Theodore Ts'o wrote:
> On Sat, Apr 30, 2022 at 10:08:50PM -0700, Eric Biggers wrote:
> > We can either take all these patches through the fscrypt tree, or we can
> > take them in multiple cycles as follows:
> >
> > 1. patch 1 via ext4, patch 2 via f2fs, patch 3-4 via fscrypt
> > 2. patch 5 via ext4, patch 6 via f2fs
> > 3. patch 7 via fscrypt
> >
> > Ted and Jaegeuk, let me know what you prefer.
>
> In order to avoid patch conflicts with other patch series, what I'd
> prefer is to take them in multiple cycles. I can take patch #1 in my
> initial pull request to Linus, and then do a second pull request to
> Linus with patch #5 post -rc1 or -rc2 (depending on when patches #3
> and #4 hit Linus's tree).
>
> Does that sound good?
That basically sounds fine. I've just sent out v3 of this series, with the fix
for the memory leak in parse_apply_sb_mount_options() as its own patch. That
patch can be applied now too, so you can take patches 1-2 of the v3 series in
your initial pull request.
- Eric
^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2022-05-13 23:35 UTC | newest]
Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-01 5:08 [PATCH v2 0/7] test_dummy_encryption fixes and cleanups Eric Biggers
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
2022-05-01 5:08 ` [PATCH v2 1/7] ext4: only allow test_dummy_encryption when supported Eric Biggers
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
2022-05-11 12:50 ` Ritesh Harjani
2022-05-11 12:50 ` [f2fs-dev] " Ritesh Harjani
2022-05-11 17:18 ` Eric Biggers
2022-05-11 17:18 ` [f2fs-dev] " Eric Biggers
2022-05-01 5:08 ` [PATCH v2 2/7] f2fs: reject test_dummy_encryption when !CONFIG_FS_ENCRYPTION Eric Biggers
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
2022-05-01 5:08 ` [PATCH v2 3/7] fscrypt: factor out fscrypt_policy_to_key_spec() Eric Biggers
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
2022-05-01 5:08 ` [PATCH v2 4/7] fscrypt: add new helper functions for test_dummy_encryption Eric Biggers
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
2022-05-01 5:08 ` [PATCH v2 5/7] ext4: fix up test_dummy_encryption handling for new mount API Eric Biggers
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
2022-05-09 23:40 ` Eric Biggers
2022-05-09 23:40 ` Eric Biggers
2022-05-11 17:54 ` [f2fs-dev] " Ritesh Harjani
2022-05-11 17:54 ` Ritesh Harjani
2022-05-11 18:03 ` Eric Biggers
2022-05-11 18:03 ` Eric Biggers
2022-05-13 10:58 ` Ritesh Harjani
2022-05-13 10:58 ` Ritesh Harjani
2022-05-13 22:24 ` Eric Biggers
2022-05-13 22:24 ` [f2fs-dev] " Eric Biggers
2022-05-13 11:07 ` Ritesh Harjani
2022-05-13 11:07 ` [f2fs-dev] " Ritesh Harjani
2022-05-13 21:59 ` Eric Biggers
2022-05-13 21:59 ` [f2fs-dev] " Eric Biggers
2022-05-01 5:08 ` [PATCH v2 6/7] f2fs: use the updated test_dummy_encryption helper functions Eric Biggers
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
2022-05-01 5:08 ` [PATCH v2 7/7] fscrypt: remove fscrypt_set_test_dummy_encryption() Eric Biggers
2022-05-01 5:08 ` [f2fs-dev] " Eric Biggers
2022-05-09 23:36 ` [f2fs-dev] [PATCH v2 0/7] test_dummy_encryption fixes and cleanups Eric Biggers
2022-05-09 23:36 ` Eric Biggers
2022-05-10 23:23 ` Jaegeuk Kim
2022-05-10 23:23 ` [f2fs-dev] " Jaegeuk Kim
2022-05-13 19:36 ` Theodore Ts'o
2022-05-13 19:36 ` Theodore Ts'o
2022-05-13 23:26 ` [f2fs-dev] " Eric Biggers
2022-05-13 23:26 ` Eric Biggers
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.