linux-kernel - Re: [PATCH 0/4] btrfs: define and apply the AUTO_K(V)FREE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251022072240.GW13776@twin.jikos.cz>
Date: Wed, 22 Oct 2025 09:22:40 +0200
From: David Sterba <dsterba@...e.cz>
To: Miquel Sabaté Solà <mssola@...ola.com>
Cc: linux-btrfs@...r.kernel.org, clm@...com, dsterba@...e.com,
	johannes.thumshirn@....com, fdmanana@...e.com, boris@....io,
	wqu@...e.com, neal@...pa.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/4] btrfs: define and apply the AUTO_K(V)FREE_PTR macros

On Tue, Oct 21, 2025 at 04:27:45PM +0200, Miquel Sabaté Solà wrote:
> This patchset introduces and applies throughout the btrfs tree two new
> macros: AUTO_KFREE_PTR and AUTO_KVFREE_PTR. Each macro defines a pointer,
> initializes it to NULL, and sets the kfree/kvfree cleanup attribute. It was
> suggested by David Sterba in the review of a patch that I submitted here
> [1]. I have added the _PTR suffix because I think that it reads more easily
> and it makes things more explicit, but I don't have any strong feelings
> about the naming either.

I'd rather drop the _PTR part as it's not bringing new information
(kfree takes a pointer) and it makes the lines longer.

> I have not applied these macros blindly through the tree, but only when
> using a cleanup attribute actually made things easier for
> maintainers/developers, and didn't obfuscate things like lifetimes of
> objects on a given function. So, I've mostly avoided applying this when:
> 
> - The object was being re-allocated in the middle of the function
>   (e.g. object re-allocation in a loop).
> - The ownership of the object was transferred between functions.
> - The value of a given object might depend on functions returning ERR_PTR()
>   et al.
> - The cleanup section of a function was a bunch of labels with different
>   exit paths with non-trivial cleanup code (or code that depended on things
>   to go on a specific order).

This looks reasonable, some of them may be relaxed eventually.

> To come up with this patchset I have glanced through the tree in order to
> find where and how kfree()/kvfree() were being used, and while doing so I
> have submitted [2], [3] and [4] separately as they were fixing memory
> related issues. All in all, this patchset can be divided in three parts:
> 
> 1. Patch 1: transforms free_ipath() to be defined via DEFINE_FREE(), which
>    will be useful in order to further simplify some code in patch 3.
> 2. Patch 2 and 3: define and use the two macros.
> 3. Patch 4: removing some unneeded kfree() calls from qgroup.c as they were
>    not needed. Since these occurrences weren't memory bugs, and it was a
>    somewhat simple patch, I've refrained from sending this separately as I
>    did in [2], [3] and [4]; but I'll gladly do it if you think it's better
>    for the review.
> 
> Note that after these changes some 'return' statements could be made more
> explicit, and I've also written an explicit 'return 0' whenever it would
> make more explicit the "happy" path for a given branch, or whenever a 'ret'
> variable could be avoided that way.
> 
> Last, checkpatch.pl script doesn't seem to like patches 2 and 3; but so far
> it looks like false positives to me. But of course I might just be wrong :)

We take checkpatch as a recommendation but still a good base line, the
local btrfs coding style may diverge and is fixed manually at commit
time.