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

Borislav Petkov
2021-02-08T15:58:30Z

On Thu, Feb 04, 2021 at 09:49:08PM +0100, Thomas Gleixner wrote:
> The effort to make the ASM entry code slim and unified moved the irq stack
> switching out of the low level ASM code so that the whole return from
> interrupt work and state handling can be done in C and the ASM code just
> handles the low level details of entry and exit.
>
> This ended up being a suboptimal implementation for various reasons
> (including tooling). The main pain points are:
>
> - The indirect call which is expensive thanks to retpoline
>
> - The inability to stay on the irq stack for softirq processing on return
> from interrupt
>
> - The fact that the stack switching code ends up being an easy to target
> exploit gadget.
>
> Prepare for inlining the stack switching logic into the C entry points by
> providing a ASM macro which contains the guts of the switching mechanism:
>
> 1) Store RSP at the top of the irq stack
> 2) Switch RSP to the irq stack
> 3) Invoke code
> 4) Pop the original RSP back
>
> Document the unholy asm() logic while at it to reduce the amount of head
> scratching required a half year from now.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> arch/x86/include/asm/irq_stack.h | 104 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 104 insertions(+)
>
> --- a/arch/x86/include/asm/irq_stack.h
> +++ b/arch/x86/include/asm/irq_stack.h
> @@ -7,6 +7,110 @@
> #include <asm/processor.h>
>
> #ifdef CONFIG_X86_64
> +
> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
> +# define IRQSTACK_CALL_CONSTRAINT , ASM_CALL_CONSTRAINT
> +#else
> +# define IRQSTACK_CALL_CONSTRAINT
> +#endif
> +
> +/*
> + * Macro to inline switching to an interrupt stack and invoking function
> + * calls from there. The following rules apply:
> + *
> + * - Ordering:
> + *
> + * 1. Write the stack pointer content into the top most place of

I think "content" is not needed here - just "Write the stack pointer".

> + * the irq stack. This ensures that the various unwinders can
> + * link back to the original stack.
> + *
> + * 2. Switch the stack pointer to the top of the irq stack.
> + *
> + * 3. Invoke whatever needs to be done (@asm_call argument)
> + *
> + * 4. Pop the original stack pointer from the top of the irq stack
> + * which brings it back to the original stack where it left off.
> + *
> + * - Function invocation:
> + *
> + * To allow flexible usage of the macro, the actual function code including
> + * the store of the arguments in the call ABI registers is handed in via
> + * the @asm_call argument.
> + *
> + * - Local variables:
> + *
> + * @tos:
> + * The @tos variable holds a pointer to the top of the irq stack and
> + * _must_ be allocated in a non-callee saved register as this is a
> + * restriction coming from objtool.
> + *
> + * Note, that (tos) is both in input and output constraints to ensure
> + * that the compiler does not assume that R11 is left untouched in
> + * case this macro is used in some place where the per cpu interrupt
> + * stack pointer is used again afterwards
> + *
> + * - Function arguments:
> + * The function argument(s) if any have to be defined in register

Commas:

The function argument(s), if any, ...

> + * variables at the place where this is invoked. Storing the
> + * argument(s) in the proper register(s) is part of the @asm_call
> + *
> + * - Constraints:
> + *
> + * The constraints have to be done very carefully because the compiler
> + * does not know about the assembly call.
> + *
> + * output:
> + * As documented already above the @tos variable is required to be in
> + * the output constraints to make the compiler aware that R11 cannot be
> + * reused after the asm() statement.
> + *
> + * For builds with CONFIG_UNWIND_FRAME_POINTER ASM_CALL_CONSTRAINT is
> + * required as well as this prevents certain creative GCC variants from
> + * misplacing the ASM code.
> + *
> + * input:
> + * - func:
> + * Immediate, which tells the compiler that the function is referenced.
> + *
> + * - tos:
> + * Register. The actual register is defined by the variable declaration.
> + *
> + * - function arguments:
> + * The constraints are handed in via the 'argconstr' argument list. They

"argconstr" or "constr"?

> + * describe the register arguments which are used in @asm_call.
> + *
> + * clobbers:
> + * Function calls can clobber anything except the callee-saved
> + * registers. Tell the compiler.
> + */
> +#define __call_on_irqstack(func, asm_call, constr...) \

Does the name need to be prepended with "__"? I don't see a
"call_on_irqstack" variant...

> +{ \
> + 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 \
> + : [__func] "i" (func), [__tos] "r" (tos) constr \

I think you can call the symbolic names the same as the variables -
i.e., without the "__" so that there's less confusion when looking at
the code.

> + : "cc", "rax", "rcx", "rdx", "rsi", "rdi", "r8", "r9", "r10", \
> + "memory" \
> + ); \
> +}
> +
> +/* Macros to assert type correctness for run_*_on_irqstack macros */
> +#define assert_function_type(func, proto) \
> + static_assert(__builtin_types_compatible_p(typeof(&func), proto))
> +
> +#define assert_arg_type(arg, proto) \
> + static_assert(__builtin_types_compatible_p(typeof(arg), proto))
> +
> static __always_inline bool irqstack_active(void)
> {
> return __this_cpu_read(hardirq_stack_inuse);
>

--
Regards/Gruss,
Boris.