ptrace && fpu_lazy_restore

ptrace && fpu_lazy_restore

[Oleg Nesterov]

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.

--- x/arch/x86/kernel/i387.c~	2012-04-09 20:12:12.000000000 +0200
+++ x/arch/x86/kernel/i387.c	2012-04-15 01:34:09.000000000 +0200
@@ -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);
 

Re: ptrace && fpu_lazy_restore

[Oleg Nesterov]

forgot to mention just in case...

On 04/15, Oleg Nesterov wrote:
>
> --- x/arch/x86/kernel/i387.c~	2012-04-09 20:12:12.000000000 +0200
> +++ x/arch/x86/kernel/i387.c	2012-04-15 01:34:09.000000000 +0200
> @@ -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);
>

with this hack the test-case works fine.

Oleg.

Re: ptrace && fpu_lazy_restore

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1876 bytes --]

On Sat, Apr 14, 2012 at 4:52 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> But I am not sure about the fix, and in any case I need more time
> to read this new code.

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

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 421 bytes --]

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

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 7734bcbb5a3a..2d6e6498c176 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -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;
 	}
 

Re: ptrace && fpu_lazy_restore

[Oleg Nesterov]

On 04/14, Linus Torvalds wrote:
>
> 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()".

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

> The only case where we care about the FPU caches remaining is actually
> the nice normal "we just switched tasks through normal scheduling".

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.

Re: ptrace && fpu_lazy_restore

[Linus Torvalds]

On Sun, Apr 15, 2012 at 3:38 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> 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?

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.

> 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.

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

                  Linus

Re: ptrace && fpu_lazy_restore

[Linus Torvalds]

On Sun, Apr 15, 2012 at 4:42 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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.

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

[PATCH 0/1] i387: ptrace breaks the lazy-fpu-restore logic

[Oleg Nesterov]

On 04/15, Linus Torvalds wrote:
>
> 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).

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.

[PATCH 1/1] i387: ptrace breaks the lazy-fpu-restore logic

[Oleg Nesterov]

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.

Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/i387.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 7734bcb..2d6e649 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -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

Re: [PATCH 1/1] i387: ptrace breaks the lazy-fpu-restore logic

[Oleg Nesterov]

Damn, I forgot to add

Cc: <stable@vger.kernel.org> # 3.3

On 04/16, Oleg Nesterov wrote:
> 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.
> 
> Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/x86/kernel/i387.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> index 7734bcb..2d6e649 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -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
> 

[tip:x86/urgent] i387: ptrace breaks the lazy-fpu-restore logic

[tip-bot for Oleg Nesterov]

Commit-ID:  089f9fba56faf33cc6dd2a6442b7ac92c58b8209
Gitweb:     http://git.kernel.org/tip/089f9fba56faf33cc6dd2a6442b7ac92c58b8209
Author:     Oleg Nesterov <oleg@redhat.com>
AuthorDate: Mon, 16 Apr 2012 22:48:15 +0200
Committer:  H. Peter Anvin <hpa@linux.intel.com>
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.

Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Link: http://lkml.kernel.org/r/20120416204815.GB24884@redhat.com
Cc: <stable@vger.kernel.org> v3.3
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/kernel/i387.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 7734bcb..2d6e649 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -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;
 	}