linux-kernel - Re: [RFC PATCH 1/5] x86: introduce preemption disable prefix

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:   Wed, 17 Oct 2018 18:22:48 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     Nadav Amit <namit@...are.com>
Cc:     Ingo Molnar <mingo@...hat.com>, Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "H . Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org, Nadav Amit <nadav.amit@...il.com>,
        x86@...nel.org, Borislav Petkov <bp@...en8.de>,
        David Woodhouse <dwmw@...zon.co.uk>
Subject: Re: [RFC PATCH 1/5] x86: introduce preemption disable prefix


> On Oct 17, 2018, at 5:54 PM, Nadav Amit <namit@...are.com> wrote:
> 
> It is sometimes beneficial to prevent preemption for very few
> instructions, or prevent preemption for some instructions that precede
> a branch (this latter case will be introduced in the next patches).
> 
> To provide such functionality on x86-64, we use an empty REX-prefix
> (opcode 0x40) as an indication that preemption is disabled for the
> following instruction.

Nifty!

That being said, I think you have a few bugs.  First, you can’t just ignore a rescheduling interrupt, as you introduce unbounded latency when this happens — you’re effectively emulating preempt_enable_no_resched(), which is not a drop-in replacement for preempt_enable(). To fix this, you may need to jump to a slow-path trampoline that calls schedule() at the end or consider rewinding one instruction instead. Or use TF, which is only a little bit terrifying...

You also aren’t accounting for the case where you get an exception that is, in turn, preempted.



> 
> It is expected that this opcode is not in common use.
> 
> Signed-off-by: Nadav Amit <namit@...are.com>
> ---
> arch/x86/entry/entry_64.S            | 10 ++++++++++
> arch/x86/include/asm/nospec-branch.h | 12 ++++++++++++
> 2 files changed, 22 insertions(+)
> 
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index cb8a5893fd33..31d59aad496e 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -643,6 +643,16 @@ retint_kernel:
>    jnc    1f
> 0:    cmpl    $0, PER_CPU_VAR(__preempt_count)
>    jnz    1f
> +
> +    /*
> +     * Allow to use hint to prevent preemption on a certain instruction.
> +     * Consider an instruction with the first byte having REX prefix
> +     * without any bits set as an indication for preemption disabled.
> +     */
> +    movq    RIP(%rsp), %rax
> +    cmpb    $PREEMPT_DISABLE_PREFIX, (%rax)
> +    jz    1f
> +
>    call    preempt_schedule_irq
>    jmp    0b
> 1:
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index 80dc14422495..0267611eb247 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -52,6 +52,12 @@
>    jnz    771b;                \
>    add    $(BITS_PER_LONG/8) * nr, sp;
> 
> +/*
> + * An empty REX-prefix is an indication that preemption should not take place on
> + * this instruction.
> + */
> +#define PREEMPT_DISABLE_PREFIX                 (0x40)
> +
> #ifdef __ASSEMBLY__
> 
> /*
> @@ -148,6 +154,12 @@
> #endif
> .endm
> 
> +.macro preempt_disable_prefix
> +#ifdef CONFIG_PREEMPT
> +    .byte    PREEMPT_DISABLE_PREFIX
> +#endif
> +.endm
> +
> #else /* __ASSEMBLY__ */
> 
> #define ANNOTATE_NOSPEC_ALTERNATIVE                \
> -- 
> 2.17.1
>