Re: [PATCH v6 35/43] btrfs: make btrfs_ref_to_path handle encrypted filenames

[Daniel Vacek <neelx@suse.com>]

On Sun, 8 Feb 2026 at 16:04, Chris Mason <clm@meta.com> wrote:
> Daniel Vacek <neelx@suse.com> wrote:
> > From: Josef Bacik <josef@toxicpanda.com>
> >
> > We use this helper for inode-resolve and path resolution in send, so
> > update this helper to properly decrypt any encrypted names it finds.
> >
>
> 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 473bbb12052f731a4e2ce3c48e7d7e9f2510528b
> Author: Josef Bacik <josef@toxicpanda.com>
>
> btrfs: make btrfs_ref_to_path handle encrypted filenames
>
> This commit updates btrfs_ref_to_path() to handle encrypted filenames by
> introducing a new copy_resolved_iref_to_buf() helper that can decrypt
> names when the filesystem has encryption enabled.
>
> > diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> > index 9bb406f7dd302..577c3ef87791a 100644
> > --- a/fs/btrfs/backref.c
> > +++ b/fs/btrfs/backref.c
>
> [ ... ]
>
> > @@ -2107,6 +2108,39 @@ int btrfs_find_one_extref(struct btrfs_root *root, u64 inode_objectid,
> >       return ret;
> >  }
> >
> > +static int copy_resolved_iref_to_buf(struct btrfs_root *fs_root,
> > +                                  struct extent_buffer *eb, char *dest,
> > +                                  u64 parent, unsigned long name_off,
> > +                                  u32 name_len, s64 *bytes_left)
> > +{
> > +     struct btrfs_fs_info *fs_info = fs_root->fs_info;
> > +     struct fscrypt_str fname = FSTR_INIT(NULL, 0);
> > +     int ret;
> > +
> > +     /* No encryption, just copy the name in. */
> > +     if (!btrfs_fs_incompat(fs_info, ENCRYPT)) {
> > +             *bytes_left -= name_len;
> > +             if (*bytes_left >= 0)
> > +                     read_extent_buffer(eb, dest + *bytes_left, name_off, name_len);
> > +             return 0;
> > +     }
> > +
> > +     ret = fscrypt_fname_alloc_buffer(BTRFS_NAME_LEN, &fname);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = btrfs_decrypt_name(fs_root, eb, name_off, name_len, parent, &fname);
> > +     if (ret)
> > +             goto out;
> > +
> > +     *bytes_left -= fname.len;
> > +     if (*bytes_left >= 0)
> > +             memcpy(dest + *bytes_left, fname.name, fname.len);
> > +out:
> > +     fscrypt_fname_free_buffer(&fname);
> > +     return ret;
> > +}
> > +
> >  /*
> >   * this iterates to turn a name (from iref/extref) into a full filesystem path.
> >   * Elements of the path are separated by '/' and the path is guaranteed to be
> > @@ -2138,10 +2172,10 @@ char *btrfs_ref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
> >               dest[bytes_left] = '\0';
> >
> >       while (1) {
> > -             bytes_left -= name_len;
> > -             if (bytes_left >= 0)
> > -                     read_extent_buffer(eb, dest + bytes_left,
> > -                                        name_off, name_len);
> > +             ret = copy_resolved_iref_to_buf(fs_root, eb, dest, parent,
> > +                                             name_off, name_len, &bytes_left);
> > +             if (ret)
> > +                     break;
> >               if (eb != eb_in) {
> >                       if (!path->skip_locking)
> >                               btrfs_tree_read_unlock(eb);
>
> Does this code leak the extent buffer when copy_resolved_iref_to_buf()
> fails? Looking at the loop structure in btrfs_ref_to_path(), when
> copy_resolved_iref_to_buf() returns an error and we break at line 2178,
> the cleanup code that follows never executes:
>
>     if (eb != eb_in) {
>         if (!path->skip_locking)
>             btrfs_tree_read_unlock(eb);
>         free_extent_buffer(eb);
>     }
>
> This can happen in subsequent iterations where eb != eb_in (the extent
> buffer was detached from path->nodes[0] and set to NULL in the previous
> iteration). The cleanup at the end of the function (btrfs_release_path()
> at line 2216) cannot free eb since it's no longer in the path.

Right, the break should be placed only after the cleanup.

--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -2174,13 +2174,14 @@ char *btrfs_ref_to_path(struct btrfs_root
*fs_root, struct btrfs_path *path,
     while (1) {
         ret = copy_resolved_iref_to_buf(fs_root, eb, dest, parent,
                         name_off, name_len, &bytes_left);
-        if (ret)
-            break;
         if (eb != eb_in) {
             if (!path->skip_locking)
                 btrfs_tree_read_unlock(eb);
             free_extent_buffer(eb);
         }
+        if (ret)
+            break;
+
         ret = btrfs_find_item(fs_root, path, parent, 0,
                 BTRFS_INODE_REF_KEY, &found_key);
         if (ret > 0)

Thanks.

--nX

> This could be triggered when decryption fails in copy_resolved_iref_to_buf()
> due to btrfs_iget() error, fscrypt_fname_alloc_buffer() ENOMEM, or
> fscrypt_prepare_readdir() failure on an encrypted filesystem.
>
> Before this patch, the name copy operation (read_extent_buffer) could not
> fail, so the cleanup always happened before any error check.
>

On Sun, 8 Feb 2026 at 16:04, Chris Mason <clm@meta.com> wrote:
>
> Daniel Vacek <neelx@suse.com> wrote:
> > From: Josef Bacik <josef@toxicpanda.com>
> >
> > We use this helper for inode-resolve and path resolution in send, so
> > update this helper to properly decrypt any encrypted names it finds.
> >
>
> 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 473bbb12052f731a4e2ce3c48e7d7e9f2510528b
> Author: Josef Bacik <josef@toxicpanda.com>
>
> btrfs: make btrfs_ref_to_path handle encrypted filenames
>
> This commit updates btrfs_ref_to_path() to handle encrypted filenames by
> introducing a new copy_resolved_iref_to_buf() helper that can decrypt
> names when the filesystem has encryption enabled.
>
> > diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> > index 9bb406f7dd302..577c3ef87791a 100644
> > --- a/fs/btrfs/backref.c
> > +++ b/fs/btrfs/backref.c
>
> [ ... ]
>
> > @@ -2107,6 +2108,39 @@ int btrfs_find_one_extref(struct btrfs_root *root, u64 inode_objectid,
> >       return ret;
> >  }
> >
> > +static int copy_resolved_iref_to_buf(struct btrfs_root *fs_root,
> > +                                  struct extent_buffer *eb, char *dest,
> > +                                  u64 parent, unsigned long name_off,
> > +                                  u32 name_len, s64 *bytes_left)
> > +{
> > +     struct btrfs_fs_info *fs_info = fs_root->fs_info;
> > +     struct fscrypt_str fname = FSTR_INIT(NULL, 0);
> > +     int ret;
> > +
> > +     /* No encryption, just copy the name in. */
> > +     if (!btrfs_fs_incompat(fs_info, ENCRYPT)) {
> > +             *bytes_left -= name_len;
> > +             if (*bytes_left >= 0)
> > +                     read_extent_buffer(eb, dest + *bytes_left, name_off, name_len);
> > +             return 0;
> > +     }
> > +
> > +     ret = fscrypt_fname_alloc_buffer(BTRFS_NAME_LEN, &fname);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = btrfs_decrypt_name(fs_root, eb, name_off, name_len, parent, &fname);
> > +     if (ret)
> > +             goto out;
> > +
> > +     *bytes_left -= fname.len;
> > +     if (*bytes_left >= 0)
> > +             memcpy(dest + *bytes_left, fname.name, fname.len);
> > +out:
> > +     fscrypt_fname_free_buffer(&fname);
> > +     return ret;
> > +}
> > +
> >  /*
> >   * this iterates to turn a name (from iref/extref) into a full filesystem path.
> >   * Elements of the path are separated by '/' and the path is guaranteed to be
> > @@ -2138,10 +2172,10 @@ char *btrfs_ref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
> >               dest[bytes_left] = '\0';
> >
> >       while (1) {
> > -             bytes_left -= name_len;
> > -             if (bytes_left >= 0)
> > -                     read_extent_buffer(eb, dest + bytes_left,
> > -                                        name_off, name_len);
> > +             ret = copy_resolved_iref_to_buf(fs_root, eb, dest, parent,
> > +                                             name_off, name_len, &bytes_left);
> > +             if (ret)
> > +                     break;
> >               if (eb != eb_in) {
> >                       if (!path->skip_locking)
> >                               btrfs_tree_read_unlock(eb);
>
> Does this code leak the extent buffer when copy_resolved_iref_to_buf()
> fails? Looking at the loop structure in btrfs_ref_to_path(), when
> copy_resolved_iref_to_buf() returns an error and we break at line 2178,
> the cleanup code that follows never executes:
>
>     if (eb != eb_in) {
>         if (!path->skip_locking)
>             btrfs_tree_read_unlock(eb);
>         free_extent_buffer(eb);
>     }
>
> This can happen in subsequent iterations where eb != eb_in (the extent
> buffer was detached from path->nodes[0] and set to NULL in the previous
> iteration). The cleanup at the end of the function (btrfs_release_path()
> at line 2216) cannot free eb since it's no longer in the path.
>
> This could be triggered when decryption fails in copy_resolved_iref_to_buf()
> due to btrfs_iget() error, fscrypt_fname_alloc_buffer() ENOMEM, or
> fscrypt_prepare_readdir() failure on an encrypted filesystem.
>
> Before this patch, the name copy operation (read_extent_buffer) could not
> fail, so the cleanup always happened before any error check.
>