Hi Sweet Tea,
On Wed, Jun 28, 2023 at 08:29:30PM -0400, Sweet Tea Dorminy wrote:
> This changeset adds extent-based data encryption to fscrypt.
> Some filesystems need to encrypt data based on extents, rather than on
> inodes, due to features incompatible with inode-based encryption. For
> instance, btrfs can have multiple inodes referencing a single block of
> data, and moves logical data blocks to different physical locations on
> disk in the background.
>
> As per discussion last year in [1] and later in [2], we would like to
> allow the use of fscrypt with btrfs, with authenticated encryption. This
> is the first step of that work, adding extent-based encryption to
> fscrypt; authenticated encryption is the next step. Extent-based
> encryption should be usable by other filesystems which wish to support
> snapshotting or background data rearrangement also, but btrfs is the
> first user.
>
> This changeset requires extent encryption to use inlinecrypt, as
> discussed previously. There are two questionable parts: the
> forget_extent_info hook is not yet in use by btrfs, as I haven't yet
> written a test exercising a race where it would be relevant; and saving
> the session key credentials just to enable v1 session-based policies is
> perhaps less good than
>
> This applies atop [3], which itself is based on kdave/misc-next. It
> passes most encryption fstests with suitable changes to btrfs-progs, but
> not generic/580 or generic/595 due to different timing involved in
> extent encryption. Tests and btrfs progs updates to follow.
>
>
> [1] https://docs.google.com/document/d/1janjxewlewtVPqctkWOjSa7OhCgB8Gdx7iDaCDQQNZA/edit?usp=sharing
> [2] https://lore.kernel.org/linux-fscrypt/80496cfe-161d-fb0d-8230-93818b966b1b@xxxxxxxxxx/T/#t
> [3]
> https://lore.kernel.org/linux-fscrypt/cover.1687988119.git.sweettea-kernel@xxxxxxxxxx/
>
> Sweet Tea Dorminy (12):
> fscrypt: factor helper for locking master key
> fscrypt: factor getting info for a specific block
> fscrypt: adjust effective lblks based on extents
> fscrypt: add a super_block pointer to fscrypt_info
> fscrypt: setup leaf inodes for extent encryption
> fscrypt: allow infos to be owned by extents
> fscrypt: notify per-extent infos if master key vanishes
> fscrypt: use an optional ino equivalent for per-extent infos
> fscrypt: add creation/usage/freeing of per-extent infos
> fscrypt: allow load/save of extent contexts
> fscrypt: save session key credentials for extent infos
> fscrypt: update documentation for per-extent keys
>
> Documentation/filesystems/fscrypt.rst | 38 +++-
> fs/crypto/crypto.c | 6 +-
> fs/crypto/fscrypt_private.h | 91 ++++++++++
> fs/crypto/inline_crypt.c | 28 ++-
> fs/crypto/keyring.c | 32 +++-
> fs/crypto/keysetup.c | 244 ++++++++++++++++++++++----
> fs/crypto/keysetup_v1.c | 7 +-
> fs/crypto/policy.c | 20 +++
> include/linux/fscrypt.h | 74 ++++++++
> 9 files changed, 480 insertions(+), 60 deletions(-)
>
>
> base-commit: accadeb67609a5a5d088ebde8409c3f6db0b84b4
Thanks for sending this out!
It's going to take me a while to go through everything, so please bear with me.
In general I'd also really like to be seeing more feedback from the other btrfs
developers. This is a hard project that really needs more eyes on it.
>From a brief look through your patchsets, there's one thing I want to bring up
right away. It seems that one important design choice that you've made that has
impacted much of your patchsets is that you've made each extent a fully
standalone thing, similar to inodes currently. I.e.,
(a) Each extent gets a full 'fscrypt_context' stored along with it. That
includes not just the nonce, but also the encryption modes and master
key identifier.
(b) For runtime caching, each extent gets a full 'struct fscrypt_info'
object. It doesn't "belong" to any inode; it's set up in a fully
standalone way, and the master key lookup and removal logic operates
directly on the extent's 'struct fscrypt_info'.
I'm not sure this is a good idea. What I had thought it was going to look like
is that the encryption context/policy and 'struct fscrypt_info' would stay a
property of the inode, and the extents themselves would be much more lightweight
-- both on disk and in the cache. On-disk, all that should really be needed for
an extent is the nonce for deriving the per-extent key. And in-memory, all that
should really be needed is a "fscrypt_prepared_key" for the per-extent key, and
a reference to the owning inode.
I think that would avoid many of the problems that it seems you've had to work
around or have had to change user-visible semantics for. For example the
problems involving master keys being added and removed. It would also avoid
having to overload 'fscrypt_info' to be either a per-inode or a per-extent key.
And it would save space on disk and in memory.
Can you elaborate on why you went with a more "heavyweight" extents design?
Maybe your motivation is that extents can be referenced by more than one inode
and thus do not have a unique owning inode? That's true, but I don't think that
really matters. All the inodes that reference an extent will have the same
encryption policy, right? Also, it looks like the "struct extent_map" that
you're caching the per-extent key in is already cached on a per-inode basis, in
btrfs_inode::extent_tree, similar to the pagecache which is also per-inode. So
if the same extent happens to be accessed via multiple inodes, that's still
going to cause the fscrypt key to be set up twice anyway.
- Eric
|