linux-kernel - Re: [PATCH v2] ethernet: hnae: drop adhoc assert() macros

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: <2f439541-154f-1f17-180e-db0d8f95e010@gmail.com>
Date:   Wed, 12 Sep 2018 18:04:30 +0300
From:   Igor Stoppa <igor.stoppa@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     igor.stoppa@...wei.com, huangdaode@...ilicon.com,
        yisen.zhuang@...wei.com, salil.mehta@...wei.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] ethernet: hnae: drop adhoc assert() macros

On 12/09/18 09:07, David Miller wrote:
> From: Igor Stoppa <igor.stoppa@...il.com>
> Date: Sat,  8 Sep 2018 18:01:42 +0300
> 
>> Replace assert() with a less misleading test_condition() using WARN()
>> Drop one check which had bitrotted and didn't compile anymore.
>>
>> Signed-off-by: Igor Stoppa <igor.stoppa@...wei.com>
> 
> I'm still kind of not happy about this.
> 
> Make the driver use kernel interfaces like WARN_ON_ONCE()
> etc. directly instead of defining alias CPP macros private to the
> driver.
> 
> If it needs to be conditional upon DEBUG, we have pr_debug() and
> the likes as well.

WARN_ON_ONCE() will display the error message only once, but from its 
implementation it looks like it will evaluate the condition anyways, 
every time, unless the compiler can do some magic and somehow know that 
the condition was true already onece (/include/asm-generic/bug.h:146)

pr_debug() otoh, will print or not print, depending on 
CONFIG_DYNAMIC_DEBUG being set or not (/include/linux/printk.h:399)

but the logic we are trying to clean up here seems a bit different:

if (in_debug_mode && error_condition_is_verified)
	generate_warning()

Which, I guess, is meant to completely remove *any* test outside of 
debug mode.

That is why I used the test_condition() macro, instead of using directly 
WARN_ON_ONCE().  Admittedly, it probably would have been better to 
depend on CONFIG_DYNAMIC_DEBUG.

Neither using WARN_ON_ONCE(), nor pr_debug() seem to replicate the 
initial behavior of the code that is currently in the tree, with assert().

How would you suggest that I keep the same behavior, without the helper 
macro? I am probably missing it, but I do not see some facility that 
would support the double condition I described above.

--
thanks, igor