[Patch] arm/arm64: Thumb=>ARM signal handling setup skips the a few instructions on Snapdragon S4/Krait

[Patch] arm/arm64: Thumb=>ARM signal handling setup skips the a few instructions on Snapdragon S4/Krait

[T.J. Purtell]

The Thumb instruction set include an If-Then instruction.  If an ARM
function is registered as a signal handler, and that signal is
delivered inside the block of instructions follow the IT instruction,
some of the instructions at the beginning of the signal handler are
skipped.  This occurs because the IT state bits of the Program Status
Register are not cleared by the kernel.

The ARM architecture reference specifies that the IT state bits in the
PSR must be all zeros in ARM mode or behavior is unspecified.  On the
Qualcomm Snapdragon S4/Krait architecture CPUs the processor continues
to consider the IT state bits while in ARM mode.  This makes it so
that some instructions are skipped by the CPU.

The relevant clipping of the ARM architecture  document is available
here: https://www.dropbox.com/s/6hpwey3kklw20c1/ITState-ARM-Architecture.pdf

This manifests itself in code that uses a lot of signal handling
concurrently with complicated logic code compiled for thumb when the
signal handler compiled as ARM code.  One example is the mono runtime.
 It uses a signal based mechanism to perform stop the world operation
needed by its garbage collector.  It is compiled for ARM and generates
ARM code, however, bionic (libc for android) is compiled for thumb.
During a test case involving native allocation in parallel with
garbage collection (signal handling), we found that the test case
would crash only when running on a Krait architecture CPU.  We found
the preceding instructions before the signal to always point to
something inside an IT sequence, in this case the logic in the dlfree
merging of free heap blocks.  After adding a sequence of NOPs to the
beginning of the signal handlers, the problem disappeared.  We applied
the attached patch to the kernel on one of the Krait devices and found
that it also alleviated the crashes.


The patch simply clears the IT State bits regardless of if the signal
handler is an ARM/Thumb function.  It also includes the same fix
applied to the arm64 compat handler.

-tj



diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index ab33042..782657a 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -375,12 +375,16 @@ setup_return(struct pt_regs *regs, struct ksignal *ksig,
  */
  thumb = handler & 1;

+#if __LINUX_ARM_ARCH__ >= 7
+ /*
+  * Clear the If-Then Thumb-2 execution state
+  * ARM spec requires this to be all 000s in ARM mode
+ */
+ cpsr &= ~PSR_IT_MASK;
+#endif
+
  if (thumb) {
  cpsr |= PSR_T_BIT;
-#if __LINUX_ARM_ARCH__ >= 7
- /* clear the If-Then Thumb-2 execution state */
- cpsr &= ~PSR_IT_MASK;
-#endif
  } else
  cpsr &= ~PSR_T_BIT;
  }
diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
index e393174..8f6c3be 100644
--- a/arch/arm64/kernel/signal32.c
+++ b/arch/arm64/kernel/signal32.c
@@ -474,9 +474,11 @@ static void compat_setup_return(struct pt_regs
*regs, struct k_sigaction *ka,
  /* Check if the handler is written for ARM or Thumb */
  thumb = handler & 1;

+ /* ARM spec requires this to be all 000s in ARM mode */
+ spsr &= ~COMPAT_PSR_IT_MASK;
+
  if (thumb) {
  spsr |= COMPAT_PSR_T_BIT;
- spsr &= ~COMPAT_PSR_IT_MASK;
  } else {
  spsr &= ~COMPAT_PSR_T_BIT;
  }

Re: [Patch] arm/arm64: Thumb=>ARM signal handling setup skips the a few instructions on Snapdragon S4/Krait

[Catalin Marinas]

(cc'ing the Linux ARM kernel list)

On 25 October 2013 05:54, T.J. Purtell <tj@mobisocial.us> wrote:
> The ARM architecture reference specifies that the IT state bits in the
> PSR must be all zeros in ARM mode or behavior is unspecified.  On the
> Qualcomm Snapdragon S4/Krait architecture CPUs the processor continues
> to consider the IT state bits while in ARM mode.  This makes it so
> that some instructions are skipped by the CPU.

Good catch.

> diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
> index e393174..8f6c3be 100644
> --- a/arch/arm64/kernel/signal32.c
> +++ b/arch/arm64/kernel/signal32.c
> @@ -474,9 +474,11 @@ static void compat_setup_return(struct pt_regs
> *regs, struct k_sigaction *ka,
>   /* Check if the handler is written for ARM or Thumb */
>   thumb = handler & 1;
>
> + /* ARM spec requires this to be all 000s in ARM mode */
> + spsr &= ~COMPAT_PSR_IT_MASK;
> +
>   if (thumb) {
>   spsr |= COMPAT_PSR_T_BIT;
> - spsr &= ~COMPAT_PSR_IT_MASK;
>   } else {
>   spsr &= ~COMPAT_PSR_T_BIT;
>   }

Please send me a separate arm64 patch, I'll push it via my tree (and
cc stable as well).

Catalin