On Thu, Oct 20, 2022 at 12:58:31PM -0400, Sweet Tea Dorminy wrote:
> From: Omar Sandoval <osandov@xxxxxxxxxxx>
>
> In order to store per-inode information such as the inode nonce and the
> key identifier, fscrypt stores a context item with each encrypted inode.
> This can be implemented as a new item type, as fscrypt provides an
> arbitrary blob for the filesystem to store.
>
> This also provides a good place to implement full-subvolume encryption:
> a subvolume flag permits setting one context for the whole subvolume.
> However, since an unencrypted subvolume would be unable to read
> encrypted data, encrypted subvolumes should only be snapshottable to
> other encrypted subvolumes.
>
> Signed-off-by: Omar Sandoval <osandov@xxxxxxxxxxx>
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@xxxxxxxxxx>
> ---
> fs/btrfs/ctree.h | 1 +
> fs/btrfs/fscrypt.c | 170 ++++++++++++++++++++++++++++++++
> fs/btrfs/inode.c | 39 ++++++++
> fs/btrfs/ioctl.c | 7 +-
> fs/btrfs/tree-checker.c | 1 +
> include/uapi/linux/btrfs_tree.h | 12 +++
> 6 files changed, 229 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 389c4e988318..eb5bbed90e2e 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -32,6 +32,7 @@
> #include "extent-io-tree.h"
> #include "extent_io.h"
> #include "extent_map.h"
> +#include "fscrypt.h"
> #include "async-thread.h"
> #include "block-rsv.h"
> #include "locking.h"
> diff --git a/fs/btrfs/fscrypt.c b/fs/btrfs/fscrypt.c
> index 48ab99dfe48d..4533ef922d8b 100644
> --- a/fs/btrfs/fscrypt.c
> +++ b/fs/btrfs/fscrypt.c
> @@ -1,7 +1,177 @@
> // SPDX-License-Identifier: GPL-2.0
>
> +#include <linux/iversion.h>
> #include "ctree.h"
> +#include "btrfs_inode.h"
> +#include "disk-io.h"
> #include "fscrypt.h"
> +#include "transaction.h"
> +#include "xattr.h"
> +
> +static int btrfs_fscrypt_get_context(struct inode *inode, void *ctx, size_t len)
> +{
> + struct btrfs_root *root = BTRFS_I(inode)->root;
> + struct btrfs_key key = {
> + .objectid = btrfs_ino(BTRFS_I(inode)),
> + .type = BTRFS_FSCRYPT_CTXT_ITEM_KEY,
> + .offset = 0,
> + };
> + struct inode *put_inode = NULL;
> + struct btrfs_path *path;
> + struct extent_buffer *leaf;
> + unsigned long ptr;
> + int ret;
> +
> +
> + if (btrfs_root_flags(&root->root_item) & BTRFS_ROOT_SUBVOL_FSCRYPT) {
> + inode = btrfs_iget(inode->i_sb, BTRFS_FIRST_FREE_OBJECTID,
> + root);
> + if (IS_ERR(inode))
> + return PTR_ERR(inode);
> + put_inode = inode;
> + }
> +
> + path = btrfs_alloc_path();
> + if (!path)
> + return -ENOMEM;
> +
> + ret = btrfs_search_slot(NULL, BTRFS_I(inode)->root, &key, path, 0, 0);
> + if (ret) {
> + len = -EINVAL;
> + goto out;
> + }
> +
> + leaf = path->nodes[0];
> + ptr = btrfs_item_ptr_offset(leaf, path->slots[0]);
> + /* fscrypt provides max context length, but it could be less */
> + len = min_t(size_t, len, btrfs_item_size(leaf, path->slots[0]));
> + read_extent_buffer(leaf, ctx, ptr, len);
> +
> +out:
> + btrfs_free_path(path);
> + iput(put_inode);
> + return len;
> +}
> +
> +static int btrfs_fscrypt_set_context(struct inode *inode, const void *ctx,
> + size_t len, void *fs_data)
> +{
> + struct btrfs_root *root = BTRFS_I(inode)->root;
> + struct btrfs_trans_handle *trans;
> + int is_subvolume = inode->i_ino == BTRFS_FIRST_FREE_OBJECTID;
> + int ret;
> + struct btrfs_path *path;
> + struct btrfs_key key = {
> + .objectid = btrfs_ino(BTRFS_I(inode)),
> + .type = BTRFS_FSCRYPT_CTXT_ITEM_KEY,
> + .offset = 0,
> + };
> +
> + /*
> + * If the whole subvolume is encrypted, we expect that all children
> + * have the same policy.
> + */
> + if (btrfs_root_flags(&root->root_item) & BTRFS_ROOT_SUBVOL_FSCRYPT) {
> + bool same_policy;
> + struct inode *root_inode = NULL;
> +
> + root_inode = btrfs_iget(inode->i_sb, BTRFS_FIRST_FREE_OBJECTID,
> + root);
> + if (IS_ERR(inode))
> + return PTR_ERR(inode);
> + same_policy = fscrypt_have_same_policy(inode, root_inode);
> + iput(root_inode);
> + if (same_policy)
> + return 0;
> + }
> +
> + if (fs_data) {
> + /*
> + * We are setting the context as part of an existing
> + * transaction. This happens when we are inheriting the context
> + * for a new inode.
> + */
> + trans = fs_data;
> + } else {
> + /*
> + * 1 for the inode item
> + * 1 for the fscrypt item
> + * 1 for the root item if the inode is a subvolume
> + */
> + trans = btrfs_start_transaction(root, 2 + is_subvolume);
> + if (IS_ERR(trans))
> + return PTR_ERR(trans);
> + }
I don't love this, instead lets have a
__btrfs_set_fscrypt_context(trans, inode, whatever)
and in here you do
if (fs_data)
return __btrfs_set_fscrypt_context(trans, inode, whatever);
trans = btrfs_start_transaction(root, 2 + is_subvolume);
ret = __btrfs_set_fscrypt_context(trans, inode, whatever);
btrfs_end_transaction(trans);
return ret;
That'll make this a bit cleaner, especially in the error handling.
> +
> + path = btrfs_alloc_path();
> + if (!path)
> + return -ENOMEM;
> + ret = btrfs_search_slot(trans, BTRFS_I(inode)->root, &key, path, 0, 1);
> + if (ret == 0) {
> + struct extent_buffer *leaf = path->nodes[0];
> + unsigned long ptr = btrfs_item_ptr_offset(leaf, path->slots[0]);
> +
> + len = min_t(size_t, len, btrfs_item_size(leaf, path->slots[0]));
> + write_extent_buffer(leaf, ctx, ptr, len);
> + btrfs_mark_buffer_dirty(leaf);
> + btrfs_free_path(path);
> + goto out;
> + } else if (ret < 0) {
> + goto out;
You're leaking the path in this case. In fact I'd like to see this reworked
into a helper since you're only using this to shortcut adding the item if it
already exists. Thanks,
Josef
|