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?
