| Message ID | d7b68e60fac6c4d73214854f08cf755f781edf00.1723245033.git.loemra.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add __free attribute and improve xattr cleanup | expand |
On Fri, Aug 09, 2024 at 04:11:49PM -0700, Leo Martins wrote: > Introduces the __free attribute to xattr.c. Marks the path variable in > the btrfs_getxattr(), btrfs_setxattr(), and btrfs_listxattr() functions > with the __free(btrfs_free_path) attribute. When a variable is marked > with the __free attribute, the kernel will automatically call the > specified function (in this case, btrfs_free_path()) on the variable > when it goes out of scope. This ensures that the memory allocated for > the variable is properly released, preventing potential memory leaks. By > using the __free attribute, we can simplify the code and reduce the risk > of memory-related bugs. > > Test Plan: > Built and booted the kernel with patch applied > Ran btrfs/fstests to make sure that no regressions were introduced > > Signed-off-by: Leo Martins <loemra.dev@gmail.com> > --- > fs/btrfs/xattr.c | 28 ++++++++-------------------- > 1 file changed, 8 insertions(+), 20 deletions(-) > > diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c > index 738c7bb8ea7c..a8d5db02202b 100644 > --- a/fs/btrfs/xattr.c > +++ b/fs/btrfs/xattr.c > @@ -29,9 +29,8 @@ int btrfs_getxattr(const struct inode *inode, const char *name, > { > struct btrfs_dir_item *di; > struct btrfs_root *root = BTRFS_I(inode)->root; > - struct btrfs_path *path; > + struct btrfs_path *path __free(btrfs_free_path) = NULL; > struct extent_buffer *leaf; > - int ret = 0; > unsigned long data_ptr; > > path = btrfs_alloc_path(); > @@ -42,24 +41,20 @@ int btrfs_getxattr(const struct inode *inode, const char *name, > di = btrfs_lookup_xattr(NULL, root, path, btrfs_ino(BTRFS_I(inode)), > name, strlen(name), 0); > if (!di) { > - ret = -ENODATA; > - goto out; > + return -ENODATA; > } else if (IS_ERR(di)) { > - ret = PTR_ERR(di); > - goto out; > + return PTR_ERR(di); > } > > leaf = path->nodes[0]; > /* if size is 0, that means we want the size of the attr */ > if (!size) { > - ret = btrfs_dir_data_len(leaf, di); > - goto out; > + return btrfs_dir_data_len(leaf, di); > } > > /* now get the data out of our dir_item */ > if (btrfs_dir_data_len(leaf, di) > size) { > - ret = -ERANGE; > - goto out; > + return -ERANGE; > } > > /* > @@ -73,11 +68,7 @@ int btrfs_getxattr(const struct inode *inode, const char *name, > btrfs_dir_name_len(leaf, di)); > read_extent_buffer(leaf, buffer, data_ptr, > btrfs_dir_data_len(leaf, di)); > - ret = btrfs_dir_data_len(leaf, di); > - > -out: > - btrfs_free_path(path); > - return ret; > + return btrfs_dir_data_len(leaf, di); > } > > int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode, > @@ -86,7 +77,7 @@ int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode, > struct btrfs_dir_item *di = NULL; > struct btrfs_root *root = BTRFS_I(inode)->root; > struct btrfs_fs_info *fs_info = root->fs_info; > - struct btrfs_path *path; > + struct btrfs_path *path __free(btrfs_free_path) = NULL; > size_t name_len = strlen(name); > int ret = 0; > > @@ -214,7 +205,6 @@ int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode, > */ > } > out: > - btrfs_free_path(path); I replied to the cover letter, this is the example where removing the explicit free is delayed after the code below > if (!ret) { > set_bit(BTRFS_INODE_COPY_EVERYTHING, > &BTRFS_I(inode)->runtime_flags); which in full is 218 if (!ret) { 219 set_bit(BTRFS_INODE_COPY_EVERYTHING, 220 &BTRFS_I(inode)->runtime_flags); 221 clear_bit(BTRFS_INODE_NO_XATTRS, &BTRFS_I(inode)->runtime_flags); 222 } 223 return ret; so path is locked when the calls set_bit and clear_bit are done. Not critical in this case but an example where resource is not released as soon as possible.
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c index 738c7bb8ea7c..a8d5db02202b 100644 --- a/fs/btrfs/xattr.c +++ b/fs/btrfs/xattr.c @@ -29,9 +29,8 @@ int btrfs_getxattr(const struct inode *inode, const char *name, { struct btrfs_dir_item *di; struct btrfs_root *root = BTRFS_I(inode)->root; - struct btrfs_path *path; + struct btrfs_path *path __free(btrfs_free_path) = NULL; struct extent_buffer *leaf; - int ret = 0; unsigned long data_ptr; path = btrfs_alloc_path(); @@ -42,24 +41,20 @@ int btrfs_getxattr(const struct inode *inode, const char *name, di = btrfs_lookup_xattr(NULL, root, path, btrfs_ino(BTRFS_I(inode)), name, strlen(name), 0); if (!di) { - ret = -ENODATA; - goto out; + return -ENODATA; } else if (IS_ERR(di)) { - ret = PTR_ERR(di); - goto out; + return PTR_ERR(di); } leaf = path->nodes[0]; /* if size is 0, that means we want the size of the attr */ if (!size) { - ret = btrfs_dir_data_len(leaf, di); - goto out; + return btrfs_dir_data_len(leaf, di); } /* now get the data out of our dir_item */ if (btrfs_dir_data_len(leaf, di) > size) { - ret = -ERANGE; - goto out; + return -ERANGE; } /* @@ -73,11 +68,7 @@ int btrfs_getxattr(const struct inode *inode, const char *name, btrfs_dir_name_len(leaf, di)); read_extent_buffer(leaf, buffer, data_ptr, btrfs_dir_data_len(leaf, di)); - ret = btrfs_dir_data_len(leaf, di); - -out: - btrfs_free_path(path); - return ret; + return btrfs_dir_data_len(leaf, di); } int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode, @@ -86,7 +77,7 @@ int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode, struct btrfs_dir_item *di = NULL; struct btrfs_root *root = BTRFS_I(inode)->root; struct btrfs_fs_info *fs_info = root->fs_info; - struct btrfs_path *path; + struct btrfs_path *path __free(btrfs_free_path) = NULL; size_t name_len = strlen(name); int ret = 0; @@ -214,7 +205,6 @@ int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode, */ } out: - btrfs_free_path(path); if (!ret) { set_bit(BTRFS_INODE_COPY_EVERYTHING, &BTRFS_I(inode)->runtime_flags); @@ -280,7 +270,7 @@ ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size) struct btrfs_key key; struct inode *inode = d_inode(dentry); struct btrfs_root *root = BTRFS_I(inode)->root; - struct btrfs_path *path; + struct btrfs_path *path __free(btrfs_free_path) = NULL; int iter_ret = 0; int ret = 0; size_t total_size = 0, size_left = size; @@ -356,8 +346,6 @@ ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size) else ret = total_size; - btrfs_free_path(path); - return ret; }
Introduces the __free attribute to xattr.c. Marks the path variable in the btrfs_getxattr(), btrfs_setxattr(), and btrfs_listxattr() functions with the __free(btrfs_free_path) attribute. When a variable is marked with the __free attribute, the kernel will automatically call the specified function (in this case, btrfs_free_path()) on the variable when it goes out of scope. This ensures that the memory allocated for the variable is properly released, preventing potential memory leaks. By using the __free attribute, we can simplify the code and reduce the risk of memory-related bugs. Test Plan: Built and booted the kernel with patch applied Ran btrfs/fstests to make sure that no regressions were introduced Signed-off-by: Leo Martins <loemra.dev@gmail.com> --- fs/btrfs/xattr.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)