x86/ftrace: Don't bother preserving/restoring R10/R11

[PATCH] x86/ftrace: Don't bother preserving/restoring R10/R11

Posted by Ard Biesheuvel 1 month, 2 weeks ago

From: Ard Biesheuvel <ardb@kernel.org>

The x86_64 calling convention specifies that R10 and R11 are
caller-save, and are not used to pass function arguments. Given that any
compiler optimizations that may deviate from this rule (e.g., -fipa-ra)
are disabled when profiling is enabled, R10 and R11 are dead when the
ftrace hook is called, and so there is no point in preserving and
restoring them.

Case in point: the code inadvertently never restored R11 to begin with
(added via [0] in 2012)

[0] 08f6fba50311 ("ftrace/x86: Add separate function to save regs")

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/kernel/ftrace_64.S | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 214f30e9f0c0..9e13f9b203d1 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -216,8 +216,6 @@ SYM_INNER_LABEL(ftrace_regs_caller_op_ptr, SYM_L_GLOBAL)
 	movq %r14, R14(%rsp)
 	movq %r13, R13(%rsp)
 	movq %r12, R12(%rsp)
-	movq %r11, R11(%rsp)
-	movq %r10, R10(%rsp)
 	movq %rbx, RBX(%rsp)
 	/* Copy saved flags */
 	movq MCOUNT_REG_SIZE(%rsp), %rcx
@@ -256,7 +254,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
 	movq R14(%rsp), %r14
 	movq R13(%rsp), %r13
 	movq R12(%rsp), %r12
-	movq R10(%rsp), %r10
 	movq RBX(%rsp), %rbx
 
 	movq ORIG_RAX(%rsp), %rax
-- 
2.47.0.rc0.187.ge670bccf7e-goog

Re: [PATCH] x86/ftrace: Don't bother preserving/restoring R10/R11

Posted by Steven Rostedt 1 month, 2 weeks ago

On Wed,  9 Oct 2024 17:39:01 +0200
Ard Biesheuvel <ardb+git@google.com> wrote:

> diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
> index 214f30e9f0c0..9e13f9b203d1 100644
> --- a/arch/x86/kernel/ftrace_64.S
> +++ b/arch/x86/kernel/ftrace_64.S
> @@ -216,8 +216,6 @@ SYM_INNER_LABEL(ftrace_regs_caller_op_ptr, SYM_L_GLOBAL)
>  	movq %r14, R14(%rsp)
>  	movq %r13, R13(%rsp)
>  	movq %r12, R12(%rsp)
> -	movq %r11, R11(%rsp)
> -	movq %r10, R10(%rsp)
>  	movq %rbx, RBX(%rsp)

This may need to still be saved. Even though they are pretty much useless,
it can be used by kprobes, and perhaps they want to see what's in R10 or
R11 from the caller. I'm not sure we want to remove this. Especially since
we are even saving "flags"!

That is, this part is visible by the callbacks, and the ftrace_get_regs()
is to return a fully populated pt_regs if FTRACE_OPS_FL_SAVE_REGS is set.

>  	/* Copy saved flags */
>  	movq MCOUNT_REG_SIZE(%rsp), %rcx
> @@ -256,7 +254,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
>  	movq R14(%rsp), %r14
>  	movq R13(%rsp), %r13
>  	movq R12(%rsp), %r12
> -	movq R10(%rsp), %r10

This part of the patch I think is perfectly fine. We haven't been restoring
R11 for 12 years I'm sure nobody will notice if we don't restore R10.

-- Steve

>  	movq RBX(%rsp), %rbx
>  
>  	movq ORIG_RAX(%rsp), %rax
> --

Re: [PATCH] x86/ftrace: Don't bother preserving/restoring R10/R11

Posted by Ard Biesheuvel 1 month, 2 weeks ago

On Wed, 9 Oct 2024 at 18:02, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed,  9 Oct 2024 17:39:01 +0200
> Ard Biesheuvel <ardb+git@google.com> wrote:
>
> > diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
> > index 214f30e9f0c0..9e13f9b203d1 100644
> > --- a/arch/x86/kernel/ftrace_64.S
> > +++ b/arch/x86/kernel/ftrace_64.S
> > @@ -216,8 +216,6 @@ SYM_INNER_LABEL(ftrace_regs_caller_op_ptr, SYM_L_GLOBAL)
> >       movq %r14, R14(%rsp)
> >       movq %r13, R13(%rsp)
> >       movq %r12, R12(%rsp)
> > -     movq %r11, R11(%rsp)
> > -     movq %r10, R10(%rsp)
> >       movq %rbx, RBX(%rsp)
>
> This may need to still be saved. Even though they are pretty much useless,
> it can be used by kprobes, and perhaps they want to see what's in R10 or
> R11 from the caller. I'm not sure we want to remove this. Especially since
> we are even saving "flags"!
>

Are you saying kprobes will call this hook from arbitrary other
locations (where R10/R11 might be live), rather than only from the
fentry hook?

> That is, this part is visible by the callbacks, and the ftrace_get_regs()
> is to return a fully populated pt_regs if FTRACE_OPS_FL_SAVE_REGS is set.
>

Fair enough.

> >       /* Copy saved flags */
> >       movq MCOUNT_REG_SIZE(%rsp), %rcx
> > @@ -256,7 +254,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
> >       movq R14(%rsp), %r14
> >       movq R13(%rsp), %r13
> >       movq R12(%rsp), %r12
> > -     movq R10(%rsp), %r10
>
> This part of the patch I think is perfectly fine. We haven't been restoring
> R11 for 12 years I'm sure nobody will notice if we don't restore R10.
>

