| Message ID | 20190604185358.GA820@sol.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [5.2,regression] copy_fpstate_to_sigframe() change causing crash in 32-bit process | expand |
On 2019-06-04 11:53:58 [-0700], Eric Biggers wrote: > On latest Linus' tree I'm getting a crash in a 32-bit Wine process. > > I bisected it to the following commit: > > commit 39388e80f9b0c3788bfb6efe3054bdce0c3ead45 > Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Date: Wed Apr 3 18:41:35 2019 +0200 > > x86/fpu: Don't save fxregs for ia32 frames in copy_fpstate_to_sigframe() > > Reverting the commit by applying the following diff makes the problem go away. This looked like a merge artifact and it has been confirmed as such. Now you say that this was a needed piece of code. Interesting. Is that wine process/testcase something you can share? I will try to take a closer look. Sebastian
On Wed, Jun 05, 2019 at 04:04:05PM +0200, Sebastian Andrzej Siewior wrote: > On 2019-06-04 11:53:58 [-0700], Eric Biggers wrote: > > On latest Linus' tree I'm getting a crash in a 32-bit Wine process. > > > > I bisected it to the following commit: > > > > commit 39388e80f9b0c3788bfb6efe3054bdce0c3ead45 > > Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > > Date: Wed Apr 3 18:41:35 2019 +0200 > > > > x86/fpu: Don't save fxregs for ia32 frames in copy_fpstate_to_sigframe() > > > > Reverting the commit by applying the following diff makes the problem go away. > > This looked like a merge artifact and it has been confirmed as such. Now > you say that this was a needed piece of code. Interesting. > Is that wine process/testcase something you can share? I will try to > take a closer look. > > Sebastian As I said, the commit looks broken to me. save_fsave_header() reads from tsk->thread.fpu.state.fxsave, which due to that commit isn't being updated with the latest registers. Am I missing something? Note the comment you deleted: /* Update the thread's fxstate to save the fsave header. */ My test case was "run some Win32 game for a few minutes and see if it crashes" so it's not really sharable, sorry. But I expect it would be possible to write a minimal test case, where a 32-bit process sends a signal to itself and checks whether the i387 floating point stuff gets restored correctly afterwards. - Eric
On 2019-06-05 10:32:57 [-0700], Eric Biggers wrote: > As I said, the commit looks broken to me. save_fsave_header() reads from > tsk->thread.fpu.state.fxsave, which due to that commit isn't being updated with > the latest registers. Am I missing something? Note the comment you deleted: So if your system uses fxsr() then that function shouldn't matter. If your system uses xsave() (which I believe it does) then the first section is the "fxregs state" which is the same as in fxsr's case (see struct xregs_state). So it shouldn't make a difference and that is why I strongly assumed it is a miss-merge. However it makes a difference… So the hunk at the end should make things work again (my FPU test case passes). I don't know why we convert things forth and back in the signal handler but I think something here is different for xsave's legacy area vs fxsave. diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c index 060d6188b4533..c653c9920c5e0 100644 --- a/arch/x86/kernel/fpu/signal.c +++ b/arch/x86/kernel/fpu/signal.c @@ -62,16 +62,7 @@ static inline int save_fsave_header(struct task_struct *tsk, void __user *buf) struct user_i387_ia32_struct env; struct _fpstate_32 __user *fp = buf; - convert_from_fxsr(&env, tsk); - - if (__copy_to_user(buf, &env, sizeof(env)) || - __put_user(xsave->i387.swd, &fp->status) || - __put_user(X86_FXSR_MAGIC, &fp->magic)) - return -1; - } else { - struct fregs_state __user *fp = buf; - u32 swd; - if (__get_user(swd, &fp->swd) || __put_user(swd, &fp->status)) + if (__put_user(X86_FXSR_MAGIC, &fp->magic)) return -1; } @@ -236,9 +227,6 @@ sanitize_restored_xstate(union fpregs_state *state, * reasons. */ xsave->i387.mxcsr &= mxcsr_feature_mask; - - if (ia32_env) - convert_to_fxsr(&state->fxsave, ia32_env); } } > - Eric Sebastian
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c index 5a8d118bc423e..ed16a24aab497 100644 --- a/arch/x86/kernel/fpu/signal.c +++ b/arch/x86/kernel/fpu/signal.c @@ -157,6 +157,7 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf) */ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size) { + struct fpu *fpu = ¤t->thread.fpu; struct task_struct *tsk = current; int ia32_fxstate = (buf != buf_fx); int ret; @@ -202,6 +203,10 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size) return -EFAULT; } + /* Update the thread's fxstate to save the fsave header. */ + if (ia32_fxstate) + copy_fxregs_to_kernel(fpu); + /* Save the fsave header for the 32-bit frames. */ if ((ia32_fxstate || !use_fxsr()) && save_fsave_header(tsk, buf)) return -1;
On latest Linus' tree I'm getting a crash in a 32-bit Wine process. I bisected it to the following commit: commit 39388e80f9b0c3788bfb6efe3054bdce0c3ead45 Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Date: Wed Apr 3 18:41:35 2019 +0200 x86/fpu: Don't save fxregs for ia32 frames in copy_fpstate_to_sigframe() Reverting the commit by applying the following diff makes the problem go away. Apparently the problem is that save_fsave_header() assumes the registers have been saved to fpu->state.fxsave, yet the code that does so was removed. Note, bisection was not straightforward because there was another bug also causing a crash temporarily introduced during the FPU code rework: commit 39ea9baffda9 ("x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig()") forgot to call fpstate_init() on the temporary 'state' buffer, so XCOMP_BV_COMPACTED_FORMAT was never set, causing xrstors to fail. But that bug went away in later commits. - Eric