Subject: Re: [PATCH] rewrite restore_fp_context/save_fp_context

From: Ralf Baechle <[email-masked]>
Date: Tue, 14 Nov 2006 17:46:08 +0000

On Tue, Aug 29, 2006 at 10:56:31PM +0900, Atsushi Nemoto wrote:

> The setup_sigcontect()/restore_sigcontext() might sleep on
> put_user()/get_user() with preemption disabled (i.e. atomic context).
> Sleeping in atomic context is not allowed.  This patch fixes this
> problem by rewriting restore_fp_context()/save_fp_context().

So with this patch applied the context will be copied around twice, first
save the fp registers to memory then copied from memory to userspace and
as the result the non-preemptible kernel will suffer from fixing the
preemptible ...

To me it looks like the real problem that setup_sigcontext and
restore_sigcontext need to disable preemption.  And the reason for that
is probably that 87d54649f67d8ffe0a8d8176de8c210a6c4bb4a7 around 2.6.9
took the wrong.  The better fix would probably have been to allow
at least some fp instructions from kernel mode.  The sole reason for
the die_if_kernel() call is to tell people attempting to put FPU code
into the kernel that they're screwing up.

  Ralf

---

From: Atsushi Nemoto <[email-masked]>
Date: Thu, 16 Nov 2006 00:17:25 +0900 (JST)

> as the result the non-preemptible kernel will suffer from fixing the

Yes, it is true if the signaled process owned FPU at the time.  I
thought in many case the signaled process does not own FPU, but I
might be too optimistic indeed.

> into the kernel that they're screwing up.

Hmm.  Two yeas ago I tried this approach to fix fpu-emulator issues
and gave up :) It was a bit complex more than it looked.  Here is
excerption:

On Tue, 12 Oct 2004 19:11:54 +0900 (JST), Atsushi Nemoto <[email-masked]> wrote:
> jsun> In terms of being simple, allowing kernel mode FPU trap is
> jsun> definitely simpler.
> 
> jsun> If you can't find any pitfalls of this approach it is actually
> jsun> robust.  The new FPU code is already greatly simplified.  It is
> jsun> possible kernel FPU trap is not that evil anymore (assuming
> jsun> kernel continues voluntarily not using FPU).
> 
> Hmm... OK, I agree enabling FPU trap in kernel seems simple.  I tried
> it today but it did not work unfortunately.  Just modifying a
> following line in traps.c was not enough.
> 
> 	die_if_kernel("do_cpu invoked from kernel context!", regs);
> 
> One point I found is do_cpu() must enable CU1 bit in pt_regs also.
> Another problem is that resume(), own_fpu() and lose_fpu() manipulate
> CU1 bit in only first level kernel stack (KSTK_STATUS(current)).
> current->thread.cp0_status may be manipulated also.  Modifying
> resume() looks too dangerous to me.

But it might be a good time to try again.  Do you think modifying
resume() etc. is OK?

---
Atsushi Nemoto

---

From: Atsushi Nemoto <[email-masked]>
Date: Sat, 18 Nov 2006 23:27:17 +0900 (JST)

> might be too optimistic indeed.

Correction: This problem is not only preemptible kernel.
save_fp_context() might sleep on access to user stack, so it might
lose fpu ownership.  Then subsequent FP instruction cause CpU
exception in kernel code.

> ...

It still a bit complex more than it looked :)

Allowing CpU exception for CP1 in kernel can fix some problem, but it
is somewhat fragile I think.  For example, I first thought the last
part of setup_sigcontext() would become something like this:

	err |= __put_user(!!used_math(), &sc->sc_used_math);

	/*
	 * Save FPU state to signal context.  Signal handler will "inherit"
	 * current FPU state.
	 */
	if (used_math())
		err |= save_fp_context(sc);
	return err;

If save_fp_context cause CpU exception, kernel will handle it
correctly and take fpu owner ship.  Very simple.

BUT it would _not_ work!

In kernel we can not assume C0_STATUS.CU1==0 if is_fpu_owner()==0.
This is because we might lose fpu ownership in middle of
C0_STATUS manipulation such as local_irq_disable(), etc.

local_irq_disable:
	mfc0	$1,$12
	ori	$1,0x1f
	xori	$1,0x1f
	mtc0	$1,$12

If the task was preempted on the ORI instruction, the task's
TIF_USEDFPU flag and CU1 bit in saved user stack will be cleared.
When the task restart from ORI instruction, C0_STATUS.CU1 bit will
have been cleared too, but $1 register still hold old C0_STATUS value.
Then the MTC0 instruction will set C0_STATUS.CU1 without restoring
real FPU context.

So, we still should very careful to using fp instruction in kernel
even if the instruction did not change the fpu state.  The last part
of setup_sigcontext() should become something like this:

	err |= __put_user(!!used_math(), &sc->sc_used_math);

	/*
	 * Save FPU state to signal context.  Signal handler will "inherit"
	 * current FPU state.
	 */
	if (used_math()) {
		preempt_disable();
		if (!is_fpu_owner()) {
			own_fpu();
			restore_fp(current);
		}
		preempt_enable();
		err |= save_fp_context(sc);
	}
	return err;

Still somewhat ugly but the original problem (calling save_fp_context
in atomic context) seems fixed.

I still investigating and for now I think allowing CpU exception for
CP1 in kernel _unconditionally_ is very fragile.  Maybe something like
TIF_ALLOW_FP_IN_KERNEL flag to enable/disable that feature might be
worth to consider.

---
Atsushi Nemoto

---

From: Atsushi Nemoto <[email-masked]>
Date: Sun, 19 Nov 2006 22:54:03 +0900 (JST)

> 		err |= save_fp_context(sc);

Unfortunately, still this is not safe.  preempt_enable() might call
local_irq_enable() so it might lose fpu ownership without clearing CU1
bit...

So something like this ugly check should be needed.

 		preempt_disable();
 		if (!is_fpu_owner()) {
 			own_fpu();
 			restore_fp(current);
 		}
 		preempt_enable();
		/* make sure CU1 and FPU ownership are consistent */
		if (!__is_fpu_owner() && (read_c0_status() & ST0_CU1))
			__disable_fpu();
 		err |= save_fp_context(sc);