Not sure I follow this reasoning tbh. R10/R11 are guaranteed to be
dead, so I don't see any point in preserving them. But if you do
capture them, shouldn't you at least ensure that the captured values
match the values that the callee will observe? (even though they are
dead and we know it will never look at them)

Re: [PATCH] x86/ftrace: Don't bother preserving/restoring R10/R11

Posted by Steven Rostedt 1 month, 2 weeks ago

On Wed, 9 Oct 2024 18:13:54 +0200
Ard Biesheuvel <ardb@kernel.org> wrote:

> > > @@ -256,7 +254,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
> > >       movq R14(%rsp), %r14
> > >       movq R13(%rsp), %r13
> > >       movq R12(%rsp), %r12
> > > -     movq R10(%rsp), %r10  
> >
> > This part of the patch I think is perfectly fine. We haven't been restoring
> > R11 for 12 years I'm sure nobody will notice if we don't restore R10.
> >  
> 
> Not sure I follow this reasoning tbh. R10/R11 are guaranteed to be
> dead, so I don't see any point in preserving them. But if you do
> capture them, shouldn't you at least ensure that the captured values
> match the values that the callee will observe? (even though they are
> dead and we know it will never look at them)

Say we have code of:

	pushq	r10
	pushq	r11
	call	foo
	popq	r11
	popq	r10

Where we add a kprobe to the start of foo, the callback should be able to
see what r10 and r11 were. But the restore part is for the function foo to
see. It shouldn't care about r10 or r11 and if a kprobe updates them, it
should not have any effect.

What does restoring r10 and r11 give us?

-- Steve

Re: [PATCH] x86/ftrace: Don't bother preserving/restoring R10/R11

Posted by Ard Biesheuvel 1 month, 2 weeks ago

On Wed, 9 Oct 2024 at 18:19, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 9 Oct 2024 18:13:54 +0200
> Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > > > @@ -256,7 +254,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
> > > >       movq R14(%rsp), %r14
> > > >       movq R13(%rsp), %r13
> > > >       movq R12(%rsp), %r12
> > > > -     movq R10(%rsp), %r10
> > >
> > > This part of the patch I think is perfectly fine. We haven't been restoring
> > > R11 for 12 years I'm sure nobody will notice if we don't restore R10.
> > >
> >
> > Not sure I follow this reasoning tbh. R10/R11 are guaranteed to be
> > dead, so I don't see any point in preserving them. But if you do
> > capture them, shouldn't you at least ensure that the captured values
> > match the values that the callee will observe? (even though they are
> > dead and we know it will never look at them)
>
> Say we have code of:
>
>         pushq   r10
>         pushq   r11
>         call    foo
>         popq    r11
>         popq    r10
>
> Where we add a kprobe to the start of foo, the callback should be able to
> see what r10 and r11 were.

Why exactly is that? The contents of R10 and R11 have no purpose going
forward, so is it just to see what some previous code may have left in
them?

> But the restore part is for the function foo to
> see. It shouldn't care about r10 or r11 and if a kprobe updates them, it
> should not have any effect.
>
> What does restoring r10 and r11 give us?
>

Nothing. Which is why I don't understand why you would need to record
them in the first place.

Re: [PATCH] x86/ftrace: Don't bother preserving/restoring R10/R11

Posted by Steven Rostedt 1 month, 2 weeks ago

On Wed, 9 Oct 2024 18:25:25 +0200
Ard Biesheuvel <ardb@kernel.org> wrote:

> > Say we have code of:
> >
> >         pushq   r10
> >         pushq   r11
> >         call    foo
> >         popq    r11
> >         popq    r10
> >
> > Where we add a kprobe to the start of foo, the callback should be able to
> > see what r10 and r11 were.  
> 
> Why exactly is that? The contents of R10 and R11 have no purpose going
> forward, so is it just to see what some previous code may have left in
> them?

Because the probe is on the call. Unless they were used between the push
and the call, they still have the value you may be looking for.

> 
> > But the restore part is for the function foo to
> > see. It shouldn't care about r10 or r11 and if a kprobe updates them, it
> > should not have any effect.
> >
> > What does restoring r10 and r11 give us?
> >  
> 
> Nothing. Which is why I don't understand why you would need to record
> them in the first place.

As I mentioned above. Unless they are used after they are pushed, you still
have access to them on the call (or the kprobe attached to ftrace).

-- Steve

Re: [PATCH] x86/ftrace: Don't bother preserving/restoring R10/R11

Posted by Ard Biesheuvel 1 month, 2 weeks ago

On Wed, 9 Oct 2024 at 18:31, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 9 Oct 2024 18:25:25 +0200
> Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > > Say we have code of:
> > >
> > >         pushq   r10
> > >         pushq   r11
> > >         call    foo
> > >         popq    r11
> > >         popq    r10
> > >
> > > Where we add a kprobe to the start of foo, the callback should be able to
> > > see what r10 and r11 were.
> >
> > Why exactly is that? The contents of R10 and R11 have no purpose going
> > forward, so is it just to see what some previous code may have left in
> > them?
>
> Because the probe is on the call. Unless they were used between the push
> and the call, they still have the value you may be looking for.
>

Right. So putting a probe on foo() is a way to inspect the register
values during the execution if its caller. Fair enough.

> >
> > > But the restore part is for the function foo to
> > > see. It shouldn't care about r10 or r11 and if a kprobe updates them, it
> > > should not have any effect.
> > >
> > > What does restoring r10 and r11 give us?
> > >
> >
> > Nothing. Which is why I don't understand why you would need to record
> > them in the first place.
>
> As I mentioned above. Unless they are used after they are pushed, you still
> have access to them on the call (or the kprobe attached to ftrace).
>

OK. I just didn't imagine this usage mode, where you probe foo() to
capture the values of dead registers in its callers.

I'll send a v2 and drop the first hunk.