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

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

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

> +		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;

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.

I think you can work around this race by adding kpreempt_disable/enable at
the beginning/end of the function. ISTR there is the same problem in the
other fpregs ptrace functions. In fact, there are a lot of problems in our
FPU code right now.

Otherwise the rfbm approach seems good to me.