lkml.org 
[lkml]   [2015]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 02/19] x86, fpu: Wrap get_xsave_addr() to make it safer

* Dave Hansen <dave@sr71.net> wrote:

> On 05/28/2015 01:41 AM, Ingo Molnar wrote:
> >> > + union fpregs_state *xstate;
> >> > +
> >> > + if (!current->thread.fpu.fpstate_active)
> >> > + return NULL;
> >> > + /*
> >> > + * fpu__save() takes the CPU's xstate registers
> >> > + * and saves them off to the 'fpu memory buffer.
> >> > + */
> >> > + fpu__save(&current->thread.fpu);
> >> > + xstate = &current->thread.fpu.state;
> >> > +
> >> > + return get_xsave_addr(&xstate->xsave, xsave_state);
> > Small nit, this would become a lot shorter if you introduced a helper local
> > variable:
> >
> > struct fpu *fpu = &current->thread.fpu;
> >
> > But more importantly, for a generic get_xsave_field_ptr() API, fpu__save() is
> > not enough: fpu__save() will only save FPU registers into memory if necessary
> > (i.e. if the FPU is already in use), and if you call it on a task with no FPU
> > state then it will still have an !fpu->fpstate_active FPU state after the
> > call, with random, invalid data in the xsave area.
>
> But why does this matter? We just did a !fpu.fpstate_active check, so we can't
> have a !fpu.fpstate_active before or after the call.

Ah yes, you are right, I missed this:

> >> > + if (!current->thread.fpu.fpstate_active)
> >> > + return NULL;

because the usual pattern is:

if (!fpu->fpstate_active)
return NULL;

:-)

So your variant is fine too.

Thanks,

Ingo


\
 
 \ /
  Last update: 2015-05-29 21:21    [W:0.094 / U:0.564 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site