linux-kernel - Re: [PATCH v2] btrfs: make ASSERT no-op in release builds

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: <98e33c86-f5f3-46c5-8dba-c28a459b4a45@foxido.dev>
Date: Tue, 4 Nov 2025 14:37:30 +0300
From: Gladyshev Ilya <foxido@...ido.dev>
To: dsterba@...e.cz
Cc: Chris Mason <clm@...com>, David Sterba <dsterba@...e.com>,
 linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] btrfs: make ASSERT no-op in release builds

On 11/4/25 03:18, David Sterba wrote:
> On Sun, Nov 02, 2025 at 10:38:52AM +0300, Gladyshev Ilya wrote:
>> The current definition of `ASSERT(cond)` as `(void)(cond)` is redundant,
>> since these checks have no side effects and don't affect code logic.
> 
> Have you checked that none of the assert expressions really don't have
> side effects other than touching the memory?

Yes, but visually only. Most checks are plain C comparisons, and some 
call folio/btrfs/refcount _check/test_ functions where I didn't find 
side effects.

However, fs/btrfs/ has ~880 asserts, so if you know more robust 
verification methods, I'd be glad to try them.

>> However, some checks contain READ_ONCE or other compiler-unfriendly
>> constructs. For example, ASSERT(list_empty) in btrfs_add_dealloc_inode
>> was compiled to a redundant mov instruction due to this issue.
>>
>> This patch defines ASSERT as BUILD_BUG_ON_INVALID for !CONFIG_BTRFS_ASSERT
>> builds. It also marks `full_page_sectors_uptodate` as __maybe_unused to
>> suppress "unneeded declaration" warning (it's needed in compile time)
>>
>> Signed-off-by: Gladyshev Ilya <foxido@...ido.dev>
>> ---
>> Changes from v1:
>> - Annotate full_page_sectors_uptodate as __maybe_unused to avoid
>>    compiler warning
>>
>> Link to v1: https://lore.kernel.org/linux-btrfs/20251030182322.4085697-1-foxido@foxido.dev/
>> ---
>>   fs/btrfs/messages.h | 2 +-
>>   fs/btrfs/raid56.c   | 4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h
>> index 4416c165644f..f80fe40a2c2b 100644
>> --- a/fs/btrfs/messages.h
>> +++ b/fs/btrfs/messages.h
>> @@ -168,7 +168,7 @@ do {										\
>>   #endif
>>   
>>   #else
>> -#define ASSERT(cond, args...)			(void)(cond)
>> +#define ASSERT(cond, args...)			BUILD_BUG_ON_INVALID(cond)
> 
> I'd rather have the expression open coded rather than using
> BUILD_BUG_ON_INVALID, the name is confusing as it's not checking build
> time condtitons.

The name kinda indicates that it triggers on invalid conditions, not 
false ones, but I understand that it can be confusing. While we could 
use direct sizeof() magic here, I prefer reusing the same infrastructure 
as VM_BUG_ON(), VFS_*_ON() and others.

Maybe adding a comment about its semantics above ASSERT definition will 
help clarify the usage? But if you prefer the sizeof() approach, I can 
change it - it's not a big deal.