On Sun, 8 Feb 2026 at 16:38, Chris Mason <[email protected]> wrote:
> Daniel Vacek <[email protected]> wrote:
> > From: Omar Sandoval <[email protected]>
> >
> > fscrypt stores a context item with encrypted inodes that contains the
> > related encryption information. fscrypt provides an arbitrary blob for
> > the filesystem to store, and it does not clearly fit into an existing
> > structure, so this goes in a new item type.
>
> 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 2d6114c2937b897d844735aa16d5369e3ec6d135
> Author: Omar Sandoval <[email protected]>
>
> btrfs: add inode encryption contexts
>
> fscrypt stores a context item with encrypted inodes that contains the
> related encryption information. fscrypt provides an arbitrary blob for
> the filesystem to store, and it does not clearly fit into an existing
> structure, so this goes in a new item type.
>
> > diff --git a/fs/btrfs/fscrypt.c b/fs/btrfs/fscrypt.c
> > index 6cfba7d94e726..e9b024d671a22 100644
> > --- a/fs/btrfs/fscrypt.c
> > +++ b/fs/btrfs/fscrypt.c
>
> [ ... ]
>
> > @@ -1,10 +1,126 @@
> > // SPDX-License-Identifier: GPL-2.0
> >
> > +#include <linux/iversion.h>
> > #include "ctree.h"
> > +#include "accessors.h"
> > #include "btrfs_inode.h"
> > +#include "disk-io.h"
> > +#include "fs.h"
> > #include "fscrypt.h"
> > +#include "ioctl.h"
> > +#include "messages.h"
> > +#include "transaction.h"
> > +#include "xattr.h"
> > +
> > +static int btrfs_fscrypt_get_context(struct inode *inode, void *ctx,
> > size_t len)
> > +{
> > + struct btrfs_key key = {
> > + .objectid = btrfs_ino(BTRFS_I(inode)),
> > + .type = BTRFS_FSCRYPT_INODE_CTX_KEY,
> > + .offset = 0,
> > + };
> > + struct btrfs_path *path;
> > + struct extent_buffer *leaf;
> > + unsigned long ptr;
> > + int ret;
> > +
> > +
> > + path = btrfs_alloc_path();
> > + if (!path)
> > + return -ENOMEM;
> > +
> > + ret = btrfs_search_slot(NULL, BTRFS_I(inode)->root, &key, path, 0, 0);
> > + if (ret) {
> > + len = -ENOENT;
> > + goto out;
> > + }
> > +
> > + leaf = path->nodes[0];
> > + ptr = btrfs_item_ptr_offset(leaf, path->slots[0]);
> > + /* fscrypt provides max context length, but it could be less */
> > + len = min_t(size_t, len, btrfs_item_size(leaf, path->slots[0]));
> > + read_extent_buffer(leaf, ctx, ptr, len);
> > +
> > +out:
> > + btrfs_free_path(path);
> > + return len;
> > +}
> > +
> > +static int btrfs_fscrypt_set_context(struct inode *inode, const void *ctx,
> > + size_t len, void *fs_data)
> > +{
> > + struct btrfs_trans_handle *trans = fs_data;
> > + struct btrfs_key key = {
> > + .objectid = btrfs_ino(BTRFS_I(inode)),
> > + .type = BTRFS_FSCRYPT_INODE_CTX_KEY,
> > + .offset = 0,
> > + };
> > + struct btrfs_path *path = NULL;
> > + struct extent_buffer *leaf;
> > + unsigned long ptr;
> > + int ret;
> > +
> > + if (!trans)
> > + trans = btrfs_start_transaction(BTRFS_I(inode)->root, 2);
> > + if (IS_ERR(trans))
> > + return PTR_ERR(trans);
> > +
> > + path = btrfs_alloc_path();
> > + if (!path) {
> > + ret = -ENOMEM;
> > + goto out_err;
> > + }
> > +
> > + ret = btrfs_search_slot(trans, BTRFS_I(inode)->root, &key, path, 0,
> > 1);
> > + if (ret < 0)
> > + goto out_err;
> > +
> > + if (ret > 0) {
> > + btrfs_release_path(path);
> > + ret = btrfs_insert_empty_item(trans, BTRFS_I(inode)->root,
> > path, &key, len);
> > + if (ret)
> > + goto out_err;
> > + }
> > +
> > + leaf = path->nodes[0];
> > + ptr = btrfs_item_ptr_offset(leaf, path->slots[0]);
> > +
> > + len = min_t(size_t, len, btrfs_item_size(leaf, path->slots[0]));
> > + write_extent_buffer(leaf, ctx, ptr, len);
> > + btrfs_mark_buffer_dirty(trans, leaf);
> > + btrfs_release_path(path);
> > +
> > + if (fs_data)
> > + return ret;
>
> Does this code leak the path when fs_data is non-NULL?
>
> In btrfs_fscrypt_set_context(), a path is allocated via btrfs_alloc_path()
> but when fs_data is provided (during inode creation with a transaction
> handle), the function returns here without calling btrfs_free_path(path).
>
> The btrfs_release_path() call above only releases internal references to
> extent buffers and locks but does not free the path structure itself.
>
> This leak occurs on every encrypted inode creation. The fix would be to add
> btrfs_free_path(path) before the return.
That's correct. Good catch!
--- a/fs/btrfs/fscrypt.c
+++ b/fs/btrfs/fscrypt.c
@@ -155,7 +155,7 @@ static int btrfs_fscrypt_set_context(struct inode
*inode, const void *ctx,
btrfs_release_path(path);
if (fs_data)
- return ret;
+ goto out_err;
BTRFS_I(inode)->flags |= BTRFS_INODE_ENCRYPT;
btrfs_sync_inode_flags_to_i_flags(BTRFS_I(inode));
Thanks.
--nX
> > +
> > + BTRFS_I(inode)->flags |= BTRFS_INODE_ENCRYPT;
> > + btrfs_sync_inode_flags_to_i_flags(BTRFS_I(inode));
> > + inode_inc_iversion(inode);
> > + inode_set_ctime_current(inode);
> > + ret = btrfs_update_inode(trans, BTRFS_I(inode));
> > + if (ret)
> > + goto out_abort;
> > + btrfs_free_path(path);
> > + btrfs_end_transaction(trans);
> > + return 0;
> > +out_abort:
> > + btrfs_abort_transaction(trans, ret);
> > +out_err:
> > + if (!fs_data)
> > + btrfs_end_transaction(trans);
> > + btrfs_free_path(path);
> > + return ret;
> > +}
>
> [ ... ]
>
