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

[Daniel Vacek]

On Tue, 2 Jun 2026 at 05:14, Eric Biggers <ebiggers@xxxxxxxxxx> 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.josef@xxxxxxxxxxxxxx/

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