Re: [PATCH v3 12/22] btrfs: add fscrypt_context items

[Josef Bacik <josef@toxicpanda.com>]

On Thu, Oct 20, 2022 at 12:58:31PM -0400, Sweet Tea Dorminy wrote:
> From: Omar Sandoval <osandov@osandov.com>
> 
> 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@osandov.com>
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>  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