On Sun, 8 Feb 2026 at 16:46, Chris Mason <[email protected]> wrote:
> Daniel Vacek <[email protected]> wrote:
> > From: Omar Sandoval <[email protected]>
> >
> > 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 <[email protected]>
>
> 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?
True. Fixed with the below hunk. Thanks!
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6574,7 +6574,7 @@ int btrfs_new_inode_prepare(struct
btrfs_new_inode_args *args,
ret = fscrypt_prepare_new_inode(dir, inode, &args->encrypt);
if (ret) {
- fscrypt_free_filename(&args->fname);
+ btrfs_new_inode_args_destroy(args);
return ret;
}
> [ ... ]
>
> > @@ -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?
Agreed. I think this fixes that leak (untested):
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9196,7 +9196,7 @@ static int btrfs_symlink(struct mnt_idmap
*idmap, struct inode *dir,
btrfs_free_path(path);
discard_new_inode(inode);
inode = NULL;
- goto out;
+ goto free_name;
}
leaf = path->nodes[0];
ei = btrfs_item_ptr(leaf, path->slots[0],
@@ -9215,6 +9215,9 @@ static int btrfs_symlink(struct mnt_idmap
*idmap, struct inode *dir,
d_instantiate_new(dentry, inode);
ret = 0;
+free_name:
+ if (disk_link.name != (unsigned char *)symname)
+ kfree(disk_link.name);
out:
btrfs_end_transaction(trans);
btrfs_btree_balance_dirty(fs_info);
Thanks again!
--nX
