Re: [PATCH v3 00/16] fscrypt: add extent encryption

[Eric Biggers <ebiggers@kernel.org>]

Hi Sweet Tea,

On Tue, Aug 08, 2023 at 01:08:17PM -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. 
> 
> This applies atop [3], which itself is based on kdave/misc-next. It
> passes encryption fstests with suitable changes to btrfs-progs.
> 
> Changelog:
> v3:
>  - Added four additional changes:
>    - soft-deleting keys that extent infos might later need to use, so
>      the behavior of an open file after key removal matches inode-based
>      fscrypt.
>    - a set of changes to allow asynchronous info freeing for extents,
>      necessary due to locking constraints in btrfs.
> 
> v2: 
>  - https://lore.kernel.org/linux-fscrypt/cover.1688927487.git.sweettea-kernel@dorminy.me/T/#t
> 
> 
> [1] https://docs.google.com/document/d/1janjxewlewtVPqctkWOjSa7OhCgB8Gdx7iDaCDQQNZA/edit?usp=sharing
> [2] https://lore.kernel.org/linux-fscrypt/80496cfe-161d-fb0d-8230-93818b966b1b@dorminy.me/T/#t
> [3] https://lore.kernel.org/linux-fscrypt/cover.1691505830.git.sweettea-kernel@dorminy.me/
> 
> Sweet Tea Dorminy (16):
>   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: use an optional ino equivalent for per-extent infos
>   fscrypt: move function call warning of busy inodes
>   fscrypt: revamp key removal for extent encryption
>   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: allow multiple extents to reference one info
>   fscrypt: cache list of inlinecrypt devices
>   fscrypt: allow asynchronous info freeing
>   fscrypt: update documentation for per-extent keys
> 
>  Documentation/filesystems/fscrypt.rst |  43 +++-
>  fs/crypto/crypto.c                    |   6 +-
>  fs/crypto/fscrypt_private.h           | 158 +++++++++++-
>  fs/crypto/inline_crypt.c              |  49 ++--
>  fs/crypto/keyring.c                   |  78 +++---
>  fs/crypto/keysetup.c                  | 336 ++++++++++++++++++++++----
>  fs/crypto/keysetup_v1.c               |  10 +-
>  fs/crypto/policy.c                    |  20 ++
>  include/linux/fscrypt.h               |  67 +++++
>  9 files changed, 654 insertions(+), 113 deletions(-)

I'm taking a look at this, but I don't think it's really reviewable in its
current state.  The main problem is how the new functionality is reusing struct
fscrypt_info.  Before an fscrypt_info was the information (key, policy, inode
back-pointer, master key link, etc.) associated with an inode.  But now it can
be any of the following:

- Information for an inode
- Cached policy (?) for a "leaf inode"
- Information for an extent

Everything just seems kind of mixed together.  It's not clear which fields of
fscrypt_info apply to which scenario, and which scenario(s) is being handled
when code works with fscrypt_info.  There seem to be bugs caused by these
scenarios being mixed up.  Comments are also inconsistent; a huge number of them
still refer only to "inode" or "file" when that is no longer correct.  So it's
quite hard to understand the code now.

I think there really needs to be some work on designing and documenting the data
structures for what you are trying to accomplish.  That really ought to have
been the first step before getting deep into coding the actual functionality.

Have you considered creating a new struct like fscrypt_extent_info, instead of
reusing fscrypt_info?  I think that would make things much clearer.  I suppose
there is code that needs to operate on the shared fields of both, but that could
be done by putting the shared fields in a sub-struct like 'struct
fscrypt_common_info common;', and passing around a pointer to that where needed.

I'd also like to reiterate the concern I raised last month
(https://lore.kernel.org/linux-fscrypt/20230703045417.GA3057@sol.localdomain/):
a lot of this complexity seems to have been contributed to by the "heavyweight
extents" design choice.  I was hoping to see a detailed design for the "change a
directory's tree encryption policy" feature you are planning to use this for, in
order to get some confidence that it will actually be implemented.  Otherwise, I
fear we'll end up building in a lot of complexity for something that never gets
implemented.

- Eric