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

[Eric Biggers <ebiggers@kernel.org>]

On Wed, May 13, 2026 at 10:52:44AM +0200, Daniel Vacek wrote:
> @@ -9041,20 +9063,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;

This is off by one from the other filesystems.  Yes, the way the other
filesystems do encrypted symlinks is weird, but this still doesn't fix
it, since the unnecessary 'struct fscrypt_symlink_data' is still stored.
If it's not being fixed completely, it should just be done the same way.

Did you do it this way because you're trying to squeeze out an extra
byte, to allow 4094-byte symlink targets instead of 4093 as the other
filesystems do?  Or did you do it this way because btrfs doesn't count a
nul terminator when checking unencrypted symlinks against
BTRFS_MAX_INLINE_DATA_SIZE(fs_info), and you needed to preserve that
behavior?  But at the same time, btrfs *does* count the nul terminator
when validating against 'fs_info->sectorsize', and this changes that
behavior.  So it's not clear what was intended here.

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

fscrypt_encrypt_symlink() already has an IS_ENCRYPTED(inode) check
built-in.

> +static const char *btrfs_get_link(struct dentry *dentry, struct inode *inode,
> +				  struct delayed_call *done)
> +{
> +	struct page *cpage;
> +	const char *paddr;
> +	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> +
> +	if (!IS_ENCRYPTED(inode))
> +		return page_get_link(dentry, inode, done);
> +
> +	if (!dentry)
> +		return ERR_PTR(-ECHILD);
> +
> +	cpage = read_mapping_page(inode->i_mapping, 0, NULL);
> +	if (IS_ERR(cpage))
> +		return ERR_CAST(cpage);
> +
> +	paddr = fscrypt_get_symlink(inode, page_address(cpage),
> +				    BTRFS_MAX_INLINE_DATA_SIZE(fs_info), done);
> +	put_page(cpage);

This uses a different max_len from btrfs_symlink().

Speaking of symlinks, btrfs is also missing a hookup to
fscrypt_symlink_getattr().

- Eric