linux-kernel - Re: [PATCH v2 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] [day] [month] [year] [list]

Message-ID: <87v7jv89t6.fsf@>
Date: Fri, 31 Oct 2025 14:44:53 +0100
From: Miquel Sabaté Solà <mssola@...ola.com>
To: David Sterba <dsterba@...e.cz>
Cc: linux-btrfs@...r.kernel.org,  clm@...com,  dsterba@...e.com,
  johannes.thumshirn@....com,  fdmanana@...e.com,  boris@....io,
  wqu@...e.com,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/4] btrfs: define and apply the AUTO_K(V)FREE_PTR
 macros

David Sterba @ 2025-10-31 03:22 +01:

> On Fri, Oct 24, 2025 at 12:21:39PM +0200, Miquel Sabaté Solà wrote:
>> Changes since v1:
>>   - Remove the _PTR suffix
>>   - Rename the ipath cleanup function to inode_fs_paths, so it's more
>>     explicit on the type.
>>   - Improve git message in patch 1.
>>
>> This patchset introduces and applies throughout the btrfs tree two new
>> macros: AUTO_KFREE and AUTO_KVFREE. 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 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).
>>
>> 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 :)
>>
>> [1] https://lore.kernel.org/all/20250922103442.GM5333@twin.jikos.cz/
>> [2] https://lore.kernel.org/all/20250925184139.403156-1-mssola@mssola.com/
>> [3] https://lore.kernel.org/all/20250930130452.297576-1-mssola@mssola.com/
>> [4] https://lore.kernel.org/all/20251008121859.440161-1-mssola@mssola.com/
>>
>> Miquel Sabaté Solà (4):
>>   btrfs: declare free_ipath() via DEFINE_FREE()
>>   btrfs: define the AUTO_K(V)FREE helper macros
>>   btrfs: apply the AUTO_K(V)FREE macros throughout the tree
>>   btrfs: add ASSERTs on prealloc in qgroup functions
>
> Thanks, patches now added to for-next with some minor adjustments. Feel
> free to send more conversions, there are still some kvfree candidate
> calls. I think we would not mind using it even for the short functions
> (re what's mentioned in the 3rd patch), so it's established as a common
> coding pattern. This change has net negative effect on lines and also
> simplifies the control flow.

Thanks for adding them!

Will keep an eye then if there are places where it's safe to use them.

Greetings,
Miquel

Download attachment "signature.asc" of type "application/pgp-signature" (898 bytes)