lkml.org 
[lkml]   [2008]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@intel.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


\
 
 \ /
  Last update: 2008-03-11 22:01    [W:0.052 / U:0.288 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site