Various VFP fixes

Hi,

This is version two of the patches I posted on Friday:

http://lists.infradead.org/pipermail/linux-arm-kernel/2012-January/082168.html

The only changes are that patch 4 is no longer insane and the whole series has had an extra weekend's worth of testing.

Feedback welcome.

Will

Dave Martin (1):
ARM: vfp: fix ptrace regset modification race

Will Deacon (3):
ARM: vfp: flush thread hwstate before restoring context from sigframe
ARM: vfp: flush thread hwstate before copying ptrace registers
ARM: vfp: clear fpscr length and stride bits on entry to sig handler

 arch/arm/kernel/ptrace.c |    8 +++++---
 arch/arm/kernel/signal.c |   20 +++++++++++++++++---
 2 files changed, 22 insertions(+), 6 deletions(-)

1.7.4.1

Following execution of a signal handler, we currently restore the VFP context from the ucontext in the signal frame. This involves copying from the user stack into the current thread's vfp_hard_struct and then flushing the new data out to the hardware registers.

This is problematic when using a preemptible kernel because we could be context switched whilst updating the vfp_hard_struct. If the current thread has made use of VFP since the last context switch, the VFP notifier will copy from the hardware registers into the vfp_hard_struct, overwriting any data that had been partially copied by the signal code.

Disabling preemption across copy_from_user calls is a terrible idea, so instead we move the VFP thread flush *before* we update the vfp_hard_struct. Since the flushing is performed lazily, this has the effect of disabling VFP and clearing the CPU's VFP state pointer, therefore preventing the thread from being updated with stale data on the next context switch.

---
 arch/arm/kernel/signal.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

index 0340224..9e617bd 100644
@@ -227,6 +227,8 @@ static int restore_vfp_context(struct vfp_sigframe __user *frame)
 	if (magic != VFP_MAGIC || size != VFP_STORAGE_SIZE)
 		return -EINVAL;
 
+	vfp_flush_hwstate(thread);
+
 	/*
 	 * Copy the floating point registers. There can be unused
 	 * registers see asm/hwcap.h for details.

@@ -251,9 +253,6 @@ static int restore_vfp_context(struct vfp_sigframe __user *frame)
 	__get_user_error(h->fpinst, &frame->ufp_exc.fpinst, err);
 	__get_user_error(h->fpinst2, &frame->ufp_exc.fpinst2, err);
 
-	if (!err)
-		vfp_flush_hwstate(thread);
-
 	return err ? -EFAULT : 0;
 }
 

1.7.4.1

From: Dave Martin <[email protected]>

In a preemptible kernel, vfp_set() can be preempted, causing the hardware VFP context to be switched while the thread vfp state is being read and modified. This leads to a race condition which can cause the thread vfp state to become corrupted if lazy VFP context save occurs due to preemption in between the time thread->vfpstate is read and the time the modified state is written back.

This may occur if preemption occurs during the execution of a ptrace() call which modifies the VFP register state of a thread. Such instances should be very rare in most realistic scenarios -- none has been reported, so far as I am aware. Only uniprocessor systems should be affected, since VFP context save is not currently lazy in SMP kernels.

The problem was introduced by my earlier patch migrating to use regsets to implement ptrace.

This patch does a vfp_sync_hwstate() before reading thread->vfpstate, to make sure that the thread's VFP state is not live in the hardware registers while the registers are modified.

Thanks to Will Deacon for spotting this.

---
 arch/arm/kernel/ptrace.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

index e1d5e19..d001be4 100644
@@ -699,10 +699,13 @@ static int vfp_set(struct task_struct *target,
 {
 	int ret;
 	struct thread_info *thread = task_thread_info(target);
-	struct vfp_hard_struct new_vfp = thread->vfpstate.hard;
+	struct vfp_hard_struct new_vfp;
 	const size_t user_fpregs_offset = offsetof(struct user_vfp, fpregs);
 	const size_t user_fpscr_offset = offsetof(struct user_vfp, fpscr);
 
+	vfp_sync_hwstate(thread);
+	new_vfp = thread->vfpstate.hard;
+
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
 				  &new_vfp.fpregs,
 				  user_fpregs_offset,

@@ -723,7 +726,6 @@ static int vfp_set(struct task_struct *target,
 	if (ret)
 		return ret;
 
-	vfp_sync_hwstate(thread);
 	thread->vfpstate.hard = new_vfp;
 	vfp_flush_hwstate(thread);
 

1.7.4.1

If we are context switched whilst copying into a thread's vfp_hard_struct then the partial copy may be corrupted by the VFP context switching code (see "ARM: vfp: flush thread hwstate before restoring context from sigframe").

This patch updates the ptrace VFP set code so that the thread state is flushed before the copy, therefore disabling VFP and preventing corruption from occurring.

---
 arch/arm/kernel/ptrace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index d001be4..e33870f 100644
@@ -726,8 +726,8 @@ static int vfp_set(struct task_struct *target,
 	if (ret)
 		return ret;
 
-	thread->vfpstate.hard = new_vfp;
 	vfp_flush_hwstate(thread);
+	thread->vfpstate.hard = new_vfp;
 
 	return 0;
 }

1.7.4.1

The ARM PCS mandates that the length and stride bits of the fpscr are cleared on entry to and return from a public interface. Although signal handlers run asynchronously with respect to the interrupted function, the handler itself expects to run as though it has been called like a normal function.

This patch updates the state mirroring the VFP hardware before entry to a signal handler so that it adheres to the PCS. Furthermore, we disable VFP to ensure that we trap on any floating point operation performed by the signal handler and synchronise the hardware appropriately. A check is inserted after the signal handler to avoid redundant flushing if VFP was not used.

---
 arch/arm/kernel/signal.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

index 9e617bd..3de59a0 100644
@@ -207,6 +207,20 @@ static int preserve_vfp_context(struct vfp_sigframe __user *frame)
 	__put_user_error(h->fpinst, &frame->ufp_exc.fpinst, err);
 	__put_user_error(h->fpinst2, &frame->ufp_exc.fpinst2, err);
 
+	vfp_flush_hwstate(thread);
+
+	/*
+	 * As per the PCS, clear the length and stride bits before entry
+	 * to the signal handler.
+	 */
+	h->fpscr &= ~(FPSCR_LENGTH_MASK | FPSCR_STRIDE_MASK);
+
+	/*
+	 * Disable VFP so that we can detect if it was used by the
+	 * signal handler.
+	 */
+	h->fpexc &= ~FPEXC_EN;
+
 	return err ? -EFAULT : 0;
 }
 

@@ -227,7 +241,8 @@ static int restore_vfp_context(struct vfp_sigframe __user *frame)
 	if (magic != VFP_MAGIC || size != VFP_STORAGE_SIZE)
 		return -EINVAL;
 
-	vfp_flush_hwstate(thread);
+	if (h->fpexc & FPEXC_EN)
+		vfp_flush_hwstate(thread);
 
 	/*
 	 * Copy the floating point registers. There can be unused

1.7.4.1

Hmmm, that looks more like it. I've not tried this out, but it looks sensible.

Ignore my stale comments on your previous post.

Cheers
---Dave