linux-kernel - Re: [patch 05/12] x86/irq: Provide macro for inlining irq stack switching

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] [day] [month] [year] [list]

Message-ID: <87wnvm8x6r.fsf@nanos.tec.linutronix.de>
Date:   Fri, 05 Feb 2021 14:24:12 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Uros Bizjak <ubizjak@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [patch 05/12] x86/irq: Provide macro for inlining irq stack switching

Uros,

On Fri, Feb 05 2021 at 12:03, Uros Bizjak wrote:

can you please fix your mail client to generate at least the 'In-Reply-to:'
header? Lacking that header breaks threading on lore:

        https://lore.kernel.org/lkml/20210204204903.350275743@linutronix.de/

Your mail is missing there. Ideally it also emits 'References'.

> #define __call_on_irqstack(func, asm_call, constr...) \
> +{ \
> + register void *tos asm("r11"); \
> + \
> + tos = ((void *)__this_cpu_read(hardirq_stack_ptr)); \
> + \
> + asm_inline volatile( \
> + "movq %%rsp, (%[__tos]) \n" \
> + "movq %[__tos], %%rsp \n" \
> + \
> + asm_call \
> + \
> + "popq %%rsp \n" \
> + \
> + : "+r" (tos) IRQSTACK_CALL_CONSTRAINT \
>
> Please note that GCC documents "U" register constraint that can be
> used here instead of declaring hard register in the variable
> declaration:
>
>     'U'
>          The call-clobbered integer registers.

That's not really helpful because clang does not support 'U'.

> + : [__func] "i" (func), [__tos] "r" (tos) constr \
>
> There is no need to declare "tos" as read operand again, it is already
> declared above as readwrite (+) operand.

It makes clang builds fail.

> Considering that (according to the above documentation) it is
> necessary to list all input registers that pass function arguments,
> the compiler is free to allocate any remaining register from "U"
> register class, not only r11. Using an earlyclobber modifier prevents
> the compiler from allocating a register that carries input argument,
> so:
>
> : [__tos] "+&U" (tos) IRQSTACK_CALL_CONSTRAINT \
> : [__func] "i" (func) constr \
>
> could be used.

See above. Without the U constraint we can't rely on the compiler to do
the right thing without the explicit register asm("r11"); And even with
'U' we need to enforce that there is only one U register left to use.

The problem is that the compiler does not know about the call. So we
need to ensure via the clobbers and input/output arguments that it can't
use any of the callee clobbered registers accross the inline asm.

With

        void *tos = this_cpu_read(...);

        : "cc", .... "r9", "r10"

the compiler could still use "r11" for some other stuff and stick tos
into a callee saved register, e.g. r15. If the called function then
clobbers "r11" everything goes south.

There is no point in being extra smart here. The functions have no
register pressure as they are small so enforcing the register allocation
is not restricting the compiler freedom to much. But it ensures that the
compiler can't do anything subtly wrong which would end up being a hard
to debug disaster.

> Also note that functions with variable arguments pass information
> about the number of vector registers used in %rax, so %rax should be
> listed as input argument in this case. But this should be of no issue
> here.

That's really irrelevant as it's a very narrow use case for functions
with 0..2 arguments.

Thanks,

        tglx