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. > > > + 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. > 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. Do we need extra guards for that? > > 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. -- Best regards, Michał Górny
Attachment:
signature.asc
Description: This is a digitally signed message part