[PATCH v2] ARM: alignment: Prevent ignoring of faults with ARMv6 unaligned access model

[PATCH v2] ARM: alignment: Prevent ignoring of faults with ARMv6 unaligned access model

[Dave Martin]

Currently, it's possible to set the kernel to ignore alignment
faults when changing the alignment fault handling mode at runtime
via /proc/sys/alignment, even though this is undesirable on ARMv6
and above, where it can result in infinite spins where an un-fixed-
up instruction repeatedly faults.

In addition, the kernel clobbers any alignment mode specified on
the command-line if running on ARMv6 or above.

This patch factors out the necessary safety check into a couple of
new helper functions, and checks and modifies the fault handling
mode as appropriate on boot and on writes to /proc/cpu/alignment.

Prior to ARMv6, the behaviour is unchanged.

For ARMv6 and above, the behaviour changes as follows:

  * Attempting to ignore faults on ARMv6 results in the mode being
    forced to UM_FIXUP instead.  A warning is printed if this
    happened as a result of a write to /proc/cpu/alignment.  The
    user's UM_WARN bit (if present) is still honoured.

  * An alignment= argument from the kernel command-line is now
    honoured, except that the kernel will modify the specified mode
    as described above.  This is allows modes such as UM_SIGNAL and
    UM_WARN to be active immediately from boot, which is useful for
    debugging purposes.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
v2: Missing semicolon added in safe_usermode();

 arch/arm/mm/alignment.c |   42 ++++++++++++++++++++++++++++++------------
 1 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index 724ba3b..8611f5d 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -95,6 +95,33 @@ static const char *usermode_action[] = {
 	"signal+warn"
 };
 
+/* Return true if and only if the ARMv6 unaligned access model is in use. */
+static bool cpu_is_v6_unaligned(void)
+{
+	return cpu_architecture() >= CPU_ARCH_ARMv6 && (cr_alignment & CR_U);
+}
+
+static int safe_usermode(int new_usermode, bool warn)
+{
+	/*
+	 * ARMv6 and later CPUs can perform unaligned accesses for
+	 * most single load and store instructions up to word size.
+	 * LDM, STM, LDRD and STRD still need to be handled.
+	 *
+	 * Ignoring the alignment fault is not an option on these
+	 * CPUs since we spin re-faulting the instruction without
+	 * making any progress.
+	 */
+	if (cpu_is_v6_unaligned() && !(new_usermode & (UM_FIXUP | UM_SIGNAL))) {
+		new_usermode |= UM_FIXUP;
+
+		if (warn)
+			printk(KERN_WARNING "alignment: ignoring faults is unsafe on this CPU.  Defaulting to fixup mode.\n");
+	}
+
+	return new_usermode;
+}
+
 static int alignment_proc_show(struct seq_file *m, void *v)
 {
 	seq_printf(m, "User:\t\t%lu\n", ai_user);
@@ -125,7 +152,7 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer
 		if (get_user(mode, buffer))
 			return -EFAULT;
 		if (mode >= '0' && mode <= '5')
-			ai_usermode = mode - '0';
+			ai_usermode = safe_usermode(mode - '0', true);
 	}
 	return count;
 }
@@ -923,20 +950,11 @@ static int __init alignment_init(void)
 		return -ENOMEM;
 #endif
 
