[PATCH 0/4] Various VFP fixes

[PATCH 0/4] Various VFP fixes

[Will Deacon]

Hello,

I've recently experienced a few VFP problems that cropped up during runs
of Peter Maydell's excellent `risu' tool:

	https://wiki.linaro.org/PeterMaydell/Risu

After lots of debugging, it became apparent that modifying the ucontext
VFP state from within a signal handler was sometimes unreliable in a
preemptible kernel.

These patches fix the problem and some others that were identified along
the way. The first in the series should probably go to -stable once it's
agreed. The others aren't as critical: patches 2 and 3 could be merged
if there's good reason to do so and the final patch is really just
pedantry.

All feedback welcome.

Cheers,

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 return from sig handler

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

-- 
1.7.4.1

[PATCH 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 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 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 4/4] ARM: vfp: clear fpscr length and stride bits on return from 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.

This patch ensures that the VFP context restored from a signal frame
is made to adhere to this specification.

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

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 9e617bd..274b8fc 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -216,7 +216,7 @@ static int restore_vfp_context(struct vfp_sigframe __user *frame)
 	struct vfp_hard_struct *h = &thread->vfpstate.hard;
 	unsigned long magic;
 	unsigned long size;
-	unsigned long fpexc;
+	unsigned long fpexc, fpscr;
 	int err = 0;
 
 	__get_user_error(magic, &frame->magic, err);
@@ -238,7 +238,13 @@ static int restore_vfp_context(struct vfp_sigframe __user *frame)
 	/*
 	 * Copy the status and control register.
 	 */
-	__get_user_error(h->fpscr, &frame->ufp.fpscr, err);
+	__get_user_error(fpscr, &frame->ufp.fpscr, err);
+
+	/*
+	 * As per the PCS, clear the length and stride bits.
+	 */
+	fpscr &= ~(FPSCR_LENGTH_MASK | FPSCR_STRIDE_MASK);
+	h->fpscr = fpscr;
 
 	/*
 	 * Sanitise and restore the exception registers.
-- 
1.7.4.1

[PATCH 4/4] ARM: vfp: clear fpscr length and stride bits on return from sig handler

[Will Deacon]

On Fri, Jan 27, 2012 at 04:24:03PM +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.
> 
> This patch ensures that the VFP context restored from a signal frame
> is made to adhere to this specification.
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/kernel/signal.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index 9e617bd..274b8fc 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -216,7 +216,7 @@ static int restore_vfp_context(struct vfp_sigframe __user *frame)
>  	struct vfp_hard_struct *h = &thread->vfpstate.hard;
>  	unsigned long magic;
>  	unsigned long size;
> -	unsigned long fpexc;
> +	unsigned long fpexc, fpscr;
>  	int err = 0;
>  
>  	__get_user_error(magic, &frame->magic, err);
> @@ -238,7 +238,13 @@ static int restore_vfp_context(struct vfp_sigframe __user *frame)
>  	/*
>  	 * Copy the status and control register.
>  	 */
> -	__get_user_error(h->fpscr, &frame->ufp.fpscr, err);
> +	__get_user_error(fpscr, &frame->ufp.fpscr, err);
> +
> +	/*
> +	 * As per the PCS, clear the length and stride bits.
> +	 */
> +	fpscr &= ~(FPSCR_LENGTH_MASK | FPSCR_STRIDE_MASK);
> +	h->fpscr = fpscr;
>  
>  	/*
>  	 * Sanitise and restore the exception registers.

Actually, this isn't quite right. We need to clear the bits in *hardware*
before the signal handler to be PCS compliant from its point-of-view. Since
the handler is asynchronous with respect to the interrupted function, there
is no function boundary on the return path to worry about.

I'll have a crack at fixing this for v2, but if it's too complicated I'm
tempted to leave this as-is and drop the patch altogether.

Will

[PATCH 4/4] ARM: vfp: clear fpscr length and stride bits on return from sig handler

[Dave Martin]

On Fri, Jan 27, 2012 at 04:24:03PM +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.

That means we need to reset that state on _entry_ to the signal handler,
since the interrupted thread is interrupted at an arbitrarily location
which need not be a public interface, whereas the signal handler entry
point definitely is a public interface.

On exit from the signal handler, surely we must restore the interrupted
thread's state for these bits, not reset them to zero?  The interrupted
thread could be in the middle of something which makes assumptions about
what these bits are set to.


This is rather analogous to the handling of the CPSR.E bit.

Cheers
---Dave

> 
> This patch ensures that the VFP context restored from a signal frame
> is made to adhere to this specification.
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/kernel/signal.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index 9e617bd..274b8fc 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -216,7 +216,7 @@ static int restore_vfp_context(struct vfp_sigframe __user *frame)
>  	struct vfp_hard_struct *h = &thread->vfpstate.hard;
>  	unsigned long magic;
>  	unsigned long size;
> -	unsigned long fpexc;
> +	unsigned long fpexc, fpscr;
>  	int err = 0;
>  
>  	__get_user_error(magic, &frame->magic, err);
> @@ -238,7 +238,13 @@ static int restore_vfp_context(struct vfp_sigframe __user *frame)
>  	/*
>  	 * Copy the status and control register.
>  	 */
> -	__get_user_error(h->fpscr, &frame->ufp.fpscr, err);
> +	__get_user_error(fpscr, &frame->ufp.fpscr, err);
> +
> +	/*
> +	 * As per the PCS, clear the length and stride bits.
> +	 */
> +	fpscr &= ~(FPSCR_LENGTH_MASK | FPSCR_STRIDE_MASK);
> +	h->fpscr = fpscr;
>  
>  	/*
>  	 * Sanitise and restore the exception registers.
> -- 
> 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