[PATCH v3 00/16] fscrypt: add extent encryption

[PATCH v3 00/16] fscrypt: add extent encryption

[Sweet Tea Dorminy]

This changeset adds extent-based data encryption to fscrypt.
Some filesystems need to encrypt data based on extents, rather than on
inodes, due to features incompatible with inode-based encryption. For
instance, btrfs can have multiple inodes referencing a single block of
data, and moves logical data blocks to different physical locations on
disk in the background. 

As per discussion last year in [1] and later in [2], we would like to
allow the use of fscrypt with btrfs, with authenticated encryption. This
is the first step of that work, adding extent-based encryption to
fscrypt; authenticated encryption is the next step. Extent-based
encryption should be usable by other filesystems which wish to support
snapshotting or background data rearrangement also, but btrfs is the
first user. 

This changeset requires extent encryption to use inlinecrypt, as
discussed previously. 

This applies atop [3], which itself is based on kdave/misc-next. It
passes encryption fstests with suitable changes to btrfs-progs.

Changelog:
v3:
 - Added four additional changes:
   - soft-deleting keys that extent infos might later need to use, so
     the behavior of an open file after key removal matches inode-based
     fscrypt.
   - a set of changes to allow asynchronous info freeing for extents,
     necessary due to locking constraints in btrfs.

v2: 
 - https://lore.kernel.org/linux-fscrypt/cover.1688927487.git.sweettea-kernel@dorminy.me/T/#t


[1] https://docs.google.com/document/d/1janjxewlewtVPqctkWOjSa7OhCgB8Gdx7iDaCDQQNZA/edit?usp=sharing
[2] https://lore.kernel.org/linux-fscrypt/80496cfe-161d-fb0d-8230-93818b966b1b@dorminy.me/T/#t
[3] https://lore.kernel.org/linux-fscrypt/cover.1691505830.git.sweettea-kernel@dorminy.me/

Sweet Tea Dorminy (16):
  fscrypt: factor helper for locking master key
  fscrypt: factor getting info for a specific block
  fscrypt: adjust effective lblks based on extents
  fscrypt: add a super_block pointer to fscrypt_info
  fscrypt: setup leaf inodes for extent encryption
  fscrypt: allow infos to be owned by extents
  fscrypt: use an optional ino equivalent for per-extent infos
  fscrypt: move function call warning of busy inodes
  fscrypt: revamp key removal for extent encryption
  fscrypt: add creation/usage/freeing of per-extent infos
  fscrypt: allow load/save of extent contexts
  fscrypt: save session key credentials for extent infos
  fscrypt: allow multiple extents to reference one info
  fscrypt: cache list of inlinecrypt devices
  fscrypt: allow asynchronous info freeing
  fscrypt: update documentation for per-extent keys

 Documentation/filesystems/fscrypt.rst |  43 +++-
 fs/crypto/crypto.c                    |   6 +-
 fs/crypto/fscrypt_private.h           | 158 +++++++++++-
 fs/crypto/inline_crypt.c              |  49 ++--
 fs/crypto/keyring.c                   |  78 +++---
 fs/crypto/keysetup.c                  | 336 ++++++++++++++++++++++----
 fs/crypto/keysetup_v1.c               |  10 +-
 fs/crypto/policy.c                    |  20 ++
 include/linux/fscrypt.h               |  67 +++++
 9 files changed, 654 insertions(+), 113 deletions(-)


base-commit: 54d2161835d828a9663f548f61d1d9c3d3482122
prerequisite-patch-id: 2f1424d04bb5a76abf0ecf2c9cd8426d300078ae
prerequisite-patch-id: ab342a72cf967dadfb8bec1320c5906fd3c6800f
prerequisite-patch-id: ced2a9dab36539f55c14cd74a28950087c475ff2
prerequisite-patch-id: d4f1a64c994c2fa0d2d4cab83f9ddff52f0622e9
prerequisite-patch-id: 1af0fc98277159b31c26bc5751663efc0d322d75
prerequisite-patch-id: 3b21b62208587486cf9b31618f7c3bc875362f1a
prerequisite-patch-id: c43d693f5b7c498a876d9ffcfc49c11a8ca93d80
prerequisite-patch-id: f120bde1cf47fbef1d9f8fd09cdcccc1408c3ff4
prerequisite-patch-id: c6a1f087d4a67b928b9c6af04e00310bfa74ace1
-- 
2.41.0

[PATCH v3 01/16] fscrypt: factor helper for locking master key

[Sweet Tea Dorminy]

When we are making extent infos, we'll need to lock the master key in
more places, so go on and factor out a helper.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/keysetup.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index a19650f954e2..c3d3da5da449 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -106,7 +106,18 @@ select_encryption_mode(const union fscrypt_policy *policy,
 	return ERR_PTR(-EINVAL);
 }
 
-/* Create a symmetric cipher object for the given encryption mode and key */
+static int lock_master_key(struct fscrypt_master_key *mk)
+{
+	down_read(&mk->mk_sem);
+
+	/* Has the secret been removed (via FS_IOC_REMOVE_ENCRYPTION_KEY)? */
+	if (!is_master_key_secret_present(&mk->mk_secret))
+		return -ENOKEY;
+
+	return 0;
+}
+
+/* Create a symmetric cipher object for the given encryption mode */
 static struct crypto_skcipher *
 fscrypt_allocate_skcipher(struct fscrypt_mode *mode, const u8 *raw_key,
 			  const struct inode *inode)
@@ -556,13 +567,10 @@ static int find_and_lock_master_key(const struct fscrypt_info *ci,
 		*mk_ret = NULL;
 		return 0;
 	}
-	down_read(&mk->mk_sem);
 
