perf core: Use KSTK_ESP() instead of pt_regs->sp while output user regs

From: Chenggang Qin <[email protected]>

For x86_64, the exact value of user stack's esp should be got by KSTK_ESP(current). current->thread.usersp is copied from PDA while enter ring0.
Now, we output the value of sp from pt_regs. But pt_regs->sp has changed before it was pushed into kernel stack.

So, we cannot get the correct callchain while unwind some user stacks. For example, if the stack contains __lll_unlock_wake()/__lll_lock_wait(), the callchain will break some times with the latest version of libunwind. The root cause is the sp that is used by libunwind may be wrong.

If we use KSTK_ESP(current), the correct callchain can be got everytime. Other architectures also have KSTK_ESP() macro.

Signed-off-by Chenggang Qin <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Arjan van de Ven <[email protected]>

Cc David Ahern <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc Yanmin Zhang <[email protected]>

---
 arch/x86/kernel/perf_regs.c | 3 +++
 1 file changed, 3 insertions(+)

arch/x86/kernel/perf_regs.c +0 -0

index e309cc5..5da8df8 100644

arch/x86/kernel/perf_regs.c +0 -0

 	return regs_get_register(regs, pt_regs_offset[idx]);
 }
 

The context is NMI (PMU) or IRQ (hrtimer). It is a bit complex. The process we want to sample is the current, so it is always running.
We need to distinguish between IRQ context, syscall or user context that are interrupted by NMI.

Syscall: sp = current->thread.usersp;

             old_rsp always point to current->thread.usersp. May be we

shouldn't use current_user_stack_pointer();
User: sp = task_pt_regs(task)->sp;
current's pt_regs are stored in kernel stack while NMI or IRQ
occured.

IRQ: sp = task_pt_regs(task)->sp;
current's pt_regs are stored in kernel stack while IRQ which was
interrupted occured.

Regards
Chenggang

------------------------------------------------------------------
发件人：Andy Lutomirski <[email protected]> 发送时间：2014年12月23日(星期二) 16:30
收件人：root <[email protected]>，linux-kernel <[email protected]>
抄　送：秦承刚(承刚) <[email protected]>，Andrew Morton <[email protected]>，Arjan van de Ven <[email protected]>，David Ahern <[email protected]>，Ingo Molnar <[email protected]>，Mike Galbraith <[email protected]>，Namhyung Kim <[email protected]>，Paul Mackerras <[email protected]>，Peter Zijlstra <[email protected]>，Wu Fengguang <[email protected]>，Yanmin Zhang <[email protected]> 主　题：Re: [PATCH] perf core: Use KSTK_ESP() instead of pt_regs->sp while output user regs

Quoted 39 lines · root

From: Chenggang Qin <[email protected]>

For x86_64, the exact value of user stack's esp should be got by KSTK_ESP(current).
current->thread.usersp is copied from PDA while enter ring0. Now, we output the value of sp from pt_regs. But pt_regs->sp has changed before
it was pushed into kernel stack.

So, we cannot get the correct callchain while unwind some user stacks. For example, if the stack contains __lll_unlock_wake()/__lll_lock_wait(), the
callchain will break some times with the latest version of libunwind. The root cause is the sp that is used by libunwind may be wrong.

If we use KSTK_ESP(current), the correct callchain can be got everytime. Other architectures also have KSTK_ESP() macro.

Signed-off-by Chenggang Qin <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Arjan van de Ven <[email protected]>

Cc David Ahern <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc Yanmin Zhang <[email protected]>

---
arch/x86/kernel/perf_regs.c | 3 +++

1 file changed, 3 insertions(+)

arch/x86/kernel/perf_regs.c +0 -0

index e309cc5..5da8df8 100644

arch/x86/kernel/perf_regs.c u64 perf_reg_value(struct pt_regs *regs, int idx) · line 65

Quoted 6 lines

arch/x86/kernel/perf_regs.c +3 -0

@@ -60,6 +60,9 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
 if (WARN_ON_ONCE(idx >= ARRAY_SIZE(pt_regs_offset)))
 return 0;
 
+ if (idx == PERF_REG_X86_SP)
+ return KSTK_ESP(current);
+

This patch is probably fine, but KSTK_ESP seems to be bogus:

unsigned long KSTK_ESP(struct task_struct *task) {
return (test_tsk_thread_flag(task, TIF_IA32)) ? (task_pt_regs(task)->sp) : ((task)->thread.usersp); }

