linux-kernel - Re: [GIT pull] irq updates for 4.13

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: <CA+55aFx2aSFwrnw_s3vvNTni5e0M8BNuMrJZapvF4rM7eApyQA@mail.gmail.com>
Date:   Tue, 11 Jul 2017 11:16:03 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Tony Lindgren <tony@...mide.com>,
        Sebastian Reichel <sebastian.reichel@...labora.co.uk>,
        LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>, Pavel Machek <pavel@....cz>,
        Linus Walleij <linus.walleij@...aro.org>,
        Grygorii Strashko <grygorii.strashko@...com>
Subject: Re: [GIT pull] irq updates for 4.13

On Tue, Jul 11, 2017 at 10:52 AM, Thomas Gleixner <tglx@...utronix.de> wrote:
>
> Not completely, because of the free path issues. See the other mail. Tony
> confirmed that it works. I wait for Sebastian and queue it with a proper
> changelog, ok?

Ugh, I absolutely detest your ugly "bool buslock" parameter to
irq_release_resources().

And there seems to be no reason for it.

Why don't you just move the

        chip_bus_sync_unlock(desc);

call in __free_irq() down to just before you release the request_mutex?

In fact, looking at __free_irq(), I note that it's locking is
completely broken shit. Look at the

                if (!action) {
                        WARN(1, "Trying to free already-free IRQ %d\n", irq);

error case, and look for where it unlocks request_mutex. Yeah, it doesn't.

So honestly, I think this code is broken, and it's broken partly
because it has some really bad locking and logic rules.

Why not fix those stupid bugs and clean things up at the same time?
Make the rule be that as you take the request_mutex lock, you then
also do the chip_bus_lock().

And when you release the request_mutex lock, you do
chip_bus_sync_unlock() just before.

And no, I have no idea what the locking rules are for
irq_finalize_oneshot() - it does that chip_bus_lock() without having
any external serialization. Is that ok? Are the chip handlers able to
deal with that? Same seems to go for free_percpu_irq().

Anyway, patch attached (AGAIN, TOTALLY UNTESTED) showing what I mean,
and fixing (well, modulo any bugs I introduced by my untested sh*t)
that definite bug in lack of unlocking.

But that "bool buslock" thing really is too disgusting. Conditional
locking should not be done. It's a sign of serious problems, imnsho.

Comments? Even if they are "Linus, you're way out of line, and you
can't just move that chip_bus_sync_unlock() down like that because of
XYZ, you moron".

For example, it's entirely possible that we can't do the
"synchronize_irq()" waiting while we hold that chip_bus_lock().  But
the ones I looked at seemed to all take sleeping locks (or no locks at
all - doing other things), which implies that they certainly can't be
blocking irq delivery.

So I'm *not* claiming that the attached patch is necessarily right. I
just really don't like your conditional lock thing, and this would
seem to possibly be a clean way around it if it works.

                    Linus

View attachment "patch.diff" of type "text/plain" (1917 bytes)