[RFC,00/13] fscrypt: add extent encryption

Message

Sweet Tea Dorminy Sept. 2, 2023, 5:54 a.m. UTC

This is a replacement for the former changeset (previously v3). This
doesn't reflect all the smaller feedback on v3: it's an attempt to address
the major points of giving extents and inodes different objects, and to
clearly define lightweight and heavyweight extent contexts. Currently,
with minor changes to the btrfs patchset building on it, it passes
tests.

Hopefully I understood the proposed alternate design and this is indeed
more elegant, reviewable, and maintainable. 

This applies atop [3], which itself is based on kdave/misc-next.

Changelog:
RFC:
 - Split fscrypt_info into a general fscrypt_common_info, an
   inode-specific fscrypt_info, and an extent-specific
   fscrypt_extent_info. All external interfaces use either an inode or
   extent specific structure; most internal functions handle the common
   structure.
 - Tried to fix up more places to refer to infos instead of inodes and
   files.
 - Changed to use lightweight extent contexts containing just a nonce,
   and then a following change to do heavyweight extent contexts
   identical to inode contexts, so they're easily comparable.
 - Dropped factoring lock_master_key() and adding super block pointer to
   fscrypt_info changes, as they didn't seem necessary.
 - Temporarily dropped optimization where leaf inodes with extents don't
   have on-disk fscrypt_contexts. It's a convenient optimization and
   affects btrfs disk format, but it's not very big and not strictly
   needed to check whether the new structural arrangement is better.

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.
 - https://lore.kernel.org/linux-fscrypt/[e-mail]/

v2: 
 - https://lore.kernel.org/linux-fscrypt/[e-mail]/T/#t


[3] https://lore.kernel.org/linux-fscrypt/[e-mail]/

Sweet Tea Dorminy (13):
  fscrypt: factor getting info for a specific block
  fscrypt: adjust effective lblks based on extents
  fscrypt: move function call warning of busy inodes
  fscrypt: split fscrypt_info into general and inode specific parts
  fscrypt: add creation/usage/freeing of per-extent infos
  fscrypt: allow load/save of extent contexts
  fscrypt: store full fscrypt_contexts for each extent
  fscrypt: save session key credentials for extent infos
  fscrypt: revamp key removal for extent encryption
  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                    |  48 ++-
 fs/crypto/fname.c                     |  13 +-
 fs/crypto/fscrypt_private.h           | 245 +++++++++---
 fs/crypto/hooks.c                     |   6 +-
 fs/crypto/inline_crypt.c              |  93 +++--
 fs/crypto/keyring.c                   | 110 +++---
 fs/crypto/keysetup.c                  | 530 ++++++++++++++++++++------
 fs/crypto/keysetup_v1.c               |  77 ++--
 fs/crypto/policy.c                    |  34 +-
 include/linux/fscrypt.h               |  60 +++
 11 files changed, 919 insertions(+), 340 deletions(-)


base-commit: 764e1420e0806a3536b53b4c52c1b08ae8425f7e

Comments

Josef Bacik Sept. 5, 2023, 5:33 p.m. UTC | #1

On Sat, Sep 02, 2023 at 01:54:18AM -0400, Sweet Tea Dorminy wrote:
> This is a replacement for the former changeset (previously v3). This
> doesn't reflect all the smaller feedback on v3: it's an attempt to address
> the major points of giving extents and inodes different objects, and to
> clearly define lightweight and heavyweight extent contexts. Currently,
> with minor changes to the btrfs patchset building on it, it passes
> tests.
> 
> Hopefully I understood the proposed alternate design and this is indeed
> more elegant, reviewable, and maintainable. 
> 
> This applies atop [3], which itself is based on kdave/misc-next.
> 
> Changelog:
> RFC:
>  - Split fscrypt_info into a general fscrypt_common_info, an
>    inode-specific fscrypt_info, and an extent-specific
>    fscrypt_extent_info. All external interfaces use either an inode or
>    extent specific structure; most internal functions handle the common
>    structure.
>  - Tried to fix up more places to refer to infos instead of inodes and
>    files.
>  - Changed to use lightweight extent contexts containing just a nonce,
>    and then a following change to do heavyweight extent contexts
>    identical to inode contexts, so they're easily comparable.
>  - Dropped factoring lock_master_key() and adding super block pointer to
>    fscrypt_info changes, as they didn't seem necessary.
>  - Temporarily dropped optimization where leaf inodes with extents don't
>    have on-disk fscrypt_contexts. It's a convenient optimization and
>    affects btrfs disk format, but it's not very big and not strictly
>    needed to check whether the new structural arrangement is better.

