[PATCH v2 0/4] Various VFP fixes

[PATCH v2 0/4] Various VFP fixes

[Will Deacon]

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

Cc: Dave Martin <dave.martin@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>

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

[PATCH v2 1/4] ARM: vfp: flush thread hwstate before restoring context from sigframe

[Will Deacon]

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.

Tested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/signal.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 0340224..9e617bd 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -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

[PATCH v2 2/4] ARM: vfp: fix ptrace regset modification race

[Will Deacon]

From: Dave Martin <dave.martin@linaro.org>

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.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/ptrace.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index e1d5e19..d001be4 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -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

[PATCH v2 3/4] ARM: vfp: flush thread hwstate before copying ptrace registers

[Will Deacon]

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.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/ptrace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index d001be4..e33870f 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -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

[PATCH v2 4/4] ARM: vfp: clear fpscr length and stride bits on entry to sig handler

[Will Deacon]

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.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/signal.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 9e617bd..3de59a0 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -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

[PATCH v2 4/4] ARM: vfp: clear fpscr length and stride bits on entry to sig handler

[Dave Martin]

On Mon, Jan 30, 2012 at 11:24:55AM +0000, Will Deacon wrote:
> 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.

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

> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/kernel/signal.c |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index 9e617bd..3de59a0 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -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
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel