> fix Suresh's email...
(Damn, really fix this time, sorry for resend)
And the patch is buggy, fpu_finit(&tsk->thread.fpu) if __copy_from_user()
fails is obviously wrong, but this is fixable.
On 08/24, Linus Torvalds wrote:
>
> I really dislike this one.
>
> If I read it right, you now do *two* math_state_restore calls for each
> FPU signal state restore. That's potentially quite expensive.
Yes, this adds one restore_fpu_checking().
But only if a 32bit task does this. And only if use_eager_fpu(), and in
this case we do this on every context switch unconditionally.
So personally I think it is not that bad. And this allows to do more
cleanups (if this can actually work of course). But I can't really
judge.
> Also, you can actually end up with multiple threads pointing to the
> same math state in init_task.thread.fpu.state, right?
Yes. I think this should be fine, but let me remind that I do not
understand i387.
I think this should be safe, because this thread and/or swapper/0 can
do nothing with with fpu->state, and they should not use fpu. So I
hope that, say, __save_init_fpu() and restore_fpu_checking() can race
with each other using the same fpu->state without any problem.
kernel_fpu_begin() looks fine to, fpu_save_init() should not hurt.
But again, again, this is only my speculation.
> Why is that any
> better than just having the save state temporarily contain garbage?
I do not know if restore_fpu_checking(garbage) is safe without
sanitize_restored_xstate(). Can't this, say, trigger an exception?
But there is another reason. Any preemption will overwrite ->xsave,
and I think this is the main reason why we should be careful.
> The other patches look sane, this one I really don't like. You may
> have good reasons for it, but it's disgusting.
5/5 (and other potential cleanups) depends on this change.
So do you still think this change is really bad? Or perhaps it is just
technically wrong?
We can probably do fault_in_pages() + __copy_from_user_inatomic(), but
this will complicate the code more... Something like
__copy_from_user(&env);
while (!fatal_signal_pending() && !fault_in_pages_readable(buf_fx)) {
return -1;
preempt_disable();
if (!__copy_from_user_in_atomic(buf_fx)) {
sanitize_restored_xstate(...);
math_state_restore();
done = true;
}
preempt_disable();
if (done)
break;
}
not sure this looks better.
Other ideas or should I simply forget about these cleanups?
OK. Given that this patch at least needs more discussion, let me send another
simple fix first. This code calls math_state_restore() without preempt_disable()
and afaics this is very wrong and can lead to FPU corruption: if this task gets
a preemption after __thread_fpu_begin(), __save_init_fpu() will overwrite the
registers we are going to restore.
Btw, do you see any problem with another "shift drop_init_fpu() from
save_xstate_sig() to handle_signal()" fix I sent? I think that Bean Anderson
is right, this should be fixed.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
