linux-kernel - Re: [PATCH] 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: <20251031010822.GD13846@suse.cz>
Date: Fri, 31 Oct 2025 02:08:22 +0100
From: David Sterba <dsterba@...e.cz>
To: Qu Wenruo <quwenruo.btrfs@....com>
Cc: Gladyshev Ilya <foxido@...ido.dev>, Chris Mason <clm@...com>,
	David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] btrfs: make ASSERT no-op in release builds

On Fri, Oct 31, 2025 at 07:35:29AM +1030, Qu Wenruo wrote:
> 在 2025/10/31 04:53, Gladyshev Ilya 写道:
> > The current definition of `ASSERT(cond)` as `(void)(cond)` is redundant,
> > because all checks are without side effects and don't affect code logic.
> > 
> > However, some checks has READ_ONCE in them or other 'compiler-unfriendly'
> > behaviour. For example, ASSERT(list_empty) in btrfs_add_dealloc_inode
> > was compiled to redundant mov because of this.
> > 
> > This patch replaces ASSERT with BUILD_BUG_ON_INVALID for
> > !CONFIG_BTRFS_ASSERT builds.
> > 
> > Signed-off-by: Gladyshev Ilya <foxido@...ido.dev>
> > 
> > ---
> > .o size reductions are not that big, for example on defconfig + btrfs
> > fs/btrfs/*.o size went from 3280528 to 3277936, so compiler was pretty
> > efficient on his own
> > ---
> >   fs/btrfs/messages.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 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)
> 
> And I do not think it's a good idea to use BUILD_BUG_ON_INVALID() here, 
> most ASSERT()s are checking runtime conditions, I understand you want to 
> avoid real code generation, but in that case there are more 
> straightforward solutions, like "do {} while (0)" as no-op.

It's supposed to be no-op but also compile checked, so the do/while(0)
will not do that. What BUILD_BUG_ON_INVALID is basically a sizeof(cond)
so it's the right thing but the naming is confusing, we can possibly
open code it.