| Date | Thu, 5 Mar 2015 22:25:32 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 1/1] x86/fpu: math_state_restore() should not blindly disable irqs |
| |
On 03/05, Ingo Molnar wrote: > > * Oleg Nesterov <oleg@redhat.com> wrote: > > > --- a/arch/x86/kernel/traps.c > > +++ b/arch/x86/kernel/traps.c > > @@ -774,7 +774,10 @@ void math_state_restore(void) > > struct task_struct *tsk = current; > > > > if (!tsk_used_math(tsk)) { > > - local_irq_enable(); > > + bool disabled = irqs_disabled(); > > + > > + if (disabled) > > + local_irq_enable(); > > /* > > * does a slab alloc which can sleep > > */ > > @@ -785,7 +788,9 @@ void math_state_restore(void) > > do_group_exit(SIGKILL); > > return; > > } > > - local_irq_disable(); > > + > > + if (disabled) > > + local_irq_disable(); > > } > > Yuck! > > Is there a fundamental reason why we cannot simply enable irqs and > leave them enabled? Math state restore is not atomic and cannot really > be atomic.
You know, I didn't even try to verify ;) but see below.
Most probably we can simply enable irqs, yes. But what about older kernels, how can we check?
And let me repeat, I strongly believe that this !tsk_used_math() case in math_state_restore() must die. And unlazy_fpu() in init_fpu(). And both __restore_xstate_sig() and flush_thread() should not use math_state_restore() at all. At least in its current form.
But this is obviously not -stable material.
That said, I'll try to look into git history tomorrow. The patch above looks "obviously safe", but perhaps I am paranoid too much...
Oleg.
|