I swear that every time I've looked at anything that references TIF_IA32 in the last two weeks, it's been wrong. This should be something like:

if (task_thread_info(task)->status & TS_COMPAT) return task_pt_regs(task)->sp;
else if (task == current && task is in a syscall) return current_user_stack_pointer();
else if (task is not running && task is in a syscall) return task->thread.usersp;
else if (task is not in a syscall)
return task_pt_regs(task)->sp;
else
we're confused; give up.

What context are you using KSTK_ESP in?

--Andy

Quoted 4 lines

arch/x86/kernel/perf_regs.c +0 -0

 return regs_get_register(regs, pt_regs_offset[idx]);
 }
 

Quoted 4 lines · 秦承刚(承刚)

The context is NMI (PMU) or IRQ (hrtimer). It is a bit complex. The process we want to sample is the current, so it is always running.
We need to distinguish between IRQ context, syscall or user context that are interrupted by NMI.

Oh. So you really are trying to get the user regs from NMI context even if you interrupted the kernel. This will be an unpleasant thing to do correctly.

Quoted 1 line

Syscall: sp = current->thread.usersp;

Nope. For x86_64 at least, sp is in old_rsp, not usersp, *if* the syscall you interrupted was syscall64 instead of one of the ia32 entries, which are differently strange. If you've context switched out and back, then usersp will match it. If not, then you're looking at some random stale sp value.

Keep in mind that there is absolutely no guarantee that TIF_IA32 matches the syscall entry type.

Quoted 5 lines

             old_rsp always point to current->thread.usersp. May be we

shouldn't use current_user_stack_pointer();
User: sp = task_pt_regs(task)->sp;
current's pt_regs are stored in kernel stack while NMI or IRQ
occured.

This is the only easy case.

Quoted 3 lines

IRQ: sp = task_pt_regs(task)->sp;
current's pt_regs are stored in kernel stack while IRQ which was
interrupted occured.

Sort of. It's true by the time you actually execute the IRQ handler.

I think that trying to do this is doomed to either failure or extreme complexity. You're in an NMI, so you could be part-way through a context switch or you could be in the very first instruction of the syscall handler.

On a quick look, there are plenty of other bugs in there besides just the stack pointer issue. The ABI check that uses TIF_IA32 in the perf core is completely wrong. TIF_IA32 may be equal to the actual userspace bitness by luck, but, if so, that's more or less just luck. And there's a user_mode test that should be user_mode_vm.

Also, it's not just sp that's wrong. There are various places that you can interrupt in which many of the registers have confusing locations. You could try using the cfi unwind data, but that's unlikely to work for regs like cs and ss, and, during context switch, this has very little chance of working.

What's the point of this feature? Honestly, my suggestion would be to delete it instead of trying to fix it. It's also not clear to me that there aren't serious security problems here -- it's entirely possible for sensitive *kernel* values to and up in task_pt_regs at certain times, and if you run during context switch and there's no code to suppress this dump during context switch, then you could be showing regs that belong to the wrong task.

--Andy

Quoted 87 lines

Regards
Chenggang

------------------------------------------------------------------
发件人：Andy Lutomirski <[email protected]> 发送时间：2014年12月23日(星期二) 16:30
收件人：root <[email protected]>，linux-kernel <[email protected]>
抄　送：秦承刚(承刚) <[email protected]>，Andrew Morton <[email protected]>，Arjan van de Ven <[email protected]>，David Ahern <[email protected]>，Ingo Molnar <[email protected]>，Mike Galbraith <[email protected]>，Namhyung Kim <[email protected]>，Paul Mackerras <[email protected]>，Peter Zijlstra <[email protected]>，Wu Fengguang <[email protected]>，Yanmin Zhang <[email protected]> 主　题：Re: [PATCH] perf core: Use KSTK_ESP() instead of pt_regs->sp while output user regs

Quoted 39 lines · root

From: Chenggang Qin <[email protected]>

For x86_64, the exact value of user stack's esp should be got by KSTK_ESP(current).
current->thread.usersp is copied from PDA while enter ring0. Now, we output the value of sp from pt_regs. But pt_regs->sp has changed before
it was pushed into kernel stack.

So, we cannot get the correct callchain while unwind some user stacks. For example, if the stack contains __lll_unlock_wake()/__lll_lock_wait(), the
callchain will break some times with the latest version of libunwind. The root cause is the sp that is used by libunwind may be wrong.

