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@xxxxxxxxxx
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
|