ptrace && fpu_lazy_restore

Credits to Jan and Chuck, see

	https://bugzilla.redhat.com/show_bug.cgi?id=810668

with reproducer:

	https://bugzilla.redhat.com/attachment.cgi?id=577270

But afaics the root of the problem is clear. Once PTRACE_SETFPREGS changes fpu.state->fxsave the task obviously needs restore_fpu_checking() on context switch.

But I am not sure about the fix, and in any case I need more time to read this new code.

Oleg.

@@ -301,6 +301,8 @@ int xfpregs_set(struct task_struct *targ
 
 	sanitize_i387_state(target);
 
+	target->thread.fpu.last_cpu = ~0;
+
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
 				 &target->thread.fpu.state->fxsave, 0, -1);
 

@@ -563,6 +565,8 @@ int fpregs_set(struct task_struct *targe
 
 	sanitize_i387_state(target);
 
+	target->thread.fpu.last_cpu = ~0;
+
 	if (!HAVE_HWFP)
 		return fpregs_soft_set(target, regset, pos, count, kbuf, ubuf);
 

forgot to mention just in case...

Oh, the fix is obviously correct. Yes, when we write to the FPU state area, we need to invalidate the cached copy in the FPU itself.

It used to be that you could never have cached FPU contents unless you owned the CPU (well, not strictly true - we used to do *really* lazy FPU save/restone long long long ago in the pre-SMP days too), so this bug is new with the lazy restore.

The only part I don't like about it is how hacky the location is. It turns out that if the process *itself* does a "init_fpu()" call, then the call to "unlazy_fpu()" will do the right thing and invalidate the FPU state by setting fpu_owner_task to zero. But when we're ptracing and doing an "init_fpu()" for somebody else, we'll skip that all, because you can only really unlazy_fpu() yourself.

But that means that "init_fpu()" acts subtly differently depending on whether you call it on yourself, or on another process.

So I actually think that I would prefer the patch that invalidates the FPU caches more aggressively. Sure, we don't really *need* to invalidate if we're just reading, but I'd almost prefer to just have it done once in "init_fpu()".

The only case where we care about the FPU caches remaining is actually the nice normal "we just switched tasks through normal scheduling". Any path that calls "init_fpu()" is special enough that I think we should just make sure the FPU state is all in memory. Hmm?

So I think I'd prefer the attached (UNTESTED!) oneliner instead.

If that works fine for people, how about sending it back to me with a commit log and sign-off? I don't want to get credit for this patch that is just a "let's do it in a different place" version of yours.

Linus

 arch/x86/kernel/i387.c |    1 +
 1 file changed, 1 insertion(+)

index 7734bcbb5a3a..2d6e6498c176 100644
@@ -235,6 +235,7 @@ int init_fpu(struct task_struct *tsk)
 	if (tsk_used_math(tsk)) {
 		if (HAVE_HWFP && tsk == current)
 			unlazy_fpu(tsk);
+		tsk->thread.fpu.last_cpu = ~0;
 		return 0;
 	}
 

Agreed. I'll send your patch back to you tomorrow.

Yes. And there is another case when fpu_lazy_restore() returns the false positive.

Suppose that fpu_owner_task exits on CPU_0, and then fork() reuses its task_struct. The new child is still fpu_owner_task and this is obviously wrong (unless of course another thread uses fpu).

Initially I thought this should be fixed too, but it seems that "p->fpu_counter = 0" in copy_thread() saves us.

This looks a bit fragile... And could you confirm this is really fine?

Btw, do we really need this "old->thread.fpu.last_cpu = ~0" in the "else" branch of switch_fpu_prepare()? Just curious, I guees this doesn't matter since we reset old->fpu_counter. But if we can remove this line, then perhaps we can another optimization.

Oleg.

That one is done by design. That fpu_counter=0 in copy_thread() is there explicitly to avoid the problem. Although it's possible that we should reset last_cpu instead. However, that line was actually added before the lazy thing - see commit cea20ca3f318.

Possibly not needed, but quite frankly, I'd rather have last_cpu never contain some stale value.

Linus

Put another way: I do think it would be a good idea to do the "reset last_cpu" in copy_thread() too. It doesn't really cost us anything, and it's cleaner to always just make sure that last_cpu is "valid" (even if the fpu_owner_task is *also* used to invalidate it, and even if we never use the lazy restore if fpu_counter is zero and thus fpu.preload isn't set).

Linus

Yes, this was my thinking too, and initially I was going to include this change into this patch. But lets do it separately.

I feel we need some cleanups. For example, it seems that flush_thread() can do this too, although in this case we can rely on __thread_fpu_end() which clears fpu_owner_task.

So I am just sending your one-liner, and according to my testing it fixes the problem.

And A couple of off-topic questions...

Why unlazy_fpu() clears ->fpu_counter? Afaics, this doesn't make sense and unneeded.

And it is not clear to me why init_fpu() does unlazy_fpu(), afaics tsk_used_math() "tsk == current" is only possible if this task dumps the core.

arch_dup_task_struct() checks fpu_allocated(), this doesn't look exactly right to me. Suppose that a task without PF_USED_MATH uses FPU only once in the signal handler. If it forks after that, we allocate and copy fpu->state for no reason. IOW, we probably should check tsk_used_math() instead, but do memzero(&dst->thread.fpu) unconditionally. And perhaps this memzero() deserves a helper which can set .last_cpu = -1, and this is what copy_thread() should call.

OTOH, this reminds me, a long ago I noticed by accident that all threads on the testing machine have PF_USED_MATH set. IIRC, This is because /sbin/init does memset or memcpy and glibc uses xmm for this. Not that I really suggest this, but perhaps prctl(PR_DROP_FPU) makes some sense.

Oleg.

Starting from 7e16838d "i387: support lazy restore of FPU state" we assume that fpu_owner_task doesn't need restore_fpu_checking() on the context switch, its FPU state should match what we already have in the FPU on this CPU.

However, debugger can change the tracee's FPU state, in this case we should reset fpu.last_cpu to ensure fpu_lazy_restore() can't return true.

Change init_fpu() to do this, it is called by user_regset->set() methods.

---
 arch/x86/kernel/i387.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index 7734bcb..2d6e649 100644
@@ -235,6 +235,7 @@ int init_fpu(struct task_struct *tsk)
 	if (tsk_used_math(tsk)) {
 		if (HAVE_HWFP && tsk == current)
 			unlazy_fpu(tsk);
+		tsk->thread.fpu.last_cpu = ~0;
 		return 0;
 	}
 

1.5.5.1

Damn, I forgot to add

Commit-ID:  089f9fba56faf33cc6dd2a6442b7ac92c58b8209
Gitweb:     http://git.kernel.org/tip/089f9fba56faf33cc6dd2a6442b7ac92c58b8209
Author:     Oleg Nesterov <[email protected]>
AuthorDate: Mon, 16 Apr 2012 22:48:15 +0200
Committer:  H. Peter Anvin <[email protected]>
CommitDate: Mon, 16 Apr 2012 16:23:59 -0700

i387: ptrace breaks the lazy-fpu-restore logic

Starting from 7e16838d "i387: support lazy restore of FPU state" we assume that fpu_owner_task doesn't need restore_fpu_checking() on the context switch, its FPU state should match what we already have in the FPU on this CPU.

However, debugger can change the tracee's FPU state, in this case we should reset fpu.last_cpu to ensure fpu_lazy_restore() can't return true.

Change init_fpu() to do this, it is called by user_regset->set() methods.

---
 arch/x86/kernel/i387.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index 7734bcb..2d6e649 100644
@@ -235,6 +235,7 @@ int init_fpu(struct task_struct *tsk)
 	if (tsk_used_math(tsk)) {
 		if (HAVE_HWFP && tsk == current)
 			unlazy_fpu(tsk);
+		tsk->thread.fpu.last_cpu = ~0;
 		return 0;
 	}