Re: [PATCH v6 10/43] btrfs: start using fscrypt hooks

[Chris Mason <clm@meta.com>]

Daniel Vacek <neelx@suse.com> wrote:
> From: Omar Sandoval <osandov@osandov.com>
> 
> In order to appropriately encrypt, create, open, rename, and various
> symlink operations must call fscrypt hooks. These determine whether the
> inode should be encrypted and do other preparatory actions. The
> superblock must have fscrypt operations registered, so implement the
> minimal set also, and introduce the new fscrypt.[ch] files to hold the
> fscrypt-specific functionality.

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 4d7d769810b1d47ec75a6efa0f691c43d9969b54
Author: Omar Sandoval <osandov@osandov.com>

btrfs: start using fscrypt hooks

This commit adds fscrypt support to btrfs by implementing fscrypt hooks
in various inode operations including create, open, rename, and symlink
operations.

> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index b6c763a17406b..9d90e2e946fc4 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c

[ ... ]

> @@ -6506,6 +6510,12 @@ int btrfs_new_inode_prepare(struct btrfs_new_inode_args *args,
>  		return ret;
>  	}
>
> +	ret = fscrypt_prepare_new_inode(dir, inode, &args->encrypt);
> +	if (ret) {
> +		fscrypt_free_filename(&args->fname);
> +		return ret;
> +	}
> +

When fscrypt_prepare_new_inode() fails, are the ACLs leaked?

Looking earlier in the function, posix_acl_create() allocates args->acl
and args->default_acl. When fscrypt_prepare_new_inode() fails, the
error path frees the filename but doesn't release these ACLs.

Callers like btrfs_symlink() skip btrfs_new_inode_args_destroy() on
error, so the ACLs would never be released. Should this add
posix_acl_release() calls before returning?

[ ... ]

> @@ -8994,20 +9015,28 @@ static int btrfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  	};
>  	unsigned int trans_num_items;
>  	int ret;
> -	int name_len;
>  	int datasize;
>  	unsigned long ptr;
>  	struct btrfs_file_extent_item *ei;
>  	struct extent_buffer *leaf;
> +	struct fscrypt_str disk_link;
> +	size_t max_len;
> +	u32 name_len = strlen(symname);
> +
> +	/*
> +	 * BTRFS_MAX_INLINE_DATA_SIZE() isn't actually telling the truth, we actually
> +	 * limit inline data extents to min(BTRFS_MAX_INLINE_DATA_SIZE(), sectorsize),
> +	 * so adjust max_len given this wonderful bit of inconsistency.
> +	 */
> +	max_len = min_t(size_t, BTRFS_MAX_INLINE_DATA_SIZE(fs_info), fs_info->sectorsize);
>
> -	name_len = strlen(symname);
>  	/*
> -	 * Symlinks utilize uncompressed inline extent data, which should not
> -	 * reach block size.
> +	 * fscrypt sets disk_link.len to be len + 1, including a NUL terminator,
> +	 * but we don't store that '\0' character.
>  	 */
> -	if (name_len > BTRFS_MAX_INLINE_DATA_SIZE(fs_info) ||
> -	    name_len >= fs_info->sectorsize)
> -		return -ENAMETOOLONG;
> +	ret = fscrypt_prepare_symlink(dir, symname, name_len, max_len + 1, &disk_link);
> +	if (ret)
> +		return ret;
>
>  	inode = new_inode(dir->i_sb);
>  	if (!inode)

[ ... ]

> @@ -9044,10 +9073,22 @@ static int btrfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  		inode = NULL;
>  		goto out;
>  	}
> +
> +	if (IS_ENCRYPTED(inode)) {
> +		ret = fscrypt_encrypt_symlink(inode, symname, name_len, &disk_link);
> +		if (ret) {
> +			btrfs_abort_transaction(trans, ret);
> +			btrfs_free_path(path);
> +			discard_new_inode(inode);
> +			inode = NULL;
> +			goto out;
> +		}
> +	}
> +
>  	key.objectid = btrfs_ino(BTRFS_I(inode));
>  	key.type = BTRFS_EXTENT_DATA_KEY;
>  	key.offset = 0;
> -	datasize = btrfs_file_extent_calc_inline_size(name_len);
> +	datasize = btrfs_file_extent_calc_inline_size(disk_link.len - 1);
>  	ret = btrfs_insert_empty_item(trans, root, path, &key, datasize);
>  	if (unlikely(ret)) {
>  		btrfs_abort_transaction(trans, ret);

[ ... ]

> @@ -9084,6 +9125,29 @@ static int btrfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  	return ret;
>  }

Does this code leak disk_link.name when creating encrypted symlinks?

According to fscrypt.h, when fscrypt_encrypt_symlink() is called,
'one will be kmalloc()'ed and the filesystem will be responsible
for freeing it'. However, btrfs_symlink() never frees disk_link.name
in any exit path, whether success or failure.

Looking at ext4/namei.c:3443-3444, it correctly handles this:

    if (disk_link.name != (unsigned char *)symname)
        kfree(disk_link.name);

Should similar cleanup be added here?