| Message ID | cover.1693630890.git.sweettea-kernel@dorminy.me (mailing list archive) |
|---|---|
| Headers | show |
| Series | fscrypt: add extent encryption | expand |
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
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/20230812223408.GA41642@sol.localdomain https://lore.kernel.org/r/20230812224144.GB41642@sol.localdomain https://lore.kernel.org/r/20230812224301.GC41642@sol.localdomain. 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/20230812223408.GA41642@sol.localdomain 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
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/cover.1691505882.git.sweettea-kernel@dorminy.me/ v2: - https://lore.kernel.org/linux-fscrypt/cover.1688927487.git.sweettea-kernel@dorminy.me/T/#t [3] https://lore.kernel.org/linux-fscrypt/cover.1691505830.git.sweettea-kernel@dorminy.me/ 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