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@xxxxxxxxxx> > --- > 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
![]() |