On Mon, Jan 02, 2023 at 05:31:02PM -0500, Sweet Tea Dorminy wrote: > (in which I fail to reply-all the first time) > > > > > 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. > > > 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... > > > > 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. "Loading the parent dir's fscrypt_info" isn't relevant here, since the filenames encryption mode can be different from the contents encryption mode. It's also possible for an encrypted file to be located in an unencrypted directory. Maybe you meant to be talking about the file itself being opened? Anyway, crypto_alloc_skcipher() takes a lock (crypto_alg_sem) under which memory is allocated with GFP_KERNEL. So neither preloading kernel modules nor memalloc_nofs_save() helps for it; it's still not GFP_NOFS-safe. I don't think we should allow files to have extents with different encryption modes. Encryption policies will still be a property of files. > > 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? It probably should be global, or at least per-master-key, as it would be a massive overhead to allocate (e.g.) 128 crypto_skcipher objects for every file. blk-crypto-fallback uses a global cache. It does introduce a bottleneck and memory that can't be reclaimed, though. I'd appreciate any thoughts about other solutions. Maybe the number of objects in the cache could be scaled up and down as the number of in-core inodes changes. > > 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. > Same question: just for extent infos, or can this be generalized to all > infos? I think that we should definitely still cache the per-file key in inode::i_crypt_info and not in some global cache. - Eric
|