Hi Sweet Tea, On Fri, May 05, 2023 at 08:35:53PM -0400, Sweet Tea Dorminy wrote: > > > On 5/5/23 18:40, Eric Biggers wrote: > > On Fri, May 05, 2023 at 08:15:44AM -0400, Sweet Tea Dorminy wrote: > > > > > > > As I mentioned earlier > > > > (https://lore.kernel.org/r/Y7NQ1CvPyJiGRe00@sol.localdomain), > > > > blk-crypto-fallback actually already solved the problem of caching > > > > crypto_skcipher objects for I/O. And, it's possible for a filesystem to *only* > > > > support blk-crypto, not filesystem-layer contents encryption. You'd just need > > > > to put btrfs encryption behind a new kconfig option that is automatically > > > > selected by CONFIG_FS_ENCRYPTION_INLINE_CRYPT && CONFIG_BLK_ENCRYPTION_FALLBACK. > > > > > > > > (BTW, I'm thinking of simplifying the kconfig options by removing > > > > CONFIG_FS_ENCRYPTION_INLINE_CRYPT. Then, the blk-crypto code in fs/crypto/ will > > > > be built if CONFIG_FS_ENCRYPTION && CONFIG_BLK_INLINE_ENCRYPTION.) > > > > > > > > Indeed, filesystem-layer contents encryption is a bit redundant these days now > > > > that blk-crypto-fallback exists. I'm even tempted to make ext4 and f2fs support > > > > blk-crypto only someday. That was sort of the original plan, actually... > > > > > > > > So, I'm wondering if you've considered going the blk-crypto-fallback route? > > > > > > I did, and gave it a shot, but ran into problems because as far as I can > > > tell it requires having a bio to crypt. For verity data and inline extents, > > > there's no obvious bio, and even if we tried to construct a bio pointing at > > > the relevant data, it's not necessarily sector- sized or aligned. I couldn't > > > figure out a good way to make it work, but maybe it's better to special-case > > > those or there's something I'm not seeing. > > > > ext4 and f2fs just don't use inline data on encrypted files. I.e. when an encrypted file is > > created, it always uses non-inline data. Is that an option for btrfs? > > It's not impossible (though it's been viewed as a fair deficiency in last > year's changesets), but it's not the only user of data needing encryption > stored inline instead of separately: > > > > For the verity metadata, how are you thinking of encrypting it, exactly? Verity metadata is > > immutable once written, so surely it avoids many of the issues you are dealing with for extents? It > > should just need one key, and that key could be set up at file open time. So I don't think it will > > need the key pool at all? > > Yes, it should be able to use whatever the interface is for extent > encryption, whether that uses pooled keys or something else. However, btrfs > stores verity data in 2k chunks in the tree, similar to inline data, so it > has the same difficulties. > > (I realized after sending that we also want to encrypt xattrs, which are > similarly stored as items in the tree instead of blocks on disk.) > > We could have separate pools for inline and non-inline prepared keys (or not > pool inline keys at all?) > To clarify my suggestion, blk-crypto could be used for file contents en/decryption at the same time that filesystem-layer crypto is used for verity metadata en/decryption. blk-crypto and filesystem-layer crypto don't need to be mutually exclusive, even on the same file. Also, I'm glad that you're interested in xattr encryption, but unfortunately it's a tough problem, and all the other filesystems that implement fscrypt have left it out. You have enough other things to worry about, so I think leaving xattr encryption out for now is the right call. Similarly, the other filesystems that implement fscrypt have left out encryption of inline data, instead opting to disable inline data on encrypted files. Anyway, the main reason I'm sending this email is actually that I wanted to mention another possible solution to the per-extent key problem that I just became aware of. In v6.4-rc1, the crypto API added a new function crypto_clone_tfm() which allocates a new tfm object, given an existing one. Unlike crypto_alloc_tfm(), crypto_clone_tfm() doesn't take any locks. See: https://lore.kernel.org/linux-crypto/ZDefxOq6Ax0JeTRH@xxxxxxxxxxxxxxxxxxx/T/#u For now, only shash and ahash tfms can be cloned. However, it looks like support for cloning skcipher tfms could be added. With "cloning" skcipher tfms, there could just be a crypto_skcipher per extent, allocated on the I/O path. That would solve the problem we've been trying to solve, without having to bring in the complexity of "pooled prepared keys". I think we should go either with that or with the blk-crypto-fallback solution. - Eric
|