| Message ID | 2a9bf42af2b2ac6289d0ac886d1f07042feafbe5.1681155143.git.sweettea-kernel@dorminy.me (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fscrypt: rearrangements preliminary to extent encryption | expand |
On Mon, Apr 10, 2023 at 03:40:03PM -0400, Sweet Tea Dorminy wrote: > So far, it has sufficed to allocate and prepare the block key or the TFM > completely before ever setting the relevant field in the prepared key. > This is necessary for mode keys -- because multiple inodes could be > trying to set up the same per-mode prepared key at the same time on > different threads, we currently must not set the prepared key's tfm or > block key pointer until that key is completely set up. Otherwise, > another inode could see the key to be present and attempt to use it > before it is fully set up. > > But when using pooled prepared keys, we'll have pre-allocated fields, > and if we separate allocating the fields of a prepared key from > preparing the fields, that inherently sets the fields before they're > ready to use. So, either pooled prepared keys must use different > allocation and setup functions, or we can split allocation and > preparation for all prepared keys and use some other mechanism to signal > that the key is fully prepared. > > In order to avoid having similar yet different functions, this function > adds a new field to the prepared key to explicitly track which parts of > it are prepared, setting it explicitly. The same acquire/release > semantics are used to check it in the case of shared mode keys; the cost > lies in the extra byte per prepared key recording which members are > fully prepared. > > Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me> > --- > fs/crypto/fscrypt_private.h | 26 +++++++++++++++----------- > fs/crypto/inline_crypt.c | 8 +------- > fs/crypto/keysetup.c | 36 ++++++++++++++++++++++++++---------- > 3 files changed, 42 insertions(+), 28 deletions(-) I wonder if this is overcomplicating things and we should simply add a new rw_semaphore to struct fscrypt_master_key and use it to protect the per-mode key preparation, instead of trying to keep the fast path lockless? So the flow (for setting up a file that uses a per-mode key) would look like: down_read(&mk->mk_mode_key_prep_sem); if key already prepared, unlock and return up_read(&mk->mk_mode_key_prep_sem); down_write(&mk->mk_mode_key_prep_sem); if key already prepared, unlock and return prepare the key up_write(&mk->mk_mode_key_prep_sem); Lockless algorithms are nice, but we shouldn't take them too far if they cause too much trouble... - Eric
On 4/11/23 00:05, Eric Biggers wrote: > On Mon, Apr 10, 2023 at 03:40:03PM -0400, Sweet Tea Dorminy wrote: >> So far, it has sufficed to allocate and prepare the block key or the TFM >> completely before ever setting the relevant field in the prepared key. >> This is necessary for mode keys -- because multiple inodes could be >> trying to set up the same per-mode prepared key at the same time on >> different threads, we currently must not set the prepared key's tfm or >> block key pointer until that key is completely set up. Otherwise, >> another inode could see the key to be present and attempt to use it >> before it is fully set up. >> >> But when using pooled prepared keys, we'll have pre-allocated fields, >> and if we separate allocating the fields of a prepared key from >> preparing the fields, that inherently sets the fields before they're >> ready to use. So, either pooled prepared keys must use different >> allocation and setup functions, or we can split allocation and >> preparation for all prepared keys and use some other mechanism to signal >> that the key is fully prepared. >> >> In order to avoid having similar yet different functions, this function >> adds a new field to the prepared key to explicitly track which parts of >> it are prepared, setting it explicitly. The same acquire/release >> semantics are used to check it in the case of shared mode keys; the cost >> lies in the extra byte per prepared key recording which members are >> fully prepared. >> >> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me> >> --- >> fs/crypto/fscrypt_private.h | 26 +++++++++++++++----------- >> fs/crypto/inline_crypt.c | 8 +------- >> fs/crypto/keysetup.c | 36 ++++++++++++++++++++++++++---------- >> 3 files changed, 42 insertions(+), 28 deletions(-) > > I wonder if this is overcomplicating things and we should simply add a new > rw_semaphore to struct fscrypt_master_key and use it to protect the per-mode key > preparation, instead of trying to keep the fast path lockless? > > So the flow (for setting up a file that uses a per-mode key) would look like: > > down_read(&mk->mk_mode_key_prep_sem); > if key already prepared, unlock and return > up_read(&mk->mk_mode_key_prep_sem); > > down_write(&mk->mk_mode_key_prep_sem); > if key already prepared, unlock and return > prepare the key > up_write(&mk->mk_mode_key_prep_sem); > > Lockless algorithms are nice, but we shouldn't take them too far if they cause > too much trouble... You're noting that we only really need preparedness for per-mode keys, and that's a point I didn't explicitly grasp before; everywhere else we know whether it's merely allocated or fully prepared. Two other thoughts on ways we could pull the preparedness out of fscrypt_prepared_key and still keep locklessness: fscrypt_master_key could setup both block key and tfm (if block key is applicable) when it sets up a prepared key, so we can use just one bit of preparedness information, and keep a bitmap recording which prepared keys are ready in fscrypt_master_key. Or we could have struct fscrypt_master_key_prepared_key { u8 preparedness; struct fscrypt_prepared_key prep_key; } and similarly isolate the preparedness tracking from per-file keys. Do either of those sound appealing as alternatives to the semaphore?
On Tue, Apr 11, 2023 at 12:45:28PM -0400, Sweet Tea Dorminy wrote: > You're noting that we only really need preparedness for per-mode keys, and > that's a point I didn't explicitly grasp before; everywhere else we know > whether it's merely allocated or fully prepared. Two other thoughts on ways > we could pull the preparedness out of fscrypt_prepared_key and still keep > locklessness: > > fscrypt_master_key could setup both block key and tfm (if block key is > applicable) when it sets up a prepared key, so we can use just one bit of > preparedness information, and keep a bitmap recording which prepared keys > are ready in fscrypt_master_key. > > Or we could have > struct fscrypt_master_key_prepared_key { > u8 preparedness; > struct fscrypt_prepared_key prep_key; > } > and similarly isolate the preparedness tracking from per-file keys. > > Do either of those sound appealing as alternatives to the semaphore? Not really. The bitmaps add extra complexity. Also note that the tfm and blk-crypto key do need to be considered separately, as there can be cases where blk-crypto supports the algorithm but the crypto API doesn't, and vice versa. I think that for the per-mode keys, we should either keep the current behavior where prep_key->tfm and prep_key->blk_key aren't set until they're fully ready (in which case the lockless check continues to be fairly straightforward), *or* we should make it no longer lockless. - Eric
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h index e726a1fb9f7e..7253cdb5e4d8 100644 --- a/fs/crypto/fscrypt_private.h +++ b/fs/crypto/fscrypt_private.h @@ -197,6 +197,8 @@ enum fscrypt_prepared_key_type { * @tfm: crypto API transform object * @blk_key: key for blk-crypto * @type: records the ownership type of the prepared key + * @prepared_members: records which of @tfm and @blk_key are prepared. tfm + * corresponds to bit 0; blk_key corresponds to bit 1. * * Normally only one of @tfm and @blk_key will be non-NULL, although it is * possible if @type is FSCRYPT_KEY_MASTER_KEY. @@ -207,6 +209,7 @@ struct fscrypt_prepared_key { struct blk_crypto_key *blk_key; #endif enum fscrypt_prepared_key_type type; + u8 prepared_members; }; /* @@ -363,24 +366,25 @@ void fscrypt_destroy_inline_crypt_key(struct super_block *sb, struct fscrypt_prepared_key *prep_key); /* - * Check whether the crypto transform or blk-crypto key has been allocated in + * Check whether the crypto transform or blk-crypto key has been prepared in * @prep_key, depending on which encryption implementation the file will use. */ static inline bool fscrypt_is_key_prepared(struct fscrypt_prepared_key *prep_key, const struct fscrypt_info *ci) { + u8 prepared_members = smp_load_acquire(&prep_key->prepared_members); + bool inlinecrypt = fscrypt_using_inline_encryption(ci); + /* - * The two smp_load_acquire()'s here pair with the smp_store_release()'s - * in fscrypt_prepare_inline_crypt_key() and fscrypt_prepare_key(). - * I.e., in some cases (namely, if this prep_key is a per-mode - * encryption key) another task can publish blk_key or tfm concurrently, - * executing a RELEASE barrier. We need to use smp_load_acquire() here - * to safely ACQUIRE the memory the other task published. + * The smp_load_acquire() here pairs with the smp_store_release() + * in fscrypt_prepare_key(). I.e., in some cases (namely, if this + * prep_key is a per-mode encryption key) another task can publish + * blk_key or tfm concurrently, executing a RELEASE barrier. We need + * to use smp_load_acquire() here to safely ACQUIRE the memory the + * other task published. */ - if (fscrypt_using_inline_encryption(ci)) - return smp_load_acquire(&prep_key->blk_key) != NULL; - return smp_load_acquire(&prep_key->tfm) != NULL; + return prepared_members & (1U << inlinecrypt); } #else /* CONFIG_FS_ENCRYPTION_INLINE_CRYPT */ @@ -415,7 +419,7 @@ static inline bool fscrypt_is_key_prepared(struct fscrypt_prepared_key *prep_key, const struct fscrypt_info *ci) { - return smp_load_acquire(&prep_key->tfm) != NULL; + return smp_load_acquire(&prep_key->prepared_members); } #endif /* !CONFIG_FS_ENCRYPTION_INLINE_CRYPT */ diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c index 2063f7941ce6..ce952dedba77 100644 --- a/fs/crypto/inline_crypt.c +++ b/fs/crypto/inline_crypt.c @@ -191,13 +191,7 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key, goto fail; } - /* - * Pairs with the smp_load_acquire() in fscrypt_is_key_prepared(). - * I.e., here we publish ->blk_key with a RELEASE barrier so that - * concurrent tasks can ACQUIRE it. Note that this concurrency is only - * possible for per-mode keys, not for per-file keys. - */ - smp_store_release(&prep_key->blk_key, blk_key); + prep_key->blk_key = blk_key; return 0; fail: diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c index f338bb544932..6efac89d49ec 100644 --- a/fs/crypto/keysetup.c +++ b/fs/crypto/keysetup.c @@ -155,21 +155,37 @@ fscrypt_allocate_skcipher(struct fscrypt_mode *mode, const u8 *raw_key, int fscrypt_prepare_key(struct fscrypt_prepared_key *prep_key, const u8 *raw_key, const struct fscrypt_info *ci) { - struct crypto_skcipher *tfm; + int err; + bool inlinecrypt = fscrypt_using_inline_encryption(ci); + u8 prepared_member = (1 << inlinecrypt); - if (fscrypt_using_inline_encryption(ci)) - return fscrypt_prepare_inline_crypt_key(prep_key, raw_key, ci); + if (inlinecrypt) { + err = fscrypt_prepare_inline_crypt_key(prep_key, raw_key, ci); + } else { + struct crypto_skcipher *tfm; + + tfm = fscrypt_allocate_skcipher(ci->ci_mode, raw_key, ci->ci_inode); + if (IS_ERR(tfm)) + return PTR_ERR(tfm); + } + + prep_key->tfm = tfm; + } - tfm = fscrypt_allocate_skcipher(ci->ci_mode, raw_key, ci->ci_inode); - if (IS_ERR(tfm)) - return PTR_ERR(tfm); /* * Pairs with the smp_load_acquire() in fscrypt_is_key_prepared(). - * I.e., here we publish ->tfm with a RELEASE barrier so that - * concurrent tasks can ACQUIRE it. Note that this concurrency is only - * possible for per-mode keys, not for per-file keys. + * I.e., here we publish ->prepared_members with a RELEASE barrier so + * that concurrent tasks can ACQUIRE it. + * + * Note that this concurrency is only possible for per-mode keys, + * not for per-file keys. For per-mode keys, we have smp_load_acquire'd + * the value of ->prepared_members after taking a lock serializing + * preparing this key, so the value is stable and no other thread can + * have modified it since the read. So another thread can't be trying + * to run this same code in parallel, and we don't need to use cmpxchg. */ - smp_store_release(&prep_key->tfm, tfm); + smp_store_release(&prep_key->prepared_members, + prep_key->prepared_members | prepared_member); return 0; }
So far, it has sufficed to allocate and prepare the block key or the TFM completely before ever setting the relevant field in the prepared key. This is necessary for mode keys -- because multiple inodes could be trying to set up the same per-mode prepared key at the same time on different threads, we currently must not set the prepared key's tfm or block key pointer until that key is completely set up. Otherwise, another inode could see the key to be present and attempt to use it before it is fully set up. But when using pooled prepared keys, we'll have pre-allocated fields, and if we separate allocating the fields of a prepared key from preparing the fields, that inherently sets the fields before they're ready to use. So, either pooled prepared keys must use different allocation and setup functions, or we can split allocation and preparation for all prepared keys and use some other mechanism to signal that the key is fully prepared. In order to avoid having similar yet different functions, this function adds a new field to the prepared key to explicitly track which parts of it are prepared, setting it explicitly. The same acquire/release semantics are used to check it in the case of shared mode keys; the cost lies in the extra byte per prepared key recording which members are fully prepared. Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me> --- fs/crypto/fscrypt_private.h | 26 +++++++++++++++----------- fs/crypto/inline_crypt.c | 8 +------- fs/crypto/keysetup.c | 36 ++++++++++++++++++++++++++---------- 3 files changed, 42 insertions(+), 28 deletions(-)