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

[Leo Martins <loemra.dev@gmail.com>]

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.