ARM: fix gcc-10 thumb2-kernel regression
Subsystems (2)
PATCH 1/1 ARM: fix gcc-10 thumb2-kernel regression
From: Arnd Bergmann <[email protected]>
When building the kernel wtih gcc-10 or higher using the CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=y flag, the compiler picks a slightly different set of registers for the inline assembly in cpu_init() that subsequently results in a corrupt kernel stack as well as remaining in FIQ mode. If a banked register is used for the last argument, the wrong version of that register gets loaded into CPSR_c. When building in Arm mode, the arguments are passed as immediate values and the bug cannot happen.
This got introduced when Daniel reworked the FIQ handling and was technically always broken, but happened to work with both clang and gcc before gcc-10 as long as they picked one of the lower registers. This is probably an indication that still very few people build the kernel in Thumb2 mode.
Marek pointed out the problem on IRC, Arnd narrowed it down to this inline assembly and Russell pinpointed the exact bug.
Change the constraints to force the final mode switch to use a non-banked register for the argument to ensure that the correct constant gets loaded. Another alternative would be to always use registers for the constant arguments to avoid the #ifdef that has now become more complex.
--- arch/arm/kernel/setup.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
arch/arm/kernel/setup.c +9 -7
@@ -545,9 +545,11 @@ void notrace cpu_init(void)* In Thumb-2, msr with an immediate value is not allowed. */ #ifdef CONFIG_THUMB2_KERNEL -#define PLC "r" +#define PLC_l "l" +#define PLC_r "r"#else -#define PLC "I" +#define PLC_l "I" +#define PLC_r "I"#endif /*
@@ -569,15 +571,15 @@ void notrace cpu_init(void) "msr cpsr_c, %9" : : "r" (stk), - PLC (PSR_F_BIT | PSR_I_BIT | IRQ_MODE), + PLC_r (PSR_F_BIT | PSR_I_BIT | IRQ_MODE), "I" (offsetof(struct stack, irq[0])), - PLC (PSR_F_BIT | PSR_I_BIT | ABT_MODE), + PLC_r (PSR_F_BIT | PSR_I_BIT | ABT_MODE), "I" (offsetof(struct stack, abt[0])), - PLC (PSR_F_BIT | PSR_I_BIT | UND_MODE), + PLC_r (PSR_F_BIT | PSR_I_BIT | UND_MODE), "I" (offsetof(struct stack, und[0])), - PLC (PSR_F_BIT | PSR_I_BIT | FIQ_MODE), + PLC_r (PSR_F_BIT | PSR_I_BIT | FIQ_MODE), "I" (offsetof(struct stack, fiq[0])), - PLC (PSR_F_BIT | PSR_I_BIT | SVC_MODE) + PLC_l (PSR_F_BIT | PSR_I_BIT | SVC_MODE) : "r14");#endif }
Signature · 1 line
2.29.2
Quoted 26 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
When building the kernel wtih gcc-10 or higher using the
CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=yflag, the compiler picks a slightly different set of registers for the inline assembly incpu_init()that subsequently results in a corrupt kernel stack as well as remaining in FIQ mode. If a banked register is used for the last argument, the wrong version of that register gets loaded intoCPSR_c. When building in Arm mode, the arguments are passed as immediate values and the bug cannot happen.This got introduced when Daniel reworked the FIQ handling and was technically always broken, but happened to work with both clang and gcc before gcc-10 as long as they picked one of the lower registers. This is probably an indication that still very few people build the kernel in Thumb2 mode.
Marek pointed out the problem on IRC, Arnd narrowed it down to this inline assembly and Russell pinpointed the exact bug.
Change the constraints to force the final mode switch to use a non-banked register for the argument to ensure that the correct constant gets loaded. Another alternative would be to always use registers for the constant arguments to avoid the #ifdef that has now become more complex.
Quoted 5 lines
Cc <[email protected]> # v3.18+Cc Daniel Thompson <[email protected]>Reported-by Marek Vasut <[email protected]>Fixes c0e7f7ee717e ("ARM: 8150/3: fiq: Replace default FIQ handler")Signed-off-by Arnd Bergmann <[email protected]>
Nice bug!
Quoted 46 lines
--- arch/arm/kernel/setup.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)arch/arm/kernel/setup.c +9
-8@@ -545,9 +545,11 @@ void notrace cpu_init(void)* In Thumb-2, msr with an immediate value is not allowed. */ #ifdef CONFIG_THUMB2_KERNEL -#define PLC "r" +#define PLC_l "l" +#define PLC_r "r"#else -#define PLC "I" +#define PLC_l "I" +#define PLC_r "I"#endif /* @@ -569,15 +571,15 @@ void notrace cpu_init(void) "msr cpsr_c, %9" : : "r" (stk), - PLC (PSR_F_BIT | PSR_I_BIT | IRQ_MODE), + PLC_r (PSR_F_BIT | PSR_I_BIT | IRQ_MODE), "I" (offsetof(struct stack, irq[0])), - PLC (PSR_F_BIT | PSR_I_BIT | ABT_MODE), + PLC_r (PSR_F_BIT | PSR_I_BIT | ABT_MODE), "I" (offsetof(struct stack, abt[0])), - PLC (PSR_F_BIT | PSR_I_BIT | UND_MODE), + PLC_r (PSR_F_BIT | PSR_I_BIT | UND_MODE), "I" (offsetof(struct stack, und[0])), - PLC (PSR_F_BIT | PSR_I_BIT | FIQ_MODE), + PLC_r (PSR_F_BIT | PSR_I_BIT | FIQ_MODE), "I" (offsetof(struct stack, fiq[0])), - PLC (PSR_F_BIT | PSR_I_BIT | SVC_MODE) + PLC_l (PSR_F_BIT | PSR_I_BIT | SVC_MODE) : "r14");#endif } --2.29.2
Quoted 27 lines · Ard Biesheuvel
Quoted 26 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
When building the kernel wtih gcc-10 or higher using the
CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=yflag, the compiler picks a slightly different set of registers for the inline assembly incpu_init()that subsequently results in a corrupt kernel stack as well as remaining in FIQ mode. If a banked register is used for the last argument, the wrong version of that register gets loaded intoCPSR_c. When building in Arm mode, the arguments are passed as immediate values and the bug cannot happen.This got introduced when Daniel reworked the FIQ handling and was technically always broken, but happened to work with both clang and gcc before gcc-10 as long as they picked one of the lower registers. This is probably an indication that still very few people build the kernel in Thumb2 mode.
Marek pointed out the problem on IRC, Arnd narrowed it down to this inline assembly and Russell pinpointed the exact bug.
Change the constraints to force the final mode switch to use a non-banked register for the argument to ensure that the correct constant gets loaded. Another alternative would be to always use registers for the constant arguments to avoid the #ifdef that has now become more complex.
Quoted 7 lines
Quoted 5 lines
Cc <[email protected]> # v3.18+Cc Daniel Thompson <[email protected]>Reported-by Marek Vasut <[email protected]>Fixes c0e7f7ee717e ("ARM: 8150/3: fiq: Replace default FIQ handler")Signed-off-by Arnd Bergmann <[email protected]>Nice bug!
Indeed. Many thanks for those involved with the find and fix!
Daniel.
Quoted 25 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
When building the kernel wtih gcc-10 or higher using the
CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=yflag, the compiler picks a slightly different set of registers for the inline assembly incpu_init()that subsequently results in a corrupt kernel stack as well as remaining in FIQ mode. If a banked register is used for the last argument, the wrong version of that register gets loaded intoCPSR_c. When building in Arm mode, the arguments are passed as immediate values and the bug cannot happen.This got introduced when Daniel reworked the FIQ handling and was technically always broken, but happened to work with both clang and gcc before gcc-10 as long as they picked one of the lower registers. This is probably an indication that still very few people build the kernel in Thumb2 mode.
Marek pointed out the problem on IRC, Arnd narrowed it down to this inline assembly and Russell pinpointed the exact bug.
Change the constraints to force the final mode switch to use a non-banked register for the argument to ensure that the correct constant gets loaded. Another alternative would be to always use registers for the constant arguments to avoid the #ifdef that has now become more complex.
Quoted 5 lines
Cc <[email protected]> # v3.18+Cc Daniel Thompson <[email protected]>Reported-by Marek Vasut <[email protected]>Fixes c0e7f7ee717e ("ARM: 8150/3: fiq: Replace default FIQ handler")Signed-off-by Arnd Bergmann <[email protected]>
Wow. Nice bug hunt here, hats off!
Yours,
Linus Walleij