If we use KSTK_ESP(current), the correct callchain can be got everytime. Other architectures also have KSTK_ESP() macro.

Signed-off-by Chenggang Qin <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Arjan van de Ven <[email protected]>

Cc David Ahern <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc Yanmin Zhang <[email protected]>

---
arch/x86/kernel/perf_regs.c | 3 +++

1 file changed, 3 insertions(+)

arch/x86/kernel/perf_regs.c +0 -0

index e309cc5..5da8df8 100644

arch/x86/kernel/perf_regs.c u64 perf_reg_value(struct pt_regs *regs, int idx) · line 65

Quoted 6 lines

arch/x86/kernel/perf_regs.c +3 -0

@@ -60,6 +60,9 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
 if (WARN_ON_ONCE(idx >= ARRAY_SIZE(pt_regs_offset)))
 return 0;
 
+ if (idx == PERF_REG_X86_SP)
+ return KSTK_ESP(current);
+

This patch is probably fine, but KSTK_ESP seems to be bogus:

unsigned long KSTK_ESP(struct task_struct *task) {
return (test_tsk_thread_flag(task, TIF_IA32)) ? (task_pt_regs(task)->sp) : ((task)->thread.usersp); }

I swear that every time I've looked at anything that references TIF_IA32 in the last two weeks, it's been wrong. This should be something like:

if (task_thread_info(task)->status & TS_COMPAT) return task_pt_regs(task)->sp;
else if (task == current && task is in a syscall) return current_user_stack_pointer();
else if (task is not running && task is in a syscall) return task->thread.usersp;
else if (task is not in a syscall)
return task_pt_regs(task)->sp;
else
we're confused; give up.

Quoted 8 lines

What context are you using KSTK_ESP in?

--Andy

Quoted 4 lines

arch/x86/kernel/perf_regs.c +0 -0

 return regs_get_register(regs, pt_regs_offset[idx]);
 }
 

--
Andy Lutomirski
AMA Capital Management, LLC

On a quick look, there are plenty of other bugs in there besides just the stack pointer issue. The ABI check that uses TIF_IA32 in the perf core is completely wrong. TIF_IA32 may be equal to the actual userspace bitness by luck, but, if so, that's more or less just luck. And there's a user_mode test that should be user_mode_vm.

Also, it's not just sp that's wrong. There are various places that you can interrupt in which many of the registers have confusing locations. You could try using the cfi unwind data, but that's unlikely to work for regs like cs and ss, and, during context switch, this has very little chance of working.

What's the point of this feature? Honestly, my suggestion would be to delete it instead of trying to fix it. It's also not clear to me that there aren't serious security problems here -- it's entirely possible for sensitive *kernel* values to and up in task_pt_regs at certain times, and if you run during context switch and there's no code to suppress this dump during context switch, then you could be showing regs that belong to the wrong task.

Quoted 12 lines · Andy Lutomirski

On a quick look, there are plenty of other bugs in there besides just the stack pointer issue. The ABI check that uses TIF_IA32 in the perf core is completely wrong. TIF_IA32 may be equal to the actual userspace bitness by luck, but, if so, that's more or less just luck. And there's a user_mode test that should be user_mode_vm.

Also, it's not just sp that's wrong. There are various places that you can interrupt in which many of the registers have confusing locations. You could try using the cfi unwind data, but that's unlikely to work for regs like cs and ss, and, during context switch, this has very little chance of working.

Quoted 7 lines

What's the point of this feature? Honestly, my suggestion would be to delete it instead of trying to fix it. It's also not clear to me that there aren't serious security problems here -- it's entirely possible for sensitive *kernel* values to and up in task_pt_regs at certain times, and if you run during context switch and there's no code to suppress this dump during context switch, then you could be showing regs that belong to the wrong task.

Of course the people who actually wrote the code are not on CC :/

There's two users of this iirc;

1) the dwarf stack unwinder thingy, which basically dumps the userspace regs and the top of userspace stack on 'event'.

2) the recent sample_regs_intr, which dumps the register set at 'event', be it kernel or userspace.

The first is somewhat usable when lacking framepointers while still desiring some unwind information, the second is useful to things like call argument profiling and the like.

Quoted 22 lines · Peter Zijlstra

Quoted 12 lines · Andy Lutomirski

On a quick look, there are plenty of other bugs in there besides just the stack pointer issue. The ABI check that uses TIF_IA32 in the perf core is completely wrong. TIF_IA32 may be equal to the actual userspace bitness by luck, but, if so, that's more or less just luck. And there's a user_mode test that should be user_mode_vm.

