netdev - Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation

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: <1491513145.11399.8.camel@sipsolutions.net>
Date:   Thu, 06 Apr 2017 23:12:25 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     "David S . Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, Grant Grundler <grundler@...omium.org>,
        Michael Davidson <md@...gle.com>,
        Greg Hackmann <ghackmann@...gle.com>
Subject: Re: [PATCH] mac80211: Fix clang warning about constant operand in
 logical operation

On Thu, 2017-04-06 at 12:24 -0700, Matthias Kaehlcke wrote:

> I agree that the code looks worse :( I hoped to find a fix using a
> preprocessor condition but wasn't successful.

It's actually easy - just remove the 'default ""' from Kconfig, and
then the symbol won't be defined at all if it doesn't get a proper
value. Then you can ifdef the whole thing.

> Some projects (like Chrome OS) build their kernel with all warnings
> being treated as errors. Besides changing the 'offending' code the
> alternatives are to disable the warning completely or to tell clang
> not to use the builtin(s). IMO changing the code is the preferable
> solution, especially since this is so far the only occurrence of the
> warning that I have encountered.
> 
> I used goto instead of nested ifs since other functions in this file
> use the same pattern. If nested ifs are preferred I can change that.

I don't really buy either argument. The warning is simply bogus - I'm
very surprised you don't hit it with more similar macros or cases, like
for example CONFIG_ENABLED(). Try

	git grep 'IS_ENABLED(' | grep '&&'

and you'll find lots of places that seem like they should trigger this
warning.

You're advocating to make the code worse - not very significantly in
this case, but still - just to quiet a compiler warning.

johannes