[PATCH 1/2] arm64: fpsimd: Fix FPSIMD corruption in rt_sigreturn with CONFIG_PREEMPT

[Dave Martin]

On Tue, Sep 22, 2015 at 06:20:29PM -0700, Ard Biesheuvel wrote:
> On 21 September 2015 at 08:37, Dave P Martin <Dave.Martin at arm.com> wrote:
> > On Tue, Sep 15, 2015 at 03:39:53PM +0100, Will Deacon wrote:
> >> On Tue, Sep 15, 2015 at 01:11:10PM +0100, Ard Biesheuvel wrote:

[...]

> >> > On 15 September 2015 at 13:36, Will Deacon <will.deacon at arm.com> wrote:
> >> > > From: Dave P Martin <Dave.Martin at arm.com>
> >> > >
> >> > > The arm64 context switch implementation uses a flag
> >> > > TIF_FOREIGN_FPSTATE to track whether the hardware FPSIMD regs are
> >> > > out of sync with the logical state of current's registers.
> >> > >
> >> > > During sigreturn, the registers and task_struct are temporarily out
> >> > > of sync, between writing the task_struct and loading its contents
> >> > > back into the FPSIMD registers -- however, TIF_FOREIGN_FPSTATE is
> >> > > not set.  This can cause the context switch code to discard some or
> >> > > all of the restored FPSIMD state if preemption occurs during the
> >> > > critical region of rt_sigreturn.

[...]

> >> > I am not following something here. If the task gets preempted before
> >> > the call to fpsimd_update_current_state(), it will proceed to
> >> > overwrite the registers with whatever is on the stack in fpsimd after
> >> > being scheduled back in, and everything should work fine, right?.

[...]

> > Urg, looks like I confused myself here.  Ard's assessment looks correct.
> >
> > Not staging the data via the stack seemed a harmless change which I
> > thought was an improvement, since at half a K the amount of data is non-
> > trivial.
> >
> > I'll go back and make some measurements to see whether getting rid
> > of the extra copy is really worth it.  I think reading straight into
> > the task_struct with __get_user() can work, but it looks like I'll
> > need to fiddle with the preemption region, or otherwise handle the
> > ensuing race...
> >
> 
> Indeed. I think you will need something like fpsimd_load_state_user
> which runs with preemption disabled and restarts if it is interrupted.

Yep -- I'll take another look at it, and scratch my head some more.

Cheers
---Dave