Also, it's not just sp that's wrong. There are various places that you can interrupt in which many of the registers have confusing locations. You could try using the cfi unwind data, but that's unlikely to work for regs like cs and ss, and, during context switch, this has very little chance of working.

Quoted 7 lines

What's the point of this feature? Honestly, my suggestion would be to delete it instead of trying to fix it. It's also not clear to me that there aren't serious security problems here -- it's entirely possible for sensitive *kernel* values to and up in task_pt_regs at certain times, and if you run during context switch and there's no code to suppress this dump during context switch, then you could be showing regs that belong to the wrong task.

Quoted 7 lines

Of course the people who actually wrote the code are not on CC :/

There's two users of this iirc;

1) the dwarf stack unwinder thingy, which basically dumps the userspace regs and the top of userspace stack on 'event'.

Given how the x86_64* entry code works, using task_pt_regs from anywhere except explicitly supported contexts (including exceptions that originated in userspace and a small handful of system calls) is asking for trouble. NMI context is especially bad.

How important is this feature, and which registers matter? It might be possible to use a dwarf unwinder on the kernel call stack to get most of the regs from most contexts, and it might also be possible to make small changes to the entry code to make it possible to get some of the registers reliably, but it's not currently possible to safely use task_pt_regs *at all* from NMI context unless you've at least blacklisted a handful of origin RIP values that give dangerously bogus results. (Using do_nmi's regs parameter if user_mode_vm(regs) is a different story.)

* I'm not nearly as familiar with the 32-bit entry code, so I don't know whether we have the same issues there.

Quoted 3 lines

2) the recent sample_regs_intr, which dumps the register set at 'event', be it kernel or userspace.

What's wrong with the PMI's pt_regs for that? If we interrupted the kernel, they'll be kernel regs (with all their attendant security issues) and, if we interrupted userspace, then they'll be the full, correct userspace registers.

--Andy

Quoted 4 lines

The first is somewhat usable when lacking framepointers while still desiring some unwind information, the second is useful to things like call argument profiling and the like.

Quoted 1 line

Quoted 1 line

Andy Lutomirski <[email protected]> 12/31/14 3:00 AM >>>

Quoted 5 lines · Andy Lutomirski

Given how the x86_64* entry code works, using task_pt_regs from anywhere except explicitly supported contexts (including exceptions that originated in userspace and a small handful of system calls) is asking for trouble. NMI context is especially bad.

Quoted 9 lines