-	/* Has the secret been removed (via FS_IOC_REMOVE_ENCRYPTION_KEY)? */
-	if (!is_master_key_secret_present(&mk->mk_secret)) {
-		err = -ENOKEY;
+	err = lock_master_key(mk);
+	if (err)
 		goto out_release_key;
-	}
 
 	if (!fscrypt_valid_master_key_size(mk, ci)) {
 		err = -ENOKEY;
-- 
2.41.0

Re: [PATCH v3 01/16] fscrypt: factor helper for locking master key

[Josef Bacik]

On Tue, Aug 08, 2023 at 01:08:18PM -0400, Sweet Tea Dorminy wrote:
> When we are making extent infos, we'll need to lock the master key in
> more places, so go on and factor out a helper.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

You factor this out, but I went and checked your tree with all of your patches
and this is only ever used in this one case, so do you actually need this patch
anymore?  Thanks,

Josef

[PATCH v3 02/16] fscrypt: factor getting info for a specific block

[Sweet Tea Dorminy]

For filesystems using extent-based encryption, the content of each
extent will be encrypted with a different fscrypt_info for each extent.
Meanwhile, directories and symlinks will continue to use the
fscrypt_info for the inode. Therefore, merely grabbing
inode->i_crypt_info will be insufficient; the caller must specifically
request the inode info or the info for a specific block.

Add fscrypt_get_lblk_info() to get info for a specific block, and update
all relevant callsites.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/crypto.c          |  3 ++-
 fs/crypto/fscrypt_private.h | 29 +++++++++++++++++++++++++++++
 fs/crypto/inline_crypt.c    | 10 ++++++----
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 9f3bda18c797..1b7e375b1c6b 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -107,7 +107,8 @@ int fscrypt_crypt_block(const struct inode *inode, fscrypt_direction_t rw,
 	struct skcipher_request *req = NULL;
 	DECLARE_CRYPTO_WAIT(wait);
 	struct scatterlist dst, src;
-	struct fscrypt_info *ci = inode->i_crypt_info;
+	struct fscrypt_info *ci =
+		fscrypt_get_lblk_info(inode, lblk_num, NULL, NULL);
 	struct crypto_skcipher *tfm = ci->ci_enc_key->tfm;
 	int res = 0;
 
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index e2acd8894ea7..8a1fd1d33cfc 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -277,6 +277,35 @@ typedef enum {
 	FS_ENCRYPT,
 } fscrypt_direction_t;
 
+/**
+ * fscrypt_get_lblk_info() - get the fscrypt_info to crypt a particular block
+ *
+ * @inode:      the inode to which the block belongs
+ * @lblk:       the offset of the block within the file which the inode
+ *              references
+ * @offset:     a pointer to return the offset of the block from the first block
+ *              that the info covers. For inode-based encryption, this will
+ *              always be @lblk; for extent-based encryption, this will be in
+ *              the range [0, lblk]. Can be NULL
+ * @extent_len: a pointer to return the minimum number of lblks starting at
+ *              this offset which also belong to the same fscrypt_info. Can be
+ *              NULL
+ *
+ * Return: the appropriate fscrypt_info if there is one, else NULL.
+ */
+static inline struct fscrypt_info *
+fscrypt_get_lblk_info(const struct inode *inode, u64 lblk, u64 *offset,
+		      u64 *extent_len)
+{
+	if (offset)
+		*offset = lblk;
+	if (extent_len)
+		*extent_len = U64_MAX;
+
+	return inode->i_crypt_info;
+}
+
+
 /* crypto.c */
 extern struct kmem_cache *fscrypt_info_cachep;
 int fscrypt_initialize(struct super_block *sb);
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index 2063f7941ce6..885a2ec3d711 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -270,7 +270,7 @@ void fscrypt_set_bio_crypt_ctx(struct bio *bio, const struct inode *inode,
 
 	if (!fscrypt_inode_uses_inline_crypto(inode))
 		return;
-	ci = inode->i_crypt_info;
+	ci = fscrypt_get_lblk_info(inode, first_lblk, NULL, NULL);
 
 	fscrypt_generate_dun(ci, first_lblk, dun);
 	bio_crypt_set_ctx(bio, ci->ci_enc_key->blk_key, dun, gfp_mask);
@@ -349,21 +349,23 @@ bool fscrypt_mergeable_bio(struct bio *bio, const struct inode *inode,
 {
 	const struct bio_crypt_ctx *bc = bio->bi_crypt_context;
 	u64 next_dun[BLK_CRYPTO_DUN_ARRAY_SIZE];
+	struct fscrypt_info *ci;
 
 	if (!!bc != fscrypt_inode_uses_inline_crypto(inode))
 		return false;
 	if (!bc)
 		return true;
 
+	ci = fscrypt_get_lblk_info(inode, next_lblk, NULL, NULL);
 	/*
 	 * Comparing the key pointers is good enough, as all I/O for each key
 	 * uses the same pointer.  I.e., there's currently no need to support
 	 * merging requests where the keys are the same but the pointers differ.
 	 */
-	if (bc->bc_key != inode->i_crypt_info->ci_enc_key->blk_key)
+	if (bc->bc_key != ci->ci_enc_key->blk_key)
 		return false;
 
-	fscrypt_generate_dun(inode->i_crypt_info, next_lblk, next_dun);
+	fscrypt_generate_dun(ci, next_lblk, next_dun);
 	return bio_crypt_dun_is_contiguous(bc, bio->bi_iter.bi_size, next_dun);
 }
 EXPORT_SYMBOL_GPL(fscrypt_mergeable_bio);
@@ -465,7 +467,7 @@ u64 fscrypt_limit_io_blocks(const struct inode *inode, u64 lblk, u64 nr_blocks)
 	if (nr_blocks <= 1)
 		return nr_blocks;
 
-	ci = inode->i_crypt_info;
+	ci = fscrypt_get_lblk_info(inode, lblk, NULL, NULL);
 	if (!(fscrypt_policy_flags(&ci->ci_policy) &
 	      FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32))
 		return nr_blocks;
-- 
2.41.0

[PATCH v3 03/16] fscrypt: adjust effective lblks based on extents

[Sweet Tea Dorminy]

If a filesystem uses extent-based encryption, then the offset within a
file is not a constant which can be used for calculating an IV.
For instance, the same extent could be blocks 0-8 in one file, and
blocks 100-108 in another file. Instead, the block offset within the
extent must be used instead.

Update all uses of logical block offset within the file to use logical
block offset within the extent, if applicable.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/crypto.c       |  3 ++-
 fs/crypto/inline_crypt.c | 20 ++++++++++++++------
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 1b7e375b1c6b..d75f1b3f5795 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -107,8 +107,9 @@ int fscrypt_crypt_block(const struct inode *inode, fscrypt_direction_t rw,
 	struct skcipher_request *req = NULL;
 	DECLARE_CRYPTO_WAIT(wait);
 	struct scatterlist dst, src;
+	u64 ci_offset = 0;
 	struct fscrypt_info *ci =
-		fscrypt_get_lblk_info(inode, lblk_num, NULL, NULL);
+		fscrypt_get_lblk_info(inode, lblk_num, &ci_offset, NULL);
 	struct crypto_skcipher *tfm = ci->ci_enc_key->tfm;
 	int res = 0;
 
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index 885a2ec3d711..2d08abbf5892 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -267,12 +267,13 @@ void fscrypt_set_bio_crypt_ctx(struct bio *bio, const struct inode *inode,
 {
 	const struct fscrypt_info *ci;
 	u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE];
+	u64 ci_offset = 0;
 
 	if (!fscrypt_inode_uses_inline_crypto(inode))
 		return;
-	ci = fscrypt_get_lblk_info(inode, first_lblk, NULL, NULL);
+	ci = fscrypt_get_lblk_info(inode, first_lblk, &ci_offset, NULL);
 
-	fscrypt_generate_dun(ci, first_lblk, dun);
+	fscrypt_generate_dun(ci, ci_offset, dun);
 	bio_crypt_set_ctx(bio, ci->ci_enc_key->blk_key, dun, gfp_mask);
 }
 EXPORT_SYMBOL_GPL(fscrypt_set_bio_crypt_ctx);
@@ -350,13 +351,14 @@ bool fscrypt_mergeable_bio(struct bio *bio, const struct inode *inode,
 	const struct bio_crypt_ctx *bc = bio->bi_crypt_context;
 	u64 next_dun[BLK_CRYPTO_DUN_ARRAY_SIZE];
 	struct fscrypt_info *ci;
+	u64 ci_offset = 0;
 
 	if (!!bc != fscrypt_inode_uses_inline_crypto(inode))
 		return false;
 	if (!bc)
 		return true;
 
-	ci = fscrypt_get_lblk_info(inode, next_lblk, NULL, NULL);
+	ci = fscrypt_get_lblk_info(inode, next_lblk, &ci_offset, NULL);
 	/*
 	 * Comparing the key pointers is good enough, as all I/O for each key
 	 * uses the same pointer.  I.e., there's currently no need to support
@@ -365,7 +367,7 @@ bool fscrypt_mergeable_bio(struct bio *bio, const struct inode *inode,
 	if (bc->bc_key != ci->ci_enc_key->blk_key)
 		return false;
 
-	fscrypt_generate_dun(ci, next_lblk, next_dun);
+	fscrypt_generate_dun(ci, ci_offset, next_dun);
 	return bio_crypt_dun_is_contiguous(bc, bio->bi_iter.bi_size, next_dun);
 }
 EXPORT_SYMBOL_GPL(fscrypt_mergeable_bio);
@@ -460,6 +462,8 @@ u64 fscrypt_limit_io_blocks(const struct inode *inode, u64 lblk, u64 nr_blocks)
 {
 	const struct fscrypt_info *ci;
 	u32 dun;
+	u64 ci_offset = 0;
+	u64 extent_len = 0;
 
 	if (!fscrypt_inode_uses_inline_crypto(inode))
 		return nr_blocks;
@@ -467,14 +471,18 @@ u64 fscrypt_limit_io_blocks(const struct inode *inode, u64 lblk, u64 nr_blocks)
 	if (nr_blocks <= 1)
 		return nr_blocks;
 
-	ci = fscrypt_get_lblk_info(inode, lblk, NULL, NULL);
+	ci = fscrypt_get_lblk_info(inode, lblk, &ci_offset, &extent_len);
+
+	/* Spanning an extent boundary will change the DUN */
+	nr_blocks = min_t(u64, nr_blocks, extent_len);
+
 	if (!(fscrypt_policy_flags(&ci->ci_policy) &
 	      FSCRYPT_POLICY_FLAG_IV_INO_LBLK_32))
 		return nr_blocks;
 
 	/* With IV_INO_LBLK_32, the DUN can wrap around from U32_MAX to 0. */
 
-	dun = ci->ci_hashed_ino + lblk;
+	dun = ci->ci_hashed_ino + ci_offset;
 
 	return min_t(u64, nr_blocks, (u64)U32_MAX + 1 - dun);
 }
-- 
2.41.0

[PATCH v3 04/16] fscrypt: add a super_block pointer to fscrypt_info

[Sweet Tea Dorminy]

When fscrypt_infos are attached to extents instead of inodes, we can't
go through the inode to get at the filesystems's superblock. Therefore,
add a dedicated superblock pointer to fscrypt_info to keep track of it.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/fscrypt_private.h |  3 +++
 fs/crypto/inline_crypt.c    |  4 ++--
 fs/crypto/keysetup.c        | 10 +++++-----
 fs/crypto/keysetup_v1.c     |  6 +++---
 4 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 8a1fd1d33cfc..8a6e359f96cf 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -241,6 +241,9 @@ struct fscrypt_info {
 	/* Back-pointer to the inode */
 	struct inode *ci_inode;
 
+	/* The superblock of the filesystem to which this info pertains */
+	struct super_block *ci_sb;
+
 	/*
 	 * The master key with which this inode was unlocked (decrypted).  This
 	 * will be NULL if the master key was found in a process-subscribed
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index 2d08abbf5892..260152d5e673 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -41,7 +41,7 @@ static struct block_device **fscrypt_get_devices(struct super_block *sb,
 
 static unsigned int fscrypt_get_dun_bytes(const struct fscrypt_info *ci)
 {
-	struct super_block *sb = ci->ci_inode->i_sb;
+	struct super_block *sb = ci->ci_sb;
 	unsigned int flags = fscrypt_policy_flags(&ci->ci_policy);
 	int ino_bits = 64, lblk_bits = 64;
 
@@ -155,7 +155,7 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 				     const struct fscrypt_info *ci)
 {
 	const struct inode *inode = ci->ci_inode;
-	struct super_block *sb = inode->i_sb;
+	struct super_block *sb = ci->ci_sb;
 	enum blk_crypto_mode_num crypto_mode = ci->ci_mode->blk_crypto_mode;
 	struct blk_crypto_key *blk_key;
 	struct block_device **devs;
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index c3d3da5da449..c5f68cf65a6f 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -250,8 +250,7 @@ static int setup_new_mode_prepared_key(struct fscrypt_master_key *mk,
 				       struct fscrypt_prepared_key *prep_key,
 				       const struct fscrypt_info *ci)
 {
-	const struct inode *inode = ci->ci_inode;
-	const struct super_block *sb = inode->i_sb;
+	const struct super_block *sb = ci->ci_sb;
 	unsigned int policy_flags = fscrypt_policy_flags(&ci->ci_policy);
 	struct fscrypt_mode *mode = ci->ci_mode;
 	const u8 mode_num = mode - fscrypt_modes;
@@ -525,7 +524,7 @@ static bool fscrypt_valid_master_key_size(const struct fscrypt_master_key *mk,
 static int find_and_lock_master_key(const struct fscrypt_info *ci,
 				    struct fscrypt_master_key **mk_ret)
 {
-	struct super_block *sb = ci->ci_inode->i_sb;
+	struct super_block *sb = ci->ci_sb;
 	struct fscrypt_key_specifier mk_spec;
 	struct fscrypt_master_key *mk;
 	int err;
@@ -599,7 +598,7 @@ static void put_crypt_info(struct fscrypt_info *ci)
 		if (type == FSCRYPT_KEY_DIRECT_V1)
 			fscrypt_put_direct_key(ci->ci_enc_key);
 		if (type == FSCRYPT_KEY_PER_INFO) {
-			fscrypt_destroy_prepared_key(ci->ci_inode->i_sb,
+			fscrypt_destroy_prepared_key(ci->ci_sb,
 						     ci->ci_enc_key);
 			kfree_sensitive(ci->ci_enc_key);
 		}
@@ -616,7 +615,7 @@ static void put_crypt_info(struct fscrypt_info *ci)
 		spin_lock(&mk->mk_decrypted_inodes_lock);
 		list_del(&ci->ci_master_key_link);
 		spin_unlock(&mk->mk_decrypted_inodes_lock);
-		fscrypt_put_master_key_activeref(ci->ci_inode->i_sb, mk);
+		fscrypt_put_master_key_activeref(ci->ci_sb, mk);
 	}
 	memzero_explicit(ci, sizeof(*ci));
 	kmem_cache_free(fscrypt_info_cachep, ci);
@@ -642,6 +641,7 @@ fscrypt_setup_encryption_info(struct inode *inode,
 		return -ENOMEM;
 
 	crypt_info->ci_inode = inode;
+	crypt_info->ci_sb = inode->i_sb;
 	crypt_info->ci_policy = *policy;
 	memcpy(crypt_info->ci_nonce, nonce, FSCRYPT_FILE_NONCE_SIZE);
 
diff --git a/fs/crypto/keysetup_v1.c b/fs/crypto/keysetup_v1.c
index 1e785cedead0..41d317f08aeb 100644
--- a/fs/crypto/keysetup_v1.c
+++ b/fs/crypto/keysetup_v1.c
@@ -235,7 +235,7 @@ fscrypt_get_direct_key(const struct fscrypt_info *ci, const u8 *raw_key)
 	dk = kzalloc(sizeof(*dk), GFP_KERNEL);
 	if (!dk)
 		return ERR_PTR(-ENOMEM);
-	dk->dk_sb = ci->ci_inode->i_sb;
+	dk->dk_sb = ci->ci_sb;
 	refcount_set(&dk->dk_refcount, 1);
 	dk->dk_mode = ci->ci_mode;
 	dk->dk_key.type = FSCRYPT_KEY_DIRECT_V1;
@@ -309,8 +309,8 @@ int fscrypt_setup_v1_file_key_via_subscribed_keyrings(struct fscrypt_info *ci)
 	key = find_and_lock_process_key(FSCRYPT_KEY_DESC_PREFIX,
 					ci->ci_policy.v1.master_key_descriptor,
 					ci->ci_mode->keysize, &payload);
-	if (key == ERR_PTR(-ENOKEY) && ci->ci_inode->i_sb->s_cop->key_prefix) {
-		key = find_and_lock_process_key(ci->ci_inode->i_sb->s_cop->key_prefix,
+	if (key == ERR_PTR(-ENOKEY) && ci->ci_sb->s_cop->key_prefix) {
+		key = find_and_lock_process_key(ci->ci_sb->s_cop->key_prefix,
 						ci->ci_policy.v1.master_key_descriptor,
 						ci->ci_mode->keysize, &payload);
 	}
-- 
2.41.0

[PATCH v3 05/16] fscrypt: setup leaf inodes for extent encryption

[Sweet Tea Dorminy]

For extent-based encryption, leaf/regular file inodes are special: it's
useful to set their i_crypt_info field so that it's easy to inherit
their encryption policy for a new extent, but they never need to do any
encyption themselves. Additionally, since encryption can only be set up
on a directory, not a single file, their encryption policy can always
duplicate their parent inode's policy.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/fscrypt_private.h | 17 +++++++++++
 fs/crypto/keysetup.c        | 60 ++++++++++++++++++++++++++++---------
 2 files changed, 63 insertions(+), 14 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 8a6e359f96cf..df1c5ae82d85 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -308,6 +308,23 @@ fscrypt_get_lblk_info(const struct inode *inode, u64 lblk, u64 *offset,
 	return inode->i_crypt_info;
 }
 
+/**
+ * fscrypt_uses_extent_encryption() -- whether an inode uses per-extent
+ *				       encryption
+ *
+ * @inode: the inode in question
+ *
+ * Return: true if the inode uses per-extent fscrypt_infos, false otherwise
+ */
+static inline bool fscrypt_uses_extent_encryption(const struct inode *inode)
+{
+	/* Non-regular files don't have extents */
+	if (!S_ISREG(inode->i_mode))
+		return false;
+
+	/* No filesystems currently use per-extent infos */
+	return false;
+}
 
 /* crypto.c */
 extern struct kmem_cache *fscrypt_info_cachep;
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index c5f68cf65a6f..9b3806ab7ccb 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -726,6 +726,26 @@ fscrypt_setup_encryption_info(struct inode *inode,
 	return res;
 }
 
+static bool get_parent_policy_and_nonce(struct inode *inode,
+					union fscrypt_policy *policy,
+					u8 *nonce)
+{
+	struct dentry *dentry = d_find_any_alias(inode);
+	struct dentry *parent_dentry = dget_parent(dentry);
+	struct inode *dir = parent_dentry->d_inode;
+	bool found = false;
+
+	if (dir->i_crypt_info) {
+		found = true;
+		*policy = dir->i_crypt_info->ci_policy;
+		memcpy(nonce, dir->i_crypt_info->ci_nonce,
+		       FSCRYPT_FILE_NONCE_SIZE);
+	}
+	dput(parent_dentry);
+	dput(dentry);
+	return found;
+}
+
 /**
  * fscrypt_get_encryption_info() - set up an inode's encryption key
  * @inode: the inode to set up the key for.  Must be encrypted.
@@ -747,27 +767,39 @@ fscrypt_setup_encryption_info(struct inode *inode,
 int fscrypt_get_encryption_info(struct inode *inode, bool allow_unsupported)
 {
 	int res;
-	union fscrypt_context ctx;
+	union fscrypt_context ctx = { 0 };
 	union fscrypt_policy policy;
+	const u8 *nonce;
+	u8 nonce_bytes[FSCRYPT_FILE_NONCE_SIZE];
 
 	if (fscrypt_has_encryption_key(inode))
 		return 0;
 
-	res = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx));
-	if (res < 0) {
-		if (res == -ERANGE && allow_unsupported)
+	if (fscrypt_uses_extent_encryption(inode)) {
+		/*
+		 * Nothing will be encrypted with this info, so we can borrow
+		 * the parent (dir) inode's policy and use a zero nonce.
+		 */
+		if (!get_parent_policy_and_nonce(inode, &policy, nonce_bytes))
 			return 0;
-		fscrypt_warn(inode, "Error %d getting encryption context", res);
-		return res;
-	}
+		nonce = nonce_bytes;
+	} else {
+		res = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx));
+		if (res < 0) {
+			if (res == -ERANGE && allow_unsupported)
+				return 0;
+			fscrypt_warn(inode, "Error %d getting encryption context", res);
+			return res;
+		}
 
-	res = fscrypt_policy_from_context(&policy, &ctx, res);
-	if (res) {
-		if (allow_unsupported)
-			return 0;
-		fscrypt_warn(inode,
-			     "Unrecognized or corrupt encryption context");
-		return res;
+		res = fscrypt_policy_from_context(&policy, &ctx, res);
+		if (res) {
+			if (allow_unsupported)
+				return 0;
+			fscrypt_warn(inode,
+				     "Unrecognized or corrupt encryption context");
+			return res;
+		}
 	}
 
 	if (!fscrypt_supported_policy(&policy, inode)) {
-- 
2.41.0

[PATCH v3 06/16] fscrypt: allow infos to be owned by extents

[Sweet Tea Dorminy]

In order to notify extents when their info is part of a master key which
is going away, the fscrypt_info must have a backpointer to the extent
somehow. Similarly, if a fscrypt_info is owned by an extent, the info
must not have a pointer to an inode -- multiple inodes may reference a
extent, and the first inode to cause an extent's creation may have a
lifetime much shorter than the extent, so there is no inode pointer
safe to track in an extent-owned info. Therefore, this adds a new
pointer for extent-owned infos to track their extent and updates
fscrypt_setup_encryption_info() accordingly.

Since it's simple to track the piece of extent memory pointing to the
info, and for the extent to then go from such a pointer to the whole
extent via container_of(), we store that. Although some sort of generic
void * or some artificial fscrypt_extent embedded structure would also
work, those would require additional plumbing which doesn't seem
strictly required or clarifying.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/fscrypt_private.h |  6 +++++
 fs/crypto/keysetup.c        | 45 ++++++++++++++++++++++++++++---------
 2 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index df1c5ae82d85..1244797cd8a9 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -241,6 +241,12 @@ struct fscrypt_info {
 	/* Back-pointer to the inode */
 	struct inode *ci_inode;
 
+	/*
+	 * Back-pointer to the info pointer in the extent, for infos owned by
+	 * an extent.
+	 */
+	struct fscrypt_info **ci_info_ptr;
+
 	/* The superblock of the filesystem to which this info pertains */
 	struct super_block *ci_sb;
 
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 9b3806ab7ccb..c72f9015ed35 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -625,12 +625,17 @@ static int
 fscrypt_setup_encryption_info(struct inode *inode,
 			      const union fscrypt_policy *policy,
 			      const u8 nonce[FSCRYPT_FILE_NONCE_SIZE],
-			      bool need_dirhash_key)
+			      bool need_dirhash_key,
+			      struct fscrypt_info **info_ptr)
 {
 	struct fscrypt_info *crypt_info;
 	struct fscrypt_mode *mode;
 	struct fscrypt_master_key *mk = NULL;
 	int res;
+	bool info_for_extent = !!info_ptr;
+
+	if (!info_ptr)
+		info_ptr = &inode->i_crypt_info;
 
 	res = fscrypt_initialize(inode->i_sb);
 	if (res)
@@ -640,6 +645,9 @@ fscrypt_setup_encryption_info(struct inode *inode,
 	if (!crypt_info)
 		return -ENOMEM;
 
+	if (fscrypt_uses_extent_encryption(inode) && info_for_extent)
+		crypt_info->ci_info_ptr = info_ptr;
+
 	crypt_info->ci_inode = inode;
 	crypt_info->ci_sb = inode->i_sb;
 	crypt_info->ci_policy = *policy;
@@ -656,6 +664,12 @@ fscrypt_setup_encryption_info(struct inode *inode,
 	res = fscrypt_select_encryption_impl(crypt_info);
 	if (res)
 		goto out;
+	if (info_for_extent && !fscrypt_using_inline_encryption(crypt_info)) {
+		fscrypt_warn(inode,
+			     "extent encryption requires inlinecrypt mount option");
+		res = -EINVAL;
+		goto out;
+	}
 
 	res = find_and_lock_master_key(crypt_info, &mk);
 	if (res)
@@ -701,7 +715,7 @@ fscrypt_setup_encryption_info(struct inode *inode,
 	 * fscrypt_get_info().  I.e., here we publish ->i_crypt_info with a
 	 * RELEASE barrier so that other tasks can ACQUIRE it.
 	 */
-	if (cmpxchg_release(&inode->i_crypt_info, NULL, crypt_info) == NULL) {
+	if (cmpxchg_release(info_ptr, NULL, crypt_info) == NULL) {
 		/*
 		 * We won the race and set ->i_crypt_info to our crypt_info.
 		 * Now link it into the master key's inode list.
@@ -755,7 +769,7 @@ static bool get_parent_policy_and_nonce(struct inode *inode,
  *		       %false unless the operation being performed is needed in
  *		       order for files (or directories) to be deleted.
  *
- * Set up ->i_crypt_info, if it hasn't already been done.
+ * Set up inode->i_crypt_info, if it hasn't already been done.
  *
  * Note: unless ->i_crypt_info is already set, this isn't %GFP_NOFS-safe.  So
  * generally this shouldn't be called from within a filesystem transaction.
@@ -767,7 +781,7 @@ static bool get_parent_policy_and_nonce(struct inode *inode,
 int fscrypt_get_encryption_info(struct inode *inode, bool allow_unsupported)
 {
 	int res;
-	union fscrypt_context ctx = { 0 };
+	union fscrypt_context ctx;
 	union fscrypt_policy policy;
 	const u8 *nonce;
 	u8 nonce_bytes[FSCRYPT_FILE_NONCE_SIZE];
@@ -778,7 +792,7 @@ int fscrypt_get_encryption_info(struct inode *inode, bool allow_unsupported)
 	if (fscrypt_uses_extent_encryption(inode)) {
 		/*
 		 * Nothing will be encrypted with this info, so we can borrow
-		 * the parent (dir) inode's policy and use a zero nonce.
+		 * the parent (dir) inode's policy and nonce.
 		 */
 		if (!get_parent_policy_and_nonce(inode, &policy, nonce_bytes))
 			return 0;
@@ -800,6 +814,7 @@ int fscrypt_get_encryption_info(struct inode *inode, bool allow_unsupported)
 				     "Unrecognized or corrupt encryption context");
 			return res;
 		}
+		nonce = fscrypt_context_nonce(&ctx);
 	}
 
 	if (!fscrypt_supported_policy(&policy, inode)) {
@@ -808,10 +823,10 @@ int fscrypt_get_encryption_info(struct inode *inode, bool allow_unsupported)
 		return -EINVAL;
 	}
 
-	res = fscrypt_setup_encryption_info(inode, &policy,
-					    fscrypt_context_nonce(&ctx),
+	res = fscrypt_setup_encryption_info(inode, &policy, nonce,
 					    IS_CASEFOLDED(inode) &&
-					    S_ISDIR(inode->i_mode));
+					    S_ISDIR(inode->i_mode),
+					    NULL);
 
 	if (res == -ENOPKG && allow_unsupported) /* Algorithm unavailable? */
 		res = 0;
@@ -845,7 +860,8 @@ int fscrypt_prepare_new_inode(struct inode *dir, struct inode *inode,
 			      bool *encrypt_ret)
 {
 	const union fscrypt_policy *policy;
-	u8 nonce[FSCRYPT_FILE_NONCE_SIZE];
+	u8 nonce_bytes[FSCRYPT_FILE_NONCE_SIZE];
+	const u8 *nonce;
 
 	policy = fscrypt_policy_to_inherit(dir);
 	if (policy == NULL)
@@ -867,10 +883,17 @@ int fscrypt_prepare_new_inode(struct inode *dir, struct inode *inode,
 
 	*encrypt_ret = true;
 
-	get_random_bytes(nonce, FSCRYPT_FILE_NONCE_SIZE);
+	if (fscrypt_uses_extent_encryption(inode)) {
+		nonce = dir->i_crypt_info->ci_nonce;
+	} else {
+		get_random_bytes(nonce_bytes, FSCRYPT_FILE_NONCE_SIZE);
+		nonce = nonce_bytes;
+	}
+
 	return fscrypt_setup_encryption_info(inode, policy, nonce,
 					     IS_CASEFOLDED(dir) &&
-					     S_ISDIR(inode->i_mode));
+					     S_ISDIR(inode->i_mode),
+					     NULL);
 }
 EXPORT_SYMBOL_GPL(fscrypt_prepare_new_inode);
 
-- 
2.41.0

[PATCH v3 07/16] fscrypt: use an optional ino equivalent for per-extent infos

[Sweet Tea Dorminy]

Since per-extent infos are not tied to inodes, an ino-based policy
cannot access the inode's i_ino to get the necessary information.
Instead, this adds an optional fscrypt_operation pointer to get the ino
equivalent for an extent, adds a wrapper to get the ino for an info, and
uses this wrapper everywhere where the ci's inode's i_ino is currently
accessed.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/fscrypt_private.h | 18 ++++++++++++++++++
 fs/crypto/keyring.c         |  8 ++++----
 fs/crypto/keysetup.c        |  6 +++---
 include/linux/fscrypt.h     |  9 +++++++++
 4 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 1244797cd8a9..4fe79b774f1f 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -332,6 +332,24 @@ static inline bool fscrypt_uses_extent_encryption(const struct inode *inode)
 	return false;
 }
 
+/**
+ * fscrypt_get_info_ino() - get the ino or ino equivalent for an info
+ *
+ * @ci: the fscrypt_info in question
+ *
+ * Return: For inode-based encryption, this will return the info's inode's ino.
+ * For extent-based encryption, this will return the extent's ino equivalent
+ * or 0 if it is not implemented.
+ */
+static inline u64 fscrypt_get_info_ino(const struct fscrypt_info *ci)
+{
+	if (ci->ci_inode)
+		return ci->ci_inode->i_ino;
+	if (!ci->ci_sb->s_cop->get_extent_ino_equivalent)
+		return 0;
+	return ci->ci_sb->s_cop->get_extent_ino_equivalent(ci->ci_info_ptr);
+}
+
 /* crypto.c */
 extern struct kmem_cache *fscrypt_info_cachep;
 int fscrypt_initialize(struct super_block *sb);
diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index 7cbb1fd872ac..53e37b8a822c 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -914,12 +914,12 @@ static int check_for_busy_inodes(struct super_block *sb,
 	}
 
 	{
-		/* select an example file to show for debugging purposes */
-		struct inode *inode =
+		/* select an example info to show for debugging purposes */
+		struct fscrypt_info *ci =
 			list_first_entry(&mk->mk_decrypted_inodes,
 					 struct fscrypt_info,
-					 ci_master_key_link)->ci_inode;
-		ino = inode->i_ino;
+					 ci_master_key_link);
+		ino = fscrypt_get_info_ino(ci);
 	}
 	spin_unlock(&mk->mk_decrypted_inodes_lock);
 
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index c72f9015ed35..32e62cc57708 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -380,10 +380,10 @@ int fscrypt_derive_dirhash_key(struct fscrypt_info *ci,
 void fscrypt_hash_inode_number(struct fscrypt_info *ci,
 			       const struct fscrypt_master_key *mk)
 {
-	WARN_ON_ONCE(ci->ci_inode->i_ino == 0);
+	WARN_ON_ONCE(fscrypt_get_info_ino(ci) == 0);
 	WARN_ON_ONCE(!mk->mk_ino_hash_key_initialized);
 
-	ci->ci_hashed_ino = (u32)siphash_1u64(ci->ci_inode->i_ino,
+	ci->ci_hashed_ino = (u32)siphash_1u64(fscrypt_get_info_ino(ci),
 					      &mk->mk_ino_hash_key);
 }
 
@@ -705,7 +705,7 @@ fscrypt_setup_encryption_info(struct inode *inode,
 		if (res)
 			goto out;
 
-		if (inode->i_ino)
+		if (fscrypt_get_info_ino(crypt_info))
 			fscrypt_hash_inode_number(crypt_info, mk);
 	}
 
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index c895b12737a1..2a64e7a71a53 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -160,6 +160,15 @@ struct fscrypt_operations {
 	void (*get_ino_and_lblk_bits)(struct super_block *sb,
 				      int *ino_bits_ret, int *lblk_bits_ret);
 
+	/*
+	 * Get the inode number equivalent for filesystems using per-extent
+	 * encryption keys.
+	 *
+	 * This function only needs to be implemented if support for one of the
+	 * FSCRYPT_POLICY_FLAG_IV_INO_* flags is needed.
+	 */
+	u64 (*get_extent_ino_equivalent)(struct fscrypt_info **info_ptr);
+
 	/*
 	 * Return an array of pointers to the block devices to which the
 	 * filesystem may write encrypted file contents, NULL if the filesystem
-- 
2.41.0

Re: [PATCH v3 07/16] fscrypt: use an optional ino equivalent for per-extent infos

[Josef Bacik]

On Tue, Aug 08, 2023 at 01:08:24PM -0400, Sweet Tea Dorminy wrote:
> Since per-extent infos are not tied to inodes, an ino-based policy
> cannot access the inode's i_ino to get the necessary information.
> Instead, this adds an optional fscrypt_operation pointer to get the ino
> equivalent for an extent, adds a wrapper to get the ino for an info, and
> uses this wrapper everywhere where the ci's inode's i_ino is currently
> accessed.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>  fs/crypto/fscrypt_private.h | 18 ++++++++++++++++++
>  fs/crypto/keyring.c         |  8 ++++----
>  fs/crypto/keysetup.c        |  6 +++---
>  include/linux/fscrypt.h     |  9 +++++++++
>  4 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 1244797cd8a9..4fe79b774f1f 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -332,6 +332,24 @@ static inline bool fscrypt_uses_extent_encryption(const struct inode *inode)
>  	return false;
>  }
>  
> +/**
> + * fscrypt_get_info_ino() - get the ino or ino equivalent for an info
> + *
> + * @ci: the fscrypt_info in question
> + *
> + * Return: For inode-based encryption, this will return the info's inode's ino.
> + * For extent-based encryption, this will return the extent's ino equivalent
> + * or 0 if it is not implemented.
> + */
> +static inline u64 fscrypt_get_info_ino(const struct fscrypt_info *ci)
> +{
> +	if (ci->ci_inode)
> +		return ci->ci_inode->i_ino;
> +	if (!ci->ci_sb->s_cop->get_extent_ino_equivalent)
> +		return 0;
> +	return ci->ci_sb->s_cop->get_extent_ino_equivalent(ci->ci_info_ptr);
> +}
> +
>  /* crypto.c */
>  extern struct kmem_cache *fscrypt_info_cachep;
>  int fscrypt_initialize(struct super_block *sb);
> diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
> index 7cbb1fd872ac..53e37b8a822c 100644
> --- a/fs/crypto/keyring.c
> +++ b/fs/crypto/keyring.c
> @@ -914,12 +914,12 @@ static int check_for_busy_inodes(struct super_block *sb,
>  	}
>  
>  	{
> -		/* select an example file to show for debugging purposes */
> -		struct inode *inode =
> +		/* select an example info to show for debugging purposes */
> +		struct fscrypt_info *ci =
>  			list_first_entry(&mk->mk_decrypted_inodes,
>  					 struct fscrypt_info,
> -					 ci_master_key_link)->ci_inode;
> -		ino = inode->i_ino;
> +					 ci_master_key_link);
> +		ino = fscrypt_get_info_ino(ci);
>  	}
>  	spin_unlock(&mk->mk_decrypted_inodes_lock);
>  
> diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> index c72f9015ed35..32e62cc57708 100644
> --- a/fs/crypto/keysetup.c
> +++ b/fs/crypto/keysetup.c
> @@ -380,10 +380,10 @@ int fscrypt_derive_dirhash_key(struct fscrypt_info *ci,
>  void fscrypt_hash_inode_number(struct fscrypt_info *ci,
>  			       const struct fscrypt_master_key *mk)
>  {
> -	WARN_ON_ONCE(ci->ci_inode->i_ino == 0);
> +	WARN_ON_ONCE(fscrypt_get_info_ino(ci) == 0);
>  	WARN_ON_ONCE(!mk->mk_ino_hash_key_initialized);
>  
> -	ci->ci_hashed_ino = (u32)siphash_1u64(ci->ci_inode->i_ino,
> +	ci->ci_hashed_ino = (u32)siphash_1u64(fscrypt_get_info_ino(ci),
>  					      &mk->mk_ino_hash_key);
>  }
>  
> @@ -705,7 +705,7 @@ fscrypt_setup_encryption_info(struct inode *inode,
>  		if (res)
>  			goto out;
>  
> -		if (inode->i_ino)
> +		if (fscrypt_get_info_ino(crypt_info))
>  			fscrypt_hash_inode_number(crypt_info, mk);
>  	}
>  
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index c895b12737a1..2a64e7a71a53 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -160,6 +160,15 @@ struct fscrypt_operations {
>  	void (*get_ino_and_lblk_bits)(struct super_block *sb,
>  				      int *ino_bits_ret, int *lblk_bits_ret);
>  
> +	/*
> +	 * Get the inode number equivalent for filesystems using per-extent
> +	 * encryption keys.
> +	 *
> +	 * This function only needs to be implemented if support for one of the
> +	 * FSCRYPT_POLICY_FLAG_IV_INO_* flags is needed.
> +	 */
> +	u64 (*get_extent_ino_equivalent)(struct fscrypt_info **info_ptr);
> +

I went and looked at your tree and nobody actually uses this.  I understand
wanting to add something for future expansion, but we're just sort of adding
things and hoping they'll be useful one day.  I think it's best to leave this
off and if somebody needs it they can add it later.  Thanks,

Josef

[PATCH v3 08/16] fscrypt: move function call warning of busy inodes

[Sweet Tea Dorminy]

Extent encryption will want to attempt to evict inodes, and not warn of
busy ones, before removing the key instead of after as it is at present.
Therefore pull the call for check_for_busy_inodes() out of
try_to_lock_encrypted_files() into its only callsite.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/keyring.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index 53e37b8a822c..9d00cadb19ee 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -938,8 +938,7 @@ static int check_for_busy_inodes(struct super_block *sb,
 static int try_to_lock_encrypted_files(struct super_block *sb,
 				       struct fscrypt_master_key *mk)
 {
-	int err1;
-	int err2;
+	int err;
 
 	/*
 	 * An inode can't be evicted while it is dirty or has dirty pages.
@@ -951,7 +950,7 @@ static int try_to_lock_encrypted_files(struct super_block *sb,
 	 * already call sync_filesystem() via sys_syncfs() or sys_sync().
 	 */
 	down_read(&sb->s_umount);
-	err1 = sync_filesystem(sb);
+	err = sync_filesystem(sb);
 	up_read(&sb->s_umount);
 	/* If a sync error occurs, still try to evict as much as possible. */
 
@@ -963,16 +962,7 @@ static int try_to_lock_encrypted_files(struct super_block *sb,
 	 */
 	evict_dentries_for_decrypted_inodes(mk);
 
-	/*
-	 * evict_dentries_for_decrypted_inodes() already iput() each inode in
-	 * the list; any inodes for which that dropped the last reference will
-	 * have been evicted due to fscrypt_drop_inode() detecting the key
-	 * removal and telling the VFS to evict the inode.  So to finish, we
-	 * just need to check whether any inodes couldn't be evicted.
-	 */
-	err2 = check_for_busy_inodes(sb, mk);
-
-	return err1 ?: err2;
+	return err;
 }
 
 /*
@@ -1064,8 +1054,17 @@ static int do_remove_key(struct file *filp, void __user *_uarg, bool all_users)
 	up_write(&mk->mk_sem);
 
 	if (inodes_remain) {
+		int err2;
 		/* Some inodes still reference this key; try to evict them. */
 		err = try_to_lock_encrypted_files(sb, mk);
+		/* We already tried to iput() each inode referencing this key
+		 * which would cause the inode to be evicted if that was the
+		 * last reference (since fscrypt_drop_inode() would see the
+		 * key removal). So the only remaining inodes referencing this
+		 * key are still busy and couldn't be evicted; check for them.
+		 */
+		err2 = check_for_busy_inodes(sb, mk);
+		err = err ?: err2;
 		if (err == -EBUSY) {
 			status_flags |=
 				FSCRYPT_KEY_REMOVAL_STATUS_FLAG_FILES_BUSY;
-- 
2.41.0

[PATCH v3 09/16] fscrypt: revamp key removal for extent encryption

[Sweet Tea Dorminy]

Currently, for inode encryption, once an inode is open IO will not fail
due to lack of key until the inode is closed. Even if the key is
removed, open inodes will continue to use the key.

For extent encryption, it's a little harder, since the extent may not be
createdi/loaded until well after the REMOVE_KEY ioctl is called. To be
as similar to inode based encryption as plausible, this changes key
removal to be 'soft' for extent-based encryption, allowing new extents
to use keys which were in use by open inodes at the time of removal;
this hopefully follows the discussion at [1].

[1] https://lore.kernel.org/linux-fscrypt/248eac32-96cc-eb2e-85da-422a8d75a376@dorminy.me/T/#m48f43837cf98e0212de2e70aa6435320e3532d6e

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/fscrypt_private.h | 37 ++++++++++++++++++++++++++++++-----
 fs/crypto/keyring.c         | 39 +++++++++++++++++++++++++++----------
 fs/crypto/keysetup.c        | 35 ++++++++++++++++++++++++++++++++-
 3 files changed, 95 insertions(+), 16 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 4fe79b774f1f..21e4e138cfcc 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -332,6 +332,21 @@ static inline bool fscrypt_uses_extent_encryption(const struct inode *inode)
 	return false;
 }
 
+/**
+ * fscrypt_fs_uses_extent_encryption() -- whether a filesystem uses per-extent
+ *				          encryption
+ *
+ * @sb: the superblock of the filesystem in question
+ *
+ * Return: true if the fs uses per-extent fscrypt_infos, false otherwise
+ */
+static inline bool
+fscrypt_fs_uses_extent_encryption(const struct super_block *sb)
+{
+	// No filesystems currently use per-extent infos
+	return false;
+}
+
 /**
  * fscrypt_get_info_ino() - get the ino or ino equivalent for an info
  *
@@ -556,11 +571,14 @@ struct fscrypt_master_key {
 
 	/*
 	 * The secret key material.  After FS_IOC_REMOVE_ENCRYPTION_KEY is
-	 * executed, this is wiped and no new inodes can be unlocked with this
-	 * key; however, there may still be inodes in ->mk_decrypted_inodes
-	 * which could not be evicted.  As long as some inodes still remain,
-	 * FS_IOC_REMOVE_ENCRYPTION_KEY can be retried, or
-	 * FS_IOC_ADD_ENCRYPTION_KEY can add the secret again.
+	 * executed, no new inodes can be unlocked with this key; however,
+	 * there may still be inodes in ->mk_decrypted_inodes which could not
+	 * be evicted. For inode-based encryption, the secret is wiped; for
+	 * extent-based encryption, the secret is preserved while inodes still
+	 * reference it, as they may need to create new extents using the
+	 * secret to service IO; @soft_deleted is set to true then. As long as
+	 * some inodes still remain, FS_IOC_REMOVE_ENCRYPTION_KEY can be
+	 * retried, or FS_IOC_ADD_ENCRYPTION_KEY can add the secret again.
 	 *
 	 * While ->mk_secret is present, one ref in ->mk_active_refs is held.
 	 *
@@ -599,6 +617,13 @@ struct fscrypt_master_key {
 	struct list_head	mk_decrypted_inodes;
 	spinlock_t		mk_decrypted_inodes_lock;
 
+	/*
+	 * Whether the key is unavailable to new inodes, but still available
+	 * to new extents within decrypted inodes. Protected by ->mk_sem, except
+	 * for race-okay access in fscrypt_drop_inode().
+	 */
+	bool			mk_soft_deleted;
+
 	/*
 	 * Per-mode encryption keys for the various types of encryption policies
 	 * that use them.  Allocated and derived on-demand.
@@ -626,6 +651,8 @@ is_master_key_secret_present(const struct fscrypt_master_key_secret *secret)
 	return READ_ONCE(secret->size) != 0;
 }
 
+void fscrypt_wipe_master_key_secret(struct fscrypt_master_key_secret *secret);
+
 static inline const char *master_key_spec_type(
 				const struct fscrypt_key_specifier *spec)
 {
diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index 9d00cadb19ee..feca4a8410bb 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -38,7 +38,7 @@ struct fscrypt_keyring {
 	struct hlist_head key_hashtable[128];
 };
 
-static void wipe_master_key_secret(struct fscrypt_master_key_secret *secret)
+void fscrypt_wipe_master_key_secret(struct fscrypt_master_key_secret *secret)
 {
 	fscrypt_destroy_hkdf(&secret->hkdf);
 	memzero_explicit(secret, sizeof(*secret));
@@ -239,8 +239,9 @@ void fscrypt_destroy_keyring(struct super_block *sb)
 			 */
 			WARN_ON_ONCE(refcount_read(&mk->mk_active_refs) != 1);
 			WARN_ON_ONCE(refcount_read(&mk->mk_struct_refs) != 1);
-			WARN_ON_ONCE(!is_master_key_secret_present(&mk->mk_secret));
-			wipe_master_key_secret(&mk->mk_secret);
+			WARN_ON_ONCE(!mk->mk_soft_deleted &&
+				     !is_master_key_secret_present(&mk->mk_secret));
+			fscrypt_wipe_master_key_secret(&mk->mk_secret);
 			fscrypt_put_master_key_activeref(sb, mk);
 		}
 	}
@@ -485,6 +486,8 @@ static int add_existing_master_key(struct fscrypt_master_key *mk,
 		move_master_key_secret(&mk->mk_secret, secret);
 	}
 
+	mk->mk_soft_deleted = false;
+
 	return 0;
 }
 
@@ -738,7 +741,7 @@ int fscrypt_ioctl_add_key(struct file *filp, void __user *_uarg)
 		goto out_wipe_secret;
 	err = 0;
 out_wipe_secret:
-	wipe_master_key_secret(&secret);
+	fscrypt_wipe_master_key_secret(&secret);
 	return err;
 }
 EXPORT_SYMBOL_GPL(fscrypt_ioctl_add_key);
@@ -770,7 +773,7 @@ int fscrypt_get_test_dummy_key_identifier(
 				  NULL, 0, key_identifier,
 				  FSCRYPT_KEY_IDENTIFIER_SIZE);
 out:
-	wipe_master_key_secret(&secret);
+	fscrypt_wipe_master_key_secret(&secret);
 	return err;
 }
 
@@ -794,7 +797,7 @@ int fscrypt_add_test_dummy_key(struct super_block *sb,
 
 	fscrypt_get_test_dummy_secret(&secret);
 	err = add_master_key(sb, &secret, key_spec);
-	wipe_master_key_secret(&secret);
+	fscrypt_wipe_master_key_secret(&secret);
 	return err;
 }
 
@@ -1017,6 +1020,12 @@ static int do_remove_key(struct file *filp, void __user *_uarg, bool all_users)
 	mk = fscrypt_find_master_key(sb, &arg.key_spec);
 	if (!mk)
 		return -ENOKEY;
+
+	if (fscrypt_fs_uses_extent_encryption(sb)) {
+		/* Keep going even if this has an error. */
+		try_to_lock_encrypted_files(sb, mk);
+	}
+
 	down_write(&mk->mk_sem);
 
 	/* If relevant, remove current user's (or all users) claim to the key */
@@ -1043,13 +1052,23 @@ static int do_remove_key(struct file *filp, void __user *_uarg, bool all_users)
 		}
 	}
 
-	/* No user claims remaining.  Go ahead and wipe the secret. */
+	/* No user claims remaining. */
 	err = -ENOKEY;
-	if (is_master_key_secret_present(&mk->mk_secret)) {
-		wipe_master_key_secret(&mk->mk_secret);
+	if (fscrypt_fs_uses_extent_encryption(sb) && refcount_read(&mk->mk_active_refs) > 1) {
+		mk->mk_soft_deleted = true;
+		err = 0;
+	} else if (is_master_key_secret_present(&mk->mk_secret)) {
+		fscrypt_wipe_master_key_secret(&mk->mk_secret);
 		fscrypt_put_master_key_activeref(sb, mk);
 		err = 0;
+	} else if (mk->mk_soft_deleted) {
+		/*
+		 * Was soft deleted, but all inodes have stopped using it, and
+		 * the secret was wiped by the last one.
+		 */
+		err = 0;
 	}
+
 	inodes_remain = refcount_read(&mk->mk_active_refs) > 0;
 	up_write(&mk->mk_sem);
 
@@ -1149,7 +1168,7 @@ int fscrypt_ioctl_get_key_status(struct file *filp, void __user *uarg)
 	}
 	down_read(&mk->mk_sem);
 
-	if (!is_master_key_secret_present(&mk->mk_secret)) {
+	if (mk->mk_soft_deleted || !is_master_key_secret_present(&mk->mk_secret)) {
 		arg.status = refcount_read(&mk->mk_active_refs) > 0 ?
 			FSCRYPT_KEY_STATUS_INCOMPLETELY_REMOVED :
 			FSCRYPT_KEY_STATUS_ABSENT /* raced with full removal */;
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 32e62cc57708..51d3787fc964 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -576,6 +576,15 @@ static int find_and_lock_master_key(const struct fscrypt_info *ci,
 		goto out_release_key;
 	}
 
+	if (!ci->ci_info_ptr && mk->mk_soft_deleted) {
+		/*
+		 * This is an inode info, and only extent infos can use keys
+		 * that have been soft deleted
+		 */
+		err = -ENOKEY;
+		goto out_release_key;
+	}
+
 	*mk_ret = mk;
 	return 0;
 
@@ -606,6 +615,8 @@ static void put_crypt_info(struct fscrypt_info *ci)
 
 	mk = ci->ci_master_key;
 	if (mk) {
+		bool any_inodes;
+
 		/*
 		 * Remove this inode from the list of inodes that were unlocked
 		 * with the master key.  In addition, if we're removing the last
@@ -614,7 +625,28 @@ static void put_crypt_info(struct fscrypt_info *ci)
 		 */
 		spin_lock(&mk->mk_decrypted_inodes_lock);
 		list_del(&ci->ci_master_key_link);
+		any_inodes = list_empty(&mk->mk_decrypted_inodes);
 		spin_unlock(&mk->mk_decrypted_inodes_lock);
+		if (any_inodes) {
+			bool soft_deleted;
+			/* It might be that someone tried to remove this key,
+			 * but there were still inodes open that could need new
+			 * extents, which needed to be able to access the key
+			 * secret. But now this was the last reference. So we
+			 * can delete the key secret now. (We don't need to
+			 * check for new inodes on the decrypted_inode list
+			 * because once ->mk_soft_deleted is set, no new inode
+			 * can join the list.
+			 */
+			down_write(&mk->mk_sem);
+			soft_deleted = mk->mk_soft_deleted;
+			if (soft_deleted)
+				fscrypt_wipe_master_key_secret(&mk->mk_secret);
+			up_write(&mk->mk_sem);
+			if (soft_deleted)
+				fscrypt_put_master_key_activeref(ci->ci_sb, mk);
+		}
+
 		fscrypt_put_master_key_activeref(ci->ci_sb, mk);
 	}
 	memzero_explicit(ci, sizeof(*ci));
@@ -967,6 +999,7 @@ int fscrypt_drop_inode(struct inode *inode)
 	 * then the thread removing the key will either evict the inode itself
 	 * or will correctly detect that it wasn't evicted due to the race.
 	 */
-	return !is_master_key_secret_present(&ci->ci_master_key->mk_secret);
+	return READ_ONCE(ci->ci_master_key->mk_soft_deleted) ||
+		!is_master_key_secret_present(&ci->ci_master_key->mk_secret);
 }
 EXPORT_SYMBOL_GPL(fscrypt_drop_inode);
-- 
2.41.0

Re: [PATCH v3 09/16] fscrypt: revamp key removal for extent encryption

[Eric Biggers]

On Tue, Aug 08, 2023 at 01:08:26PM -0400, Sweet Tea Dorminy wrote:
> @@ -1017,6 +1020,12 @@ static int do_remove_key(struct file *filp, void __user *_uarg, bool all_users)
>  	mk = fscrypt_find_master_key(sb, &arg.key_spec);
>  	if (!mk)
>  		return -ENOKEY;
> +
> +	if (fscrypt_fs_uses_extent_encryption(sb)) {
> +		/* Keep going even if this has an error. */
> +		try_to_lock_encrypted_files(sb, mk);
> +	}

Why is this here?

> @@ -606,6 +615,8 @@ static void put_crypt_info(struct fscrypt_info *ci)
>  
>  	mk = ci->ci_master_key;
>  	if (mk) {
> +		bool any_inodes;
> +
>  		/*
>  		 * Remove this inode from the list of inodes that were unlocked
>  		 * with the master key.  In addition, if we're removing the last
> @@ -614,7 +625,28 @@ static void put_crypt_info(struct fscrypt_info *ci)
>  		 */
>  		spin_lock(&mk->mk_decrypted_inodes_lock);
>  		list_del(&ci->ci_master_key_link);
> +		any_inodes = list_empty(&mk->mk_decrypted_inodes);
>  		spin_unlock(&mk->mk_decrypted_inodes_lock);
> +		if (any_inodes) {
> +			bool soft_deleted;
> +			/* It might be that someone tried to remove this key,
> +			 * but there were still inodes open that could need new
> +			 * extents, which needed to be able to access the key
> +			 * secret. But now this was the last reference. So we
> +			 * can delete the key secret now. (We don't need to
> +			 * check for new inodes on the decrypted_inode list
> +			 * because once ->mk_soft_deleted is set, no new inode
> +			 * can join the list.
> +			 */
> +			down_write(&mk->mk_sem);
> +			soft_deleted = mk->mk_soft_deleted;
> +			if (soft_deleted)
> +				fscrypt_wipe_master_key_secret(&mk->mk_secret);
> +			up_write(&mk->mk_sem);
> +			if (soft_deleted)
> +				fscrypt_put_master_key_activeref(ci->ci_sb, mk);
> +		}
> +

What is all this for?  I'd have thought this would just use the existing
refcounting and no change would be needed here.

- Eric

[PATCH v3 10/16] fscrypt: add creation/usage/freeing of per-extent infos

[Sweet Tea Dorminy]

This change adds the superblock function pointer to get the info
corresponding to a specific block in an inode for a filesystem using
per-extent infos. It allows creating a info for a new extent and freeing
that info, and uses the extent's info if appropriate in encrypting
blocks of data.

It also makes sure that the return value of fscrypt_get_lblk_info() is
non-NULL before using it, since there's no longer a mechanical guarantee
that we'll never call fscrypt_get_lblk_info() without having the
relevant info loaded. We *oughtn't*, but we're not explicitly checking
that it's loaded before these points.

This change does not deal with saving and loading an extent's info, but
introduces the mechanics necessary therefore.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/crypto.c          |  2 +
 fs/crypto/fscrypt_private.h | 74 +++++++++++++++++++++----------------
 fs/crypto/inline_crypt.c    |  5 ++-
 fs/crypto/keysetup.c        | 54 +++++++++++++++++++++++++++
 include/linux/fscrypt.h     | 39 +++++++++++++++++++
 5 files changed, 141 insertions(+), 33 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index d75f1b3f5795..0f0c721e40fe 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -113,6 +113,8 @@ int fscrypt_crypt_block(const struct inode *inode, fscrypt_direction_t rw,
 	struct crypto_skcipher *tfm = ci->ci_enc_key->tfm;
 	int res = 0;
 
+	if (!ci)
+		return -EINVAL;
 	if (WARN_ON_ONCE(len <= 0))
 		return -EINVAL;
 	if (WARN_ON_ONCE(len % FSCRYPT_CONTENTS_ALIGNMENT != 0))
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 21e4e138cfcc..c6bf0bd0259a 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -287,31 +287,17 @@ typedef enum {
 } fscrypt_direction_t;
 
 /**
- * fscrypt_get_lblk_info() - get the fscrypt_info to crypt a particular block
+ * fscrypt_fs_uses_extent_encryption() -- whether a filesystem uses per-extent
+ *				          encryption
  *
- * @inode:      the inode to which the block belongs
- * @lblk:       the offset of the block within the file which the inode
- *              references
- * @offset:     a pointer to return the offset of the block from the first block
- *              that the info covers. For inode-based encryption, this will
- *              always be @lblk; for extent-based encryption, this will be in
- *              the range [0, lblk]. Can be NULL
- * @extent_len: a pointer to return the minimum number of lblks starting at
- *              this offset which also belong to the same fscrypt_info. Can be
- *              NULL
+ * @sb: the superblock of the filesystem in question
  *
- * Return: the appropriate fscrypt_info if there is one, else NULL.
+ * Return: true if the fs uses per-extent fscrypt_infos, false otherwise
  */
-static inline struct fscrypt_info *
-fscrypt_get_lblk_info(const struct inode *inode, u64 lblk, u64 *offset,
-		      u64 *extent_len)
+static inline bool
+fscrypt_fs_uses_extent_encryption(const struct super_block *sb)
 {
-	if (offset)
-		*offset = lblk;
-	if (extent_len)
-		*extent_len = U64_MAX;
-
-	return inode->i_crypt_info;
+	return !!sb->s_cop->get_extent_info;
 }
 
 /**
@@ -324,27 +310,51 @@ fscrypt_get_lblk_info(const struct inode *inode, u64 lblk, u64 *offset,
  */
 static inline bool fscrypt_uses_extent_encryption(const struct inode *inode)
 {
-	/* Non-regular files don't have extents */
+	/* Non-regular files don't have extents. */
 	if (!S_ISREG(inode->i_mode))
 		return false;
 
-	/* No filesystems currently use per-extent infos */
-	return false;
+	return fscrypt_fs_uses_extent_encryption(inode->i_sb);
 }
 
+
 /**
- * fscrypt_fs_uses_extent_encryption() -- whether a filesystem uses per-extent
- *				          encryption
+ * fscrypt_get_lblk_info() - get the fscrypt_info to crypt a particular block
  *
- * @sb: the superblock of the filesystem in question
+ * @inode:      the inode to which the block belongs
+ * @lblk:       the offset of the block within the file which the inode
+ *              references
+ * @offset:     a pointer to return the offset of the block from the first block
+ *              that the info covers. For inode-based encryption, this will
+ *              always be @lblk; for extent-based encryption, this will be in
+ *              the range [0, lblk]. Can be NULL
+ * @extent_len: a pointer to return the minimum number of lblks starting at
+ *              this offset which also belong to the same fscrypt_info. Can be
+ *              NULL
  *
- * Return: true if the fs uses per-extent fscrypt_infos, false otherwise
+ * Return: the appropriate fscrypt_info if there is one, else NULL.
  */
-static inline bool
-fscrypt_fs_uses_extent_encryption(const struct super_block *sb)
+static inline struct fscrypt_info *
+fscrypt_get_lblk_info(const struct inode *inode, u64 lblk, u64 *offset,
+		      u64 *extent_len)
 {
-	// No filesystems currently use per-extent infos
-	return false;
+	if (fscrypt_uses_extent_encryption(inode)) {
+		struct fscrypt_info *info;
+		int res;
+
+		res = inode->i_sb->s_cop->get_extent_info(inode, lblk, &info,
+							  offset, extent_len);
+		if (res == 0)
+			return info;
+		return NULL;
+	}
+
+	if (offset)
+		*offset = lblk;
+	if (extent_len)
+		*extent_len = U64_MAX;
+
+	return inode->i_crypt_info;
 }
 
 /**
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index 260152d5e673..76274b736e1a 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -271,7 +271,10 @@ void fscrypt_set_bio_crypt_ctx(struct bio *bio, const struct inode *inode,
 
 	if (!fscrypt_inode_uses_inline_crypto(inode))
 		return;
+
 	ci = fscrypt_get_lblk_info(inode, first_lblk, &ci_offset, NULL);
+	if (!ci)
+		return;
 
 	fscrypt_generate_dun(ci, ci_offset, dun);
 	bio_crypt_set_ctx(bio, ci->ci_enc_key->blk_key, dun, gfp_mask);
@@ -364,7 +367,7 @@ bool fscrypt_mergeable_bio(struct bio *bio, const struct inode *inode,
 	 * uses the same pointer.  I.e., there's currently no need to support
 	 * merging requests where the keys are the same but the pointers differ.
 	 */
-	if (bc->bc_key != ci->ci_enc_key->blk_key)
+	if (!ci || bc->bc_key != ci->ci_enc_key->blk_key)
 		return false;
 
 	fscrypt_generate_dun(ci, ci_offset, next_dun);
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 51d3787fc964..c4ec042ca892 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -929,6 +929,60 @@ int fscrypt_prepare_new_inode(struct inode *dir, struct inode *inode,
 }
 EXPORT_SYMBOL_GPL(fscrypt_prepare_new_inode);
 
+/**
+ * fscrypt_prepare_new_extent() - set up the fscrypt_info for a new extent
+ * @inode: the inode to which the extent belongs
+ * @info_ptr: a pointer to return the extent's fscrypt_info into. Should be
+ *	      a pointer to a member of the extent struct, as it will be passed
+ *	      back to the filesystem if key removal demands removal of the
+ *	      info from the extent
+ * @encrypt_ret: (output) set to %true if the new inode will be encrypted
+ *
+ * If the extent is part of an encrypted inode, set up its fscrypt_info in
+ * preparation for encrypting data and set *encrypt_ret=true.
+ *
+ * This isn't %GFP_NOFS-safe, and therefore it should be called before starting
+ * any filesystem transaction to create the inode.
+ *
+ * This doesn't persist the new inode's encryption context.  That still needs to
+ * be done later by calling fscrypt_set_context().
+ *
+ * Return: 0 on success, -ENOKEY if the encryption key is missing, or another
+ *	   -errno code
+ */
+int fscrypt_prepare_new_extent(struct inode *inode,
+			       struct fscrypt_info **info_ptr)
+{
+	const union fscrypt_policy *policy;
+	u8 nonce[FSCRYPT_FILE_NONCE_SIZE];
+
+	policy = fscrypt_policy_to_inherit(inode);
+	if (policy == NULL)
+		return 0;
+	if (IS_ERR(policy))
+		return PTR_ERR(policy);
+
+	/* Only regular files can have extents.  */
+	if (WARN_ON_ONCE(!S_ISREG(inode->i_mode)))
+		return -EINVAL;
+
+	get_random_bytes(nonce, FSCRYPT_FILE_NONCE_SIZE);
+	return fscrypt_setup_encryption_info(inode, policy, nonce,
+					     false, info_ptr);
+}
+EXPORT_SYMBOL_GPL(fscrypt_prepare_new_extent);
+
+/**
+ * fscrypt_free_extent_info() - free an extent's fscrypt_info
+ * @info_ptr: a pointer containing the extent's fscrypt_info pointer.
+ */
+void fscrypt_free_extent_info(struct fscrypt_info **info_ptr)
+{
+	put_crypt_info(*info_ptr);
+	*info_ptr = NULL;
+}
+EXPORT_SYMBOL_GPL(fscrypt_free_extent_info);
+
 /**
  * fscrypt_put_encryption_info() - free most of an inode's fscrypt data
  * @inode: an inode being evicted
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 2a64e7a71a53..e39165fbed41 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -113,6 +113,29 @@ struct fscrypt_operations {
 	int (*set_context)(struct inode *inode, const void *ctx, size_t len,
 			   void *fs_data);
 
+	/*
+	 * Get the fscrypt_info for the given inode at the given block, for
+	 * extent-based encryption only.
+	 *
+	 * @inode: the inode in question
+	 * @lblk: the logical block number in question
+	 * @ci: a pointer to return the fscrypt_info
+	 * @offset: a pointer to return the offset of @lblk into the extent,
+	 *          in blocks (may be NULL)
+	 * @extent_len: a pointer to return the number of blocks in this extent
+	 *              starting at this point (may be NULL)
+	 *
+	 * May cause the filesystem to allocate memory, which the filesystem
+	 * must do with %GFP_NOFS, including calls into fscrypt to create or
+	 * load an fscrypt_info.
+	 *
+	 * Return: 0 if an extent is found with an info, -ENODATA if the key is
+	 *         unavailable, or another -errno.
+	 */
+	int (*get_extent_info)(const struct inode *inode, u64 lblk,
+			       struct fscrypt_info **ci, u64 *offset,
+			       u64 *extent_len);
+
 	/*
 	 * Get the dummy fscrypt policy in use on the filesystem (if any).
 	 *
@@ -339,6 +362,10 @@ void fscrypt_put_encryption_info(struct inode *inode);
 void fscrypt_free_inode(struct inode *inode);
 int fscrypt_drop_inode(struct inode *inode);
 
+int fscrypt_prepare_new_extent(struct inode *inode,
+			       struct fscrypt_info **info_ptr);
+void fscrypt_free_extent_info(struct fscrypt_info **info_ptr);
+
 /* fname.c */
 int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname,
 			  u8 *out, unsigned int olen);
@@ -600,6 +627,18 @@ static inline int fscrypt_drop_inode(struct inode *inode)
 	return 0;
 }
 
+static inline int fscrypt_prepare_new_extent(struct inode *inode,
+					     struct fscrypt_info **info_ptr)
+{
+	if (IS_ENCRYPTED(inode))
+		return -EOPNOTSUPP;
+	return 0;
+}
+
+static inline void fscrypt_free_extent_info(struct fscrypt_info **info_ptr)
+{
+}
+
  /* fname.c */
 static inline int fscrypt_setup_filename(struct inode *dir,
 					 const struct qstr *iname,
-- 
2.41.0

[PATCH v3 11/16] fscrypt: allow load/save of extent contexts

[Sweet Tea Dorminy]

The other half of using per-extent infos is saving and loading them from
disk.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/keysetup.c    | 49 +++++++++++++++++++++++++++++++++++++++++
 fs/crypto/policy.c      | 20 +++++++++++++++++
 include/linux/fscrypt.h | 17 ++++++++++++++
 3 files changed, 86 insertions(+)

diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index c4ec042ca892..0076c7f2af3d 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -983,6 +983,55 @@ void fscrypt_free_extent_info(struct fscrypt_info **info_ptr)
 }
 EXPORT_SYMBOL_GPL(fscrypt_free_extent_info);
 
+/**
+ * fscrypt_load_extent_info() - set up a preexisting extent's fscrypt_info
+ * @inode: the inode to which the extent belongs. Must be encrypted.
+ * @buf: a buffer containing the extent's stored context
+ * @len: the length of the @ctx buffer
+ * @info_ptr: a pointer to return the extent's fscrypt_info into. Should be
+ *	      a pointer to a member of the extent struct, as it will be passed
+ *	      back to the filesystem if key removal demands removal of the
+ *	      info from the extent
+ *
+ * This is not %GFP_NOFS safe, so the caller is expected to call
+ * memalloc_nofs_save/restore() if appropriate.
+ *
+ * Return: 0 if successful, or -errno if it fails.
+ */
+int fscrypt_load_extent_info(struct inode *inode, void *buf, size_t len,
+			     struct fscrypt_info **info_ptr)
+{
+	int res;
+	union fscrypt_context ctx;
+	union fscrypt_policy policy;
+
+	if (!fscrypt_has_encryption_key(inode))
+		return -EINVAL;
+
+	memcpy(&ctx, buf, len);
+
+	res = fscrypt_policy_from_context(&policy, &ctx, len);
+	if (res) {
+		fscrypt_warn(inode,
+			     "Unrecognized or corrupt encryption context");
+		return res;
+	}
+
+	if (!fscrypt_supported_policy(&policy, inode))
+		return -EINVAL;
+
+	res = fscrypt_setup_encryption_info(inode, &policy,
+					    fscrypt_context_nonce(&ctx),
+					    IS_CASEFOLDED(inode) &&
+					    S_ISDIR(inode->i_mode),
+					    info_ptr);
+
+	if (res == -ENOPKG) /* Algorithm unavailable? */
+		res = 0;
+	return res;
+}
+EXPORT_SYMBOL_GPL(fscrypt_load_extent_info);
+
 /**
  * fscrypt_put_encryption_info() - free most of an inode's fscrypt data
  * @inode: an inode being evicted
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index f4456ecb3f87..9ecb01e49d33 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -762,6 +762,26 @@ int fscrypt_set_context(struct inode *inode, void *fs_data)
 }
 EXPORT_SYMBOL_GPL(fscrypt_set_context);
 
+/**
+ * fscrypt_set_extent_context() - Set the fscrypt context for an extent
+ * @ci: info from which to fetch policy and nonce
+ * @ctx: where context should be written
+ * @len: the size of ctx
+ *
+ * Given an fscrypt_info belonging to an extent (generated via
+ * fscrypt_prepare_new_extent()), generate a new context and write it to @ctx.
+ * len is checked to be at least FSCRYPT_SET_CONTEXT_MAX_SIZE bytes.
+ *
+ * Return: size of the resulting context or a negative error code.
+ */
+int fscrypt_set_extent_context(struct fscrypt_info *ci, void *ctx, size_t len)
+{
+	if (len < FSCRYPT_SET_CONTEXT_MAX_SIZE)
+		return -EINVAL;
+	return fscrypt_new_context(ctx, &ci->ci_policy, ci->ci_nonce);
+}
+EXPORT_SYMBOL_GPL(fscrypt_set_extent_context);
+
 /**
  * fscrypt_parse_test_dummy_encryption() - parse the test_dummy_encryption mount option
  * @param: the mount option
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index e39165fbed41..4ba624beea91 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -325,6 +325,8 @@ int fscrypt_ioctl_get_nonce(struct file *filp, void __user *arg);
 int fscrypt_has_permitted_context(struct inode *parent, struct inode *child);
 int fscrypt_context_for_new_inode(void *ctx, struct inode *inode);
 int fscrypt_set_context(struct inode *inode, void *fs_data);
+int fscrypt_set_extent_context(struct fscrypt_info *info, void *ctx,
+			       size_t len);
 
 struct fscrypt_dummy_policy {
 	const union fscrypt_policy *policy;
@@ -365,6 +367,8 @@ int fscrypt_drop_inode(struct inode *inode);
 int fscrypt_prepare_new_extent(struct inode *inode,
 			       struct fscrypt_info **info_ptr);
 void fscrypt_free_extent_info(struct fscrypt_info **info_ptr);
+int fscrypt_load_extent_info(struct inode *inode, void *buf, size_t len,
+			     struct fscrypt_info **info_ptr);
 
 /* fname.c */
 int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname,
@@ -541,6 +545,12 @@ static inline int fscrypt_set_context(struct inode *inode, void *fs_data)
 	return -EOPNOTSUPP;
 }
 
+static inline int fscrypt_set_extent_context(struct fscrypt_info *info,
+					     void *ctx, size_t len)
+{
+	return -EOPNOTSUPP;
+}
+
 struct fscrypt_dummy_policy {
 };
 
@@ -639,6 +649,13 @@ static inline void fscrypt_free_extent_info(struct fscrypt_info **info_ptr)
 {
 }
 
+static inline int fscrypt_load_extent_info(struct inode *inode, void *buf,
+					   size_t len,
+					   struct fscrypt_info **info_ptr)
+{
+	return -EOPNOTSUPP;
+}
+
  /* fname.c */
 static inline int fscrypt_setup_filename(struct inode *dir,
 					 const struct qstr *iname,
-- 
2.41.0

[PATCH v3 12/16] fscrypt: save session key credentials for extent infos

[Sweet Tea Dorminy]

For v1 encryption policies using per-session keys, the thread which
opens the inode and therefore initializes the encryption info is part of
the session, so it can get the key from the session keyring. However,
for extent encryption, the extent infos are likely loaded from a
different thread, which does not have access to the session keyring.
This change saves the credentials of the inode opening thread and reuses
those credentials temporarily when dealing with extent infos, allowing
finding the encryption key correctly.

v1 encryption policies using per-session keys should probably not exist
for new usages such as extent encryption, but this makes more tests
work without change; maybe the right answer is to disallow v1 session
keys plus extent encryption and deal with editing tests to not use v1
session encryption so much.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/fscrypt_private.h |  8 ++++++++
 fs/crypto/keysetup.c        | 14 ++++++++++++++
 fs/crypto/keysetup_v1.c     |  1 +
 3 files changed, 23 insertions(+)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index c6bf0bd0259a..cd29c71b4349 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -231,6 +231,14 @@ struct fscrypt_info {
 	 */
 	bool ci_inlinecrypt;
 #endif
+	/* Credential struct from the thread which created this info. This is
+	 * only used in v1 session keyrings with extent encryption; it allows
+	 * the thread creating extents for an inode to join the session
+	 * keyring temporarily, since otherwise the thread is usually part of
+	 * kernel writeback and therefore unrelated to the thread with the
+	 * right session key.
+	 */
+	struct cred *ci_session_creds;
 
 	/*
 	 * Encryption mode used for this inode.  It corresponds to either the
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 0076c7f2af3d..8d50716bdf11 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -649,6 +649,8 @@ static void put_crypt_info(struct fscrypt_info *ci)
 
 		fscrypt_put_master_key_activeref(ci->ci_sb, mk);
 	}
+	if (ci->ci_session_creds)
+		abort_creds(ci->ci_session_creds);
 	memzero_explicit(ci, sizeof(*ci));
 	kmem_cache_free(fscrypt_info_cachep, ci);
 }
@@ -665,6 +667,7 @@ fscrypt_setup_encryption_info(struct inode *inode,
 	struct fscrypt_master_key *mk = NULL;
 	int res;
 	bool info_for_extent = !!info_ptr;
+	const struct cred *creds = NULL;
 
 	if (!info_ptr)
 		info_ptr = &inode->i_crypt_info;
@@ -707,7 +710,18 @@ fscrypt_setup_encryption_info(struct inode *inode,
 	if (res)
 		goto out;
 
+	if (info_for_extent && inode->i_crypt_info->ci_session_creds) {
+		/*
+		 * The inode this is being created for is using a session key,
+		 * so we have to join this thread to that session temporarily
+		 * in order to be able to find the right key...
+		 */
+		creds = override_creds(inode->i_crypt_info->ci_session_creds);
+	}
+
 	res = fscrypt_setup_file_key(crypt_info, mk);
+	if (creds)
+		revert_creds(creds);
 	if (res)
 		goto out;
 
diff --git a/fs/crypto/keysetup_v1.c b/fs/crypto/keysetup_v1.c
index 41d317f08aeb..4f2be2377dfa 100644
--- a/fs/crypto/keysetup_v1.c
+++ b/fs/crypto/keysetup_v1.c
@@ -318,6 +318,7 @@ int fscrypt_setup_v1_file_key_via_subscribed_keyrings(struct fscrypt_info *ci)
 		return PTR_ERR(key);
 
 	err = fscrypt_setup_v1_file_key(ci, payload->raw);
+	ci->ci_session_creds = prepare_creds();
 	up_read(&key->sem);
 	key_put(key);
 	return err;
-- 
2.41.0

Re: [PATCH v3 12/16] fscrypt: save session key credentials for extent infos

[Eric Biggers]

On Tue, Aug 08, 2023 at 01:08:29PM -0400, Sweet Tea Dorminy wrote:
> For v1 encryption policies using per-session keys, the thread which
> opens the inode and therefore initializes the encryption info is part of
> the session, so it can get the key from the session keyring. However,
> for extent encryption, the extent infos are likely loaded from a
> different thread, which does not have access to the session keyring.
> This change saves the credentials of the inode opening thread and reuses
> those credentials temporarily when dealing with extent infos, allowing
> finding the encryption key correctly.
> 
> v1 encryption policies using per-session keys should probably not exist
> for new usages such as extent encryption, but this makes more tests
> work without change; maybe the right answer is to disallow v1 session
> keys plus extent encryption and deal with editing tests to not use v1
> session encryption so much.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

I don't understand why you would need to look up the master key for each extent.
The master key should just come from the inode, which the master key was already
looked up for when the file was opened.

- Eric

[PATCH v3 13/16] fscrypt: allow multiple extents to reference one info

[Sweet Tea Dorminy]

btrfs occasionally splits in-memory extents while holding a mutex. This
means we can't just copy the info, since setting up a new inlinecrypt
key requires taking a semaphore. Thus adding a mechanism to split
extents and merely take a new reference on the info is necessary.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/fscrypt_private.h |  5 +++++
 fs/crypto/keysetup.c        | 18 +++++++++++++++++-
 include/linux/fscrypt.h     |  2 ++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index cd29c71b4349..03be2c136c0e 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -287,6 +287,11 @@ struct fscrypt_info {
 
 	/* Hashed inode number.  Only set for IV_INO_LBLK_32 */
 	u32 ci_hashed_ino;
+
+	/* Reference count. Normally 1, unless a extent info is shared by
+	 * several virtual extents.
+	 */
+	refcount_t refs;
 };
 
 typedef enum {
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 8d50716bdf11..12c3851b7cd6 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -598,7 +598,7 @@ static void put_crypt_info(struct fscrypt_info *ci)
 {
 	struct fscrypt_master_key *mk;
 
-	if (!ci)
+	if (!ci || !refcount_dec_and_test(&ci->refs))
 		return;
 
 	if (ci->ci_enc_key) {
@@ -686,6 +686,7 @@ fscrypt_setup_encryption_info(struct inode *inode,
 	crypt_info->ci_inode = inode;
 	crypt_info->ci_sb = inode->i_sb;
 	crypt_info->ci_policy = *policy;
+	refcount_set(&crypt_info->refs, 1);
 	memcpy(crypt_info->ci_nonce, nonce, FSCRYPT_FILE_NONCE_SIZE);
 
 	mode = select_encryption_mode(&crypt_info->ci_policy, inode);
@@ -1046,6 +1047,21 @@ int fscrypt_load_extent_info(struct inode *inode, void *buf, size_t len,
 }
 EXPORT_SYMBOL_GPL(fscrypt_load_extent_info);
 
+/**
+ * fscrypt_get_extent_info_ref() - mark a second extent using the same info
+ * @info: the info to be used by another extent
+ *
+ * Sometimes, an existing extent must be split into multiple extents in memory.
+ * In such a case, this function allows multiple extents to use the same extent
+ * info without allocating or taking any lock, which is necessary in certain IO
+ * paths.
+ */
+void fscrypt_get_extent_info_ref(struct fscrypt_info *info)
+{
+	if (info)
+		refcount_inc(&info->refs);
+}
+
 /**
  * fscrypt_put_encryption_info() - free most of an inode's fscrypt data
  * @inode: an inode being evicted
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 4ba624beea91..b67054a2c965 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -370,6 +370,8 @@ void fscrypt_free_extent_info(struct fscrypt_info **info_ptr);
 int fscrypt_load_extent_info(struct inode *inode, void *buf, size_t len,
 			     struct fscrypt_info **info_ptr);
 
+void fscrypt_get_extent_info_ref(struct fscrypt_info *info);
+
 /* fname.c */
 int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname,
 			  u8 *out, unsigned int olen);
-- 
2.41.0

Re: [PATCH v3 13/16] fscrypt: allow multiple extents to reference one info

[Luís Henriques]

Sweet Tea Dorminy <sweettea-kernel@dorminy.me> writes:

> btrfs occasionally splits in-memory extents while holding a mutex. This
> means we can't just copy the info, since setting up a new inlinecrypt
> key requires taking a semaphore. Thus adding a mechanism to split
> extents and merely take a new reference on the info is necessary.
>
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>  fs/crypto/fscrypt_private.h |  5 +++++
>  fs/crypto/keysetup.c        | 18 +++++++++++++++++-
>  include/linux/fscrypt.h     |  2 ++
>  3 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index cd29c71b4349..03be2c136c0e 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -287,6 +287,11 @@ struct fscrypt_info {
>  
>  	/* Hashed inode number.  Only set for IV_INO_LBLK_32 */
>  	u32 ci_hashed_ino;
> +
> +	/* Reference count. Normally 1, unless a extent info is shared by
> +	 * several virtual extents.
> +	 */
> +	refcount_t refs;
>  };
>  
>  typedef enum {
> diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> index 8d50716bdf11..12c3851b7cd6 100644
> --- a/fs/crypto/keysetup.c
> +++ b/fs/crypto/keysetup.c
> @@ -598,7 +598,7 @@ static void put_crypt_info(struct fscrypt_info *ci)
>  {
>  	struct fscrypt_master_key *mk;
>  
> -	if (!ci)
> +	if (!ci || !refcount_dec_and_test(&ci->refs))
>  		return;
>  
>  	if (ci->ci_enc_key) {
> @@ -686,6 +686,7 @@ fscrypt_setup_encryption_info(struct inode *inode,
>  	crypt_info->ci_inode = inode;
>  	crypt_info->ci_sb = inode->i_sb;
>  	crypt_info->ci_policy = *policy;
> +	refcount_set(&crypt_info->refs, 1);
>  	memcpy(crypt_info->ci_nonce, nonce, FSCRYPT_FILE_NONCE_SIZE);
>  
>  	mode = select_encryption_mode(&crypt_info->ci_policy, inode);
> @@ -1046,6 +1047,21 @@ int fscrypt_load_extent_info(struct inode *inode, void *buf, size_t len,
>  }
>  EXPORT_SYMBOL_GPL(fscrypt_load_extent_info);
>  
> +/**
> + * fscrypt_get_extent_info_ref() - mark a second extent using the same info
> + * @info: the info to be used by another extent
> + *
> + * Sometimes, an existing extent must be split into multiple extents in memory.
> + * In such a case, this function allows multiple extents to use the same extent
> + * info without allocating or taking any lock, which is necessary in certain IO
> + * paths.
> + */
> +void fscrypt_get_extent_info_ref(struct fscrypt_info *info)
> +{
> +	if (info)
> +		refcount_inc(&info->refs);
> +}
> +

There's an EXPORT_SYMBOL_GPL() missing here, right?

Cheers,
-- 
Luís

> 
>  /**
>   * fscrypt_put_encryption_info() - free most of an inode's fscrypt data
>   * @inode: an inode being evicted
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index 4ba624beea91..b67054a2c965 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -370,6 +370,8 @@ void fscrypt_free_extent_info(struct fscrypt_info **info_ptr);
>  int fscrypt_load_extent_info(struct inode *inode, void *buf, size_t len,
>  			     struct fscrypt_info **info_ptr);
>  
> +void fscrypt_get_extent_info_ref(struct fscrypt_info *info);
> +
>  /* fname.c */
>  int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname,
>  			  u8 *out, unsigned int olen);
> -- 
>
> 2.41.0
>

Re: [PATCH v3 13/16] fscrypt: allow multiple extents to reference one info

[Sweet Tea Dorminy]

On 8/10/23 11:51, Luís Henriques wrote:
> Sweet Tea Dorminy <sweettea-kernel@dorminy.me> writes:
> 
>> btrfs occasionally splits in-memory extents while holding a mutex. This
>> means we can't just copy the info, since setting up a new inlinecrypt
>> key requires taking a semaphore. Thus adding a mechanism to split
>> extents and merely take a new reference on the info is necessary.
>>
>> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
>> ---
>>   fs/crypto/fscrypt_private.h |  5 +++++
>>   fs/crypto/keysetup.c        | 18 +++++++++++++++++-
>>   include/linux/fscrypt.h     |  2 ++
>>   3 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
>> index cd29c71b4349..03be2c136c0e 100644
>> --- a/fs/crypto/fscrypt_private.h
>> +++ b/fs/crypto/fscrypt_private.h
>> @@ -287,6 +287,11 @@ struct fscrypt_info {
>>   
>>   	/* Hashed inode number.  Only set for IV_INO_LBLK_32 */
>>   	u32 ci_hashed_ino;
>> +
>> +	/* Reference count. Normally 1, unless a extent info is shared by
>> +	 * several virtual extents.
>> +	 */
>> +	refcount_t refs;
>>   };
>>   
>>   typedef enum {
>> diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
>> index 8d50716bdf11..12c3851b7cd6 100644
>> --- a/fs/crypto/keysetup.c
>> +++ b/fs/crypto/keysetup.c
>> @@ -598,7 +598,7 @@ static void put_crypt_info(struct fscrypt_info *ci)
>>   {
>>   	struct fscrypt_master_key *mk;
>>   
>> -	if (!ci)
>> +	if (!ci || !refcount_dec_and_test(&ci->refs))
>>   		return;
>>   
>>   	if (ci->ci_enc_key) {
>> @@ -686,6 +686,7 @@ fscrypt_setup_encryption_info(struct inode *inode,
>>   	crypt_info->ci_inode = inode;
>>   	crypt_info->ci_sb = inode->i_sb;
>>   	crypt_info->ci_policy = *policy;
>> +	refcount_set(&crypt_info->refs, 1);
>>   	memcpy(crypt_info->ci_nonce, nonce, FSCRYPT_FILE_NONCE_SIZE);
>>   
>>   	mode = select_encryption_mode(&crypt_info->ci_policy, inode);
>> @@ -1046,6 +1047,21 @@ int fscrypt_load_extent_info(struct inode *inode, void *buf, size_t len,
>>   }
>>   EXPORT_SYMBOL_GPL(fscrypt_load_extent_info);
>>   
>> +/**
>> + * fscrypt_get_extent_info_ref() - mark a second extent using the same info
>> + * @info: the info to be used by another extent
>> + *
>> + * Sometimes, an existing extent must be split into multiple extents in memory.
>> + * In such a case, this function allows multiple extents to use the same extent
>> + * info without allocating or taking any lock, which is necessary in certain IO
>> + * paths.
>> + */
>> +void fscrypt_get_extent_info_ref(struct fscrypt_info *info)
>> +{
>> +	if (info)
>> +		refcount_inc(&info->refs);
>> +}
>> +
> 
> There's an EXPORT_SYMBOL_GPL() missing here, right?
> 
> Cheers,

Oof, guess I need to add 'build btrfs as a module' to my preflight 
checklist. Thank you.

[PATCH v3 14/16] fscrypt: cache list of inlinecrypt devices

[Sweet Tea Dorminy]

btrfs sometimes frees extents while holding a mutex, which makes it
impossible to free an inlinecrypt prepared key since that requires
taking a semaphore. Therefore, we will need to offload prepared key
freeing into an asynchronous process (rcu is insufficient since that can
run in softirq context which is also incompatible with taking a
semaphore). In order to avoid use-after-free on the filesystem
superblock for keys being freed during shutdown, we need to cache the
list of devices that the key has been loaded into, so that we can later
remove it without reference to the superblock.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/fscrypt_private.h | 13 +++++++++++--
 fs/crypto/inline_crypt.c    | 20 +++++++++-----------
 fs/crypto/keysetup.c        |  2 +-
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 03be2c136c0e..aba83509c735 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -205,6 +205,16 @@ struct fscrypt_prepared_key {
 	struct crypto_skcipher *tfm;
 #ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
 	struct blk_crypto_key *blk_key;
+
+	/*
+	 * The list of devices that have this block key.
+	 */
+	struct block_device **devices;
+
+	/*
+	 * The number of devices in @ci_devices.
+	 */
+	size_t device_count;
 #endif
 	enum fscrypt_prepared_key_type type;
 };
@@ -470,8 +480,7 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 				     const u8 *raw_key,
 				     const struct fscrypt_info *ci);
 
-void fscrypt_destroy_inline_crypt_key(struct super_block *sb,
-				      struct fscrypt_prepared_key *prep_key);
+void fscrypt_destroy_inline_crypt_key(struct fscrypt_prepared_key *prep_key);
 
 /*
  * Check whether the crypto transform or blk-crypto key has been allocated in
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index 76274b736e1a..91f8365b4194 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -185,12 +185,15 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 		if (err)
 			break;
 	}
-	kfree(devs);
+
 	if (err) {
 		fscrypt_err(inode, "error %d starting to use blk-crypto", err);
 		goto fail;
 	}
 
+	prep_key->devices = devs;
+	prep_key->device_count = num_devs;
+
 	/*
 	 * Pairs with the smp_load_acquire() in fscrypt_is_key_prepared().
 	 * I.e., here we publish ->blk_key with a RELEASE barrier so that
@@ -205,24 +208,19 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 	return err;
 }
 
-void fscrypt_destroy_inline_crypt_key(struct super_block *sb,
-				      struct fscrypt_prepared_key *prep_key)
+void fscrypt_destroy_inline_crypt_key(struct fscrypt_prepared_key *prep_key)
 {
 	struct blk_crypto_key *blk_key = prep_key->blk_key;
-	struct block_device **devs;
-	unsigned int num_devs;
 	unsigned int i;
 
 	if (!blk_key)
 		return;
 
 	/* Evict the key from all the filesystem's block devices. */
-	devs = fscrypt_get_devices(sb, &num_devs);
-	if (!IS_ERR(devs)) {
-		for (i = 0; i < num_devs; i++)
-			blk_crypto_evict_key(devs[i], blk_key);
-		kfree(devs);
-	}
+	for (i = 0; i < prep_key->device_count; i++)
+		blk_crypto_evict_key(prep_key->devices[i], blk_key);
+
+	kfree(prep_key->devices);
 	kfree_sensitive(blk_key);
 }
 
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index 12c3851b7cd6..fe246229c869 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -195,7 +195,7 @@ void fscrypt_destroy_prepared_key(struct super_block *sb,
 				  struct fscrypt_prepared_key *prep_key)
 {
 	crypto_free_skcipher(prep_key->tfm);
-	fscrypt_destroy_inline_crypt_key(sb, prep_key);
+	fscrypt_destroy_inline_crypt_key(prep_key);
 	memzero_explicit(prep_key, sizeof(*prep_key));
 }
 
-- 
2.41.0

Re: [PATCH v3 14/16] fscrypt: cache list of inlinecrypt devices

[Eric Biggers]

On Tue, Aug 08, 2023 at 01:08:31PM -0400, Sweet Tea Dorminy wrote:
> btrfs sometimes frees extents while holding a mutex, which makes it
> impossible to free an inlinecrypt prepared key since that requires
> taking a semaphore. Therefore, we will need to offload prepared key
> freeing into an asynchronous process (rcu is insufficient since that can
> run in softirq context which is also incompatible with taking a
> semaphore). In order to avoid use-after-free on the filesystem
> superblock for keys being freed during shutdown, we need to cache the
> list of devices that the key has been loaded into, so that we can later
> remove it without reference to the superblock.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>  fs/crypto/fscrypt_private.h | 13 +++++++++++--
>  fs/crypto/inline_crypt.c    | 20 +++++++++-----------
>  fs/crypto/keysetup.c        |  2 +-
>  3 files changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 03be2c136c0e..aba83509c735 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -205,6 +205,16 @@ struct fscrypt_prepared_key {
>  	struct crypto_skcipher *tfm;
>  #ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
>  	struct blk_crypto_key *blk_key;
> +
> +	/*
> +	 * The list of devices that have this block key.
> +	 */
> +	struct block_device **devices;
> +
> +	/*
> +	 * The number of devices in @ci_devices.
> +	 */
> +	size_t device_count;
>  #endif
>  	enum fscrypt_prepared_key_type type;
>  };

Well, this is basically reverting commit 22e9947a4b2b, but doing it in a
slightly different way.

I worry about potentially bringing back problems from doing work after the
filesystem has already been unmounted.

Can't you just make the filesystem flush its eviction work items when it is
being unmounted?

- Eric

[PATCH v3 15/16] fscrypt: allow asynchronous info freeing

[Sweet Tea Dorminy]

btrfs sometimes frees extents while holding a mutex. This makes it hard
to free the prepared keys associated therewith, as the free process may
need to take a semaphore. Just offloading freeing to rcu doesn't work,
as rcu may call the callback in softirq context, which also doesn't
allow taking a semaphore. Thus, for extent infos, offload their freeing
to the general system workqueue.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/fscrypt_private.h | 12 +++++++++---
 fs/crypto/keyring.c         |  6 +++---
 fs/crypto/keysetup.c        | 31 +++++++++++++++++++++++++++----
 fs/crypto/keysetup_v1.c     |  3 +--
 4 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index aba83509c735..2b95c3a9560f 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -216,6 +216,12 @@ struct fscrypt_prepared_key {
 	 */
 	size_t device_count;
 #endif
+	/*
+	 * For destroying asynchronously.
+	 */
+	struct work_struct work;
+	/* A pointer to free after destroy. */
+	void *ptr_to_free;
 	enum fscrypt_prepared_key_type type;
 };
 
@@ -526,8 +532,7 @@ fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 }
 
 static inline void
-fscrypt_destroy_inline_crypt_key(struct super_block *sb,
-				 struct fscrypt_prepared_key *prep_key)
+fscrypt_destroy_inline_crypt_key(struct fscrypt_prepared_key *prep_key)
 {
 }
 
@@ -748,7 +753,8 @@ int fscrypt_prepare_key(struct fscrypt_prepared_key *prep_key,
 			const u8 *raw_key, const struct fscrypt_info *ci);
 
 void fscrypt_destroy_prepared_key(struct super_block *sb,
-				  struct fscrypt_prepared_key *prep_key);
+				  struct fscrypt_prepared_key *prep_key,
+				  void *ptr_to_free);
 
 int fscrypt_set_per_file_enc_key(struct fscrypt_info *ci, const u8 *raw_key);
 
diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index feca4a8410bb..7826322b8528 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -107,11 +107,11 @@ void fscrypt_put_master_key_activeref(struct super_block *sb,
 
 	for (i = 0; i <= FSCRYPT_MODE_MAX; i++) {
 		fscrypt_destroy_prepared_key(
-				sb, &mk->mk_direct_keys[i]);
+				sb, &mk->mk_direct_keys[i], NULL);
 		fscrypt_destroy_prepared_key(
-				sb, &mk->mk_iv_ino_lblk_64_keys[i]);
+				sb, &mk->mk_iv_ino_lblk_64_keys[i], NULL);
 		fscrypt_destroy_prepared_key(
-				sb, &mk->mk_iv_ino_lblk_32_keys[i]);
+				sb, &mk->mk_iv_ino_lblk_32_keys[i], NULL);
 	}
 	memzero_explicit(&mk->mk_ino_hash_key,
 			 sizeof(mk->mk_ino_hash_key));
diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
index fe246229c869..1acb676efe3e 100644
--- a/fs/crypto/keysetup.c
+++ b/fs/crypto/keysetup.c
@@ -190,13 +190,36 @@ int fscrypt_prepare_key(struct fscrypt_prepared_key *prep_key,
 	return 0;
 }
 
-/* Destroy a crypto transform object and/or blk-crypto key. */
-void fscrypt_destroy_prepared_key(struct super_block *sb,
-				  struct fscrypt_prepared_key *prep_key)
+static void __destroy_key(struct fscrypt_prepared_key *prep_key)
 {
+	void *ptr_to_free = prep_key->ptr_to_free;
+
 	crypto_free_skcipher(prep_key->tfm);
 	fscrypt_destroy_inline_crypt_key(prep_key);
 	memzero_explicit(prep_key, sizeof(*prep_key));
+	if (ptr_to_free)
+		kfree_sensitive(ptr_to_free);
+}
+
+static void __destroy_key_work(struct work_struct *work)
+{
+	struct fscrypt_prepared_key *prep_key =
+		container_of(work, struct fscrypt_prepared_key, work);
+
+	__destroy_key(prep_key);
+}
+
+/* Destroy a crypto transform object and/or blk-crypto key. */
+void fscrypt_destroy_prepared_key(struct super_block *sb,
+				  struct fscrypt_prepared_key *prep_key,
+				  void *ptr_to_free)
+{
+	prep_key->ptr_to_free = ptr_to_free;
+	if (fscrypt_fs_uses_extent_encryption(sb)) {
+		INIT_WORK(&prep_key->work, __destroy_key_work);
+		queue_work(system_unbound_wq, &prep_key->work);
+	} else
+		__destroy_key(prep_key);
 }
 
 /* Given a per-file encryption key, set up the file's crypto transform object */
@@ -608,8 +631,8 @@ static void put_crypt_info(struct fscrypt_info *ci)
 			fscrypt_put_direct_key(ci->ci_enc_key);
 		if (type == FSCRYPT_KEY_PER_INFO) {
 			fscrypt_destroy_prepared_key(ci->ci_sb,
+						     ci->ci_enc_key,
 						     ci->ci_enc_key);
-			kfree_sensitive(ci->ci_enc_key);
 		}
 	}
 
diff --git a/fs/crypto/keysetup_v1.c b/fs/crypto/keysetup_v1.c
index 4f2be2377dfa..4f45e99290d9 100644
--- a/fs/crypto/keysetup_v1.c
+++ b/fs/crypto/keysetup_v1.c
@@ -155,8 +155,7 @@ struct fscrypt_direct_key {
 static void free_direct_key(struct fscrypt_direct_key *dk)
 {
 	if (dk) {
-		fscrypt_destroy_prepared_key(dk->dk_sb, &dk->dk_key);
-		kfree_sensitive(dk);
+		fscrypt_destroy_prepared_key(dk->dk_sb, &dk->dk_key, dk);
 	}
 }
 
-- 
2.41.0

Re: [PATCH v3 15/16] fscrypt: allow asynchronous info freeing

[Eric Biggers]

On Tue, Aug 08, 2023 at 01:08:32PM -0400, Sweet Tea Dorminy wrote:
> btrfs sometimes frees extents while holding a mutex. This makes it hard
> to free the prepared keys associated therewith, as the free process may
> need to take a semaphore. Just offloading freeing to rcu doesn't work,
> as rcu may call the callback in softirq context, which also doesn't
> allow taking a semaphore. Thus, for extent infos, offload their freeing
> to the general system workqueue.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

Please be specific about which mutex and which semaphore.

What is the specific problem?

- Eric

[PATCH v3 16/16] fscrypt: update documentation for per-extent keys

[Sweet Tea Dorminy]

Add some documentation of how extent-based encryption works, hopefully
enough for future filesystem users.

Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 Documentation/filesystems/fscrypt.rst | 43 +++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
index eccd327e6df5..e862d59bd5b5 100644
--- a/Documentation/filesystems/fscrypt.rst
+++ b/Documentation/filesystems/fscrypt.rst
@@ -31,7 +31,7 @@ However, except for filenames, fscrypt does not encrypt filesystem
 metadata.
 
 Unlike eCryptfs, which is a stacked filesystem, fscrypt is integrated
-directly into supported filesystems --- currently ext4, F2FS, and
+directly into supported filesystems --- currently btrfs, ext4, F2FS, and
 UBIFS.  This allows encrypted files to be read and written without
 caching both the decrypted and encrypted pages in the pagecache,
 thereby nearly halving the memory used and bringing it in line with
@@ -125,6 +125,11 @@ However, these ioctls have some limitations:
   well as kill any processes whose working directory is in an affected
   encrypted directory.
 
+- If the filesystem is using extent-based encryption, the master
+  encryption key will *not* be wiped from kernel memory until all
+  inodes using the key have been evicted (requiring that all files
+  using the key are closed).
+
 - The kernel cannot magically wipe copies of the master key(s) that
   userspace might have as well.  Therefore, userspace must wipe all
   copies of the master key(s) it makes as well; normally this should
@@ -280,6 +285,11 @@ included in the IV.  Moreover:
   key derived using the KDF.  Users may use the same master key for
   other v2 encryption policies.
 
+For filesystems with extent-based content encryption (e.g. btrfs),
+this is the only choice. Data shared among multiple inodes must share
+the exact same key, therefore necessitating inodes using the same key
+for contents encryption.
+
 IV_INO_LBLK_64 policies
 -----------------------
 
@@ -381,12 +391,13 @@ to individual filesystems.  However, authenticated encryption (AE)
 modes are not currently supported because of the difficulty of dealing
 with ciphertext expansion.
 
-Contents encryption
--------------------
+Inode-based contents encryption
+-------------------------------
 
-For file contents, each filesystem block is encrypted independently.
-Starting from Linux kernel 5.5, encryption of filesystems with block
-size less than system's page size is supported.
+Most filesystems use the previously discussed per-file keys. For these
+filesystems, for file contents, each filesystem block is encrypted
+independently.  Starting from Linux kernel 5.5, encryption of filesystems
+with block size less than system's page size is supported.
 
 Each block's IV is set to the logical block number within the file as
 a little endian number, except that:
@@ -410,6 +421,26 @@ Note that because file logical block numbers are included in the IVs,
 filesystems must enforce that blocks are never shifted around within
 encrypted files, e.g. via "collapse range" or "insert range".
 
+Extent-based contents encryption
+--------------------------------
+
+For certain filesystems (currently only btrfs), data is encrypted on a
+per-extent basis, for whatever the filesystem's notion of an extent is. The
+scheme is exactly as with inode-based contents encryption, except that the
+'inode number' for an extent is requested from the filesystem instead of from
+the file's inode, and the 'logical block number' refers to an offset within the
+extent.
+
+Because the encryption material is per-extent instead of per-inode, as long
+as the extent's encryption context does not change, the filesystem may shift
+around the position of the extent, and may have multiple files referring to
+the same encrypted extent.
+
+Not all extents within a file are decrypted simultaneously, so it is possible
+for a file read to fail partway through the file if it crosses into an extent
+whose key is unavailable.  However, all writes will succeed, unless the key is
+removed mid-write.
+
 Filenames encryption
 --------------------
 
-- 
2.41.0

Re: [PATCH v3 00/16] fscrypt: add extent encryption

[Josef Bacik]

On Tue, Aug 08, 2023 at 01:08:17PM -0400, Sweet Tea Dorminy wrote:
> This changeset adds extent-based data encryption to fscrypt.
> Some filesystems need to encrypt data based on extents, rather than on
> inodes, due to features incompatible with inode-based encryption. For
> instance, btrfs can have multiple inodes referencing a single block of
> data, and moves logical data blocks to different physical locations on
> disk in the background. 
> 
> As per discussion last year in [1] and later in [2], we would like to
> allow the use of fscrypt with btrfs, with authenticated encryption. This
> is the first step of that work, adding extent-based encryption to
> fscrypt; authenticated encryption is the next step. Extent-based
> encryption should be usable by other filesystems which wish to support
> snapshotting or background data rearrangement also, but btrfs is the
> first user. 
> 
> This changeset requires extent encryption to use inlinecrypt, as
> discussed previously. 
> 
> This applies atop [3], which itself is based on kdave/misc-next. It
> passes encryption fstests with suitable changes to btrfs-progs.
> 
> Changelog:
> v3:
>  - Added four additional changes:
>    - soft-deleting keys that extent infos might later need to use, so
>      the behavior of an open file after key removal matches inode-based
>      fscrypt.
>    - a set of changes to allow asynchronous info freeing for extents,
>      necessary due to locking constraints in btrfs.
> 
> v2: 
>  - https://lore.kernel.org/linux-fscrypt/cover.1688927487.git.sweettea-kernel@dorminy.me/T/#t
> 
> 
> [1] https://docs.google.com/document/d/1janjxewlewtVPqctkWOjSa7OhCgB8Gdx7iDaCDQQNZA/edit?usp=sharing
> [2] https://lore.kernel.org/linux-fscrypt/80496cfe-161d-fb0d-8230-93818b966b1b@dorminy.me/T/#t
> [3] https://lore.kernel.org/linux-fscrypt/cover.1691505830.git.sweettea-kernel@dorminy.me/
> 

Other than the patches I had comments about this series looks good to me.  You
can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

to the series once you've cleaned up my other comments.

Eric I did my best to try and review these in the context of all the code, I've
gone and looked at how it ties into the btrfs stuff and I've looked at the final
state of the code, it looks good to me, but clearly I don't have the experience
in this code that Sweet Tea or you have.  Thanks,

Josef

Re: [PATCH v3 00/16] fscrypt: add extent encryption

[Eric Biggers]

On Tue, Aug 08, 2023 at 01:08:17PM -0400, Sweet Tea Dorminy wrote:
> This applies atop [3], which itself is based on kdave/misc-next. It
> passes encryption fstests with suitable changes to btrfs-progs.

Where can I find kdave/misc-next?  The only mention of "kdave" in MAINTAINERS is
the following under BTRFS FILE SYSTEM:

T:      git git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git

But that repo doesn't contain a misc-next branch.

- Eric

Re: [PATCH v3 00/16] fscrypt: add extent encryption

[Neal Gompa]

On Thu, Aug 10, 2023 at 1:24 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Aug 08, 2023 at 01:08:17PM -0400, Sweet Tea Dorminy wrote:
> > This applies atop [3], which itself is based on kdave/misc-next. It
> > passes encryption fstests with suitable changes to btrfs-progs.
>
> Where can I find kdave/misc-next?  The only mention of "kdave" in MAINTAINERS is
> the following under BTRFS FILE SYSTEM:
>
> T:      git git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git
>
> But that repo doesn't contain a misc-next branch.
>

David Sterba's development trees are in this repository:
https://github.com/kdave/btrfs-devel.git



-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH v3 00/16] fscrypt: add extent encryption

[Sweet Tea Dorminy]

On 8/10/23 00:55, Eric Biggers wrote:
> On Tue, Aug 08, 2023 at 01:08:17PM -0400, Sweet Tea Dorminy wrote:
>> This applies atop [3], which itself is based on kdave/misc-next. It
>> passes encryption fstests with suitable changes to btrfs-progs.
> 
> Where can I find kdave/misc-next?  The only mention of "kdave" in MAINTAINERS is
> the following under BTRFS FILE SYSTEM:
> 
> T:      git git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git
> 
> But that repo doesn't contain a misc-next branch.
> 
> - Eric

I should've mentioned that more explicitly since the btrfs dev tree 
isn't listed in MAINTAINERS. 
https://github.com/kdave/btrfs-devel/tree/misc-next

Apologies

Re: [PATCH v3 00/16] fscrypt: add extent encryption

[Eric Biggers]

On Thu, Aug 10, 2023 at 10:11:56AM -0400, Sweet Tea Dorminy wrote:
> 
> 
> On 8/10/23 00:55, Eric Biggers wrote:
> > On Tue, Aug 08, 2023 at 01:08:17PM -0400, Sweet Tea Dorminy wrote:
> > > This applies atop [3], which itself is based on kdave/misc-next. It
> > > passes encryption fstests with suitable changes to btrfs-progs.
> > 
> > Where can I find kdave/misc-next?  The only mention of "kdave" in MAINTAINERS is
> > the following under BTRFS FILE SYSTEM:
> > 
> > T:      git git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git
> > 
> > But that repo doesn't contain a misc-next branch.
> > 
> > - Eric
> 
> I should've mentioned that more explicitly since the btrfs dev tree isn't
> listed in MAINTAINERS. https://github.com/kdave/btrfs-devel/tree/misc-next
> 

Does this mean that MAINTAINERS needs to be updated?

- Eric

Re: [PATCH v3 00/16] fscrypt: add extent encryption

[Eric Biggers]

Hi Sweet Tea,

On Tue, Aug 08, 2023 at 01:08:17PM -0400, Sweet Tea Dorminy wrote:
> This changeset adds extent-based data encryption to fscrypt.
> Some filesystems need to encrypt data based on extents, rather than on
> inodes, due to features incompatible with inode-based encryption. For
> instance, btrfs can have multiple inodes referencing a single block of
> data, and moves logical data blocks to different physical locations on
> disk in the background. 
> 
> As per discussion last year in [1] and later in [2], we would like to
> allow the use of fscrypt with btrfs, with authenticated encryption. This
> is the first step of that work, adding extent-based encryption to
> fscrypt; authenticated encryption is the next step. Extent-based
> encryption should be usable by other filesystems which wish to support
> snapshotting or background data rearrangement also, but btrfs is the
> first user. 
> 
> This changeset requires extent encryption to use inlinecrypt, as
> discussed previously. 
> 
> This applies atop [3], which itself is based on kdave/misc-next. It
> passes encryption fstests with suitable changes to btrfs-progs.
> 
> Changelog:
> v3:
>  - Added four additional changes:
>    - soft-deleting keys that extent infos might later need to use, so
>      the behavior of an open file after key removal matches inode-based
>      fscrypt.
>    - a set of changes to allow asynchronous info freeing for extents,
>      necessary due to locking constraints in btrfs.
> 
> v2: 
>  - https://lore.kernel.org/linux-fscrypt/cover.1688927487.git.sweettea-kernel@dorminy.me/T/#t
> 
> 
> [1] https://docs.google.com/document/d/1janjxewlewtVPqctkWOjSa7OhCgB8Gdx7iDaCDQQNZA/edit?usp=sharing
> [2] https://lore.kernel.org/linux-fscrypt/80496cfe-161d-fb0d-8230-93818b966b1b@dorminy.me/T/#t
> [3] https://lore.kernel.org/linux-fscrypt/cover.1691505830.git.sweettea-kernel@dorminy.me/
> 
> Sweet Tea Dorminy (16):
>   fscrypt: factor helper for locking master key
>   fscrypt: factor getting info for a specific block
>   fscrypt: adjust effective lblks based on extents
>   fscrypt: add a super_block pointer to fscrypt_info
>   fscrypt: setup leaf inodes for extent encryption
>   fscrypt: allow infos to be owned by extents
>   fscrypt: use an optional ino equivalent for per-extent infos
>   fscrypt: move function call warning of busy inodes
>   fscrypt: revamp key removal for extent encryption
>   fscrypt: add creation/usage/freeing of per-extent infos
>   fscrypt: allow load/save of extent contexts
>   fscrypt: save session key credentials for extent infos
>   fscrypt: allow multiple extents to reference one info
>   fscrypt: cache list of inlinecrypt devices
>   fscrypt: allow asynchronous info freeing
>   fscrypt: update documentation for per-extent keys
> 
>  Documentation/filesystems/fscrypt.rst |  43 +++-
>  fs/crypto/crypto.c                    |   6 +-
>  fs/crypto/fscrypt_private.h           | 158 +++++++++++-
>  fs/crypto/inline_crypt.c              |  49 ++--
>  fs/crypto/keyring.c                   |  78 +++---
>  fs/crypto/keysetup.c                  | 336 ++++++++++++++++++++++----
>  fs/crypto/keysetup_v1.c               |  10 +-
>  fs/crypto/policy.c                    |  20 ++
>  include/linux/fscrypt.h               |  67 +++++
>  9 files changed, 654 insertions(+), 113 deletions(-)

I'm taking a look at this, but I don't think it's really reviewable in its
current state.  The main problem is how the new functionality is reusing struct
fscrypt_info.  Before an fscrypt_info was the information (key, policy, inode
back-pointer, master key link, etc.) associated with an inode.  But now it can
be any of the following:

- Information for an inode
- Cached policy (?) for a "leaf inode"
- Information for an extent

Everything just seems kind of mixed together.  It's not clear which fields of
fscrypt_info apply to which scenario, and which scenario(s) is being handled
when code works with fscrypt_info.  There seem to be bugs caused by these
scenarios being mixed up.  Comments are also inconsistent; a huge number of them
still refer only to "inode" or "file" when that is no longer correct.  So it's
quite hard to understand the code now.

I think there really needs to be some work on designing and documenting the data
structures for what you are trying to accomplish.  That really ought to have
been the first step before getting deep into coding the actual functionality.

Have you considered creating a new struct like fscrypt_extent_info, instead of
reusing fscrypt_info?  I think that would make things much clearer.  I suppose
there is code that needs to operate on the shared fields of both, but that could
be done by putting the shared fields in a sub-struct like 'struct
fscrypt_common_info common;', and passing around a pointer to that where needed.

I'd also like to reiterate the concern I raised last month
(https://lore.kernel.org/linux-fscrypt/20230703045417.GA3057@sol.localdomain/):
a lot of this complexity seems to have been contributed to by the "heavyweight
extents" design choice.  I was hoping to see a detailed design for the "change a
directory's tree encryption policy" feature you are planning to use this for, in
order to get some confidence that it will actually be implemented.  Otherwise, I
fear we'll end up building in a lot of complexity for something that never gets
implemented.

- Eric

Re: [PATCH v3 00/16] fscrypt: add extent encryption

[Josef Bacik]

On Sat, Aug 12, 2023 at 03:15:14PM -0700, Eric Biggers wrote:
> Hi Sweet Tea,
> 
> On Tue, Aug 08, 2023 at 01:08:17PM -0400, Sweet Tea Dorminy wrote:
> > This changeset adds extent-based data encryption to fscrypt.
> > Some filesystems need to encrypt data based on extents, rather than on
> > inodes, due to features incompatible with inode-based encryption. For
> > instance, btrfs can have multiple inodes referencing a single block of
> > data, and moves logical data blocks to different physical locations on
> > disk in the background. 
> > 
> > As per discussion last year in [1] and later in [2], we would like to
> > allow the use of fscrypt with btrfs, with authenticated encryption. This
> > is the first step of that work, adding extent-based encryption to
> > fscrypt; authenticated encryption is the next step. Extent-based
> > encryption should be usable by other filesystems which wish to support
> > snapshotting or background data rearrangement also, but btrfs is the
> > first user. 
> > 
> > This changeset requires extent encryption to use inlinecrypt, as
> > discussed previously. 
> > 
> > This applies atop [3], which itself is based on kdave/misc-next. It
> > passes encryption fstests with suitable changes to btrfs-progs.
> > 
> > Changelog:
> > v3:
> >  - Added four additional changes:
> >    - soft-deleting keys that extent infos might later need to use, so
> >      the behavior of an open file after key removal matches inode-based
> >      fscrypt.
> >    - a set of changes to allow asynchronous info freeing for extents,
> >      necessary due to locking constraints in btrfs.
> > 
> > v2: 
> >  - https://lore.kernel.org/linux-fscrypt/cover.1688927487.git.sweettea-kernel@dorminy.me/T/#t
> > 
> > 
> > [1] https://docs.google.com/document/d/1janjxewlewtVPqctkWOjSa7OhCgB8Gdx7iDaCDQQNZA/edit?usp=sharing
> > [2] https://lore.kernel.org/linux-fscrypt/80496cfe-161d-fb0d-8230-93818b966b1b@dorminy.me/T/#t
> > [3] https://lore.kernel.org/linux-fscrypt/cover.1691505830.git.sweettea-kernel@dorminy.me/
> > 
> > Sweet Tea Dorminy (16):
> >   fscrypt: factor helper for locking master key
> >   fscrypt: factor getting info for a specific block
> >   fscrypt: adjust effective lblks based on extents
> >   fscrypt: add a super_block pointer to fscrypt_info
> >   fscrypt: setup leaf inodes for extent encryption
> >   fscrypt: allow infos to be owned by extents
> >   fscrypt: use an optional ino equivalent for per-extent infos
> >   fscrypt: move function call warning of busy inodes
> >   fscrypt: revamp key removal for extent encryption
> >   fscrypt: add creation/usage/freeing of per-extent infos
> >   fscrypt: allow load/save of extent contexts
> >   fscrypt: save session key credentials for extent infos
> >   fscrypt: allow multiple extents to reference one info
> >   fscrypt: cache list of inlinecrypt devices
> >   fscrypt: allow asynchronous info freeing
> >   fscrypt: update documentation for per-extent keys
> > 
> >  Documentation/filesystems/fscrypt.rst |  43 +++-
> >  fs/crypto/crypto.c                    |   6 +-
> >  fs/crypto/fscrypt_private.h           | 158 +++++++++++-
> >  fs/crypto/inline_crypt.c              |  49 ++--
> >  fs/crypto/keyring.c                   |  78 +++---
> >  fs/crypto/keysetup.c                  | 336 ++++++++++++++++++++++----
> >  fs/crypto/keysetup_v1.c               |  10 +-
> >  fs/crypto/policy.c                    |  20 ++
> >  include/linux/fscrypt.h               |  67 +++++
> >  9 files changed, 654 insertions(+), 113 deletions(-)
> 
> I'm taking a look at this, but I don't think it's really reviewable in its
> current state.  The main problem is how the new functionality is reusing struct
> fscrypt_info.  Before an fscrypt_info was the information (key, policy, inode
> back-pointer, master key link, etc.) associated with an inode.  But now it can
> be any of the following:
> 
> - Information for an inode
> - Cached policy (?) for a "leaf inode"
> - Information for an extent
> 
> Everything just seems kind of mixed together.  It's not clear which fields of
> fscrypt_info apply to which scenario, and which scenario(s) is being handled
> when code works with fscrypt_info.  There seem to be bugs caused by these
> scenarios being mixed up.  Comments are also inconsistent; a huge number of them
> still refer only to "inode" or "file" when that is no longer correct.  So it's
> quite hard to understand the code now.
> 
> I think there really needs to be some work on designing and documenting the data
> structures for what you are trying to accomplish.  That really ought to have
> been the first step before getting deep into coding the actual functionality.
> 
> Have you considered creating a new struct like fscrypt_extent_info, instead of
> reusing fscrypt_info?  I think that would make things much clearer.  I suppose
> there is code that needs to operate on the shared fields of both, but that could
> be done by putting the shared fields in a sub-struct like 'struct
> fscrypt_common_info common;', and passing around a pointer to that where needed.
> 
> I'd also like to reiterate the concern I raised last month
> (https://lore.kernel.org/linux-fscrypt/20230703045417.GA3057@sol.localdomain/):
> a lot of this complexity seems to have been contributed to by the "heavyweight
> extents" design choice.  I was hoping to see a detailed design for the "change a
> directory's tree encryption policy" feature you are planning to use this for, in
> order to get some confidence that it will actually be implemented.  Otherwise, I
> fear we'll end up building in a lot of complexity for something that never gets
> implemented.

This is partly my fault, Sweet Tea has been working on this for a while, and it
seems the lack of progress is partly to do with him wanting everything to be
perfect before sending things out, so I've been encouraging him to increase his
iteration frequency so we could get to a mergeable state sooner.

To that end, I told him to leave off the "change the encryption key"
functionality for now and send that along once we had agreement on this part.
My thinking was that's the hairier/more controversial part, and I want to see
progress made on the core fscrypt functionality so we don't get bogged down on
other discussions.

As for the data structures part I was lead to believe this was important for our
usecase.  But it appears you have another method in mind.

In the interest of getting this thing unstuck I'd like to get it clear in my
head what you're wanting to see, and explain what we're wanting to do, and then
I can be more useful in guiding Sweet Tea through this.

What we want (as I currently understand it):

- We have an un-encrypted subvolumed that contains the base image.  Think a
  chroot of centos9 or whatever.
- Start a container, we snapshot this base image, set an encryption key for this
  container, all new writes into this snapshot will now be encrypted with this
  per-container key.
- This container could potentially create a container within this encrypted
  container to run.  Think a short lived job orchestrator.  I run service X that
  is going to run N tasks, each task in it's own container.  Under my encrypted
  container I'm going to be creating new subvolumes and setting a different key
  for those subvolumes.  Then once those jobs are done I'm going to delete that
  subvolume and carry on.

Weird btrfs things that make life difficult:

- Reflink.  Obviously we're not going to be reflinking from an encrypted
  subvolume into a non-encrypted subvolume, everything will have to match, but
  this is still between different inodes, which means we need some per-extent
  context.
- Snapshots.  Technically we would be fine here since the inodes metadata would
  be the same here, so maybe not so difficult.
- Relocation.  This is our "move data around underneath everybody" thing that we
  do all the time.  I'm not actually sure if this poses a problem, I know Sweet
  Tea said it did, but my eyes sort of glaze over whenever we're talking about
  encryption so I don't remember the details.

What I think you want:

- A clearer deliniation in the fscrypt code of what we do with per-extent
  encryption vs everything else.  This is easy for me to grok.
- A lighter weight per-extent encryption scheme.  This is the part that I'm
  having trouble with.  I've been reviewing the code from a "is this broken"
  standpoint, because I don't have the expertise in this area to make a sound
  judgement.  The btrfs parts are easy, and that code is fine.  I trust Sweet
  Tea's judgement to do make decisions that fit our use case, but this seems to
  be the crux of the problem.

This series is the barebones "get fscrypt encrypting stuff on btrfs" approach,
with followups for the next, hairier bits.

But we're over a year into this process and still stuck, so I'm sitting down to
understand this code and the current situation in order to provide better
guidance for Sweet Tea.  Please correct me where I've gone wrong, I've been
going back and reading the emails trying to catch up, so I'm sure I've missed
things.  Thanks,

Josef

Re: [PATCH v3 00/16] fscrypt: add extent encryption

[Eric Biggers]

On Tue, Aug 15, 2023 at 11:12:06AM -0400, Josef Bacik wrote:
> 
> This is partly my fault, Sweet Tea has been working on this for a while, and it
> seems the lack of progress is partly to do with him wanting everything to be
> perfect before sending things out, so I've been encouraging him to increase his
> iteration frequency so we could get to a mergeable state sooner.
> 
> To that end, I told him to leave off the "change the encryption key"
> functionality for now and send that along once we had agreement on this part.
> My thinking was that's the hairier/more controversial part, and I want to see
> progress made on the core fscrypt functionality so we don't get bogged down on
> other discussions.

I've recommended, and continue to recommend, leaving out that feature from the
patchset for now too.  The question is how does the plan to implement this
feature impact the initial patchset, i.e. the basic support for btrfs
encryption.  Should we choose a "heavyweight extents" design (a full
fscrypt_context per extent: nonce, encryption modes, and key) now in preparation
for that feature, or should we choose a "lightweight extents" design (just a
nonce per extent, with modes and key coming from one of the extent's inodes
which would all be enforced to have the same values of those).  The lightweight
extents design wouldn't be compatible with the "change the encryption key"
feature, but I expect it would be simpler.  Maybe there are issues that I
haven't thought of, but that is what I expect.

So before going with a design that is potentially more complex, and won't be
able to be changed later as it will define the on-disk format, I'm hoping to see
a little more confirmation of "yes, this is really needed".  That could mean
getting the design, or even better the implementation, ready for the "change the
encryption key" feature.  Or, maybe you need "heavyweight extents" anyway
because you want/need btrfs extents to be completely self-describing and don't
want to have to pull any information from an inode -- maybe for technical
reasons, maybe for philosophical reasons.  I don't know.  It's up to you guys to
provide the rationale for the design you've chosen.

BTW, one of the reasons for my concern is that the original plan for ext4
encryption, which became fscrypt, was extremely ambitious and resulted in a
complex design.  But only the first phase actually ended up getting implemented,
and as a result the design was simplified greatly just before ext4 encryption
was upstreamed.  I worry about something similar happening, but missing the
"simplify the design" part and ending up with unnecessary complexity.

> As for the data structures part I was lead to believe this was important for our
> usecase.  But it appears you have another method in mind.
> 
> In the interest of getting this thing unstuck I'd like to get it clear in my
> head what you're wanting to see, and explain what we're wanting to do, and then
> I can be more useful in guiding Sweet Tea through this.
> 
> What we want (as I currently understand it):
> 
> - We have an un-encrypted subvolumed that contains the base image.  Think a
>   chroot of centos9 or whatever.
> - Start a container, we snapshot this base image, set an encryption key for this
>   container, all new writes into this snapshot will now be encrypted with this
>   per-container key.
> - This container could potentially create a container within this encrypted
>   container to run.  Think a short lived job orchestrator.  I run service X that
>   is going to run N tasks, each task in it's own container.  Under my encrypted
>   container I'm going to be creating new subvolumes and setting a different key
>   for those subvolumes.  Then once those jobs are done I'm going to delete that
>   subvolume and carry on.
> 
> Weird btrfs things that make life difficult:
> 
> - Reflink.  Obviously we're not going to be reflinking from an encrypted
>   subvolume into a non-encrypted subvolume, everything will have to match, but
>   this is still between different inodes, which means we need some per-extent
>   context.
> - Snapshots.  Technically we would be fine here since the inodes metadata would
>   be the same here, so maybe not so difficult.
> - Relocation.  This is our "move data around underneath everybody" thing that we
>   do all the time.  I'm not actually sure if this poses a problem, I know Sweet
>   Tea said it did, but my eyes sort of glaze over whenever we're talking about
>   encryption so I don't remember the details.

I think a big challenge which is being glossed over is how do you actually set a
new encryption policy for an entire directory tree, which can contain thousands
(or even millions!) of files.  Do you actually go through and update something
on every file, and if so who does it: userspace or kernel?  Or will there be a
layer of indirection that allows the operation to be done in a fixed amount of
work?  Maybe each inode has an encryption policy ID which points into a btree of
encryption policies, and you update that btree to change what the ID refers to?
But that doesn't work if you're changing the encryption policy of only a subset
of files that use a given encryption policy, or if the files are unencrypted.

What happens for directories?  Can their policy change?  I think it can't; all
filenames will have to be encrypted with the original policy.  Is that okay?

What about regular files?  I assume their policy would get replaced, not
appended to, since with extent-based encryption the only purpose of a regular
file's policy is to have it be inherited by new extents of that file?

> 
> What I think you want:
> 
> - A clearer deliniation in the fscrypt code of what we do with per-extent
>   encryption vs everything else.  This is easy for me to grok.
> - A lighter weight per-extent encryption scheme.  This is the part that I'm
>   having trouble with.  I've been reviewing the code from a "is this broken"
>   standpoint, because I don't have the expertise in this area to make a sound
>   judgement.  The btrfs parts are easy, and that code is fine.  I trust Sweet
>   Tea's judgement to do make decisions that fit our use case, but this seems to
>   be the crux of the problem.
> 
> This series is the barebones "get fscrypt encrypting stuff on btrfs" approach,
> with followups for the next, hairier bits.
> 
> But we're over a year into this process and still stuck, so I'm sitting down to
> understand this code and the current situation in order to provide better
> guidance for Sweet Tea.  Please correct me where I've gone wrong, I've been
> going back and reading the emails trying to catch up, so I'm sure I've missed
> things.  Thanks,

The number one thing I'm asking for is something that's maintainable and as easy
to understand as possible.  I don't think we've quite reached that yet.

- Eric

Re: [PATCH v3 00/16] fscrypt: add extent encryption

[Josef Bacik]

On Tue, Aug 15, 2023 at 08:37:20PM -0700, Eric Biggers wrote:
> On Tue, Aug 15, 2023 at 11:12:06AM -0400, Josef Bacik wrote:
> > 
> > This is partly my fault, Sweet Tea has been working on this for a while, and it
> > seems the lack of progress is partly to do with him wanting everything to be
> > perfect before sending things out, so I've been encouraging him to increase his
> > iteration frequency so we could get to a mergeable state sooner.
> > 
> > To that end, I told him to leave off the "change the encryption key"
> > functionality for now and send that along once we had agreement on this part.
> > My thinking was that's the hairier/more controversial part, and I want to see
> > progress made on the core fscrypt functionality so we don't get bogged down on
> > other discussions.
> 
> I've recommended, and continue to recommend, leaving out that feature from the
> patchset for now too.  The question is how does the plan to implement this
> feature impact the initial patchset, i.e. the basic support for btrfs
> encryption.  Should we choose a "heavyweight extents" design (a full
> fscrypt_context per extent: nonce, encryption modes, and key) now in preparation
> for that feature, or should we choose a "lightweight extents" design (just a
> nonce per extent, with modes and key coming from one of the extent's inodes
> which would all be enforced to have the same values of those).  The lightweight
> extents design wouldn't be compatible with the "change the encryption key"
> feature, but I expect it would be simpler.  Maybe there are issues that I
> haven't thought of, but that is what I expect.
> 
> So before going with a design that is potentially more complex, and won't be
> able to be changed later as it will define the on-disk format, I'm hoping to see
> a little more confirmation of "yes, this is really needed".  That could mean
> getting the design, or even better the implementation, ready for the "change the
> encryption key" feature.  Or, maybe you need "heavyweight extents" anyway
> because you want/need btrfs extents to be completely self-describing and don't
> want to have to pull any information from an inode -- maybe for technical
> reasons, maybe for philosophical reasons.  I don't know.  It's up to you guys to
> provide the rationale for the design you've chosen.

I've spent the last day getting a grasp on everything that we want and I'm in a
better place to answer these questions.

There seemed to be some confusion around having nested subvolumes with different
keys, and then reflinking between them.

So 

/container
/container/subcontainer

reflink /container/packagefile /container/subcontainer/packagefile

in this case the inode context wouldn't help, the inodes are different, thus the
heavy weight design is necessary.

However this isn't actually a thing that is a topline need.  I've told our
customers that in this case both /container and /container/subcontainer would
need to share encryption keys, and so this eliminates the need for the heavier
weight solution.  I like your design better, I think it fits most of our goals.

The actual real requirement we have is the unencrypted to encrypted, which is

/base/image
btrfs sub snap /base/image /container
turn on encryptiong /container

new writes in /container get encrypted.  We also want to be able to do

reflink /path/to/unencrypted/package/store/package /container/package

and have this work.  This still works with your design.  The only thing we would
reject with our design is

reflink /encrypted/key1/file /encrypted/key2/file

because the key id's wouldn't match.  This is an acceptable limitation for us to
acheive a more simplistic design.

> 
> BTW, one of the reasons for my concern is that the original plan for ext4
> encryption, which became fscrypt, was extremely ambitious and resulted in a
> complex design.  But only the first phase actually ended up getting implemented,
> and as a result the design was simplified greatly just before ext4 encryption
> was upstreamed.  I worry about something similar happening, but missing the
> "simplify the design" part and ending up with unnecessary complexity.
>
> > As for the data structures part I was lead to believe this was important for our
> > usecase.  But it appears you have another method in mind.
> > 
> > In the interest of getting this thing unstuck I'd like to get it clear in my
> > head what you're wanting to see, and explain what we're wanting to do, and then
> > I can be more useful in guiding Sweet Tea through this.
> > 
> > What we want (as I currently understand it):
> > 
> > - We have an un-encrypted subvolumed that contains the base image.  Think a
> >   chroot of centos9 or whatever.
> > - Start a container, we snapshot this base image, set an encryption key for this
> >   container, all new writes into this snapshot will now be encrypted with this
> >   per-container key.
> > - This container could potentially create a container within this encrypted
> >   container to run.  Think a short lived job orchestrator.  I run service X that
> >   is going to run N tasks, each task in it's own container.  Under my encrypted
> >   container I'm going to be creating new subvolumes and setting a different key
> >   for those subvolumes.  Then once those jobs are done I'm going to delete that
> >   subvolume and carry on.
> > 
> > Weird btrfs things that make life difficult:
> > 
> > - Reflink.  Obviously we're not going to be reflinking from an encrypted
> >   subvolume into a non-encrypted subvolume, everything will have to match, but
> >   this is still between different inodes, which means we need some per-extent
> >   context.
> > - Snapshots.  Technically we would be fine here since the inodes metadata would
> >   be the same here, so maybe not so difficult.
> > - Relocation.  This is our "move data around underneath everybody" thing that we
> >   do all the time.  I'm not actually sure if this poses a problem, I know Sweet
> >   Tea said it did, but my eyes sort of glaze over whenever we're talking about
> >   encryption so I don't remember the details.
> 
> I think a big challenge which is being glossed over is how do you actually set a
> new encryption policy for an entire directory tree, which can contain thousands
> (or even millions!) of files.  Do you actually go through and update something
> on every file, and if so who does it: userspace or kernel?  Or will there be a
> layer of indirection that allows the operation to be done in a fixed amount of
> work?  Maybe each inode has an encryption policy ID which points into a btree of
> encryption policies, and you update that btree to change what the ID refers to?
> But that doesn't work if you're changing the encryption policy of only a subset
> of files that use a given encryption policy, or if the files are unencrypted.

Since the scope is purely unencrypted snapshot -> encrypted this is much
simpler, only new things get the encryption policy set.

Say we snapshot, set the encryption key, and then write to the middle of a file.
That inode gets its encryption policy set, and that file extent is marked as
encrypted.  When we go to read back that whole file later we see the unencrypted
extents and read those like normal, then get to the encrypted extent and decrypt
that like normal.

> 
> > 
> > What I think you want:
> > 
> > - A clearer deliniation in the fscrypt code of what we do with per-extent
> >   encryption vs everything else.  This is easy for me to grok.
> > - A lighter weight per-extent encryption scheme.  This is the part that I'm
> >   having trouble with.  I've been reviewing the code from a "is this broken"
> >   standpoint, because I don't have the expertise in this area to make a sound
> >   judgement.  The btrfs parts are easy, and that code is fine.  I trust Sweet
> >   Tea's judgement to do make decisions that fit our use case, but this seems to
> >   be the crux of the problem.
> > 
> > This series is the barebones "get fscrypt encrypting stuff on btrfs" approach,
> > with followups for the next, hairier bits.
> > 
> > But we're over a year into this process and still stuck, so I'm sitting down to
> > understand this code and the current situation in order to provide better
> > guidance for Sweet Tea.  Please correct me where I've gone wrong, I've been
> > going back and reading the emails trying to catch up, so I'm sure I've missed
> > things.  Thanks,
> 
> The number one thing I'm asking for is something that's maintainable and as easy
> to understand as possible.  I don't think we've quite reached that yet.
> 

That's fair.  Like I said I've purposefully stayed out of this part because it's
quite a bit to page in a whole other project into my head and figure out what
would be the best course of action.  I think that narrowing our usecase to only
wanting to support unencrypted->encrypted for snapshots will make this simpler,
and integrating your more simplified light weight design for extent encryption
will result in something more to your liking.  Thanks,

Josef