(in which I fail to reply-all the first time)
memalloc_nofs_save()/memalloc_nofs_restore() could do the job of making sure allocations use GFP_NOFS, I think? I guess those calls should be in fscrypt_load_extent_info() instead of just in the doc...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.
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().
I didn't elaborate why it wasn't here and should have. With just this patchset, I thought a file only ever has extents with the same encryption mode, since an inode can't change encryption mode/key past creation at present . So loading the parent dir's fscrypt_info should be enough to ensure the module is loaded for the mode of all the extents. I suppose I'd need to ensure that reflinks also only reflink extents sharing the same encryption mode.
In the future patchset which allows changing the key being used for new extents (naming that is hard), I had envisioned requiring the filesystem to provide a list of enc modes used by an inode when opening, and then fscrypt_file_open() could make sure all the necessary modules are loaded for those modes.
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.
Could we use a mempool of skciphers for all of fscrypt, or should it only be for extents and be on a per-file basis?
Same question: just for extent infos, or can this be generalized to all infos?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
Interesting. I will have to study that more, thanks for the pointer. Thank you! Sweet Tea
|