On Sun, 8 Feb 2026 at 16:36, Chris Mason <[email protected]> wrote:
> Daniel Vacek <[email protected]> wrote:
> > From: Omar Sandoval <[email protected]>
> >
> > Deleting an encrypted file must always be permitted, even if the user
> > does not have the appropriate key. Therefore, for listing an encrypted
> > directory, so-called 'nokey' names are provided, and these nokey names
> > must be sufficient to look up and delete the appropriate encrypted
> > files. See 'struct fscrypt_nokey_name' for more information on the
> > format of these names.
>
> 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 e264482bd61ca8096981d81cc5e7c839284ac969
> Author: Omar Sandoval <[email protected]>
>
> btrfs: adapt readdir for encrypted and nokey names
>
> This commit adapts the btrfs readdir implementation to handle encrypted
> filenames and nokey names, enabling directory listing and deletion of
> encrypted files even without the encryption key.
>
> > diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> > index 1739a0b29c49d..c19213fab3dda 100644
> > --- a/fs/btrfs/delayed-inode.c
> > +++ b/fs/btrfs/delayed-inode.c
>
> [ ... ]
>
> > @@ -1820,7 +1823,25 @@ bool btrfs_readdir_delayed_dir_index(struct
> > dir_context *ctx,
> > d_type =
> > fs_ftype_to_dtype(btrfs_dir_flags_to_ftype(di->type));
> > btrfs_disk_key_to_cpu(&location, &di->location);
> >
> > - over = !dir_emit(ctx, name, name_len, location.objectid,
> > d_type);
> > + if (di->type & BTRFS_FT_ENCRYPTED) {
> > + int ret;
> > + struct fscrypt_str iname = FSTR_INIT(name, name_len);
> > +
> > + fstr->len = fstr_len;
> > + /*
> > + * The hash is only used when the encryption key is
> > not
> > + * available. But if we have delayed insertions, then
> > we
> > + * must have the encryption key available or we
> > wouldn't
> > + * have been able to create entries in the directory.
> > + * So, we don't calculate the hash.
> > + */
> > + ret = fscrypt_fname_disk_to_usr(inode, 0, 0, &iname,
> > fstr);
> > + if (ret)
> > + return ret;
> > + over = !dir_emit(ctx, fstr->name, fstr->len,
> > location.objectid, d_type);
> > + } else {
> > + over = !dir_emit(ctx, name, name_len,
> > location.objectid, d_type);
> > + }
>
> Does the return statement at line 1840 cause silent error loss?
>
> The function btrfs_readdir_delayed_dir_index returns bool, but
> 'return ret;' returns an int error code from fscrypt_fname_disk_to_usr.
> Since ret is negative on error (e.g., -EUCLEAN from fname_decrypt
> failures), this converts to 'true' in bool context.
>
> The caller btrfs_real_readdir at inode.c:6410 interprets true as "stop
> iteration normally":
>
> if (btrfs_readdir_delayed_dir_index(inode, &fstr, ctx, &ins_list))
> goto nopos;
>
> The nopos label at line 6417 sets ret = 0, which means readdir()
> completes successfully with incomplete results when crypto operations
> fail on encrypted directory entries.
This still seems correct to me. While rebasing, however, I missed that
the return type changed in 41e4ea0bf555 ("btrfs: make
btrfs_readdir_delayed_dir_index() return a bool instead").
If I'm not mistaken, we can just `return true;` here:
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1837,7 +1837,7 @@ bool btrfs_readdir_delayed_dir_index(const
struct inode *inode,
*/
ret = fscrypt_fname_disk_to_usr(inode, 0, 0, &iname, fstr);
if (ret)
- return ret;
+ return true;
over = !dir_emit(ctx, fstr->name, fstr->len,
location.objectid, d_type);
} else {
over = !dir_emit(ctx, name, name_len, location.objectid, d_type);
> [ ... ]
>
> > diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
> > index 085a83ae9e62f..6e10dd4a4e9e1 100644
> > --- a/fs/btrfs/dir-item.c
> > +++ b/fs/btrfs/dir-item.c
>
> [ ... ]
>
> > @@ -227,6 +228,47 @@ struct btrfs_dir_item *btrfs_lookup_dir_item(struct
> > btrfs_trans_handle *trans,
> > return di;
> > }
> >
> > +/*
> > + * Lookup for a directory item by fscrypt_name.
> > + *
> > + * @trans: The transaction handle to use.
> > + * @root: The root of the target tree.
> > + * @path: Path to use for the search.
> > + * @dir: The inode number (objectid) of the directory.
> > + * @name: The fscrypt_name associated to the directory entry
> > + * @mod: Used to indicate if the tree search is meant for a read only
> > + * lookup or for a deletion lookup, so its value should be 0 or
> > + * -1, respectively.
> > + *
> > + * Returns: NULL if the dir item does not exists, an error pointer if an
> > error
> > + * happened, or a pointer to a dir item if a dir item exists for the given
> > name.
> > + */
> > +struct btrfs_dir_item *btrfs_lookup_dir_item_fname(struct
> > btrfs_trans_handle *trans,
> > + struct btrfs_root *root,
> > + struct btrfs_path *path,
> > u64 dir,
> > + struct fscrypt_name *name,
> > int mod)
> > +{
> > + struct btrfs_key key;
> > + struct btrfs_dir_item *di = NULL;
> > + int ret = 0;
> > +
> > + key.objectid = dir;
> > + key.type = BTRFS_DIR_ITEM_KEY;
> > + key.offset = btrfs_name_hash(name->disk_name.name,
> > name->disk_name.len);
> > + /* XXX get the right hash for no-key names */
>
> Can this dereference a NULL pointer for long nokey names?
>
> When btrfs_unlink() calls fscrypt_setup_filename() with lookup=1 for a
> nokey encrypted filename, fname->disk_name.name is NULL for long nokey
> names (see fscrypt_setup_filename lines 370-375 in fs/crypto/fname.c).
>
> The call chain is: btrfs_unlink -> btrfs_unlink_inode ->
> __btrfs_unlink_inode (line 4396) -> btrfs_lookup_dir_item_fname.
>
> This passes NULL to btrfs_name_hash() at line 257, which calls
> crc32c(NULL, len), causing a kernel crash. The XXX comment at line 258
> acknowledges this is unhandled.
>
> Note: This issue is addressed in a later commit in the patch series
> (fabad1c3ea821 'btrfs: handle nokey names').
Yes, this is handled right in the next commit.
Thanks.
--nX
