Re: [PATCH v3 17/17] btrfs: save and load fscrypt extent contexts

[Josef Bacik <josef@toxicpanda.com>]

On Tue, Aug 08, 2023 at 01:12:19PM -0400, Sweet Tea Dorminy wrote:
> This change actually saves and loads the extent contexts created and
> freed by the last change.
> 
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>  fs/btrfs/file-item.c |   7 +++
>  fs/btrfs/fscrypt.c   | 108 ++++++++++++++++++++++++++++++++++++++++++-
>  fs/btrfs/fscrypt.h   |  35 ++++++++++++++
>  fs/btrfs/inode.c     |  37 +++++++++++++--
>  fs/btrfs/tree-log.c  |  14 +++++-
>  5 files changed, 194 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index f83f7020ed89..880fb7810152 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -1299,6 +1299,13 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
>  
>  		ctxsize = btrfs_file_extent_ctxsize_from_item(leaf, path);
>  		ASSERT(ctxsize == btrfs_file_extent_encryption_ctxsize(leaf, fi));
> +
> +		if (ctxsize) {
> +			unsigned long ptr = (unsigned long)fi->encryption_context;
> +			int res = btrfs_fscrypt_load_extent_info(inode, leaf, ptr,
> +								 ctxsize, &em->fscrypt_info);
> +			ASSERT(res == 0);
> +		}
>  	} else if (type == BTRFS_FILE_EXTENT_INLINE) {
>  		em->block_start = EXTENT_MAP_INLINE;
>  		em->start = extent_start;
> diff --git a/fs/btrfs/fscrypt.c b/fs/btrfs/fscrypt.c
> index 5508cbc6bccb..7324375af0ac 100644
> --- a/fs/btrfs/fscrypt.c
> +++ b/fs/btrfs/fscrypt.c
> @@ -75,6 +75,65 @@ bool btrfs_fscrypt_match_name(struct fscrypt_name *fname,
>  	return !memcmp(digest, nokey_name->sha256, sizeof(digest));
>  }
>  
> +int btrfs_fscrypt_fill_extent_context(struct btrfs_inode *inode,
> +				      struct fscrypt_info *info,
> +				      u8 *context_buffer, size_t *context_len)
> +{
> +	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> +	int ret;
> +
> +	if (!IS_ENCRYPTED(&inode->vfs_inode))
> +		return 0;
> +
> +
> +	ret = fscrypt_set_extent_context(info, context_buffer + sizeof(u32),
> +					 FSCRYPT_SET_CONTEXT_MAX_SIZE);
> +	if (ret < 0) {
> +		btrfs_err(fs_info, "fscrypt context could not be saved");
> +		return ret;
> +	}
> +
> +	/* the return value, if nonnegative, is the fscrypt context size */
> +	ret += sizeof(u32);
> +
> +	put_unaligned_le32(ret, context_buffer);
> +
> +	*context_len = ret;
> +	return 0;
> +}
> +
> +int btrfs_fscrypt_load_extent_info(struct btrfs_inode *inode,
> +				  struct extent_buffer *leaf,
> +				  unsigned long ptr,
> +				  u8 ctxsize,
> +				  struct fscrypt_info **info_ptr)
> +{
> +	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> +	u8 context[BTRFS_FSCRYPT_EXTENT_CONTEXT_MAX_SIZE];
> +	int res;
> +	unsigned int nofs_flags;
> +	u32 len;
> +
> +	read_extent_buffer(leaf, context, ptr, ctxsize);
> +
> +	nofs_flags = memalloc_nofs_save();
> +	res = fscrypt_load_extent_info(&inode->vfs_inode,
> +				       context + sizeof(u32),
> +				       ctxsize - sizeof(u32), info_ptr);
> +	memalloc_nofs_restore(nofs_flags);
> +
> +	if (res)
> +		btrfs_err(fs_info, "Unable to load fscrypt info: %d", res);

We are we not returning an error here?  Seems like we could end up with garbage
in context and then we'd just turn the error into -EINVAL below.

> +
> +	len = get_unaligned_le32(context);
> +	if (len != ctxsize) {
> +		res = -EINVAL;
> +		btrfs_err(fs_info, "fscrypt info size mismatches");
> +	}
> +
> +	return res;
> +}
> +
>  static int btrfs_fscrypt_get_context(struct inode *inode, void *ctx, size_t len)
>  {
>  	struct btrfs_key key = {
> @@ -138,11 +197,14 @@ static int btrfs_fscrypt_set_context(struct inode *inode, const void *ctx,
>  
>  	if (!trans)
>  		trans = btrfs_start_transaction(BTRFS_I(inode)->root, 1);
> -	if (IS_ERR(trans))
> +	if (IS_ERR(trans)) {
> +		btrfs_free_path(path);
>  		return PTR_ERR(trans);
> +	}
>  
>  	ret = btrfs_search_slot(trans, BTRFS_I(inode)->root, &key, path, 0, 1);
>  	if (ret < 0) {
> +		btrfs_free_path(path);
>  		btrfs_abort_transaction(trans, ret);
>  		return ret;
>  	}
> @@ -151,12 +213,13 @@ static int btrfs_fscrypt_set_context(struct inode *inode, const void *ctx,
>  		btrfs_release_path(path);
>  		ret = btrfs_insert_empty_item(trans, BTRFS_I(inode)->root, path, &key, len);
>  		if (ret) {
> +			btrfs_free_path(path);
>  			btrfs_abort_transaction(trans, ret);
>  			return ret;
>  		}
>  	}
> -
>  	btrfs_fscrypt_update_context(path, ctx, len);
> +	btrfs_free_path(path);
>  

These are unrelated changes, but my earlier comment was about re-working this
function, so I assume this will go away once you implement the changes I asked
for.  Thanks,

Josef