This series eliminates the call to fscrypt_destroy_keyring() from
__put_super(), which is causing confusion because it looks like (but
actually isn't) a sleep-in-atomic bug. See the thread "block: sleeping
in atomic warnings", i.e.
https://lore.kernel.org/linux-fsdevel/CAHk-=wg6ohuyrmLJYTfEpDbp2Jwnef54gkcpZ3-BYgy4C6UxRQ@mail.gmail.com
and its responses.
To do this, this series makes it so that the key associated with the
"test_dummy_encryption" mount option is added on-demand when files are
accessed, instead of immediately when the filesystem is mounted.
I was going back and forth between this solution and instead having ext4
and f2fs call fscrypt_destroy_keyring() on ->fill_super failure. (Or
using Linus's suggestion of adding the test dummy key as the very last
step of ->fill_super.) It does seem ideal to add the key at mount time,
but I ended up going with this solution instead because it reduces the
number of things the individual filesystems have to handle.
Eric Biggers (5):
fscrypt: add the test dummy encryption key on-demand
ext4: stop calling fscrypt_add_test_dummy_key()
f2fs: stop calling fscrypt_add_test_dummy_key()
fs/super.c: stop calling fscrypt_destroy_keyring() from __put_super()
fscrypt: clean up fscrypt_add_test_dummy_key()
fs/crypto/fscrypt_private.h | 4 ++++
fs/crypto/keyring.c | 26 +++++++-------------------
fs/crypto/keysetup.c | 23 +++++++++++++++++++++--
fs/crypto/policy.c | 3 +--
fs/ext4/super.c | 13 +------------
fs/f2fs/super.c | 6 ------
fs/super.c | 1 -
include/linux/fscrypt.h | 9 ---------
8 files changed, 34 insertions(+), 51 deletions(-)
base-commit: 6d796c50f84ca79f1722bb131799e5a5710c4700
--
2.39.1
From: Eric Biggers <[email protected]>
Now that the key associated with the "test_dummy_operation" mount option
is added on-demand when it's needed, rather than immediately when the
filesystem is mounted, fscrypt_destroy_keyring() no longer needs to be
called from __put_super() to avoid a memory leak on mount failure.
Remove this call, which was causing confusion because it appeared to be
a sleep-in-atomic bug (though it wasn't, for a somewhat-subtle reason).
Signed-off-by: Eric Biggers <[email protected]>
---
fs/super.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/super.c b/fs/super.c
index 12c08cb20405d..ce45b7fd27f90 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -291,7 +291,6 @@ static void __put_super(struct super_block *s)
WARN_ON(s->s_inode_lru.node);
WARN_ON(!list_empty(&s->s_mounts));
security_sb_free(s);
- fscrypt_destroy_keyring(s);
put_user_ns(s->s_user_ns);
kfree(s->s_subtype);
call_rcu(&s->rcu, destroy_super_rcu);
--
2.39.1
From: Eric Biggers <[email protected]>
Now that fscrypt_add_test_dummy_key() is only called by
setup_file_encryption_key() and not by the individual filesystems,
un-export it. Also change its prototype to take the
fscrypt_key_specifier directly, as the caller already has it.
Signed-off-by: Eric Biggers <[email protected]>
---
fs/crypto/fscrypt_private.h | 3 +++
fs/crypto/keyring.c | 26 +++++++-------------------
fs/crypto/keysetup.c | 4 +---
include/linux/fscrypt.h | 9 ---------
4 files changed, 11 insertions(+), 31 deletions(-)
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 17dd33d9a522e..0fec2dfc36ebe 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -573,6 +573,9 @@ fscrypt_find_master_key(struct super_block *sb,
int fscrypt_get_test_dummy_key_identifier(
u8 key_identifier[FSCRYPT_KEY_IDENTIFIER_SIZE]);
+int fscrypt_add_test_dummy_key(struct super_block *sb,
+ struct fscrypt_key_specifier *key_spec);
+
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 78dd2ff306bd7..78086f8dbda52 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -211,10 +211,6 @@ static int allocate_filesystem_keyring(struct super_block *sb)
* are still available at this time; this is important because after user file
* accesses have been allowed, this function may need to evict keys from the
* keyslots of an inline crypto engine, which requires the block device(s).
- *
- * This is also called when the super_block is being freed. This is needed to
- * avoid a memory leak if mounting fails after the "test_dummy_encryption"
- * option was processed, as in that case the unmount-time call isn't made.
*/
void fscrypt_destroy_keyring(struct super_block *sb)
{
@@ -778,34 +774,26 @@ int fscrypt_get_test_dummy_key_identifier(
/**
* 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
+ * @key_spec: the key specifier of the test dummy encryption key
*
- * 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.
+ * 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)
+ struct fscrypt_key_specifier *key_spec)
{
- const union fscrypt_policy *policy = dummy_policy->policy;
- struct fscrypt_key_specifier key_spec;
struct fscrypt_master_key_secret secret;
int err;
- 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);
+ 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/keysetup.c b/fs/crypto/keysetup.c
index 20323c0ba4c5e..aa94fba9d17e7 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -464,9 +464,7 @@ static int setup_file_encryption_key(struct fscrypt_info *ci,
*/
if (dummy_policy &&
fscrypt_policies_equal(dummy_policy, &ci->ci_policy)) {
- struct fscrypt_dummy_policy tmp = { dummy_policy };
-
- err = fscrypt_add_test_dummy_key(sb, &tmp);
+ err = fscrypt_add_test_dummy_key(sb, &mk_spec);
if (err)
return err;
mk = fscrypt_find_master_key(sb, &mk_spec);
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 4f5f8a6512132..44848d870046a 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -309,8 +309,6 @@ fscrypt_free_dummy_policy(struct fscrypt_dummy_policy *dummy_policy)
/* keyring.c */
void fscrypt_destroy_keyring(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);
@@ -530,13 +528,6 @@ 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.39.1
From: Eric Biggers <[email protected]>
Now that fs/crypto/ adds the test dummy encryption key on-demand when
it's needed, there's no need for individual filesystems to call
fscrypt_add_test_dummy_key(). Remove the call to it from f2fs.
Signed-off-by: Eric Biggers <[email protected]>
---
fs/f2fs/super.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 1f812b9ce985b..64d3556d61a55 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -540,12 +540,6 @@ 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;
}
--
2.39.1
From: Eric Biggers <[email protected]>
When the key for an inode is not found but the inode is using the
test_dummy_encryption policy, automatically add the
test_dummy_encryption key to the filesystem keyring. This eliminates
the need for all the individual filesystems to do this at mount time,
which is a bit tricky to clean up from on failure.
Note: this covers the call to fscrypt_find_master_key() from inode key
setup, but not from the fscrypt ioctls. So, this isn't *exactly* the
same as the key being present from the very beginning. I think we can
tolerate that, though, since the inode key setup caller is the only one
that actually matters in the context of test_dummy_encryption.
Signed-off-by: Eric Biggers <[email protected]>
---
fs/crypto/fscrypt_private.h | 1 +
fs/crypto/keysetup.c | 25 +++++++++++++++++++++++--
fs/crypto/policy.c | 3 +--
3 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 316a778cec0ff..17dd33d9a522e 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -651,6 +651,7 @@ 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);
+const union fscrypt_policy *fscrypt_get_dummy_policy(struct super_block *sb);
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 94757ccd30568..20323c0ba4c5e 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -438,6 +438,7 @@ static int setup_file_encryption_key(struct fscrypt_info *ci,
bool need_dirhash_key,
struct fscrypt_master_key **mk_ret)
{
+ struct super_block *sb = ci->ci_inode->i_sb;
struct fscrypt_key_specifier mk_spec;
struct fscrypt_master_key *mk;
int err;
@@ -450,8 +451,28 @@ static int setup_file_encryption_key(struct fscrypt_info *ci,
if (err)
return err;
- mk = fscrypt_find_master_key(ci->ci_inode->i_sb, &mk_spec);
- if (!mk) {
+ mk = fscrypt_find_master_key(sb, &mk_spec);
+ if (unlikely(!mk)) {
+ const union fscrypt_policy *dummy_policy =
+ fscrypt_get_dummy_policy(sb);
+
+ /*
+ * Add the test_dummy_encryption key on-demand. In principle,
+ * it should be added at mount time. Do it here instead so that
+ * the individual filesystems don't need to worry about adding
+ * this key at mount time and cleaning up on mount failure.
+ */
+ if (dummy_policy &&
+ fscrypt_policies_equal(dummy_policy, &ci->ci_policy)) {
+ struct fscrypt_dummy_policy tmp = { dummy_policy };
+
+ err = fscrypt_add_test_dummy_key(sb, &tmp);
+ if (err)
+ return err;
+ mk = fscrypt_find_master_key(sb, &mk_spec);
+ }
+ }
+ if (unlikely(!mk)) {
if (ci->ci_policy.version != FSCRYPT_POLICY_V1)
return -ENOKEY;
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 893661b523769..69dca4ff5f488 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -53,8 +53,7 @@ int fscrypt_policy_to_key_spec(const union fscrypt_policy *policy,
}
}
-static const union fscrypt_policy *
-fscrypt_get_dummy_policy(struct super_block *sb)
+const union fscrypt_policy *fscrypt_get_dummy_policy(struct super_block *sb)
{
if (!sb->s_cop->get_dummy_policy)
return NULL;
--
2.39.1
From: Eric Biggers <[email protected]>
Now that fs/crypto/ adds the test dummy encryption key on-demand when
it's needed, there's no need for individual filesystems to call
fscrypt_add_test_dummy_key(). Remove the call to it from ext4.
Signed-off-by: Eric Biggers <[email protected]>
---
fs/ext4/super.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 260c1b3e3ef2c..260bbab25db38 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2635,7 +2635,6 @@ 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 (!fscrypt_is_dummy_policy_set(&ctx->dummy_enc_policy))
return 0;
@@ -2668,17 +2667,7 @@ static int ext4_check_test_dummy_encryption(const struct fs_context *fc,
"Conflicting test_dummy_encryption options");
return -EINVAL;
}
- /*
- * fscrypt_add_test_dummy_key() technically changes the super_block, so
- * technically it 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;
+ return 0;
}
static void ext4_apply_test_dummy_encryption(struct ext4_fs_context *ctx,
--
2.39.1
On Tue, Feb 7, 2023 at 10:21 PM Eric Biggers <[email protected]> wrote:
>
> I was going back and forth between this solution and instead having ext4
> and f2fs call fscrypt_destroy_keyring() on ->fill_super failure. (Or
> using Linus's suggestion of adding the test dummy key as the very last
> step of ->fill_super.) It does seem ideal to add the key at mount time,
> but I ended up going with this solution instead because it reduces the
> number of things the individual filesystems have to handle.
Well, the diffstat certainly looks nice:
> 8 files changed, 34 insertions(+), 51 deletions(-)
with that
> fs/super.c | 1 -
removing the offending line that made Dan's static detection tool so
unhappy, so this all looks lovely to me.
Thanks,
Linus
Hello:
This series was applied to jaegeuk/f2fs.git (dev)
by Eric Biggers <[email protected]>:
On Tue, 7 Feb 2023 22:21:02 -0800 you wrote:
> This series eliminates the call to fscrypt_destroy_keyring() from
> __put_super(), which is causing confusion because it looks like (but
> actually isn't) a sleep-in-atomic bug. See the thread "block: sleeping
> in atomic warnings", i.e.
> https://lore.kernel.org/linux-fsdevel/CAHk-=wg6ohuyrmLJYTfEpDbp2Jwnef54gkcpZ3-BYgy4C6UxRQ@mail.gmail.com
> and its responses.
>
> [...]
Here is the summary with links:
- [f2fs-dev,1/5] fscrypt: add the test dummy encryption key on-demand
https://git.kernel.org/jaegeuk/f2fs/c/60e463f0be98
- [f2fs-dev,2/5] ext4: stop calling fscrypt_add_test_dummy_key()
https://git.kernel.org/jaegeuk/f2fs/c/7959eb19e4a3
- [f2fs-dev,3/5] f2fs: stop calling fscrypt_add_test_dummy_key()
https://git.kernel.org/jaegeuk/f2fs/c/1ad2a626762d
- [f2fs-dev,4/5] fs/super.c: stop calling fscrypt_destroy_keyring() from __put_super()
https://git.kernel.org/jaegeuk/f2fs/c/ec64036e6863
- [f2fs-dev,5/5] fscrypt: clean up fscrypt_add_test_dummy_key()
https://git.kernel.org/jaegeuk/f2fs/c/097d7c1fcb8d
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html