Re: [PATCH v2 10/11] fscrypt: explicitly track prepared parts of key

[Eric Biggers <ebiggers@kernel.org>]

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