linux-kernel - Re: [PATCH] firmware: dmi_scan: Drop dmi

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]

Date:   Wed, 27 Sep 2017 10:56:17 +0200
From:   Jean Delvare <jdelvare@...e.de>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Ingo Molnar <mingo@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] firmware: dmi_scan: Drop dmi_initialized

Hi Peter,

On Mon, 25 Sep 2017 11:26:44 +0200, Peter Zijlstra wrote:
> On Mon, Sep 25, 2017 at 11:00:11AM +0200, Jean Delvare wrote:
> > Then we have that in common. While reading the code and its history, I
> > was worried that the justification to add this warning in the first
> > place was technically weak. Not every coding error must automatically
> > translate to a patch to make the code robust against said error.
> > Sometimes you just have to admit that you did not pay attention as you
> > should have, fix your mistake, possibly document it for others, and
> > move on. Otherwise we end up with slow bloated code.  
> 
> That WARN_ON() is a form of documentation.

Sort of, but not the best form in my opinion. I mean, if a WARN
triggers, you'll have to spot it in the kernel log, and then figure out
why the condition which triggered it was evaluated to false. In this
specific case, this means grepping the code for "dmi_initialized" to
figure out where it should have been set. The error string ("dmi check:
not initialized yet") helps a bit, but that's still not as clear and
immediate as an explicit "dmi_scan_machine must be called before this
function is called" in the function description.

> And if you care about performance for your code path, hide it under some
> CONFIG_*_DEBUG,

I'd love to, but I couldn't find a generic one which would actually be
disabled on production kernels, and introducing a configuration option
for just this seems overkill.

> (...) but in general WARN_ON() isn't terribly expensive
> (depending entirely on the complexity of the condition of course).

The condition is pretty trivial here, so it should indeed be fairly
cheap. However you have to multiply it by the number of times it is
called. When this WARN was introduced, there were 61 calls to
dmi_check_system() in the whole kernel tree. Today we have 164 and the
count keeps increasing. And some of them in common paths which get
called repeatedly. I just instrumented the function to see how many
times it was called on my x86_64 workstation and the answer is 110.
That's not a one-time thing.

-- 
Jean Delvare
SUSE L3 Support