netdev - Re: [PATCH net-next 0/4] net: ipa: kill IPA

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: <YP/rFwvIHOvIwMNO@unreal>
Date:   Tue, 27 Jul 2021 14:16:39 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Alex Elder <elder@...aro.org>
Cc:     davem@...emloft.net, kuba@...nel.org, bjorn.andersson@...aro.org,
        evgreen@...omium.org, cpratapa@...eaurora.org,
        subashab@...eaurora.org, elder@...nel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 0/4] net: ipa: kill IPA_VALIDATION

On Mon, Jul 26, 2021 at 12:40:06PM -0500, Alex Elder wrote:
> A few months ago I proposed cleaning up some code that validates
> certain things conditionally, arguing that doing so once is enough,
> thus doing so always should not be necessary.
>   https://lore.kernel.org/netdev/20210320141729.1956732-1-elder@linaro.org/
> Leon Romanovsky felt strongly that this was a mistake, and in the
> end I agreed to change my plans.

<...>

> The second patch fixes a bug that wasn't normally exposed because of
> the conditional compilation (a reason Leon was right about this).

Thanks Alex,

If you want another anti pattern that is very popular in netdev, the following pattern is
wrong by definition :):
if (WARN_ON(...))
  return ...

The WARN_*() macros are intended catch impossible flows, something that
shouldn't exist. The idea that printed stack to dmesg and return to the
caller will fix the situation is a very naive one. That stack already
says that something very wrong in the system.

If such flow can be valid use "if(...) return ..", if not use plain
WARN_ON(...).

Thanks