Re: [PATCH v3 2/2] Implement PT_GETXSTATE and PT_SETXSTATE

[Maxime Villard <max%m00nbsd.net@localhost>]

Le 22/06/2019 à 07:42, Michał Górny a écrit :
> On Sat, 2019-06-22 at 07:35 +0200, Maxime Villard wrote:
> > Le 19/06/2019 à 14:20, Michał Górny a écrit :> On Wed, 2019-06-19 at 06:32 +0200, Maxime Villard wrote:
> > > > Can you provide a unified patch so I can review quickly? Sorry for the delay
> > >
> > > Here you are.  Note that the manpage is not updated yet, as I'm waiting
> > > for your comments on the xs_rfbm approach.
> > > +		memcpy(&(xstate -> field),					\
> > > +				(char*)fpu_save + x86_xsave_offsets[xsave_val],	\
> > > +				sizeof(xstate -> field));			\
> >
> > Please use proper KNF here and in other places
>
> Are you talking of indentation?  It would be really helpful to be more
> specific here.

Yes

		memcpy(&xstate->field,					\
		    (char*)fpu_save + x86_xsave_offsets[xsave_val],	\
		    sizeof(xstate->field));				\

But that's just cosmetic

> > > +		fpu_save->sv_xsave_hdr.xsh_xstate_bv =
> > > +			(~xstate->xs_rfbm | xstate->xs_xstate_bv) &
> > > +			(xstate->xs_rfbm | fpu_save->sv_xsave_hdr.xsh_xstate_bv);
> >
> > Can't we just do this? Seems simpler
> >
> > 	fpu_save->sv_xsave_hdr.xsh_xstate_bv =
> > 	    (fpu_save->sv_xsave_hdr.xsh_xstate_bv & ~xstate->xs_rfbm) |
> > 	    xstate->xs_xstate_bv;
>
> Unless I'm mistaken, this would prevent the user from being able to
> force xs_xstate_bv to 0 if it was set previously, and therefore clear
> registers to 'unmodified' state.

If the user gives RFBM=all and XSTATE_BV=0, it does force the result to zero

> > Unfortunately, in this code you will likely hit a race; between the moment you
> > set fpu_save->sv_xsave_hdr.xsh_xstate_bv and the moment you reach COPY_COMPONENT,
> > the thread could have been context-switched, and xstate_bv could have been
> > clobbered.
>
> Unless I'm mistaken, this code should only be executed on stopped
> processes.  Or at least that was the idea Kamil suggested to me.

Mmh yes indeed, I even said it earlier in the thread; forget it, I was trying
to remember where I had seen races in the FPU code, and it's not a problem
here