Re: [PATCH v6 14/43] btrfs: handle nokey names

[Chris Mason <clm@meta.com>]

Daniel Vacek <neelx@suse.com> wrote:
> From: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> 
> For encrypted or unencrypted names, we calculate the offset for the dir
> item by hashing the name for the dir item. However, this doesn't work
> for a long nokey name, where we do not have the complete ciphertext.
> Instead, fscrypt stores the filesystem-provided hash in the nokey name,
> and we can extract it from the fscrypt_name structure in such a case.
> 
> Additionally, for nokey names, if we find the nokey name on disk we can
> update the fscrypt_name with the disk name, so add that to searching for
> diritems.

Hi everyone,

I'm running the btrfs for-next branch, along with the encryption patch set
through my AI review prompts:

https://github.com/masoncl/review-prompts

I'm working on some btrfs specific instructions, but hopefully these reviews
are already useful.  I've kept the headers to make it easier to double check
that I'm actually replying to the right email, but please let me know if any
parts of this are incorrect:

commit fabad1c3ea82184fba8e78ce1da1b2fbe8c34fae
Author: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

btrfs: handle nokey names

This commit adds support for handling nokey names in btrfs, enabling
proper hash calculation and disk name extraction for long encrypted
filenames when the encryption key is unavailable.

> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
> index 6e10dd4a4e9e1..75c213bd826e9 100644
> --- a/fs/btrfs/dir-item.c
> +++ b/fs/btrfs/dir-item.c
> @@ -228,6 +228,27 @@ struct btrfs_dir_item *btrfs_lookup_dir_item(struct btrfs_trans_handle *trans,
>  	return di;
>  }
>
> +/*
> + * If appropriate, populate the disk name for a fscrypt_name looked up without
> + * a key.
> + *
> + * @path:	The path to the extent buffer in which the name was found.
> + * @di:		The dir item corresponding.
> + * @fname:	The fscrypt_name to perhaps populate.
> + *
> + * Returns: 0 if the name is already populated or the dir item doesn't exist
> + * or the name was successfully populated, else an error code.
> + */
> +static int ensure_disk_name_from_dir_item(struct btrfs_path *path,
> +					  struct btrfs_dir_item *di,
> +					  struct fscrypt_name *name)
> +{
> +	if (name->disk_name.name || !di)
> +		return 0;
> +
> +	return btrfs_fscrypt_get_disk_name(path->nodes[0], di, &name->disk_name);
> +}
> +

[ ... ]

> @@ -263,6 +287,8 @@ struct btrfs_dir_item *btrfs_lookup_dir_item_fname(struct btrfs_trans_handle *tr
>
>  	if (ret == -ENOENT || (di && IS_ERR(di) && PTR_ERR(di) == -ENOENT))
>  		return NULL;
> +	if (ret == 0)
> +		ret = ensure_disk_name_from_dir_item(path, di, name);
>  	if (ret < 0)
>  		di = ERR_PTR(ret);
>

Does ensure_disk_name_from_dir_item() leak memory when it allocates a
disk name? Looking at the call chain:

btrfs_unlink_subvol() at inode.c:4583 calls btrfs_lookup_dir_item_fname(),
which calls ensure_disk_name_from_dir_item(), which calls
btrfs_fscrypt_get_disk_name() at fscrypt.c:34, allocating memory via
kmalloc() and storing it in name->disk_name.name.

However, btrfs_unlink_subvol() cleans up with fscrypt_free_filename() at
include/linux/fscrypt.h:503, which only frees name->crypto_buf.name, not
name->disk_name.name.

The same issue appears in btrfs_search_dir_index_item() at line 405, which
also calls ensure_disk_name_from_dir_item().

For nokey encrypted filenames, wouldn't this leak memory on every
successful lookup?