linux-kernel - Re: [patch V4 part 3 12/29] x86/entry/common: Provide idtentry

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALCETrXwuxtZgniJxKZOy5ryqXSbbGMHMBwgEb400Pn9XpynzQ@mail.gmail.com>
Date:   Sun, 10 May 2020 21:34:13 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Andy Lutomirski <luto@...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>
Subject: Re: [patch V4 part 3 12/29] x86/entry/common: Provide idtentry_enter/exit()

On Tue, May 5, 2020 at 7:15 AM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> Provide functions which handle the low level entry and exit similiar to
> enter/exit from user mode.
>

> +
> +/**
> + * idtentry_exit - Common code to handle return from exceptions
> + * @regs:      Pointer to pt_regs (exception entry regs)
> + *
> + * Depending on the return target (kernel/user) this runs the necessary
> + * preemption and work checks if possible and reguired and returns to
> + * the caller with interrupts disabled and no further work pending.
> + *
> + * This is the last action before returning to the low level ASM code which
> + * just needs to return to the appropriate context.
> + *
> + * Invoked by all exception/interrupt IDTENTRY handlers which are not
> + * returning through the paranoid exit path (all except NMI, #DF and the IST
> + * variants of #MC and #DB).

The paranoid-exit bit is not really relevant.  The important part is
which stack we're on.  See below.

> + */
> +void noinstr idtentry_exit(struct pt_regs *regs)
> +{
> +       lockdep_assert_irqs_disabled();

How about:

#ifdef CONFIG_DEBUG_ENTRY
WARN_ON_ONCE(!on_thread_stack());
#endif

> +
> +       /* Check whether this returns to user mode */
> +       if (user_mode(regs)) {
> +               prepare_exit_to_usermode(regs);
> +       } else if (regs->flags & X86_EFLAGS_IF) {
> +               /* Check kernel preemption, if enabled */
> +               if (IS_ENABLED(CONFIG_PREEMPTION)) {
> +                       /*
> +                        * This needs to be done very carefully.
> +                        * idtentry_enter() invoked rcu_irq_enter(). This
> +                        * needs to undone before scheduling.
> +                        *
> +                        * Preemption is disabled inside of RCU idle
> +                        * sections. When the task returns from
> +                        * preempt_schedule_irq(), RCU is still watching.
> +                        *
> +                        * rcu_irq_exit_preempt() has additional state
> +                        * checking if CONFIG_PROVE_RCU=y
> +                        */
> +                       if (!preempt_count()) {
> +                               instr_begin();
> +                               rcu_irq_exit_preempt();
> +                               if (need_resched())
> +                                       preempt_schedule_irq();

This is an excellent improvement.  Thanks!

> +                               /* Covers both tracing and lockdep */
> +                               trace_hardirqs_on();
> +                               instr_end();
> +                               return;
> +                       }
> +               }
> +               instr_begin();
> +               /* Tell the tracer that IRET will enable interrupts */
> +               trace_hardirqs_on_prepare();

Why is trace_hardirqs_on() okay above but not here?  Is it that we
know we weren't RCU-quiescent if we had preemption and IF on?  But
even this code path came from an IF-on context.  I'm confused.  Maybe
some comments as to why this case seems to be ordered so differently
from the !preempt_count() case would be helpful.

> +               lockdep_hardirqs_on_prepare(CALLER_ADDR0);
> +               instr_end();
> +               rcu_irq_exit();
> +               lockdep_hardirqs_on(CALLER_ADDR0);
> +       } else {
> +               /* IRQ flags state is correct already. Just tell RCU */
> +               rcu_irq_exit();
> +       }
> +}
> --- a/arch/x86/include/asm/idtentry.h
> +++ b/arch/x86/include/asm/idtentry.h
> @@ -7,6 +7,9 @@
>
>  #ifndef __ASSEMBLY__
>
> +void idtentry_enter(struct pt_regs *regs);
> +void idtentry_exit(struct pt_regs *regs);
> +
>  /**
>   * DECLARE_IDTENTRY - Declare functions for simple IDT entry points
>   *                   No error code pushed by hardware
>