linux-kernel - Re: [RFC GIT Pull] core watchdog sanitizing

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:   Sun, 1 Oct 2017 12:59:37 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Don Zickus <dzickus@...hat.com>
Subject: Re: [RFC GIT Pull] core watchdog sanitizing

I refuse to pull this.

Look, I understand what you want to do, but the code is disgusting.

Maybe most of it is fine, but I just couldn't stomach looking at it
after just a few lines.

Look at that abortion called "watchdog_nmi_reconfigure()".

It's one single function that does two completely different things
based on a static argument.

Whaa?  So now you have doubly illegible code: the callers have that
insane set of

     watchdog_nmi_reconfigure(true/false);

in it, which is illegible garbage. There's no way that makes sense to anybody.

And the actual implementation has

    void watchdog_nmi_reconfigure(bool run)

where the whole function is basically a single if-statement on that
"run" argument that does two totally different things.

If you don't see how that is pure and utter garbage, I don't know what
to say. It's just bad code. Don't do that in the first place, but
*definitely* don't do that and then try to send it to me during an rc.

Seriously, instead of that retarded

    watchdog_nmi_reconfigure(false);
    ...
    watchdog_nmi_reconfigure(true);

you could have written it something like

    watchdog_nmi_stop();
    ...
    watchdog_nmi_start();

and it would be _understandable_ code. You don't have to even know the
code to understand what's going on.

So get rid of that kind of shit, and I may reconsider. But as is, I
look at that patch and say "no, this is worse than the garbage it used
to be".

Yeah, yeah, I'll be honest, and you were unlucky, and just happened to
hit a pet peeve of mine, but I absolutely *detest* code that describes
obvious static things as non-obvious dynamic "flag" arguments to a
function for no good reason. There's not even any code sharing between
the two cases going on aside from the trivial locking.

That kind of code makes it harder for everybody. It makes it harder
for the compiler to generate good code (you need to inline it to get
the obvious code generation), it makes it harder for checkers to
verify things, and it makes it harder for humans to read and
understand what is going on.

If you see code like

    watchdog_nmi_reconfigure(true);

and you don't ask yourself "what does 'true' mean in this call site",
you're either not human, or you just don't care. Neither of which are
good.

In contrast, when you see code like

    watchdog_nmi_start();

it kind of documents itself, don't you think?

And yes, I see the comment above the function definition. No, the
comment doesn't help. The comment just makes it obvious that somebody
realized that the calling convention was too damn confusing. Good. But
even better would have been to just not make it that confusing in the
first place.

                     Linus

On Sun, Oct 1, 2017 at 3:34 AM, Thomas Gleixner <tglx@...utronix.de> wrote:
>
> The watchdog (hard/softlockup detector) code is pretty much broken in its
> current state. The patch series addresses this by removing all duct tape
> and refactoring it into a workable state.