linux-kernel - Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG

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]

Date:   Thu, 7 Jan 2021 09:33:23 +0100
From:   Michal Hocko <mhocko@...e.com>
To:     Hugh Dickins <hughd@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Alex Shi <alex.shi@...ux.alibaba.com>,
        Minchan Kim <minchan@...nel.org>,
        Andrea Arcangeli <aarcange@...hat.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/mmap: replace if (cond) BUG() with BUG_ON()

On Wed 06-01-21 12:10:30, Hugh Dickins wrote:
> On Wed, 6 Jan 2021, Andrew Morton wrote:
> > On Tue, 5 Jan 2021 20:28:27 -0800 (PST) Hugh Dickins <hughd@...gle.com> wrote:
> > 
> > > Alex, please consider why the authors of these lines (whom you
> > > did not Cc) chose to write them without BUG_ON(): it has always
> > > been preferred practice to use BUG_ON() on predicates, but not on
> > > functionally effective statements (sorry, I've forgotten the proper
> > > term: I'd say statements with side-effects, but here they are not
> > > just side-effects: they are their main purpose).
> > > 
> > > We prefer not to hide those away inside BUG macros
> > 
> > Should we change that?  I find BUG_ON(something_which_shouldnt_fail())
> > to be quite natural and readable.
> 
> Fair enough.  Whereas my mind tends to filter out the BUG lines when
> skimming code, knowing they can be skipped, not needing that effort
> to pull out what's inside them.
> 
> Perhaps I'm a relic and everyone else is with you: I can only offer
> my own preference, which until now was supported by kernel practice.

I agree with Hugh. BUG_ON on something that is not a trivial predicate
makes the code slightly harder to follow.

I also do agree that accomodating the coding style to the existing code
is better as well because the resulting code is more compact.

In general I consider code transformations like this without a higher
goal that is stated explicitly a pointless churn which doesn't bring
much while it consumes a very scarce review bandwidth. Even when those
look trivial there is always a room to introduce silent breakage.
Be it a checkpatch or coccinelle the change shouldn't be based solely on
the script complains. Really, what is the point of changing an existing
if (cond) BUG into BUG_ON? Fewer lines? Taste? Code consistency?
-- 
Michal Hocko
SUSE Labs