[PATCH 0/2] btrfs: add __free attribute and improve xattr cleanup

[PATCH 0/2] btrfs: add __free attribute and improve xattr cleanup

[Leo Martins]

The first patch introduces the __free attribute to the btrfs code, allowing
for automatic memory management of certain variables. This attribute enables
the kernel to automatically call a specified function (in this case,
btrfs_free_path()) on a variable when it goes out of scope, ensuring proper
memory release and preventing potential memory leaks.

The second patch applies the __free attribute to the path variable in the
btrfs_getxattr(), btrfs_setxattr(), and btrfs_listxattr() functions, ensuring
that the memory allocated for this variable is properly released when it goes
out of scope. This improves the memory management of xattr operations in
btrfs, reducing the risk of memory-related bugs and improving overall system
stability.

As a next step, I want to extend the use of the __free attribute to other
instances where btrfs_free_path is being manually called.

Leo Martins (2):
  btrfs: use __free in linux/cleanup.h to reduce btrfs_free_path
    boilerplate
  btrfs: use __free to automatically free btrfs_path on exit

 fs/btrfs/ctree.c |  3 ++-
 fs/btrfs/ctree.h |  1 +
 fs/btrfs/xattr.c | 28 ++++++++--------------------
 3 files changed, 11 insertions(+), 21 deletions(-)

-- 
2.43.5

[PATCH 1/2] btrfs: use __free in linux/cleanup.h to reduce btrfs_free_path boilerplate

[Leo Martins]

This patch lays the groundwork for future improvements to the btrfs file
system code by introducing the __free(btrfs_free_path) attribute. This
attribute allows the kernel to automatically call btrfs_free_path() on
variables marked with it when they go out of scope, ensuring proper
memory management and preventing potential memory leaks.

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/ctree.c | 3 ++-
 fs/btrfs/ctree.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 451203055bbf..9938664d7dbb 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -3,6 +3,7 @@
  * Copyright (C) 2007,2008 Oracle.  All rights reserved.
  */
 
+#include <linux/cleanup.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/rbtree.h>
@@ -196,7 +197,7 @@ struct btrfs_path *btrfs_alloc_path(void)
 /* this also releases the path */
 void btrfs_free_path(struct btrfs_path *p)
 {
-	if (!p)
+	if (IS_ERR_OR_NULL(p))
 		return;
 	btrfs_release_path(p);
 	kmem_cache_free(btrfs_path_cachep, p);
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 75fa563e4cac..be4e14b6e39a 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -599,6 +599,7 @@ int btrfs_search_slot_for_read(struct btrfs_root *root,
 void btrfs_release_path(struct btrfs_path *p);
 struct btrfs_path *btrfs_alloc_path(void);
 void btrfs_free_path(struct btrfs_path *p);
+DEFINE_FREE(btrfs_free_path, struct btrfs_path *, if (_T) btrfs_free_path(_T));
 
 int btrfs_del_items(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		   struct btrfs_path *path, int slot, int nr);
-- 
2.43.5

[PATCH 2/2] btrfs: use __free to automatically free btrfs_path on exit

[Leo Martins]

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);
 	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;
 }
 
-- 
2.43.5

Re: [PATCH 0/2] btrfs: add __free attribute and improve xattr cleanup

[David Sterba]

On Fri, Aug 09, 2024 at 04:11:47PM -0700, Leo Martins wrote:
> The first patch introduces the __free attribute to the btrfs code, allowing
> for automatic memory management of certain variables. This attribute enables
> the kernel to automatically call a specified function (in this case,
> btrfs_free_path()) on a variable when it goes out of scope, ensuring proper
> memory release and preventing potential memory leaks.
> 
> The second patch applies the __free attribute to the path variable in the
> btrfs_getxattr(), btrfs_setxattr(), and btrfs_listxattr() functions, ensuring
> that the memory allocated for this variable is properly released when it goes
> out of scope. This improves the memory management of xattr operations in
> btrfs, reducing the risk of memory-related bugs and improving overall system
> stability.
> 
> As a next step, I want to extend the use of the __free attribute to other
> instances where btrfs_free_path is being manually called.

Hold on. Adding the automatic memory management can be done but in the
example patches you sent it's IMHO making things worse on the code
level.

The btrfs_free_path (or btrfs_release_path for that matter) are not
simple free helpers but also part of the b-tree locking primitives,
pairing with btrfs_search_slot and nontrivial semantics depending on the
various setting flags.

Dropping the explicit marker from the code is obscuring where the
locked section is.

Another problem is that this will make any backports less obviously
correct from releases that use the __free attribue to older kernels.

In the second patch in btrfs_setxattr() you removed btrfs_free_path()
but there's still some code after that. In this case it's harmless and
only slightly extending the section covered by path, ie. just by a few
instructions, but this won't be always possible.

In some cases the placement of freeing the path unlocks the tree so it
has a strong reason to be there.

Overall, we could the automatic memory management, although for kernel,
for me, it's on the same level as trying to use other fancy C++
features. We could start using __free in new structures so it's used
consistently from the beginning and not mixing two styles namely when
not all instances of btrfs_path can use it.

In justified cases the auto freeing may make sense but not at the cost
of making the code confusing about the pairing free or extending the
locked section unnecessarily. The btrfs_path is not a good example where
to start with that.

Re: [PATCH 2/2] btrfs: use __free to automatically free btrfs_path on exit

[David Sterba]

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.

Re: [PATCH 0/2] btrfs: add __free attribute and improve xattr cleanup

[Leo Martins]

