| Message ID | cover.1662420176.git.sweettea-kernel@dorminy.me (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: add fscrypt integration | expand |
On Mon, Sep 05, 2022 at 08:35:15PM -0400, Sweet Tea Dorminy wrote:
> This is a changeset adding encryption to btrfs.
What git tree and commit does this apply to?
- Eric
On 9/6/22 18:35, Eric Biggers wrote: > On Mon, Sep 05, 2022 at 08:35:15PM -0400, Sweet Tea Dorminy wrote: >> This is a changeset adding encryption to btrfs. > > What git tree and commit does this apply to? https://github.com/kdave/btrfs-devel/; branch misc-next. Should apply cleanly to its current tip, 76ccdc004e12312ea056811d530043ff11d050c6 .
On Tue, Sep 06, 2022 at 07:01:15PM -0400, Sweet Tea Dorminy wrote: > > > On 9/6/22 18:35, Eric Biggers wrote: > > On Mon, Sep 05, 2022 at 08:35:15PM -0400, Sweet Tea Dorminy wrote: > > > This is a changeset adding encryption to btrfs. > > > > What git tree and commit does this apply to? > > https://github.com/kdave/btrfs-devel/; branch misc-next. Should apply > cleanly to its current tip, 76ccdc004e12312ea056811d530043ff11d050c6 . Patch 8 wasn't received by linux-fscrypt for some reason, any idea why? $ b4 am cover.1662420176.git.sweettea-kernel@dorminy.me Looking up https://lore.kernel.org/linux-fscrypt/cover.1662420176.git.sweettea-kernel%40dorminy.me Grabbing thread from lore.kernel.org/linux-fscrypt/cover.1662420176.git.sweettea-kernel%40dorminy.me/t.mbox.gz Analyzing 22 messages in the thread Checking attestation on all messages, may take a moment... --- [PATCH v2 1/20] fscrypt: expose fscrypt_nokey_name [PATCH v2 2/20] fscrypt: add flag allowing partially-encrypted directories [PATCH v2 3/20] fscrypt: add fscrypt_have_same_policy() to check inode compatibility [PATCH v2 4/20] fscrypt: allow fscrypt_generate_iv() to distinguish filenames [PATCH v2 5/20] fscrypt: add extent-based encryption [PATCH v2 6/20] fscrypt: document btrfs' fscrypt quirks. [PATCH v2 7/20] btrfs: store directory's encryption state ERROR: missing [8/20]! [PATCH v2 9/20] btrfs: setup fscrypt_names from dentrys using helper [PATCH v2 10/20] btrfs: factor a fscrypt_name matching method [PATCH v2 11/20] btrfs: disable various operations on encrypted inodes [PATCH v2 12/20] btrfs: start using fscrypt hooks. [PATCH v2 13/20] btrfs: add fscrypt_context items. [PATCH v2 14/20] btrfs: translate btrfs encryption flags and encrypted inode flag. [PATCH v2 15/20] btrfs: store a fscrypt extent context per normal file extent [PATCH v2 16/20] btrfs: Add new FEATURE_INCOMPAT_FSCRYPT feature flag. [PATCH v2 17/20] btrfs: reuse encrypted filename hash when possible. [PATCH v2 18/20] btrfs: adapt directory read and lookup to potentially encrypted filenames [PATCH v2 19/20] btrfs: encrypt normal file extent data if appropriate [PATCH v2 20/20] btrfs: implement fscrypt ioctls
On 2022-09-06 19:10, Eric Biggers wrote: > On Tue, Sep 06, 2022 at 07:01:15PM -0400, Sweet Tea Dorminy wrote: >> >> >> On 9/6/22 18:35, Eric Biggers wrote: >> > On Mon, Sep 05, 2022 at 08:35:15PM -0400, Sweet Tea Dorminy wrote: >> > > This is a changeset adding encryption to btrfs. >> > >> > What git tree and commit does this apply to? >> >> https://github.com/kdave/btrfs-devel/; branch misc-next. Should apply >> cleanly to its current tip, 76ccdc004e12312ea056811d530043ff11d050c6 . > > Patch 8 wasn't received by linux-fscrypt for some reason, any idea why? > > $ b4 am cover.1662420176.git.sweettea-kernel@dorminy.me > Looking up > https://lore.kernel.org/linux-fscrypt/cover.1662420176.git.sweettea-kernel%40dorminy.me > Grabbing thread from > lore.kernel.org/linux-fscrypt/cover.1662420176.git.sweettea-kernel%40dorminy.me/t.mbox.gz > Analyzing 22 messages in the thread > Checking attestation on all messages, may take a moment... > --- > [PATCH v2 1/20] fscrypt: expose fscrypt_nokey_name > [PATCH v2 2/20] fscrypt: add flag allowing partially-encrypted > directories > [PATCH v2 3/20] fscrypt: add fscrypt_have_same_policy() to check > inode compatibility > [PATCH v2 4/20] fscrypt: allow fscrypt_generate_iv() to distinguish > filenames > [PATCH v2 5/20] fscrypt: add extent-based encryption > [PATCH v2 6/20] fscrypt: document btrfs' fscrypt quirks. > [PATCH v2 7/20] btrfs: store directory's encryption state > ERROR: missing [8/20]! > [PATCH v2 9/20] btrfs: setup fscrypt_names from dentrys using helper > [PATCH v2 10/20] btrfs: factor a fscrypt_name matching method > [PATCH v2 11/20] btrfs: disable various operations on encrypted > inodes > [PATCH v2 12/20] btrfs: start using fscrypt hooks. > [PATCH v2 13/20] btrfs: add fscrypt_context items. > [PATCH v2 14/20] btrfs: translate btrfs encryption flags and > encrypted inode flag. > [PATCH v2 15/20] btrfs: store a fscrypt extent context per normal > file extent > [PATCH v2 16/20] btrfs: Add new FEATURE_INCOMPAT_FSCRYPT feature > flag. > [PATCH v2 17/20] btrfs: reuse encrypted filename hash when possible. > [PATCH v2 18/20] btrfs: adapt directory read and lookup to > potentially encrypted filenames > [PATCH v2 19/20] btrfs: encrypt normal file extent data if > appropriate > [PATCH v2 20/20] btrfs: implement fscrypt ioctls Patch 8 is large and dull, and bounced from linux-fscrypt@ for size, but b4 gets it for me. b4 -p linux-fscrypt recreates the missing [8/20]; is it possible that option is set somewhere in your configuration? $ b4 am -o- cover.1662420176.git.sweettea-kernel@dorminy.me | git am Looking up https://lore.kernel.org/r/cover.1662420176.git.sweettea-kernel%40dorminy.me Grabbing thread from lore.kernel.org/all/cover.1662420176.git.sweettea-kernel%40dorminy.me/t.mbox.gz Analyzing 23 messages in the thread Checking attestation on all messages, may take a moment... --- ✓ [PATCH v2 1/20] fscrypt: expose fscrypt_nokey_name ✓ [PATCH v2 2/20] fscrypt: add flag allowing partially-encrypted directories ✓ [PATCH v2 3/20] fscrypt: add fscrypt_have_same_policy() to check inode compatibility ✓ [PATCH v2 4/20] fscrypt: allow fscrypt_generate_iv() to distinguish filenames ✓ [PATCH v2 5/20] fscrypt: add extent-based encryption ✓ [PATCH v2 6/20] fscrypt: document btrfs' fscrypt quirks. ✓ [PATCH v2 7/20] btrfs: store directory's encryption state ✓ [PATCH v2 8/20] btrfs: use fscrypt_names instead of name/len everywhere. ✓ [PATCH v2 9/20] btrfs: setup fscrypt_names from dentrys using helper ✓ [PATCH v2 10/20] btrfs: factor a fscrypt_name matching method ✓ [PATCH v2 11/20] btrfs: disable various operations on encrypted inodes + Reported-by: kernel test robot <lkp@intel.com> (✓ DKIM/intel.com) ✓ [PATCH v2 12/20] btrfs: start using fscrypt hooks. ✓ [PATCH v2 13/20] btrfs: add fscrypt_context items. ✓ [PATCH v2 14/20] btrfs: translate btrfs encryption flags and encrypted inode flag. ✓ [PATCH v2 15/20] btrfs: store a fscrypt extent context per normal file extent ✓ [PATCH v2 16/20] btrfs: Add new FEATURE_INCOMPAT_FSCRYPT feature flag. ✓ [PATCH v2 17/20] btrfs: reuse encrypted filename hash when possible. ✓ [PATCH v2 18/20] btrfs: adapt directory read and lookup to potentially encrypted filenames ✓ [PATCH v2 19/20] btrfs: encrypt normal file extent data if appropriate ✓ [PATCH v2 20/20] btrfs: implement fscrypt ioctls --- ✓ Signed: DKIM/dorminy.me --- Total patches: 20 --- Link: https://lore.kernel.org/r/cover.1662420176.git.sweettea-kernel@dorminy.me ...
On Mon, Sep 05, 2022 at 08:35:15PM -0400, Sweet Tea Dorminy wrote: > This is a changeset adding encryption to btrfs. > > Last October, Omar Sandoval sent out a design document for having fscrypt > integration with btrfs [1]. In summary, it proposes btrfs storing its > own encryption IVs on a per-file-extent basis. fscrypt usually encrypts > files using an IV derived from per-inode information; this would prevent > snapshotting or reflinking or data relocation for btrfs. We have > refined this into a fscrypt extent context object, opaque to the > filesystem, which fscrypt uses to generate an IV associated with each > block in an extent. Thus, all the inodes sharing a particular > key and file extent may decrypt the extent. > > This series implements this integration for the simple > case, non-compressed data extents. Followup changes will allow > encryption of compressed extents, inline extents, and verity items, and > will add tests around subvolume encryption. This series should provide > encryption for the simplest cases, but this series should not be used in > production, as there are likely bugs. > > Preliminary btrfs-progs changes are available at [2]; fstests changes > are available at [3]. I did a quick pass to check if there's anything that could be merged to 6.1 as preparatory, the fname is a candidate but I've also seen random coding style issues that I'd like to get fixed first. One thing I've noticed is that the incompat bit is only defined but not used anywhere. Any functional change should be guarded behind it, and once the implementation is complete the enabling part is in a separate patch. Regarding the build config options, I assume that the fscrypt support is optional, so it should build with and without the option. I'm not sure I've see enough ifdefs for that. For such features there should be a line in btrfs_print_mod_info, like we have eg. for verity. I'll post other comments under the patches.
On Mon, Sep 05, 2022 at 08:35:23PM -0400, Sweet Tea Dorminy wrote: > For encryption, the plaintext filenames provided by the VFS will need to > be translated to ciphertext filenames on disk. Fscrypt provides a struct > to encapsulate a potentially encrypted filename, struct fscrypt_name. > This change converts every (name, len) pair to be a struct fscrypt_name, > statically initialized, for ease of review and uniformity. Is there some clear boundary where the name needs to be encoded or decoded? I don't think we should use fscrypt_name in so many functions, namely internal helpers that really only care about the plain name + length. Such widespread use fscrypt structure would make it hard to synchronize with the user space sources. What we could do to ease the integragtion with the fscrypt name is to use the qstr structure, ie. something that's easily convertible to the fscrypt_name::usr_fname. > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -21,11 +21,13 @@ > #include <linux/pagemap.h> > #include <linux/btrfs.h> > #include <linux/btrfs_tree.h> > +#include <linux/fscrypt.h> > #include <linux/workqueue.h> > #include <linux/security.h> > #include <linux/sizes.h> > #include <linux/dynamic_debug.h> > #include <linux/refcount.h> > +#include <linux/crc32.h> > #include <linux/crc32c.h> > #include <linux/iomap.h> > #include "extent-io-tree.h" > @@ -2803,18 +2805,19 @@ static inline void btrfs_crc32c_final(u32 crc, u8 *result) > put_unaligned_le32(~crc, result); > } > > -static inline u64 btrfs_name_hash(const char *name, int len) > +static inline u64 btrfs_name_hash(const struct fscrypt_name *name) > { > - return crc32c((u32)~1, name, len); > + return crc32c((u32)~1, fname_name(name), fname_len(name)); This for example is a primitive helper that just hashes the correct bytes and does not need to know anything about whether it's encrypted or not. That should be set up higher in the call chain. > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -3863,11 +3863,19 @@ static noinline int acls_after_inode_item(struct extent_buffer *leaf, > static u64 xattr_default = 0; > int scanned = 0; > > + struct fscrypt_name name_access = { > + .disk_name = FSTR_INIT(XATTR_NAME_POSIX_ACL_ACCESS, > + strlen(XATTR_NAME_POSIX_ACL_ACCESS)) > + }; > + > + struct fscrypt_name name_default = { > + .disk_name = FSTR_INIT(XATTR_NAME_POSIX_ACL_DEFAULT, > + strlen(XATTR_NAME_POSIX_ACL_DEFAULT)) > + }; > + > if (!xattr_access) { > - xattr_access = btrfs_name_hash(XATTR_NAME_POSIX_ACL_ACCESS, > - strlen(XATTR_NAME_POSIX_ACL_ACCESS)); > - xattr_default = btrfs_name_hash(XATTR_NAME_POSIX_ACL_DEFAULT, > - strlen(XATTR_NAME_POSIX_ACL_DEFAULT)); > + xattr_access = btrfs_name_hash(&name_access); > + xattr_default = btrfs_name_hash(&name_default); And here it needs extra structure just to pass plain strings. > + __func__, fname_name(&fname), btrfs_ino(BTRFS_I(dir)), > location->objectid, location->type, location->offset); > } > if (!ret) > @@ -6243,6 +6258,14 @@ int btrfs_new_inode_prepare(struct btrfs_new_inode_args *args, > if (ret) > return ret; > > + if (!args->orphan) { > + char *name = (char *) args->dentry->d_name.name; > + int name_len = args->dentry->d_name.len; Please put a newline between declaration and statement block. > + args->fname = (struct fscrypt_name) { > + .disk_name = FSTR_INIT(name, name_len), > + }; Please don't use this construct to intialize compounds, we don't use it anywhere. There are more examples for other structures too. > + } > + > /* 1 to add inode item */ > *trans_num_items = 1; > /* 1 to add compression property */ > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -1596,8 +1596,9 @@ static int qgroup_account_snapshot(struct btrfs_trans_handle *trans, > * happens, we should return the error number. If the error which just affect > * the creation of the pending snapshots, just return 0. > */ > -static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > - struct btrfs_pending_snapshot *pending) > +static noinline int > +create_pending_snapshot(struct btrfs_trans_handle *trans, > + struct btrfs_pending_snapshot *pending) Please keep the specifiers and type on the same line as the function name, the parameters can slightly overfrlow the 80 char limit if it avoids a line break, otherwise the patameters go on the next line.
This is a changeset adding encryption to btrfs. Last October, Omar Sandoval sent out a design document for having fscrypt integration with btrfs [1]. In summary, it proposes btrfs storing its own encryption IVs on a per-file-extent basis. fscrypt usually encrypts files using an IV derived from per-inode information; this would prevent snapshotting or reflinking or data relocation for btrfs. We have refined this into a fscrypt extent context object, opaque to the filesystem, which fscrypt uses to generate an IV associated with each block in an extent. Thus, all the inodes sharing a particular key and file extent may decrypt the extent. This series implements this integration for the simple case, non-compressed data extents. Followup changes will allow encryption of compressed extents, inline extents, and verity items, and will add tests around subvolume encryption. This series should provide encryption for the simplest cases, but this series should not be used in production, as there are likely bugs. Preliminary btrfs-progs changes are available at [2]; fstests changes are available at [3]. [1] https://lore.kernel.org/linux-btrfs/YXGyq+buM79A1S0L@relinquished.localdomain/ Changelog: v2: - Amended the fscrypt side to generically add extent contexts, hopefully as per Eric Biggers' past comments. IVs are now entirely abstracted within an extent context, and there is no longer a new encryption policy, as DIRECT_KEY sufficiently encapsulates the needs of extent-based encryption. Documented its usage in btrfs briefly in the documentation. - Adjusted the btrfs side to deal in opaque extent contexts. Improved optimization to skip storing inode contexts if they are the same as the inode's root item's inode context. - Combined 'add fscrypt operation table to superblock' into 'start using fscrypt hooks'. - https://lore.kernel.org/linux-btrfs/cover.1662420176.git.sweettea-kernel@dorminy.me - progs: https://lore.kernel.org/linux-btrfs/cover.1662417859.git.sweettea-kernel@dorminy.me - tests: https://lore.kernel.org/linux-btrfs/cover.1662417905.git.sweettea-kernel@dorminy.me v1: - Recombined the fscrypt changes back into this patchset. - Fixed several races and incorrectly ordered operations. - Improved IV retrieval to correctly distinguish between filename/symlink encryption and encryption of block 0 of a file. - https://lore.kernel.org/linux-btrfs/cover.1660744500.git.sweettea-kernel@dorminy.me - progs: https://lore.kernel.org/linux-btrfs/cover.1660729916.git.sweettea-kernel@dorminy.me - tests: https://lore.kernel.org/linux-btrfs/cover.1660729861.git.sweettea-kernel@dorminy.me RFC v2: - Fixed all warnings and known incorrectnesses. - Split fscrypt changes into their own patchset: https://lore.kernel.org/linux-fscrypt/cover.1658623235.git.sweettea-kernel@dorminy.me - Combined and reordered changes so that enabling fscrypt is the last change. - Removed unnecessary factoring. - Split a cleanup change off. - https://lore.kernel.org/linux-btrfs/cover.1658623319.git.sweettea-kernel@dorminy.me RFC v1: - https://lore.kernel.org/linux-btrfs/cover.1657707686.git.sweettea-kernel@dorminy.me Omar Sandoval (14): fscrypt: expose fscrypt_nokey_name fscrypt: add flag allowing partially-encrypted directories fscrypt: add fscrypt_have_same_policy() to check inode compatibility btrfs: store directory's encryption state btrfs: factor a fscrypt_name matching method btrfs: disable various operations on encrypted inodes btrfs: start using fscrypt hooks. btrfs: add fscrypt_context items. btrfs: translate btrfs encryption flags and encrypted inode flag. btrfs: Add new FEATURE_INCOMPAT_FSCRYPT feature flag. btrfs: reuse encrypted filename hash when possible. btrfs: adapt directory read and lookup to potentially encrypted filenames btrfs: encrypt normal file extent data if appropriate btrfs: implement fscrypt ioctls Sweet Tea Dorminy (6): fscrypt: allow fscrypt_generate_iv() to distinguish filenames fscrypt: add extent-based encryption fscrypt: document btrfs' fscrypt quirks. btrfs: use fscrypt_names instead of name/len everywhere. btrfs: setup fscrypt_names from dentrys using helper btrfs: store a fscrypt extent context per normal file extent Documentation/filesystems/fscrypt.rst | 62 ++- fs/btrfs/Makefile | 1 + fs/btrfs/btrfs_inode.h | 3 + fs/btrfs/ctree.h | 119 ++++-- fs/btrfs/delayed-inode.c | 48 ++- fs/btrfs/delayed-inode.h | 9 +- fs/btrfs/dir-item.c | 119 +++--- fs/btrfs/extent_io.c | 93 ++++- fs/btrfs/extent_io.h | 2 + fs/btrfs/extent_map.h | 4 + fs/btrfs/file-item.c | 22 +- fs/btrfs/file.c | 11 +- fs/btrfs/fscrypt.c | 244 +++++++++++ fs/btrfs/fscrypt.h | 49 +++ fs/btrfs/inode-item.c | 84 ++-- fs/btrfs/inode-item.h | 14 +- fs/btrfs/inode.c | 581 +++++++++++++++++++------- fs/btrfs/ioctl.c | 80 +++- fs/btrfs/ordered-data.c | 9 +- fs/btrfs/ordered-data.h | 4 +- fs/btrfs/print-tree.c | 4 +- fs/btrfs/props.c | 11 +- fs/btrfs/reflink.c | 8 + fs/btrfs/root-tree.c | 20 +- fs/btrfs/send.c | 141 ++++--- fs/btrfs/super.c | 8 +- fs/btrfs/transaction.c | 43 +- fs/btrfs/tree-checker.c | 56 ++- fs/btrfs/tree-log.c | 237 ++++++----- fs/btrfs/tree-log.h | 4 +- fs/btrfs/xattr.c | 21 +- fs/crypto/crypto.c | 24 +- fs/crypto/fname.c | 60 +-- fs/crypto/fscrypt_private.h | 26 +- fs/crypto/inline_crypt.c | 29 +- fs/crypto/policy.c | 103 +++++ include/linux/fscrypt.h | 81 ++++ include/uapi/linux/btrfs.h | 1 + include/uapi/linux/btrfs_tree.h | 26 ++ 39 files changed, 1881 insertions(+), 580 deletions(-) create mode 100644 fs/btrfs/fscrypt.c create mode 100644 fs/btrfs/fscrypt.h