: perf/x86: store user space frame-pointer value on a sample

PATCH Superseded Alexey Budankov 4 participants 18 messages May 8, 2018 – May 24, 2018 x86

Share & tools

Alexey Budankov

author

2018-05-08 15:21 UTC #0 lore raw

PATCH v2 1/1 : perf/x86: store user space frame-pointer value on a sample

Store user space frame-pointer value (BP register) into Perf trace on a sample for a process so the value becomes available when unwinding call stacks for functions gaining event samples.

Signed-off-by Alexey Budankov <[email protected]>

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

arch/x86/kernel/perf_regs.c +7 -1

index e47b2dbbdef3..8d68658eff7f 100644
@@ -156,7 +156,13 @@ void perf_get_regs_user(struct perf_regs *regs_user,
 	 * Most system calls don't save these registers, don't report them.
 	 */
 	regs_user_copy->bx = -1;
-	regs_user_copy->bp = -1;
+	/*
+	 * Store user space frame-pointer value on sample
+	 * to facilitate stack unwinding for cases when
+	 * user space executable code has such support
+	 * enabled at compile time;
+	 */
+	regs_user_copy->bp = user_regs->bp;
 	regs_user_copy->r12 = -1;
 	regs_user_copy->r13 = -1;
 	regs_user_copy->r14 = -1;

Peter Zijlstra

2018-05-09 14:54 UTC #1 lore raw

Quoted 15 lines · Alexey Budankov

Store user space frame-pointer value (BP register) into Perf trace on a sample for a process so the value becomes available when unwinding call stacks for functions gaining event samples.
Signed-off-by Alexey Budankov <[email protected]>
---
 arch/x86/kernel/perf_regs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kernel/perf_regs.c +0 -0
index e47b2dbbdef3..8d68658eff7f 100644

arch/x86/kernel/perf_regs.c void perf_get_regs_user(struct perf_regs *regs_user, · line 158

Quoted 3 lines

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

@@ -156,7 +156,13 @@ void perf_get_regs_user(struct perf_regs *regs_user,
 
 
 	 * Most system calls don't save these registers, don't report them.

^^^ that worries me and is the reason for the '-1's below. However I think with all the PTI rework this might no longer be true.

The Changelog needs to state that user_regs->bp is in fact valid and ideally point to the commits that makes it so. Also this patch should update that comment.

Cc Andy who keeps better track of all that than me.

Quoted 13 lines

arch/x86/kernel/perf_regs.c +7 -1

 	 */
 	regs_user_copy->bx = -1;
-	regs_user_copy->bp = -1;
+	/*
+	 * Store user space frame-pointer value on sample
+	 * to facilitate stack unwinding for cases when
+	 * user space executable code has such support
+	 * enabled at compile time;
+	 */
+	regs_user_copy->bp = user_regs->bp;
 	regs_user_copy->r12 = -1;

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

 	regs_user_copy->r13 = -1;
 	regs_user_copy->r14 = -1;

Alexey Budankov

author

2018-05-10 09:42 UTC #2 lore raw

Hi,

Quoted 18 lines · Peter Zijlstra

Quoted 15 lines · Alexey Budankov
Store user space frame-pointer value (BP register) into Perf trace on a sample for a process so the value becomes available when unwinding call stacks for functions gaining event samples.
Signed-off-by Alexey Budankov <[email protected]>
---
 arch/x86/kernel/perf_regs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kernel/perf_regs.c +0 -0
index e47b2dbbdef3..8d68658eff7f 100644
@@ -156,7 +156,13 @@ void perf_get_regs_user(struct perf_regs *regs_user,

Quoted 4 lines

arch/x86/kernel/perf_regs.c void perf_get_regs_user(struct perf_regs *regs_user, · line 156
Quoted 1 line
arch/x86/kernel/perf_regs.c +0 -0
 	 * Most system calls don't save these registers, don't report them.
^^^ that worries me and is the reason for the '-1's below. However I think with all the PTI rework this might no longer be true.

Well ok, at the moment I don't see the rationale behind exposure the other registers so they still may be reported as -1.

However BP may contain valid frame address not only on syscalls but also for samples landing into user space.

Quoted 2 lines

The Changelog needs to state that user_regs->bp is in fact valid and

That actually was tested on binaries compiled without and with BP exposed and in the latter case proved the value of that change.

Test executable for the example below was compiled with frame pointer support enabled:

g++ -o futex-fp -fpermissive --no-omit-frame-pointer futex.c

and profiled using:

tools/perf/perf record --user-regs=IP,SP,BP \
	-g --call-graph=dwarf,1024 -e cycles -- ./futex-fp

Output of

tools/perf/perf report -i perf.data --stdio

demonstrates the effect of the patch change so before saving BP value on a sample we have several frames missing above main function frame:

# Samples: 138K of event 'cpu-cycles'
# Event count (approx.): 92713835335
#

# Children      Self  Command   Shared Object     Symbol                                        
# ........  ........  ........  ................  ..........................
#
    96.15%     0.72%  futex-fp  futex-fp          [.] main
            |          
            |--95.43%--main
            |          |          
            |          |--71.56%--syscall
            |          |          |          
            |          |          |--57.28%--entry_SYSCALL_64_after_hwframe
            |          |          |          |          
            |          |          |           --56.95%--do_syscall_64
            |          |          |                     |          
            |          |          |                      --55.77%--sys_futex

and after saving BP value on a sample we have expected

_start
__libc_start_main

frames unwound:

# Samples: 128K of event 'cpu-cycles'
# Event count (approx.): 85349981034
#

# Children      Self  Command   Shared Object     Symbol                                        
# ........  ........  ........  ................  ..................
#
    95.83%     0.00%  futex-fp  futex-fp          [.] _start
            |
==>         ---_start
==>            __libc_start_main
               main
               |          
               |--71.28%--syscall
               |          |          
               |          |--55.67%--entry_SYSCALL_64
               |          |          |          
               |          |           --55.40%--do_syscall_64
               |          |                     |          
               |          |                      --54.21%--sys_futex

Quoted 2 lines

ideally point to the commits that makes it so. Also this patch should update that comment.

Accepted.

Quoted 2 lines

Cc Andy who keeps better track of all that than me.

Yes, any comments and feedback would be very welcome.

Thanks,
Alexey

Quoted 15 lines

Quoted 14 lines

arch/x86/kernel/perf_regs.c +7 -1

 	 */
 	regs_user_copy->bx = -1;
-	regs_user_copy->bp = -1;
+	/*
+	 * Store user space frame-pointer value on sample
+	 * to facilitate stack unwinding for cases when
+	 * user space executable code has such support
+	 * enabled at compile time;
+	 */
+	regs_user_copy->bp = user_regs->bp;
 	regs_user_copy->r12 = -1;
 	regs_user_copy->r13 = -1;
 	regs_user_copy->r14 = -1;

Peter Zijlstra

2018-05-10 10:14 UTC #3 lore raw

Quoted 4 lines · Alexey Budankov

Quoted 1 line
The Changelog needs to state that user_regs->bp is in fact valid and
That actually was tested on binaries compiled without and with BP exposed and in the latter case proved the value of that change.

Mostly works is not the same as 'always initialized', if there are entry paths that do not store that register, then using the value might leak values from the kernel stack, which would be bad.

But like said, I think much of the kernel entry code was sanitized with the PTI effort and I suspect things are in fact fine now, but lets wait for Andy to confirm.

Alexey Budankov

author

2018-05-10 10:29 UTC #4 lore raw

Hi,

Quoted 9 lines · Peter Zijlstra

Quoted 4 lines · Alexey Budankov
Quoted 1 line
The Changelog needs to state that user_regs->bp is in fact valid and
That actually was tested on binaries compiled without and with BP exposed and in the latter case proved the value of that change.
Mostly works is not the same as 'always initialized', if there are entry paths that do not store that register, then using the value might leak values from the kernel stack, which would be bad.

Yep, absolutely agree. Extra care needs to be taken here.

Quoted 5 lines

But like said, I think much of the kernel entry code was sanitized with the PTI effort and I suspect things are in fact fine now, but lets wait for Andy to confirm.

Thanks,
Alexey

Alexey Budankov

author

2018-05-21 12:44 UTC #5 lore raw

In reply to Peter Zijlstra

Hi Peter,

Quoted 6 lines · Peter Zijlstra

Quoted 4 lines · Alexey Budankov
Quoted 1 line
The Changelog needs to state that user_regs->bp is in fact valid and
That actually was tested on binaries compiled without and with BP exposed and in the latter case proved the value of that change.

Quoted 7 lines

Mostly works is not the same as 'always initialized', if there are entry paths that do not store that register, then using the value might leak values from the kernel stack, which would be bad.
But like said, I think much of the kernel entry code was sanitized with the PTI effort and I suspect things are in fact fine now, but lets wait for Andy to confirm.

It looks like, these days, all registers are saved on system calls, just like you anticipated.

So BP register value might be stored into the Perf trace on a sample.

Andy?

Thanks,
Alexey

Quoted 1 line

Andy Lutomirski

2018-05-21 14:14 UTC #6 lore raw

Quoted 20 lines · Alexey Budankov

Hi Peter,
Quoted 7 lines · Peter Zijlstra
Quoted 4 lines · Alexey Budankov
Quoted 1 line
The Changelog needs to state that user_regs->bp is in fact valid and
That actually was tested on binaries compiled without and with BP exposed and in the latter case proved the value of that change.
Quoted 7 lines
Mostly works is not the same as 'always initialized', if there are entry paths that do not store that register, then using the value might leak values from the kernel stack, which would be bad.
But like said, I think much of the kernel entry code was sanitized with the PTI effort and I suspect things are in fact fine now, but lets wait for Andy to confirm.

Quoted 6 lines

It looks like, these days, all registers are saved on system calls, just like you anticipated.
So BP register value might be stored into the Perf trace on a sample.
Andy?

Hmm, I thought I replied. Yes, they are indeed all saved, but I’m not very excited about committing to doing so forever. But storing BP should be fine.

Alexey Budankov

author

2018-05-21 16:51 UTC #7 lore raw

Hi Andy,

Quoted 20 lines · Andy Lutomirski

Quoted 20 lines · Alexey Budankov
Hi Peter,
Quoted 14 lines · Peter Zijlstra
Quoted 4 lines · Alexey Budankov
Quoted 1 line
The Changelog needs to state that user_regs->bp is in fact valid and
That actually was tested on binaries compiled without and with BP exposed and in the latter case proved the value of that change.
Mostly works is not the same as 'always initialized', if there are entry paths that do not store that register, then using the value might leak values from the kernel stack, which would be bad.
But like said, I think much of the kernel entry code was sanitized with the PTI effort and I suspect things are in fact fine now, but lets wait for Andy to confirm.

Quoted 8 lines

Quoted 6 lines
It looks like, these days, all registers are saved on system calls, just like you anticipated.
So BP register value might be stored into the Perf trace on a sample.
Andy?
Hmm, I thought I replied. Yes, they are indeed all saved, but I’m not very excited about committing to doing so forever. But storing BP should be fine.

Thanks for explicit confirmation regarding BP register. BTW, do you see any mean to prevent possible unattended regression? I guess it could be some compile time assertion or regression testing.

Thanks,
Alexey

Quoted 1 line

Andy Lutomirski

2018-05-21 17:23 UTC #8 lore raw

Quoted 31 lines · Alexey Budankov

Hi Andy,
Quoted 26 lines · Andy Lutomirski
Quoted 25 lines · Alexey Budankov
Hi Peter,
Quoted 7 lines · Peter Zijlstra
Quoted 4 lines · Alexey Budankov
Quoted 1 line
The Changelog needs to state that user_regs->bp is in fact valid and
That actually was tested on binaries compiled without and with BP exposed and in the latter case proved the value of that change.
Quoted 7 lines
Mostly works is not the same as 'always initialized', if there are entry paths that do not store that register, then using the value might leak values from the kernel stack, which would be bad.
But like said, I think much of the kernel entry code was sanitized with the PTI effort and I suspect things are in fact fine now, but lets wait for Andy to confirm.
It looks like, these days, all registers are saved on system calls, just like you anticipated.
So BP register value might be stored into the Perf trace on a sample.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Andy?
Hmm, I thought I replied. Yes, they are indeed all saved, but I’m not very excited about committing to doing so forever. But storing BP should be fine.
Thanks for explicit confirmation regarding BP register. BTW, do you see any mean to prevent possible unattended regression? I guess it could be some compile time assertion or regression testing.

Write a selftest?

The whole perf user regs mechanism is buggy and fragile. I need to massively clean it up at some point.

Quoted 5 lines

Thanks,
Alexey
Quoted 1 line

Alexey Budankov

author

2018-05-21 18:11 UTC #9 lore raw

Hi,

Quoted 33 lines · Andy Lutomirski

Quoted 33 lines · Alexey Budankov
Hi Andy,
Quoted 28 lines · Andy Lutomirski
Quoted 26 lines · Alexey Budankov
Hi Peter,
Quoted 7 lines · Peter Zijlstra
Quoted 4 lines · Alexey Budankov
Quoted 1 line
The Changelog needs to state that user_regs->bp is in fact valid and
That actually was tested on binaries compiled without and with BP exposed and in the latter case proved the value of that change.
Quoted 7 lines
Mostly works is not the same as 'always initialized', if there are entry paths that do not store that register, then using the value might leak values from the kernel stack, which would be bad.
But like said, I think much of the kernel entry code was sanitized with the PTI effort and I suspect things are in fact fine now, but lets wait for Andy to confirm.
It looks like, these days, all registers are saved on system calls, just like you anticipated.
So BP register value might be stored into the Perf trace on a sample.
Andy?

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Hmm, I thought I replied. Yes, they are indeed all saved, but I’m not very excited about committing to doing so forever. But storing BP should be fine.
Thanks for explicit confirmation regarding BP register. BTW, do you see any mean to prevent possible unattended regression? I guess it could be some compile time assertion or regression testing.
Write a selftest?

Hmm, that might be. It would be good to have some embedded notification when things change.

Quoted 2 lines

The whole perf user regs mechanism is buggy and fragile. I need to massively clean it up at some point.

Yep, making Perf user regs part more robust makes great sense.
It is critical part of perf/core subsystem providing values of IP,SP,BP registers that are needed for user stack unwinding during Perf trace file post processing.

Thanks,
Alexey

Quoted 7 lines

Quoted 6 lines
Thanks,
Alexey
Quoted 1 line

Alexey Budankov

author

2018-05-23 10:06 UTC #10 lore raw

In reply to Peter Zijlstra

Hi Peter,

Quoted 6 lines · Peter Zijlstra

Quoted 4 lines · Alexey Budankov
Quoted 1 line
The Changelog needs to state that user_regs->bp is in fact valid and
That actually was tested on binaries compiled without and with BP exposed and in the latter case proved the value of that change.

Quoted 8 lines

Mostly works is not the same as 'always initialized', if there are entry paths that do not store that register, then using the value might leak values from the kernel stack, which would be bad.
But like said, I think much of the kernel entry code was sanitized with the PTI effort and I suspect things are in fact fine now, but lets wait for Andy to confirm.

It looks like Andy confirmed on the questions above.

Is the patch ready to be up streamed now?

Thanks,
Alexey

Peter Zijlstra

2018-05-23 13:09 UTC #11 lore raw

Quoted 1 line · Alexey Budankov

Is the patch ready to be up streamed now?

Please post a new one where you modify the comment about the syscalls not saving registers and ideally find the commit that made it so.

Also; I think Andy would appreciate a comment near the syscall code that refers back to this code and states what registers we rely upon being there (+BP for this patch).

Alexey Budankov

author

2018-05-24 14:37 UTC #12 lore raw

Hi,

Quoted 6 lines · Peter Zijlstra

Quoted 1 line · Alexey Budankov
Is the patch ready to be up streamed now?
Please post a new one where you modify the comment about the syscalls not saving registers and ideally find the commit that made it so.

Sent v3 with adjusted comment.
As far comments become outdated quickly tried to be terse.

Quoted 4 lines

Also; I think Andy would appreciate a comment near the syscall code that refers back to this code and states what registers we rely upon being there (+BP for this patch).

Not sure if I can find all proper places to put comments there. However there is PUSH_AND_CLEAR_REGS macro that is employed at system call implementation so it is possible to put something like this there:

.macro PUSH_AND_CLEAR_REGS rdx=%rdx rax=%rax save_ret=0

	/*
	 ...
	 * perf/core subsystem relies on bp register value stored 
	 * at pt_regs->bp; see arch/x86/kernel/perf_regs.c: perf_get_regs_user() 
         * for more details;
         ...
	 */

Thanks,
Alexey

Quoted 2 lines

Andy Lutomirski

2018-05-24 14:52 UTC #13 lore raw

Quoted 22 lines · Alexey Budankov

Hi,
Quoted 6 lines · Peter Zijlstra
Quoted 1 line · Alexey Budankov
Is the patch ready to be up streamed now?
Please post a new one where you modify the comment about the syscalls not saving registers and ideally find the commit that made it so.
Sent v3 with adjusted comment.
As far comments become outdated quickly tried to be terse.
Quoted 4 lines
Also; I think Andy would appreciate a comment near the syscall code that refers back to this code and states what registers we rely upon being there (+BP for this patch).
Not sure if I can find all proper places to put comments there. However there is PUSH_AND_CLEAR_REGS macro that is employed at system call implementation so it is possible to put something like this there:

Quoted 5 lines

.macro PUSH_AND_CLEAR_REGS rdx=%rdx rax=%rax save_ret=0
/*
...
* perf/core subsystem relies on bp register value stored
* at pt_regs->bp; see arch/x86/kernel/perf_regs.c:

perf_get_regs_user()

Quoted 3 lines

          * for more details;
          ...
          */

Near the top of entry_SYSCALL_64 would be reasonable, as would no comment at all, I think.

Quoted 4 lines

Thanks,
Alexey
Quoted 2 lines

Alexey Budankov

author

2018-05-15 08:08 UTC #14 lore raw

In reply to Peter Zijlstra

Hi Andy,

Quoted 20 lines · Peter Zijlstra

Quoted 15 lines · Alexey Budankov
Store user space frame-pointer value (BP register) into Perf trace on a sample for a process so the value becomes available when unwinding call stacks for functions gaining event samples.
Signed-off-by Alexey Budankov <[email protected]>
---
 arch/x86/kernel/perf_regs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kernel/perf_regs.c +0 -0
index e47b2dbbdef3..8d68658eff7f 100644
arch/x86/kernel/perf_regs.c void perf_get_regs_user(struct perf_regs *regs_user, · line 158
Quoted 3 lines
arch/x86/kernel/perf_regs.c +0 -0
@@ -156,7 +156,13 @@ void perf_get_regs_user(struct perf_regs *regs_user,
 
 
 	 * Most system calls don't save these registers, don't report them.

Quoted 8 lines

^^^ that worries me and is the reason for the '-1's below. However I think with all the PTI rework this might no longer be true.
The Changelog needs to state that user_regs->bp is in fact valid and ideally point to the commits that makes it so. Also this patch should update that comment.
Cc Andy who keeps better track of all that than me.

Are there any thoughts so far? Feedback on the matter above is highly appreciated.

Thanks,
Alexey

Quoted 15 lines

Quoted 14 lines

arch/x86/kernel/perf_regs.c +7 -1

 	 */
 	regs_user_copy->bx = -1;
-	regs_user_copy->bp = -1;
+	/*
+	 * Store user space frame-pointer value on sample
+	 * to facilitate stack unwinding for cases when
+	 * user space executable code has such support
+	 * enabled at compile time;
+	 */
+	regs_user_copy->bp = user_regs->bp;
 	regs_user_copy->r12 = -1;

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

 	regs_user_copy->r13 = -1;
 	regs_user_copy->r14 = -1;

Andy Lutomirski

2018-05-15 16:30 UTC #15 lore raw

Quoted 30 lines · Alexey Budankov

Hi Andy,
Quoted 24 lines · Peter Zijlstra
Quoted 16 lines · Alexey Budankov
Store user space frame-pointer value (BP register) into Perf trace on a sample for a process so the value becomes available when unwinding call stacks for functions gaining event samples.
Signed-off-by Alexey Budankov <[email protected]>
---
arch/x86/kernel/perf_regs.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kernel/perf_regs.c +0 -0
index e47b2dbbdef3..8d68658eff7f 100644
arch/x86/kernel/perf_regs.c void perf_get_regs_user(struct perf_regs *regs_user, · line 158
Quoted 3 lines
arch/x86/kernel/perf_regs.c +0 -0
@@ -156,7 +156,13 @@ void perf_get_regs_user(struct perf_regs *regs_user,
 
 
    * Most system calls don't save these registers, don't report them.
^^^ that worries me and is the reason for the '-1's below. However I think with all the PTI rework this might no longer be true.

Quoted 7 lines

Quoted 5 lines
The Changelog needs to state that user_regs->bp is in fact valid and ideally point to the commits that makes it so. Also this patch should update that comment.
Cc Andy who keeps better track of all that than me.
Are there any thoughts so far? Feedback on the matter above is highly appreciated.

Sorry, I missed this. Can you forward the original patch? I don’t have it.
These days, system calls should save all registers, but I’m not entirely sure I want to promise that they’ll continue to do so forever.

Alexey Budankov

author

2018-05-16 08:42 UTC #16 lore raw

Hi,

Quoted 34 lines · Andy Lutomirski

Quoted 34 lines · Alexey Budankov
Hi Andy,
Quoted 28 lines · Peter Zijlstra
Quoted 16 lines · Alexey Budankov
Store user space frame-pointer value (BP register) into Perf trace on a sample for a process so the value becomes available when unwinding call stacks for functions gaining event samples.
Signed-off-by Alexey Budankov <[email protected]>
---
arch/x86/kernel/perf_regs.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kernel/perf_regs.c +0 -0
index e47b2dbbdef3..8d68658eff7f 100644
arch/x86/kernel/perf_regs.c void perf_get_regs_user(struct perf_regs *regs_user, · line 158
Quoted 3 lines
arch/x86/kernel/perf_regs.c +0 -0
@@ -156,7 +156,13 @@ void perf_get_regs_user(struct perf_regs *regs_user,
 
 
    * Most system calls don't save these registers, don't report them.
^^^ that worries me and is the reason for the '-1's below. However I think with all the PTI rework this might no longer be true.
The Changelog needs to state that user_regs->bp is in fact valid and ideally point to the commits that makes it so. Also this patch should update that comment.

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
Cc Andy who keeps better track of all that than me.
Are there any thoughts so far? Feedback on the matter above is highly appreciated.
Sorry, I missed this. Can you forward the original patch? I don’t have it.

Store user space frame-pointer value (BP register) into Perf trace on a sample for a process so the value becomes available when unwinding call stacks for functions gaining event samples.

Signed-off-by Alexey Budankov <[email protected]>

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

arch/x86/kernel/perf_regs.c +7 -1

index e47b2dbbdef3..8d68658eff7f 100644
@@ -156,7 +156,13 @@ void perf_get_regs_user(struct perf_regs *regs_user,
 	 * Most system calls don't save these registers, don't report them.
 	 */
 	regs_user_copy->bx = -1;
-	regs_user_copy->bp = -1;
+	/*
+	 * Store user space frame-pointer value on sample
+	 * to facilitate stack unwinding for cases when
+	 * user space executable code has such support
+	 * enabled at compile time;
+	 */
+	regs_user_copy->bp = user_regs->bp;
 	regs_user_copy->r12 = -1;
 	regs_user_copy->r13 = -1;
 	regs_user_copy->r14 = -1;

Quoted 2 lines

These days, system calls should save all registers, but I’m not entirely sure I want to promise that they’ll continue to do so forever.

Alexey Budankov

author

2018-05-18 07:39 UTC #17 lore raw

Hi,

Quoted 41 lines · Alexey Budankov

Hi,
Quoted 39 lines · Andy Lutomirski
Quoted 30 lines · Alexey Budankov
Hi Andy,
Quoted 24 lines · Peter Zijlstra
Quoted 16 lines · Alexey Budankov
Store user space frame-pointer value (BP register) into Perf trace on a sample for a process so the value becomes available when unwinding call stacks for functions gaining event samples.
Signed-off-by Alexey Budankov <[email protected]>
---
arch/x86/kernel/perf_regs.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
arch/x86/kernel/perf_regs.c +0 -0
index e47b2dbbdef3..8d68658eff7f 100644
arch/x86/kernel/perf_regs.c void perf_get_regs_user(struct perf_regs *regs_user, · line 158
Quoted 3 lines
arch/x86/kernel/perf_regs.c +0 -0
@@ -156,7 +156,13 @@ void perf_get_regs_user(struct perf_regs *regs_user,
 
 
    * Most system calls don't save these registers, don't report them.
^^^ that worries me and is the reason for the '-1's below. However I think with all the PTI rework this might no longer be true.
Quoted 7 lines
Quoted 5 lines
The Changelog needs to state that user_regs->bp is in fact valid and ideally point to the commits that makes it so. Also this patch should update that comment.
Cc Andy who keeps better track of all that than me.
Are there any thoughts so far? Feedback on the matter above is highly appreciated.
Sorry, I missed this. Can you forward the original patch? I don’t have it.

Just to make sure this and below didn't sneak out of your attention.

Quoted 29 lines

Store user space frame-pointer value (BP register) into Perf trace on a sample for a process so the value becomes available when unwinding call stacks for functions gaining event samples.

Signed-off-by Alexey Budankov <[email protected]>

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

arch/x86/kernel/perf_regs.c +7 -1

index e47b2dbbdef3..8d68658eff7f 100644
@@ -156,7 +156,13 @@ void perf_get_regs_user(struct perf_regs *regs_user,
 	 * Most system calls don't save these registers, don't report them.
 	 */
 	regs_user_copy->bx = -1;
-	regs_user_copy->bp = -1;
+	/*
+	 * Store user space frame-pointer value on sample
+	 * to facilitate stack unwinding for cases when
+	 * user space executable code has such support
+	 * enabled at compile time;
+	 */
+	regs_user_copy->bp = user_regs->bp;
 	regs_user_copy->r12 = -1;
 	regs_user_copy->r13 = -1;
 	regs_user_copy->r14 = -1;

Quoted 1 line

Quoted 1 line
Quoted 1 line
These days, system calls should save all registers, but I’m not entirely sure I want to promise that they’ll continue to do so forever.

Thanks,
Alexey