Re: [PATCH v2 15/20] btrfs: store a fscrypt extent context per normal file extent

David Sterba
2022-09-07T21:16:22Z

On Mon, Sep 05, 2022 at 08:35:30PM -0400, Sweet Tea Dorminy wrote:
> In order to encrypt data, each file extent must have its own persistent
> fscrypt_extent_context, which is then provided to fscrypt upon request.
> This is only needed for encrypted extents and is of variable size on
> disk, so file extents must additionally keep track of their actual
> length.
>
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
> fs/btrfs/ctree.h | 30 +++++++++++
> fs/btrfs/extent_map.h | 4 ++
> fs/btrfs/file-item.c | 13 +++++
> fs/btrfs/file.c | 4 +-
> fs/btrfs/fscrypt.c | 21 ++++++++
> fs/btrfs/fscrypt.h | 23 +++++++++
> fs/btrfs/inode.c | 89 +++++++++++++++++++++++++--------
> fs/btrfs/ordered-data.c | 9 +++-
> fs/btrfs/ordered-data.h | 4 +-
> fs/btrfs/reflink.c | 1 +
> fs/btrfs/tree-checker.c | 36 ++++++++++---
> fs/btrfs/tree-log.c | 11 +++-
> include/uapi/linux/btrfs_tree.h | 9 ++++
> 13 files changed, 220 insertions(+), 34 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index f0a16c32110d..38927a867028 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -37,6 +37,7 @@
> #include "async-thread.h"
> #include "block-rsv.h"
> #include "locking.h"
> +#include "fscrypt.h"
>
> struct btrfs_trans_handle;
> struct btrfs_transaction;
> @@ -1437,6 +1438,7 @@ struct btrfs_replace_extent_info {
> u64 file_offset;
> /* Pointer to a file extent item of type regular or prealloc. */
> char *extent_buf;
> + u32 extent_buf_size;

Please add a comment

> /*
> * Set to true when attempting to replace a file range with a new extent
> * described by this structure, set to false when attempting to clone an
> @@ -2659,6 +2661,16 @@ BTRFS_SETGET_STACK_FUNCS(stack_file_extent_disk_num_bytes,
> struct btrfs_file_extent_item, disk_num_bytes, 64);
> BTRFS_SETGET_STACK_FUNCS(stack_file_extent_compression,
> struct btrfs_file_extent_item, compression, 8);
> +BTRFS_SETGET_STACK_FUNCS(stack_file_extent_encryption,
> + struct btrfs_file_extent_item, encryption, 8);
> +
> +static inline u8
> +btrfs_stack_file_extent_encryption_ctxsize(struct btrfs_file_extent_item *e)

type name(...) on the same line

> +{
> + u8 ctxsize;
> + btrfs_unpack_encryption(e->encryption, NULL, &ctxsize);
> + return ctxsize;
> +}
>
> static inline unsigned long
> btrfs_file_extent_inline_start(const struct btrfs_file_extent_item *e)
> --- a/fs/btrfs/fscrypt.h
> +++ b/fs/btrfs/fscrypt.h
> @@ -5,6 +5,14 @@
>
> #include <linux/fscrypt.h>
>
> +#define BTRFS_ENCRYPTION_POLICY_MASK 0x03
> +#define BTRFS_ENCRYPTION_CTXSIZE_MASK 0xfc

Where do the numbers come from?

> +
> +struct btrfs_fscrypt_extent_context {
> + u8 buffer[FSCRYPT_EXTENT_CONTEXT_MAX_SIZE];

FSCRYPT_EXTENT_CONTEXT_MAX_SIZE is 33 and btrfs_fscrypt_extent_context
is part of extent_map, that's quite common object. Random sample from my
desktop right now is around 800k objects so this is noticeable. Needs a
second look.

> + size_t len;
> +};
> +
> #ifdef CONFIG_FS_ENCRYPTION
> bool btrfs_fscrypt_match_name(const struct fscrypt_name *fname,
> struct extent_buffer *leaf,
> @@ -22,5 +30,20 @@ static bool btrfs_fscrypt_match_name(const struct fscrypt_name *fname,
> }
> #endif
>
> +static inline void btrfs_unpack_encryption(u8 encryption,
> + u8 *policy,
> + u8 *ctxsize)
> +{
> + if (policy)
> + *policy = encryption & BTRFS_ENCRYPTION_POLICY_MASK;
> + if (ctxsize)
> + *ctxsize = (encryption & BTRFS_ENCRYPTION_CTXSIZE_MASK) >> 2;
> +}
> +
> +static inline u8 btrfs_pack_encryption(u8 policy, u8 ctxsize)
> +{
> + return policy | (ctxsize << 2);

What does 2 mean? It should be some symbolic define with explanation as
it's defining on-disk format.

> +}
> +
> extern const struct fscrypt_operations btrfs_fscrypt_ops;
> #endif /* BTRFS_FSCRYPT_H */
> --- a/fs/btrfs/ordered-data.h
> +++ b/fs/btrfs/ordered-data.h
> @@ -99,6 +99,7 @@ struct btrfs_ordered_extent {
> u64 disk_bytenr;
> u64 disk_num_bytes;
> u64 offset;
> + struct btrfs_fscrypt_extent_context fscrypt_context;

And another embedded btrfs_fscrypt_extent_context, that can also get a
lot of slab objects.

> /* number of bytes that still need writing */
> u64 bytes_left;
> --- a/include/uapi/linux/btrfs_tree.h
> +++ b/include/uapi/linux/btrfs_tree.h
> @@ -820,6 +820,10 @@ struct btrfs_file_extent_item {
> * but not for stat.
> */
> __u8 compression;
> + /*
> + * This field contains 4 bits of encryption type in the lower bits,
> + * 4 bits of ivsize in the upper bits. The unencrypted value is 0.

Is there some rationale for this format? Can the encryption bytes be
used only for the type and the other_encoding for the IV?

> + */
> __u8 encryption;
> __le16 other_encoding; /* spare for later use */
>