On Tue, 13 Aug 2024 14:29, David Sterba <dsterba@suse.cz> wrote:
>On Fri, Aug 09, 2024 at 04:11:47PM -0700, Leo Martins wrote:
>> The first patch introduces the __free attribute to the btrfs code, allowing
>> for automatic memory management of certain variables. This attribute enables
>> the kernel to automatically call a specified function (in this case,
>> btrfs_free_path()) on a variable when it goes out of scope, ensuring proper
>> memory release and preventing potential memory leaks.
>> 
>> The second patch applies the __free attribute to the path variable in the
>> btrfs_getxattr(), btrfs_setxattr(), and btrfs_listxattr() functions, ensuring
>> that the memory allocated for this variable is properly released when it goes
>> out of scope. This improves the memory management of xattr operations in
>> btrfs, reducing the risk of memory-related bugs and improving overall system
>> stability.
>> 
>> As a next step, I want to extend the use of the __free attribute to other
>> instances where btrfs_free_path is being manually called.
>
>Hold on. Adding the automatic memory management can be done but in the
>example patches you sent it's IMHO making things worse on the code
>level.
>
>The btrfs_free_path (or btrfs_release_path for that matter) are not
>simple free helpers but also part of the b-tree locking primitives,
>pairing with btrfs_search_slot and nontrivial semantics depending on the
>various setting flags.
>
>Dropping the explicit marker from the code is obscuring where the
>locked section is.
>
>Another problem is that this will make any backports less obviously
>correct from releases that use the __free attribue to older kernels.
>
>In the second patch in btrfs_setxattr() you removed btrfs_free_path()
>but there's still some code after that. In this case it's harmless and
>only slightly extending the section covered by path, ie. just by a few
>instructions, but this won't be always possible.
>
>In some cases the placement of freeing the path unlocks the tree so it
>has a strong reason to be there.
>
>Overall, we could the automatic memory management, although for kernel,
>for me, it's on the same level as trying to use other fancy C++
>features. We could start using __free in new structures so it's used
>consistently from the beginning and not mixing two styles namely when
>not all instances of btrfs_path can use it.
>
>In justified cases the auto freeing may make sense but not at the cost
>of making the code confusing about the pairing free or extending the
>locked section unnecessarily. The btrfs_path is not a good example where
>to start with that.

This makes sense, I will drop the xattr patch. Do you think there would
be any benefit in using the __free pattern in situations where it
is clear that btrfs_free_path is the last thing called before returning?
For example:


int btrfs_del_orphan_item(struct btrfs_trans_handle *trans,
			  struct btrfs_root *root, u64 offset)
{
	struct btrfs_path *path;
	struct btrfs_key key;
	int ret = 0;

	key.objectid = BTRFS_ORPHAN_OBJECTID;
	key.type = BTRFS_ORPHAN_ITEM_KEY;
	key.offset = offset;

	path = btrfs_alloc_path();
	if (!path)
		return -ENOMEM;

	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
	if (ret < 0)
		goto out;
	if (ret) { /* JDM: Really? */
		ret = -ENOENT;
		goto out;
	}

	ret = btrfs_del_item(trans, root, path);

out:
	btrfs_free_path(path);
	return ret;
}


In this code the behavior would be the same except it would eliminate
the need for goto out as the path is freed automatically on exit.

Re: [PATCH 0/2] btrfs: add __free attribute and improve xattr cleanup

[David Sterba]

On Thu, Aug 15, 2024 at 10:38:24AM -0700, Leo Martins wrote:
> On Tue, 13 Aug 2024 14:29, David Sterba <dsterba@suse.cz> wrote:
> >On Fri, Aug 09, 2024 at 04:11:47PM -0700, Leo Martins wrote:

> This makes sense, I will drop the xattr patch. Do you think there would
> be any benefit in using the __free pattern in situations where it
> is clear that btrfs_free_path is the last thing called before returning?
> For example:
> 
> 
> int btrfs_del_orphan_item(struct btrfs_trans_handle *trans,
> 			  struct btrfs_root *root, u64 offset)
> {
> 	struct btrfs_path *path;
> 	struct btrfs_key key;
> 	int ret = 0;
> 
> 	key.objectid = BTRFS_ORPHAN_OBJECTID;
> 	key.type = BTRFS_ORPHAN_ITEM_KEY;
> 	key.offset = offset;
> 
> 	path = btrfs_alloc_path();
> 	if (!path)
> 		return -ENOMEM;
> 
> 	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
> 	if (ret < 0)
> 		goto out;
> 	if (ret) { /* JDM: Really? */
> 		ret = -ENOENT;
> 		goto out;
> 	}
> 
> 	ret = btrfs_del_item(trans, root, path);
> 
> out:
> 	btrfs_free_path(path);
> 	return ret;
> }
> 
> 
> In this code the behavior would be the same except it would eliminate
> the need for goto out as the path is freed automatically on exit.

Yes, this is where I coudl be used, basically it's a pattern where the
lock/allocation is done early at the beginning of a function (after some
initial checks) and then right before a return and all exit paths lead
there.

For that I'd suggest to make it clear that the function uses the
automatic unlock/deletion so the declaration of the variable would be
done like

  BTRFS_PATH_AUTOCLEAN(name);

that declares it with the proper __free callback and initializes it to
NULL.

There's another thing that's a common pattern in btrfs and other kernel,
code, the single exit block. The __free callback allows to do a return
anywhere which is the opposite of that. As this is new we should look up
good examples that will be the patterns to follow or exceptions to
avoidd so we can declare it current best practice and recommended coding
style.