Re: [PATCH v6 13/43] btrfs: adapt readdir for encrypted and nokey names

[Daniel Vacek <neelx@suse.com>]

On Sun, 8 Feb 2026 at 16:36, Chris Mason <clm@meta.com> wrote:
> Daniel Vacek <neelx@suse.com> wrote:
> > From: Omar Sandoval <osandov@osandov.com>
> >
> > 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 <osandov@osandov.com>
>
> 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