-	/*
-	 * ARMv6 and later CPUs can perform unaligned accesses for
-	 * most single load and store instructions up to word size.
-	 * LDM, STM, LDRD and STRD still need to be handled.
-	 *
-	 * Ignoring the alignment fault is not an option on these
-	 * CPUs since we spin re-faulting the instruction without
-	 * making any progress.
-	 */
-	if (cpu_architecture() >= CPU_ARCH_ARMv6 && (cr_alignment & CR_U)) {
+	if (cpu_is_v6_unaligned()) {
 		cr_alignment &= ~CR_A;
 		cr_no_alignment &= ~CR_A;
 		set_cr(cr_alignment);
-		ai_usermode = UM_FIXUP;
+		ai_usermode = safe_usermode(ai_usermode, false);
 	}
 
 	hook_fault_code(1, do_alignment, SIGBUS, BUS_ADRALN,
-- 
1.7.4.1

[PATCH v2] ARM: alignment: Prevent ignoring of faults with ARMv6 unaligned access model

[Nicolas Pitre]

On Tue, 26 Jul 2011, Dave Martin wrote:

> Currently, it's possible to set the kernel to ignore alignment
> faults when changing the alignment fault handling mode at runtime
> via /proc/sys/alignment, even though this is undesirable on ARMv6
> and above, where it can result in infinite spins where an un-fixed-
> up instruction repeatedly faults.

This may be the case only if the U bit in CP15 reg 1 is set, right?

> In addition, the kernel clobbers any alignment mode specified on
> the command-line if running on ARMv6 or above.
> 
> This patch factors out the necessary safety check into a couple of
> new helper functions, and checks and modifies the fault handling
> mode as appropriate on boot and on writes to /proc/cpu/alignment.
> 
> Prior to ARMv6, the behaviour is unchanged.
> 
> For ARMv6 and above, the behaviour changes as follows:
> 
>   * Attempting to ignore faults on ARMv6 results in the mode being
>     forced to UM_FIXUP instead.  A warning is printed if this
>     happened as a result of a write to /proc/cpu/alignment.  The
>     user's UM_WARN bit (if present) is still honoured.

Why not clearing the U bit as well as the A bit to preserve consistency 
with the pre ARMv6 behavior?


Nicolas

[PATCH v2] ARM: alignment: Prevent ignoring of faults with ARMv6 unaligned access model

[Russell King - ARM Linux]

On Tue, Jul 26, 2011 at 05:28:57PM -0400, Nicolas Pitre wrote:
> Why not clearing the U bit as well as the A bit to preserve consistency 
> with the pre ARMv6 behavior?

Because some CPUs have the U bit forced to 1.  IOW, it's not possible to
disable it.

[PATCH v2] ARM: alignment: Prevent ignoring of faults with ARMv6 unaligned access model

[Nicolas Pitre]

On Tue, 26 Jul 2011, Russell King - ARM Linux wrote:

> On Tue, Jul 26, 2011 at 05:28:57PM -0400, Nicolas Pitre wrote:
> > Why not clearing the U bit as well as the A bit to preserve consistency 
> > with the pre ARMv6 behavior?
> 
> Because some CPUs have the U bit forced to 1.  IOW, it's not possible to
> disable it.

Oh well...


Nicolas

[PATCH v2] ARM: alignment: Prevent ignoring of faults with ARMv6 unaligned access model

[Dave Martin]

On Tue, Jul 26, 2011 at 05:28:57PM -0400, Nicolas Pitre wrote:
> On Tue, 26 Jul 2011, Dave Martin wrote:
> 
> > Currently, it's possible to set the kernel to ignore alignment
> > faults when changing the alignment fault handling mode at runtime
> > via /proc/sys/alignment, even though this is undesirable on ARMv6
> > and above, where it can result in infinite spins where an un-fixed-
> > up instruction repeatedly faults.
> 
> This may be the case only if the U bit in CP15 reg 1 is set, right?
> 
> > In addition, the kernel clobbers any alignment mode specified on
> > the command-line if running on ARMv6 or above.
> > 
> > This patch factors out the necessary safety check into a couple of
> > new helper functions, and checks and modifies the fault handling
> > mode as appropriate on boot and on writes to /proc/cpu/alignment.
> > 
> > Prior to ARMv6, the behaviour is unchanged.
> > 
> > For ARMv6 and above, the behaviour changes as follows:
> > 
> >   * Attempting to ignore faults on ARMv6 results in the mode being
> >     forced to UM_FIXUP instead.  A warning is printed if this
> >     happened as a result of a write to /proc/cpu/alignment.  The
> >     user's UM_WARN bit (if present) is still honoured.
> 
> Why not clearing the U bit as well as the A bit to preserve consistency 
> with the pre ARMv6 behavior?

The old unaligned access behaviour is deprecated in ARMv6 (I believe)
and is not supported at all in ARMv7 -- i.e., you can't turn the U bit
off.  So this would be an additional special behaviour for ARMv6 only.

I'm not sure that introducing yet another behaviour is useful here;
anyone with legacy userspace software relying on the old unaligned
access model presumably fixed it years ago.

Really, the patch was just intended resolve the inconsistency where
the policy the kernel enforces for the alignment fixup mode on bootup
is not enforced at run-time.

Cheers
---Dave

[PATCH v2] ARM: alignment: Prevent ignoring of faults with ARMv6 unaligned access model

[Nicolas Pitre]

On Wed, 27 Jul 2011, Dave Martin wrote:

> On Tue, Jul 26, 2011 at 05:28:57PM -0400, Nicolas Pitre wrote:
> > Why not clearing the U bit as well as the A bit to preserve consistency 
> > with the pre ARMv6 behavior?
> 
> The old unaligned access behaviour is deprecated in ARMv6 (I believe)
> and is not supported at all in ARMv7 -- i.e., you can't turn the U bit
> off.  So this would be an additional special behaviour for ARMv6 only.
> 
> I'm not sure that introducing yet another behaviour is useful here;
> anyone with legacy userspace software relying on the old unaligned
> access model presumably fixed it years ago.
> 
> Really, the patch was just intended resolve the inconsistency where
> the policy the kernel enforces for the alignment fixup mode on bootup
> is not enforced at run-time.

OK, that makes sense then.


Nicolas

[PATCH v2] ARM: alignment: Prevent ignoring of faults with ARMv6 unaligned access model

[Dave Martin]

On Wed, Jul 27, 2011 at 10:10:46AM -0400, Nicolas Pitre wrote:
> On Wed, 27 Jul 2011, Dave Martin wrote:
> 
> > On Tue, Jul 26, 2011 at 05:28:57PM -0400, Nicolas Pitre wrote:
> > > Why not clearing the U bit as well as the A bit to preserve consistency 
> > > with the pre ARMv6 behavior?
> > 
> > The old unaligned access behaviour is deprecated in ARMv6 (I believe)
> > and is not supported at all in ARMv7 -- i.e., you can't turn the U bit
> > off.  So this would be an additional special behaviour for ARMv6 only.
> > 
> > I'm not sure that introducing yet another behaviour is useful here;
> > anyone with legacy userspace software relying on the old unaligned
> > access model presumably fixed it years ago.
> > 
> > Really, the patch was just intended resolve the inconsistency where
> > the policy the kernel enforces for the alignment fixup mode on bootup
> > is not enforced at run-time.
> 
> OK, that makes sense then.

Can I take that as an Ack?

Cheers
---Dave

[PATCH v2] ARM: alignment: Prevent ignoring of faults with ARMv6 unaligned access model

[Nicolas Pitre]

On Wed, 27 Jul 2011, Dave Martin wrote:

> On Wed, Jul 27, 2011 at 10:10:46AM -0400, Nicolas Pitre wrote:
> > On Wed, 27 Jul 2011, Dave Martin wrote:
> > 
> > > On Tue, Jul 26, 2011 at 05:28:57PM -0400, Nicolas Pitre wrote:
> > > > Why not clearing the U bit as well as the A bit to preserve consistency 
> > > > with the pre ARMv6 behavior?
> > > 
> > > The old unaligned access behaviour is deprecated in ARMv6 (I believe)
> > > and is not supported at all in ARMv7 -- i.e., you can't turn the U bit
> > > off.  So this would be an additional special behaviour for ARMv6 only.
> > > 
> > > I'm not sure that introducing yet another behaviour is useful here;
> > > anyone with legacy userspace software relying on the old unaligned
> > > access model presumably fixed it years ago.
> > > 
> > > Really, the patch was just intended resolve the inconsistency where
> > > the policy the kernel enforces for the alignment fixup mode on bootup
> > > is not enforced at run-time.
> > 
> > OK, that makes sense then.
> 
> Can I take that as an Ack?

/me looks at the patch again.

OK, looks fine, ACK.


Nicolas



> 
> Cheers
> ---Dave
>