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. > + > + 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; > +} [ ... ]
