[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20080311205738.GG15909@linux-os.sc.intel.com>
Date: Tue, 11 Mar 2008 13:57:39 -0700
From: Suresh Siddha <suresh.b.siddha@...el.com>
To: Ingo Molnar <mingo@...e.hu>
Cc: Suresh Siddha <suresh.b.siddha@...el.com>, hpa@...or.com,
tglx@...utronix.de, andi@...stfloor.org, hch@...radead.org,
linux-kernel@...r.kernel.org,
Arjan van de Ven <arjan@...ux.intel.com>
Subject: Re: [patch 2/2] x86, fpu: lazy allocation of FPU area - v5
On Tue, Mar 11, 2008 at 10:08:16AM +0100, Ingo Molnar wrote:
>
> * Suresh Siddha <suresh.b.siddha@...el.com> wrote:
>
> > asmlinkage void math_state_restore(void)
> > {
> > struct task_struct *me = current;
> > - clts(); /* Allow maths ops (or we recurse) */
> >
> > - if (!used_math())
> > - init_fpu(me);
> > + if (!used_math()) {
> > + local_irq_enable();
> > + /*
> > + * does a slab alloc which can sleep
> > + */
> > + if (init_fpu(me)) {
> > + /*
> > + * ran out of memory!
> > + */
> > + do_group_exit(SIGKILL);
> > + return;
> > + }
> > + local_irq_disable();
> > + }
> > +
> > + clts(); /* Allow maths ops (or we recurse) */
> > restore_fpu_checking(&me->thread.xstate->fxsave);
> > task_thread_info(me)->status |= TS_USEDFPU;
> > me->fpu_counter++;
>
> hm, three things:
>
> firstly, the clts is now done _after_ fpu_init() - are you sure that's
> OK? We do it in this order so that FINIT [on older cpus] does not fault.
init_fpu() is getting called only if !used_math() and in this case, we don't
do any FP operations in init_fpu()
> secondly, while i know you were responding to review feedback from
> others, but the do_group_exit(SIGKILL) looks quite bad. It's totally
> undebuggable to the user - not even a coredump will be generated AFAICS
> - and the user has no idea that this all happened due to out-of-memory.
> A (forced) SIGBUS is our usual answer to out-of-memory situations. [such
> as when a pagetable allocation fails]
AFAICS, fault handler is doing do_group_exit(SIGKILL); under out-of-memory
conditions while handling page fault.
Just want to make sure that the user doesn't see this signal.
force_sig() with SIGKILL/SIGBUS along with
printk("out of memory! killing process") is fair enough, right?
> If you get review feedback that
> suggests a crappy solution then please resist it! :-)
:) Didn't feel SIGKILL was completely crappy..
>
> thirdly, the irq enable/disable worries me. Can it ever trigger in
> kernel code that has irqs off? If it happens when kernel uses the FPU in
> irqs-off sections (to do SSE optimized routines, etc.) then enabling
> irqs is dangerous - the original callsite had it disabled for a reason.
Good point. But math_state_restore() should never happen between
the kernel_fpu_begin() and end() sections. Otherwise, it will corrupt the
user's FPU data.
Today, we make sure that we don't get device not available (DNA) exceptions
in kernel_fpu_begin() by explicitly doing clts()
> At minimum we should add a debug check to math_state_restore(),
> something like:
>
> WARN_ON_ONCE(!(regs->flags & X86_EFLAGS_IF))
>
> (this means we need to pass regs to math_state_restore())
Based on above, do you think this is still needed? Even if it is needed,
the check should be
BUG_ON(!user_mode(regs))
thanks,
suresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists