On Sun, 8 Feb 2026 at 16:04, Chris Mason <[email protected]> wrote:
> Daniel Vacek <[email protected]> wrote:
> > From: Josef Bacik <[email protected]>
> >
> > 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 <[email protected]>
>
> 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 <[email protected]> wrote:
>
> Daniel Vacek <[email protected]> wrote:
> > From: Josef Bacik <[email protected]>
> >
> > 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 <[email protected]>
>
> 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.
>
