linux-kernel - Re: [PATCH 1/7] Add conditional oopsing with annotation

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: <20111218080205.GA4144@elte.hu>
Date:	Sun, 18 Dec 2011 09:02:05 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	David Howells <dhowells@...hat.com>
Cc:	linux-arch@...r.kernel.org, linux-kernel@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 1/7] Add conditional oopsing with annotation

* David Howells <dhowells@...hat.com> wrote:

> This facility can be made use of by one of:
> 
> 	ANNOTATED_BUG(const char *fmt, ...);
> 	ANNOTATED_BUG_ON(bool condition, const char *fmt, ...);

Hm, what about WARN_ON()s?

WARN_ON() and WARN_ON_ONCE() covers like 99% of the *actual* 
bugreports we get: we are actively getting rid of BUG()s that do 
trigger and are asking all new patches to come with WARN_ON()s.

BUG()s are generally a poor way of reporting bugs.

Another, much bigger issue is the actual syntax:

> -       BUG_ON(atomic_read(&cookie->usage) <= 0);
> +       ASSERTCMP(atomic_read(&cookie->usage), >, 0);

NAK on that concept on two grounds!

1) BUG_ON() is a well-known pattern. Changing it to the inverted
   assert() braindamage is going to cause confusion years down
   the road. Years ago we've settled on using BUG*() and WARN*()
   assertions to include conditions that check the 'bad'
   condition'. assert() covers the negated 'good' condition -
   which works but the two are truly awful when mixed.

2) The '>,0' syntax is ugly.

Why don't we simply extend the *existing* primitives with a 
'verbose' variant that saves the text string of the macro using 
the '#param' syntax, intead of modifying the usage sites with a 
pointless splitting along logical ops?

Doing that would also remove the rather pointess ANNOTATED_() 
prefix, which is like totally uninteresting in the actual usage 
sites. WARN()s want to be as short, obvious and low-profile as 
possible.

So the whole schizophrenic split between BUG_ON()/WARN_ON() and 
the assert() world is nonsensical and confusing - we should 
settle on *one* logical variant to make code reading easier. And 
i thought in the kernel we already settled on one of these 
variants and are using it almost exclusively ...

So this series does not look good enough to me yet.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/