Re: [PATCH v2 07/17] btrfs: adapt readdir for encrypted and nokey names

[Josef Bacik <josef@toxicpanda.com>]

On Sun, Jul 16, 2023 at 11:52:38PM -0400, Sweet Tea Dorminy wrote:
> From: Omar Sandoval <osandov@osandov.com>
> 
> Deleting an encrypted file must always be permitted, even if the user
> does not have the appropriate key. Therefore, for listing an encrypted
> directory, so-called 'nokey' names are provided, and these nokey names
> must be sufficient to look up and delete the appropriate encrypted
> files. See 'struct fscrypt_nokey_name' for more information on the
> format of these names.
> 
> The first part of supporting nokey names is allowing lookups by nokey
> name. Only a few entry points need to support these: deleting a
> directory, file, or subvolume -- each of these call
> fscrypt_setup_filename() with a '1' argument, indicating that the key is
> not required and therefore a nokey name may be provided. If a nokey name
> is provided, the fscrypt_name returned by fscrypt_setup_filename() will
> not have its disk_name field populated, but will have various other
> fields set.
> 
> This change alters the relevant codepaths to pass a complete
> fscrypt_name anywhere that it might contain a nokey name. When it does
> contain a nokey name, the first time the name is successfully matched to
> a stored name populates the disk name field of the fscrypt_name,
> allowing the caller to use the normal disk name codepaths afterward.
> Otherwise, the matching functionality is in close analogue to the
> function fscrypt_match_name().
> 
> Functions where most callers are providing a fscrypt_str are duplicated
> and adapted for a fscrypt_name, and functions where most callers are
> providing a fscrypt_name are changed to so require at all callsites.
> 
> Signed-off-by: Omar Sandoval <osandov@osandov.com>
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>  fs/btrfs/btrfs_inode.h   |   2 +-
>  fs/btrfs/delayed-inode.c |  30 +++++++-
>  fs/btrfs/delayed-inode.h |   4 +-
>  fs/btrfs/dir-item.c      |  77 ++++++++++++++++++---
>  fs/btrfs/dir-item.h      |  13 +++-
>  fs/btrfs/extent_io.c     |  38 +++++++++++
>  fs/btrfs/extent_io.h     |   3 +
>  fs/btrfs/fscrypt.c       |  46 +++++++++++++
>  fs/btrfs/fscrypt.h       |  19 ++++++
>  fs/btrfs/inode.c         | 143 ++++++++++++++++++++++++++-------------
>  fs/btrfs/root-tree.c     |   8 ++-
>  fs/btrfs/root-tree.h     |   2 +-
>  fs/btrfs/tree-log.c      |   3 +-
>  13 files changed, 320 insertions(+), 68 deletions(-)
> 
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index ec4a06a78aff..464059674ae5 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -421,7 +421,7 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry);
>  int btrfs_set_inode_index(struct btrfs_inode *dir, u64 *index);
>  int btrfs_unlink_inode(struct btrfs_trans_handle *trans,
>  		       struct btrfs_inode *dir, struct btrfs_inode *inode,
> -		       const struct fscrypt_str *name);
> +		       struct fscrypt_name *name);
>  int btrfs_add_link(struct btrfs_trans_handle *trans,
>  		   struct btrfs_inode *parent_inode, struct btrfs_inode *inode,
>  		   const struct fscrypt_str *name, int add_backref, u64 index);
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index 6b457b010cbc..919303d29b76 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -1497,6 +1497,7 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
>  
>  	ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
>  	if (unlikely(ret)) {
> +		// TODO: It would be nice to print the base64encoded name here maybe?

Generally we don't leve TODO's around unless they're big, additionally wrong
comment format.

<snip>

> +/*
> + * This function is extremely similar to fscrypt_match_name() but uses an
> + * extent_buffer. Also, it edits the provided argument to populate the disk_name
> + * if we successfully match and previously were using a nokey name.
> + */
> +bool btrfs_fscrypt_match_name(struct fscrypt_name *fname,
> +			      struct extent_buffer *leaf, unsigned long de_name,
> +			      u32 de_name_len)
> +{
> +	const struct fscrypt_nokey_name *nokey_name =
> +		(const void *)fname->crypto_buf.name;

Cast it to the thing it's going to be, als this whol function needs more
newlines.

> +	u8 digest[SHA256_DIGEST_SIZE];
> +
> +	if (likely(fname->disk_name.name)) {
> +		if (de_name_len != fname->disk_name.len)
> +			return false;
> +		return !memcmp_extent_buffer(leaf, fname->disk_name.name,
> +					     de_name, de_name_len);
> +	}
> +	if (de_name_len <= sizeof(nokey_name->bytes))
> +		return false;
> +	if (memcmp_extent_buffer(leaf, nokey_name->bytes, de_name,
> +				 sizeof(nokey_name->bytes)))
> +		return false;
> +	extent_buffer_sha256(leaf, de_name + sizeof(nokey_name->bytes),
> +			     de_name_len - sizeof(nokey_name->bytes), digest);
> +	if (!memcmp(digest, nokey_name->sha256, sizeof(digest))) {
> +		/*
> +		 * For no-key names, we use this opportunity to find the disk
> +		 * name, so future searches don't need to deal with nokey names
> +		 * and we know what the encrypted size is.
> +		 */
> +		fname->disk_name.name = kmalloc(de_name_len, GFP_KERNEL | GFP_NOFS);

GFP_NOFS is sufficient.

> +		if (!fname->disk_name.name)
> +			fname->disk_name.name = ERR_PTR(-ENOMEM);

This part worries me, we use this code everywhere and it's just screaming for a
gotcha, I'd rather return an error in this case.  Thanks,

Josef