On Sun, 8 Feb 2026 at 16:30, Chris Mason <[email protected]> wrote:
> Daniel Vacek <[email protected]> wrote:
> > From: Sweet Tea Dorminy <[email protected]>
> >
> > For encrypted or unencrypted names, we calculate the offset for the dir
> > item by hashing the name for the dir item. However, this doesn't work
> > for a long nokey name, where we do not have the complete ciphertext.
> > Instead, fscrypt stores the filesystem-provided hash in the nokey name,
> > and we can extract it from the fscrypt_name structure in such a case.
> >
> > Additionally, for nokey names, if we find the nokey name on disk we can
> > update the fscrypt_name with the disk name, so add that to searching for
> > diritems.
>
> 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 fabad1c3ea82184fba8e78ce1da1b2fbe8c34fae
> Author: Sweet Tea Dorminy <[email protected]>
>
> btrfs: handle nokey names
>
> This commit adds support for handling nokey names in btrfs, enabling
> proper hash calculation and disk name extraction for long encrypted
> filenames when the encryption key is unavailable.
>
> > diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
> > index 6e10dd4a4e9e1..75c213bd826e9 100644
> > --- a/fs/btrfs/dir-item.c
> > +++ b/fs/btrfs/dir-item.c
> > @@ -228,6 +228,27 @@ struct btrfs_dir_item *btrfs_lookup_dir_item(struct
> > btrfs_trans_handle *trans,
> > return di;
> > }
> >
> > +/*
> > + * If appropriate, populate the disk name for a fscrypt_name looked up
> > without
> > + * a key.
> > + *
> > + * @path: The path to the extent buffer in which the name was found.
> > + * @di: The dir item corresponding.
> > + * @fname: The fscrypt_name to perhaps populate.
> > + *
> > + * Returns: 0 if the name is already populated or the dir item doesn't
> > exist
> > + * or the name was successfully populated, else an error code.
> > + */
> > +static int ensure_disk_name_from_dir_item(struct btrfs_path *path,
> > + struct btrfs_dir_item *di,
> > + struct fscrypt_name *name)
> > +{
> > + if (name->disk_name.name || !di)
> > + return 0;
> > +
> > + return btrfs_fscrypt_get_disk_name(path->nodes[0], di,
> > &name->disk_name);
> > +}
> > +
>
> [ ... ]
>
> > @@ -263,6 +287,8 @@ struct btrfs_dir_item
> > *btrfs_lookup_dir_item_fname(struct btrfs_trans_handle *tr
> >
> > if (ret == -ENOENT || (di && IS_ERR(di) && PTR_ERR(di) == -ENOENT))
> > return NULL;
> > + if (ret == 0)
> > + ret = ensure_disk_name_from_dir_item(path, di, name);
> > if (ret < 0)
> > di = ERR_PTR(ret);
> >
>
> Does ensure_disk_name_from_dir_item() leak memory when it allocates a
> disk name? Looking at the call chain:
>
> btrfs_unlink_subvol() at inode.c:4583 calls btrfs_lookup_dir_item_fname(),
> which calls ensure_disk_name_from_dir_item(), which calls
> btrfs_fscrypt_get_disk_name() at fscrypt.c:34, allocating memory via
> kmalloc() and storing it in name->disk_name.name.
>
> However, btrfs_unlink_subvol() cleans up with fscrypt_free_filename() at
> include/linux/fscrypt.h:503, which only frees name->crypto_buf.name, not
> name->disk_name.name.
>
> The same issue appears in btrfs_search_dir_index_item() at line 405, which
> also calls ensure_disk_name_from_dir_item().
>
> For nokey encrypted filenames, wouldn't this leak memory on every
> successful lookup?
Should `name->crypto_buf.name` equal to `name->disk_name.name` in this case?
How about this:
--- a/fs/btrfs/dir-item.c
+++ b/fs/btrfs/dir-item.c
@@ -243,10 +243,18 @@ static int ensure_disk_name_from_dir_item(struct
btrfs_path *path,
struct btrfs_dir_item *di,
struct fscrypt_name *name)
{
+ int ret;
+
if (name->disk_name.name || !di)
return 0;
- return btrfs_fscrypt_get_disk_name(path->nodes[0], di, &name->disk_name);
+ ret = btrfs_fscrypt_get_disk_name(path->nodes[0], di, &name->disk_name);
+ if (ret)
+ return ret;
+
+ name->crypto_buf.name = name->disk_name.name;
+ name->crypto_buf.len = name->disk_name.len;
+ return 0;
}
/*
Thanks.
--nX
