| Message ID | 20241009153901.3878233-2-ardb+git@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | x86/ftrace: Don't bother preserving/restoring R10/R11 | expand |
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 > --
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)
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
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.
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
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.
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