How important is this feature, and which registers matter? It might be possible to use a dwarf unwinder on the kernel call stack to get most of the regs from most contexts, and it might also be possible to make small changes to the entry code to make it possible to get some of the registers reliably, but it's not currently possible to safely use task_pt_regs *at all* from NMI context unless you've at least blacklisted a handful of origin RIP values that give dangerously bogus results. (Using do_nmi's regs parameter if user_mode_vm(regs) is a different story.)

It's actually worse than just knowing the interrupted kernel RIP. If the call chain goes usermode -> IST exception -> NMI, then task_pt_regs is entirely uninitialized. Assuming all the CFI annotations are correct, the unwinder could still do it from the kernel.

Note that, as far as I know, Jan Beulich is the only person who uses the unwinder on kernel code. Jan, how do you do this?

Quoted 24 lines

Quoted 2 lines

Quoted 2 lines

Andy Lutomirski <[email protected]> 12/31/14 3:00 AM >>>

Quoted 5 lines · Andy Lutomirski

Given how the x86_64* entry code works, using task_pt_regs from anywhere except explicitly supported contexts (including exceptions that originated in userspace and a small handful of system calls) is asking for trouble. NMI context is especially bad.

Quoted 9 lines

How important is this feature, and which registers matter? It might be possible to use a dwarf unwinder on the kernel call stack to get most of the regs from most contexts, and it might also be possible to make small changes to the entry code to make it possible to get some of the registers reliably, but it's not currently possible to safely use task_pt_regs *at all* from NMI context unless you've at least blacklisted a handful of origin RIP values that give dangerously bogus results. (Using do_nmi's regs parameter if user_mode_vm(regs) is a different story.)

It's actually worse than just knowing the interrupted kernel RIP. If the call chain goes usermode -> IST exception -> NMI, then task_pt_regs is entirely uninitialized. Assuming all the CFI annotations are correct, the unwinder could still do it from the kernel.

Quoted 2 lines

Note that, as far as I know, Jan Beulich is the only person who uses the unwinder on kernel code. Jan, how do you do this?

Trying to guess what you mean by "this": A stack switch gets expressed by CFI annotations just like any other frame pointer adjustments. See for example the CFI_DEF_CFA_REGISTER use in the SAVE_ARGS_IRQ macro.

If that wasn't your question, please be more precise.

Jan

Quoted 1 line

Quoted 1 line

Andy Lutomirski <[email protected]> 01/02/15 7:03 PM >>>

Quoted 5 lines · Jan Beulich

Trying to guess what you mean by "this": A stack switch gets expressed by CFI annotations just like any other frame pointer adjustments. See for example the CFI_DEF_CFA_REGISTER use in the SAVE_ARGS_IRQ macro.

If that wasn't your question, please be more precise.

Sorry, my question was vague.

Is there any way to consume these annotations at runtime in the kernel? The goal would be for perf's NMI handler to consume the CFI data to figure out the userspace registers. I'm guessing that the answer might be no, because we seem to be compiling with -fno-asynchronous-unwind-tables and we don't seem to be putting any .eh_frame stuff into the final kernel image.

I had thought that someone implemented runtime DWARF unwinding, though.

Quoted 12 lines · Andy Lutomirski

On a quick look, there are plenty of other bugs in there besides just the stack pointer issue. The ABI check that uses TIF_IA32 in the perf core is completely wrong. TIF_IA32 may be equal to the actual userspace bitness by luck, but, if so, that's more or less just luck. And there's a user_mode test that should be user_mode_vm.

Also, it's not just sp that's wrong. There are various places that you can interrupt in which many of the registers have confusing locations. You could try using the cfi unwind data, but that's unlikely to work for regs like cs and ss, and, during context switch, this has very little chance of working.

Quoted 7 lines

What's the point of this feature? Honestly, my suggestion would be to delete it instead of trying to fix it. It's also not clear to me that there aren't serious security problems here -- it's entirely possible for sensitive *kernel* values to and up in task_pt_regs at certain times, and if you run during context switch and there's no code to suppress this dump during context switch, then you could be showing regs that belong to the wrong task.

Of course the people who actually wrote the code are not on CC :/

There's two users of this iirc;

1) the dwarf stack unwinder thingy, which basically dumps the userspace regs and the top of userspace stack on 'event'.

Quoted 25 lines · Peter Zijlstra

Quoted 12 lines · Andy Lutomirski

On a quick look, there are plenty of other bugs in there besides just the stack pointer issue. The ABI check that uses TIF_IA32 in the perf core is completely wrong. TIF_IA32 may be equal to the actual userspace bitness by luck, but, if so, that's more or less just luck. And there's a user_mode test that should be user_mode_vm.

Also, it's not just sp that's wrong. There are various places that you can interrupt in which many of the registers have confusing locations. You could try using the cfi unwind data, but that's unlikely to work for regs like cs and ss, and, during context switch, this has very little chance of working.

Quoted 7 lines

What's the point of this feature? Honestly, my suggestion would be to delete it instead of trying to fix it. It's also not clear to me that there aren't serious security problems here -- it's entirely possible for sensitive *kernel* values to and up in task_pt_regs at certain times, and if you run during context switch and there's no code to suppress this dump during context switch, then you could be showing regs that belong to the wrong task.

Of course the people who actually wrote the code are not on CC :/

There's two users of this iirc;

Quoted 2 lines

1) the dwarf stack unwinder thingy, which basically dumps the userspace regs and the top of userspace stack on 'event'.

looks like this solves the issue I was trying to fix long time ago:
http://marc.info/?l=linux-kernel&m=134934717011451&w=2

this seems a lot simpler ;-) I'll test..

jirka

Quoted 27 lines · Jiri Olsa

Quoted 25 lines · Peter Zijlstra

Quoted 12 lines · Andy Lutomirski

On a quick look, there are plenty of other bugs in there besides just the stack pointer issue. The ABI check that uses TIF_IA32 in the perf core is completely wrong. TIF_IA32 may be equal to the actual userspace bitness by luck, but, if so, that's more or less just luck. And there's a user_mode test that should be user_mode_vm.

Also, it's not just sp that's wrong. There are various places that you can interrupt in which many of the registers have confusing locations. You could try using the cfi unwind data, but that's unlikely to work for regs like cs and ss, and, during context switch, this has very little chance of working.

