On Tue, 2 Jun 2026 at 05:14, Eric Biggers <[email protected]> wrote: > 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.
This comes from Omar/Josef authored [v1] of the patchset from September 2023. I understand the intent was to utilize the last byte. But yeah, I think it's perfectly fine sticking to 4093 bytes limit as other FSes do. I'll drop the +1. [v1] https://lore.kernel.org/linux-btrfs/912762ca48efcc684cd5cdde99b5a78cd25f1078.1695750478.git.jo...@toxicpanda.com/ > > + 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. Correct. I'll drop the check. > > +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(). I'm not sure that matters. But unifying them shouldn't hurt. > Speaking of symlinks, btrfs is also missing a hookup to > fscrypt_symlink_getattr(). Uh, I was not aware of that. It seems like fstests is missing for this feature. Thanks a lot. --nX > - Eric
