linux-kernel - Re: [patch V6 07/37] x86/entry: Provide helpers for execute on irqstack

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: <CALCETrWD8qH-P4J3MB6Q9mr1MRLzsR7Fpab+Fk9Ac60qQBZPaA@mail.gmail.com>
Date:   Mon, 18 May 2020 16:46:22 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Andy Lutomirski <luto@...nel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Alexandre Chartre <alexandre.chartre@...cle.com>,
        Frederic Weisbecker <frederic@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Joel Fernandes <joel@...lfernandes.org>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Juergen Gross <jgross@...e.com>,
        Brian Gerst <brgerst@...il.com>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Will Deacon <will@...nel.org>,
        Tom Lendacky <thomas.lendacky@....com>,
        Wei Liu <wei.liu@...nel.org>,
        Michael Kelley <mikelley@...rosoft.com>,
        Jason Chen CJ <jason.cj.chen@...el.com>,
        Zhao Yakui <yakui.zhao@...el.com>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>
Subject: Re: [patch V6 07/37] x86/entry: Provide helpers for execute on irqstack

On Mon, May 18, 2020 at 4:11 PM Andy Lutomirski <luto@...nel.org> wrote:
>
> On Fri, May 15, 2020 at 5:10 PM Thomas Gleixner <tglx@...utronix.de> wrote:
> >
> >
> > Device interrupt handlers and system vector handlers are executed on the
> > interrupt stack. The stack switch happens in the low level assembly entry
> > code. This conflicts with the efforts to consolidate the exit code in C to
> > ensure correctness vs. RCU and tracing.
> >
> > As there is no way to move #DB away from IST due to the MOV SS issue, the
> > requirements vs. #DB and NMI for switching to the interrupt stack do not
> > exist anymore. The only requirement is that interrupts are disabled.
> >
> > That allows to move the stack switching to C code which simplifies the
> > entry/exit handling further because it allows to switch stacks after
> > handling the entry and on exit before handling RCU, return to usermode and
> > kernel preemption in the same way as for regular exceptions.
> >
> > The initial attempt of having the stack switching in inline ASM caused too
> > much headache vs. objtool and the unwinder. After analysing the use cases
> > it was agreed on that having the stack switch in ASM for the price of an
> > indirect call is acceptable as the main users are indirect call heavy
> > anyway and the few system vectors which are empty shells (scheduler IPI and
> > KVM posted interrupt vectors) can run from the regular stack.
> >
> > Provide helper functions to check whether the interrupt stack is already
> > active and whether stack switching is required.
> >
> > 64 bit only for now. 32 bit has a variant of that already. Once this is
> > cleaned up the two implementations might be consolidated as a cleanup on
> > top.
> >
>
> Acked-by: Andy Lutomirski <luto@...nel.org>
>
> Have you tested by forcing a stack trace from the IRQ stack and making
> sure it unwinds all the way out?

Actually, I revoke my ack.  Can you make one of two changes:

Option A: Add an assertion to run_on_irqstack to verify that irq_count
was -1 at the beginning?  I suppose this also means you could just
explicitly write 0 instead of adding and subtracting.

Option B: Make run_on_irqstack() just call the function on the current
stack if we're already on the irq stack.

Right now, it's too easy to mess up and not verify the right
precondition before calling run_on_irqstack().

If you choose A, perhaps add a helper to do the if(irq_needs_irqstack)
dance so that users can just do:

run_on_irqstack_if_needed(...);

instead of checking everything themselves.