| Message ID | cover.1681871298.git.sweettea-kernel@dorminy.me (mailing list archive) |
|---|---|
| Headers | show |
| Series | fscrypt: add pooled prepared keys facility | expand |
Hi Sweet Tea, On Tue, Apr 18, 2023 at 10:42:09PM -0400, Sweet Tea Dorminy wrote: > This is part two of two of preliminaries to extent-based encryption, > adding a facility to pool pre-allocated prepared keys and use them at IO > time. > > While arguably one structure within the feature, and not actually used > in this changeset at that, it's a disjoint piece that has various taste > questions so I've put it in its own changeset here for good or ill. > > The change has been tested by switching a false to true so as to use it > for leaf inodes which are doing contents encryption, and then running > the standard tests. Such a thing changes the timing of when the prepared > key is set up, obviously, so that IO which begins after a master key > secret is removed no longer succeeds; this fails generic/{580,581,593} > which don't have that expectation. However, this code has no impact on > tests if disabled. > > Known suboptimalities: > -right now at the end nothing calls fscrypt_shrink_key_pool() and it > throws an unused function warning. > -right now it's hooked up to be used by leaf inodes not using inline > encryption only. I don't know if there's any interest in pooling inode > keys -- it could reduce memory usage on memory-constrained platforms -- > and if so using it for filename encryption also might make sense. On the > other hand, if there's no interest, the code allowing use of it in the normal > inode-info path is unnecessary. > -right now it doesn't pool inline encryption objects either. > -the initialization of a key pool for each mode spams the log with > "Missing crypto API support" messages. Maybe the init of key pools > should be the first time an info using pooled prepared keys is observed? > > Some questions: > > -does the pooling mechanism need to be extended to mode keys, which can > easily be pre-allocated if needed? > -does it need to be extended to v1 policies? > -does it need to be behind a config option, perhaps with extent > encryption? > -should it be in its own, new file, since it adds a decent chunk of code > to keysetup.c most of which is arguably key-agnostic? > > This changeset should apply atop the previous one, entitled > 'fscrypt: rearrangements preliminary to extent encryption' > lore.kernel.org/r/cover.1681837335.git.sweettea-kernel@dorminy.me Sorry for the slow response; I've been catching up after being on vacation. I've also been having trouble understanding out what this patchset is doing and what the next part is likely to be after it. I'm worried that this may be going down a path of something much more complex than needed. It's hard to follow the new logic with the new data structures and locks, keys being "stolen" from one file to another, etc. I'm also confused by how the pooled keys are being assigned to a field in fscrypt_info, given that fscrypt_info is for an inode, not an extent. So it's a bit unclear (at least to me) how this proposal will lead to extent-based encryption. 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 expect that it would be a lot easier than what you seem to be trying. The main thing to consider would be exactly how to handle the key derivation. In the long run, it might make sense to add key derivation support to blk-crypto, as there is actually inline encryption hardware that supports per-file key derivation in hardware already and it would be nice to support that someday. But for now, maybe just have the filesystem derive the key for each extent, upon first access to that extent, and cache the resulting blk_crypto_key in the appropriate btrfs data structure for the extent ('struct extent_state' maybe)? Can you think of any reason why that wouldn't work? There is also the issue of authenticated encryption (which I understand you're going to add after unauthenticated encryption is working). That will take some work to add support to blk-crypto. However, the same would be true for filesystem-layer contents encryption too. So I'm not sure that would be an argument for filesystem-layer contents encryption over blk-crypto... - Eric
> 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.
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? 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? - Eric
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?) Thanks! Sweet Tea
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@gondor.apana.org.au/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
> 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. That's a great point. I'm dropping these two patchsets and updating the original one at the start of the year to use blk-crypto, as per our discussions both here and at LSF. > > 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. A good point. I'll defer xattrs and inline data (and verity) for the first round, and add in doing those with inode infos after getting extent infos working well. > > 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@gondor.apana.org.au/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". Huh. A cool new thing for sure. I suppose one would have an initial tfm per supported crypto alg, and clone it for each extent as needed. That's definitely better than pooling prepared keys. I'll explore this after everything else, and work on blk-crypto oriented for now. Thanks! Sweet Tea
This is part two of two of preliminaries to extent-based encryption, adding a facility to pool pre-allocated prepared keys and use them at IO time. While arguably one structure within the feature, and not actually used in this changeset at that, it's a disjoint piece that has various taste questions so I've put it in its own changeset here for good or ill. The change has been tested by switching a false to true so as to use it for leaf inodes which are doing contents encryption, and then running the standard tests. Such a thing changes the timing of when the prepared key is set up, obviously, so that IO which begins after a master key secret is removed no longer succeeds; this fails generic/{580,581,593} which don't have that expectation. However, this code has no impact on tests if disabled. Known suboptimalities: -right now at the end nothing calls fscrypt_shrink_key_pool() and it throws an unused function warning. -right now it's hooked up to be used by leaf inodes not using inline encryption only. I don't know if there's any interest in pooling inode keys -- it could reduce memory usage on memory-constrained platforms -- and if so using it for filename encryption also might make sense. On the other hand, if there's no interest, the code allowing use of it in the normal inode-info path is unnecessary. -right now it doesn't pool inline encryption objects either. -the initialization of a key pool for each mode spams the log with "Missing crypto API support" messages. Maybe the init of key pools should be the first time an info using pooled prepared keys is observed? Some questions: -does the pooling mechanism need to be extended to mode keys, which can easily be pre-allocated if needed? -does it need to be extended to v1 policies? -does it need to be behind a config option, perhaps with extent encryption? -should it be in its own, new file, since it adds a decent chunk of code to keysetup.c most of which is arguably key-agnostic? This changeset should apply atop the previous one, entitled 'fscrypt: rearrangements preliminary to extent encryption' lore.kernel.org/r/cover.1681837335.git.sweettea-kernel@dorminy.me Sweet Tea Dorminy (7): fscrypt: add new pooled prepared keys. fscrypt: set up pooled keys upon first use fscrypt: add pooling of pooled prepared keys. fscrypt: add pooled prepared key locking around use fscrypt: reclaim pooled prepared keys under pressure fscrypt: add facility to shrink a pool of keys fscrypt: add lru mechanism to choose pooled key fs/crypto/crypto.c | 28 ++- fs/crypto/fscrypt_private.h | 37 +++ fs/crypto/keyring.c | 7 + fs/crypto/keysetup.c | 440 +++++++++++++++++++++++++++++++++--- 4 files changed, 468 insertions(+), 44 deletions(-)