I've gone through this, does seem a bit cleaner, and the uses of ->ci_type are
limited to the soft deletion part, so the overlapping thing that Eric was
worried about seems to be very contained.

Eric, I asked Sweet Tea to do a rough run at this to see if this was more in
your liking, I specifically told him to get it down and get it out so apologies
for the rough edges.  What I'm looking for is wether or not this is an
acceptable approach, and if there's any other big changes you want to see.  If
this looks good then Sweet Tea can clean it up and we can hopefully start making
progress on the other things.  Thanks,

Josef

Eric Biggers Sept. 7, 2023, 5:52 a.m. UTC | #2

On Sat, Sep 02, 2023 at 01:54:18AM -0400, Sweet Tea Dorminy wrote:
> This is a replacement for the former changeset (previously v3). This
> doesn't reflect all the smaller feedback on v3: it's an attempt to address
> the major points of giving extents and inodes different objects, and to
> clearly define lightweight and heavyweight extent contexts.

In general it would be helpful if more of the feedback I've already given was
addressed, e.g.
https://lore.kernel.org/r/[e-mail]
https://lore.kernel.org/r/[e-mail]
https://lore.kernel.org/r/[e-mail].
It's hard to review this when things that are being proposed "for real" are
mixed in with things that just haven't had time to be fixed yet, and it's
not obvious which are which.
https://lore.kernel.org/r/[e-mail] in particular
is important and would affect the parts which it seems I'm being asked to
review.

> Changelog:
> RFC:
>  - Split fscrypt_info into a general fscrypt_common_info, an
>    inode-specific fscrypt_info, and an extent-specific
>    fscrypt_extent_info. All external interfaces use either an inode or
>    extent specific structure; most internal functions handle the common
>    structure.

If we indeed go this route, the inode one should be called fscrypt_inode_info,
right?  Also, this could be done in 3 patches to make it easier to review: (1)
rename fscrypt_info => fscrypt_inode_info, (2) add fscrypt_common_info and put
it in fscrypt_inode_info, and (3) add fscrypt_extent_info.

>  - Tried to fix up more places to refer to infos instead of inodes and
>    files.

I don't think the things that are being encrypted should be called "infos".
"Info" is just what fs/crypto/ happens to the call the data structure that holds
the key, encryption settings, etc. for an object (inode or extent) subject to FS
encryption.  It's not a great name, and in any case it's not the same as the
object itself.  When we're not literally talking about the data structure
itself, code comments should say something like "object represented by the info
@ci" or "file or extent for the info @ci".  Other suggestions appreciated, of
course.  But we should not refer to the things being encrypted as "infos".
"Info" is the C struct that the code happens to use, nothing more.

>  - Changed to use lightweight extent contexts containing just a nonce,
>    and then a following change to do heavyweight extent contexts
>    identical to inode contexts, so they're easily comparable.

This seems to be referring to "fscrypt: store full fscrypt_contexts for each
extent".  But, that just changes what is stored for each extent.  The rest of
this patchset still very much assumes the heavyweight design, and it brings in
the complexity from that.  E.g., the proposed fscrypt_extent_info has a lot of
fields which are not necessary if there was an associated per-inode struct that
the policy, mode, master key, etc. could be pulled from.  Also the master key
link, since only inodes really need to be in the master key's list, right?

I understand that you want to be able to assign an encryption policy to an
unencrypted file and have new extents be encrypted using that policy.  I never
received a real answer to my question about what the plan is to recursively
change a whole directory tree, but sure, let's assume this will be supported by
iterating through every file and directory and setting something on them (for
directories it would have to be a new kind of inherit-only policy; also for this
to work at all you'd have to relax the current fscrypt semantics described in
https://www.kernel.org/doc/html/next/filesystems/fscrypt.html#encryption-policy-enforcement).
This still means that the encryption policy for each extent, if it has one, will
match the file that contains it.  So I don't see why it's necessary to have all
complexity of setting up everything completely independently for each extent.

It does sound like you still want to store the full information for each extent
anyway.  In that case, how about doing that but just making the kernel validate
at runtime that it matches the corresponding inode's?  (Yes, extent => inode is
a one-to-many relationship on-disk, but in the cache it's one-to-one I think.)
I think that would be a good middle ground.  It would allow the implementation
to be simple for now, with lightweight "struct fscrypt_extent_info", but all the
information for each extent would be stored on-disk for futureproofing.

- Eric