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

[David Sterba <dsterba@suse.cz>]

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.