Re: [RFC PATCH 15/17] fscrypt: allow load/save of extent contexts

[Eric Biggers <ebiggers@kernel.org>]

On Sun, Jan 01, 2023 at 12:06:19AM -0500, Sweet Tea Dorminy wrote:
> 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    | 50 +++++++++++++++++++++++++++++++++++++++++
>  fs/crypto/policy.c      | 20 +++++++++++++++++
>  include/linux/fscrypt.h | 17 ++++++++++++++
>  3 files changed, 87 insertions(+)
> 
> diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> index 136156487e8f..82439fb73dd9 100644
> --- a/fs/crypto/keysetup.c
> +++ b/fs/crypto/keysetup.c
> @@ -799,6 +799,56 @@ 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)
> +{

When is the filesystem going to call fscrypt_load_extent_info()?

My concern (which we've discussed, but probably I didn't explain clearly enough)
is that the two "naive" solutions don't really work:

Option 1: Set up during the I/O to the extent.  I think this is not feasible
because the full fscrypt_setup_encryption_info() is not safe to do doing I/O.
For example, it allocates memory under GFP_KERNEL, and it uses
crypto_alloc_skcipher() which can involve loading kernel modules.

Option 2: Set up for *all* extents when the file is opened.  I expect that this
would not be feasible either, since a file can have a huge number of extents.

This patchset seems to be assuming one of those options.  (It's not clear
whether it's Option 1 or Option 2, since the caller of
fscrypt_load_extent_info() is not included in the patchset.)

That leaves the option I suggested, which probably I didn't explain clearly
enough: split up key setup so that part can be done when opening the file, and
part can be done during I/O.  Specifically, when opening the file, preallocate
some number of crypto_skcipher objects.  This would be limited to a fixed
number, like 128, even if the file has thousands of extents.  Then, when doing
I/O to an extent, temporarily take a crypto_skcipher from the cache, derive the
extent's key using fscrypt_hkdf_expand(), and set it in the crypto_skcipher
using crypto_skcipher_setkey().

That way, during I/O only fscrypt_hkdf_expand() and crypto_skcipher_setkey()
would be needed.  Those are fairly safe to call during I/O, in contrast to
crypto_alloc_skcipher() which is really problematic to call during I/O.

Of course, it will still be somewhat expensive to derive and set a key.  So it
might also make sense to maintain a map that maps (master key, extent nonce,
encryption mode) to the corresponding cached crypto_skcipher, if any, so that an
already-prepared one can be used when possible.

By the way, blk-crypto-fallback (block/blk-crypto-fallback.c) does something
similar, as it had to solve a similar problem.  The way it was solved is to
require that blk_crypto_fallback_start_using_mode() be called to preallocate the
crypto_skcipher objects for a given encryption mode.

You actually could just use blk-crypto-fallback to do the en/decryption for you,
if you wanted to.  I.e., you could use the blk-crypto API for en/decryption,
instead of going directly through crypto_skcipher.  (Note that currently
blk-crypto is opt-in via the "inlinecrypt" mount option; it's not used by
default.  But it doesn't *have* to be that way; it could just be always used.
It would be necessary to ensure that CONFIG_BLK_INLINE_ENCRYPTION_FALLBACK gets
selected.)  That would save having to reimplement the caching of crypto_skcipher
objects.  However, key derivation would still need to be done at the filesystem
level, so it probably would still make sense to cache derived keys.

- Eric