Re: [PATCH v2 13/20] btrfs: add fscrypt_context items.

[David Sterba <dsterba@suse.cz>]

On Mon, Sep 05, 2022 at 08:35:28PM -0400, Sweet Tea Dorminy wrote:
> +static int btrfs_fscrypt_get_context(struct inode *inode, void *ctx, size_t len)
> +{
> +	struct btrfs_root *root = BTRFS_I(inode)->root;
> +	struct inode *put_inode = NULL;
> +	struct btrfs_key key;
> +	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;
> +
> +	key = (struct btrfs_key) {
> +		.objectid = btrfs_ino(BTRFS_I(inode)),
> +		.type = BTRFS_FSCRYPT_CTXT_ITEM_KEY,
> +		.offset = 0,
> +	};

Please use the following for key initialization.

	key.objectid = ...;
	key.type = ...;
	key.offset = ...;

> +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,
> +	};

This kind of initialization in the declaration block is ok, possibly
with only the simple initializers like btrfs_ino or normal constants.

> +	if (btrfs_root_flags(&root->root_item) & BTRFS_ROOT_SUBVOL_FSCRYPT) {
> +		bool same_policy;
> +		struct inode *root_inode = NULL;

Newlines between declrataions and statements

> +		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);
> +	}
> +
> +	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]);

Newlines between declrataions and statements, you'll find the rest

> +		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;
> +	}
> +	btrfs_free_path(path);
> +
> +	ret = btrfs_insert_item(trans, BTRFS_I(inode)->root, &key, (void *) ctx, len);
> +	if (ret)
> +		goto out;
> +
> +	BTRFS_I(inode)->flags |= BTRFS_INODE_FSCRYPT_CONTEXT;
> +	btrfs_sync_inode_flags_to_i_flags(inode);
> +	inode_inc_iversion(inode);
> +	inode->i_ctime = current_time(inode);
> +	ret = btrfs_update_inode(trans, root, BTRFS_I(inode));
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * For new subvolumes, the root item is already initialized with
> +	 * the BTRFS_ROOT_SUBVOL_FSCRYPT flag.
> +	 */
> +	if (!fs_data && is_subvolume) {
> +		u64 root_flags = btrfs_root_flags(&root->root_item);
> +
> +		btrfs_set_root_flags(&root->root_item,
> +				     root_flags |
> +				     BTRFS_ROOT_SUBVOL_FSCRYPT);
> +		ret = btrfs_update_root(trans, root->fs_info->tree_root,
> +					&root->root_key,
> +					&root->root_item);
> +	}
> +out:
> +	if (fs_data)
> +		return ret;
> +
> +	if (ret)
> +		btrfs_abort_transaction(trans, ret);
> +	else
> +		btrfs_end_transaction(trans);
> +	return ret;
> +}
> +
> +	if (args->encrypt)
> +		(*trans_num_items)++; /* 1 to add fscrypt item */

Please put the comment on a separate line, like it's on the lines below.

>  	if (args->orphan) {
>  		/* 1 to add orphan item */
>  		(*trans_num_items)++;
> @@ -787,6 +788,13 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
>  		return -ETXTBSY;
>  	}
>  
> +	if ((btrfs_root_flags(&root->root_item) & BTRFS_ROOT_SUBVOL_FSCRYPT) &&
> +	    !IS_ENCRYPTED(dir)) {
> +		btrfs_warn(fs_info,
> +			   "cannot snapshot encrypted volume to unencrypted destination");

Do we want to print that to the log? There are several EXDEV causes,
only another one prints a message and I think it should not.

> +		return -EXDEV;
> +	}
> +
>  	pending_snapshot = kzalloc(sizeof(*pending_snapshot), GFP_KERNEL);
>  	if (!pending_snapshot)
>  		return -ENOMEM;
> --- a/include/uapi/linux/btrfs_tree.h
> +++ b/include/uapi/linux/btrfs_tree.h
> @@ -144,6 +144,8 @@
>  #define BTRFS_VERITY_DESC_ITEM_KEY	36
>  #define BTRFS_VERITY_MERKLE_ITEM_KEY	37
>  
> +#define BTRFS_FSCRYPT_CTXT_ITEM_KEY	41

The context is per inode, so OK the key is needed and the number is
leaving enough space in case we'd need to add more.