Quoted 7 lines

What's the point of this feature? Honestly, my suggestion would be to delete it instead of trying to fix it. It's also not clear to me that there aren't serious security problems here -- it's entirely possible for sensitive *kernel* values to and up in task_pt_regs at certain times, and if you run during context switch and there's no code to suppress this dump during context switch, then you could be showing regs that belong to the wrong task.

Of course the people who actually wrote the code are not on CC :/

There's two users of this iirc;

Quoted 8 lines

Quoted 2 lines

1) the dwarf stack unwinder thingy, which basically dumps the userspace regs and the top of userspace stack on 'event'.

looks like this solves the issue I was trying to fix long time ago:
http://marc.info/?l=linux-kernel&m=134934717011451&w=2

this seems a lot simpler ;-) I'll test..

I suspect that, if it works, then it's pure luck. (task)->thread.usersp in KSTK_ESP is bogus -- your code was more accurate.

I think we should seriously consider making use of this feature by non-root users require an explicit sysctl. Sending values to user code that are, at best, free of sensitive kernel data most of the time is IMO inappropriate for an unprivileged API.

I'm currently working on a patch to try to clean this up better.

On x86_64, at least, task_pt_regs may be only partially initialized in many contexts, so x86_64 should not use it without extra care from interrupt context, let alone NMI context.

This will allow x86_64 to override the logic and will supply some scratch space to use to make a cleaner copy of user regs.

Quoted 6 lines · Andy Lutomirski

On x86_64, at least, task_pt_regs may be only partially initialized in many contexts, so x86_64 should not use it without extra care from interrupt context, let alone NMI context.

This will allow x86_64 to override the logic and will supply some scratch space to use to make a cleaner copy of user regs.

Just wondering how bad it would be to fill out the actual pt_regs that was previously partially initialized?

Quoted 2 lines

Just wondering how bad it would be to fill out the actual pt_regs that was previously partially initialized?

Bad, for at least two reasons.

One is that we don't actually know which regs are initialized. bx, bp, r12 etc are particularly bad in this regard, due to the FORK_LIKE mechanism and similar optimizations.

The other is that the uninitialized part of task_pt_regs can be used for something else entirely. If we have a syscall instruction immediately followed by a regular interrupt, then the interrupt's hardware frame will overlap task_pt_regs. (I'm not going to claim that this design is sensible, but it is what it is. IIRC Denys Vlasenko had some patches to partially clean this up.)

It would be possible to rework the code to avoid an extra pt_regs copy, but I don't see an obvious way to do it cleanly.

Quoted 8 lines · Andy Lutomirski

Quoted 2 lines

Just wondering how bad it would be to fill out the actual pt_regs that was previously partially initialized?

Bad, for at least two reasons.

One is that we don't actually know which regs are initialized. bx, bp, r12 etc are particularly bad in this regard, due to the FORK_LIKE mechanism and similar optimizations.

Right, but you need to deal with that anyhow.

Quoted 7 lines

The other is that the uninitialized part of task_pt_regs can be used for something else entirely. If we have a syscall instruction immediately followed by a regular interrupt, then the interrupt's hardware frame will overlap task_pt_regs. (I'm not going to claim that this design is sensible, but it is what it is. IIRC Denys Vlasenko had some patches to partially clean this up.)

Ah, urgh. Yes painful that.

Quoted 2 lines

It would be possible to rework the code to avoid an extra pt_regs copy, but I don't see an obvious way to do it cleanly.

Yeah, we'll see how this works, I was just curious on the exact need for the copy, but if as you say, the original structure might not even exist properly (even though we have a pointer to it) that's bad (TM).

This seems to improve the situation on x86_64 from almost entirely broken to mostly working. Tested with:

perf record -e cycles --call-graph=dwarf ls

Andy Lutomirski (2):
perf: Move task_pt_regs sampling into arch code
x86_64, perf: Improve user regs sampling

 arch/arm/kernel/perf_regs.c   |  8 ++++
 arch/arm64/kernel/perf_regs.c |  8 ++++
 arch/x86/kernel/perf_regs.c   | 90 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/perf_event.h    | 12 +++---
 include/linux/perf_regs.h     | 16 ++++++++
 kernel/events/core.c          | 19 ++++-----
 6 files changed, 137 insertions(+), 16 deletions(-)

Signature · 1 line

2.1.0