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

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

[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 <alexey.budankov@linux.intel.com>
---
 arch/x86/kernel/perf_regs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
index e47b2dbbdef3..8d68658eff7f 100644
--- a/arch/x86/kernel/perf_regs.c
+++ b/arch/x86/kernel/perf_regs.c
@@ -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;

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

[Peter Zijlstra]

On Tue, May 08, 2018 at 06:21:36PM +0300, Alexey Budankov wrote:
> 
> 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 <alexey.budankov@linux.intel.com>
> ---
>  arch/x86/kernel/perf_regs.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
> index e47b2dbbdef3..8d68658eff7f 100644
> --- a/arch/x86/kernel/perf_regs.c
> +++ b/arch/x86/kernel/perf_regs.c
> @@ -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.

>  	 */
>  	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;

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

[Alexey Budankov]

Hi,

On 09.05.2018 17:54, Peter Zijlstra wrote:
> On Tue, May 08, 2018 at 06:21:36PM +0300, Alexey Budankov wrote:
>>
>> 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 <alexey.budankov@linux.intel.com>
>> ---
>>  arch/x86/kernel/perf_regs.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
>> index e47b2dbbdef3..8d68658eff7f 100644
>> --- a/arch/x86/kernel/perf_regs.c
>> +++ b/arch/x86/kernel/perf_regs.c
>> @@ -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.

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. 

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


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

Accepted.

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

Yes, any comments and feedback would be very welcome.

Thanks,
Alexey

> 
>>  	 */
>>  	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;
> 

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

[Peter Zijlstra]

On Thu, May 10, 2018 at 12:42:38PM +0300, Alexey Budankov wrote:
> > 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.

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

[Alexey Budankov]

Hi,

On 10.05.2018 13:14, Peter Zijlstra wrote:
> On Thu, May 10, 2018 at 12:42:38PM +0300, Alexey Budankov wrote:
>>> 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.

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

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

[Alexey Budankov]

Hi Andy,

On 09.05.2018 17:54, Peter Zijlstra wrote:
> On Tue, May 08, 2018 at 06:21:36PM +0300, Alexey Budankov wrote:
>>
>> 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 <alexey.budankov@linux.intel.com>
>> ---
>>  arch/x86/kernel/perf_regs.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
>> index e47b2dbbdef3..8d68658eff7f 100644
>> --- a/arch/x86/kernel/perf_regs.c
>> +++ b/arch/x86/kernel/perf_regs.c
>> @@ -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.

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

Thanks,
Alexey

> 
>>  	 */
>>  	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;
> 

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

[Andy Lutomirski]

> On May 15, 2018, at 1:08 AM, Alexey Budankov <alexey.budankov@linux.intel.com> wrote:
> 
> 
> Hi Andy,
> 
>> On 09.05.2018 17:54, Peter Zijlstra wrote:
>>> On Tue, May 08, 2018 at 06:21:36PM +0300, Alexey Budankov wrote:
>>> 
>>> 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 <alexey.budankov@linux.intel.com>
>>> ---
>>> arch/x86/kernel/perf_regs.c | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
>>> index e47b2dbbdef3..8d68658eff7f 100644
>>> --- a/arch/x86/kernel/perf_regs.c
>>> +++ b/arch/x86/kernel/perf_regs.c
>>> @@ -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.
> 
> 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.

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

[Alexey Budankov]

Hi,
On 15.05.2018 19:30, Andy Lutomirski wrote:
> 
>> On May 15, 2018, at 1:08 AM, Alexey Budankov <alexey.budankov@linux.intel.com> wrote:
>>
>>
>> Hi Andy,
>>
>>> On 09.05.2018 17:54, Peter Zijlstra wrote:
>>>> On Tue, May 08, 2018 at 06:21:36PM +0300, Alexey Budankov wrote:
>>>>
>>>> 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 <alexey.budankov@linux.intel.com>
>>>> ---
>>>> arch/x86/kernel/perf_regs.c | 8 +++++++-
>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
>>>> index e47b2dbbdef3..8d68658eff7f 100644
>>>> --- a/arch/x86/kernel/perf_regs.c
>>>> +++ b/arch/x86/kernel/perf_regs.c
>>>> @@ -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.
>>
>> 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 <alexey.budankov@linux.intel.com>
---
 arch/x86/kernel/perf_regs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
index e47b2dbbdef3..8d68658eff7f 100644
--- a/arch/x86/kernel/perf_regs.c
+++ b/arch/x86/kernel/perf_regs.c
@@ -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;

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

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

[Alexey Budankov]

Hi,
On 16.05.2018 11:42, Alexey Budankov wrote:
> Hi,
> On 15.05.2018 19:30, Andy Lutomirski wrote:
>>
>>> On May 15, 2018, at 1:08 AM, Alexey Budankov <alexey.budankov@linux.intel.com> wrote:
>>>
>>>
>>> Hi Andy,
>>>
>>>> On 09.05.2018 17:54, Peter Zijlstra wrote:
>>>>> On Tue, May 08, 2018 at 06:21:36PM +0300, Alexey Budankov wrote:
>>>>>
>>>>> 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 <alexey.budankov@linux.intel.com>
>>>>> ---
>>>>> arch/x86/kernel/perf_regs.c | 8 +++++++-
>>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
>>>>> index e47b2dbbdef3..8d68658eff7f 100644
>>>>> --- a/arch/x86/kernel/perf_regs.c
>>>>> +++ b/arch/x86/kernel/perf_regs.c
>>>>> @@ -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.
>>>
>>> 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.

> 
> 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 <alexey.budankov@linux.intel.com>
> ---
>  arch/x86/kernel/perf_regs.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
> index e47b2dbbdef3..8d68658eff7f 100644
> --- a/arch/x86/kernel/perf_regs.c
> +++ b/arch/x86/kernel/perf_regs.c
> @@ -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;
>>> 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

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

[Alexey Budankov]

Hi Peter,

On 10.05.2018 13:14, Peter Zijlstra wrote:
> On Thu, May 10, 2018 at 12:42:38PM +0300, Alexey Budankov wrote:
>>> 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.

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

> 

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

[Andy Lutomirski]

> On May 21, 2018, at 5:44 AM, Alexey Budankov <alexey.budankov@linux.intel.com> wrote:
> 
> Hi Peter,
> 
>> On 10.05.2018 13:14, Peter Zijlstra wrote:
>> On Thu, May 10, 2018 at 12:42:38PM +0300, Alexey Budankov wrote:
>>>> 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.
> 
> 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.

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

[Alexey Budankov]

Hi Andy,
On 21.05.2018 17:14, Andy Lutomirski wrote:
> 
>> On May 21, 2018, at 5:44 AM, Alexey Budankov <alexey.budankov@linux.intel.com> wrote:
>>
>> Hi Peter,
>>
>>> On 10.05.2018 13:14, Peter Zijlstra wrote:
>>> On Thu, May 10, 2018 at 12:42:38PM +0300, Alexey Budankov wrote:
>>>>> 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.
>>
>> 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

> 

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

[Andy Lutomirski]

> On May 21, 2018, at 9:51 AM, Alexey Budankov <alexey.budankov@linux.intel.com> wrote:
> 
> 
> Hi Andy,
>> On 21.05.2018 17:14, Andy Lutomirski wrote:
>> 
>>> On May 21, 2018, at 5:44 AM, Alexey Budankov <alexey.budankov@linux.intel.com> wrote:
>>> 
>>> Hi Peter,
>>> 
>>>> On 10.05.2018 13:14, Peter Zijlstra wrote:
>>>> On Thu, May 10, 2018 at 12:42:38PM +0300, Alexey Budankov wrote:
>>>>>> 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.
>>> 
>>> 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.

Write a selftest?

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

> 
> Thanks,
> Alexey
> 
>> 

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

[Alexey Budankov]

Hi,

On 21.05.2018 20:23, Andy Lutomirski wrote:
> 
>> On May 21, 2018, at 9:51 AM, Alexey Budankov <alexey.budankov@linux.intel.com> wrote:
>>
>>
>> Hi Andy,
>>> On 21.05.2018 17:14, Andy Lutomirski wrote:
>>>
>>>> On May 21, 2018, at 5:44 AM, Alexey Budankov <alexey.budankov@linux.intel.com> wrote:
>>>>
>>>> Hi Peter,
>>>>
>>>>> On 10.05.2018 13:14, Peter Zijlstra wrote:
>>>>> On Thu, May 10, 2018 at 12:42:38PM +0300, Alexey Budankov wrote:
>>>>>>> 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.
>>>>
>>>> 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.
> 
> Write a selftest?

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

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

> 
>>
>> Thanks,
>> Alexey
>>
>>>
> 

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

[Alexey Budankov]

Hi Peter,

On 10.05.2018 13:14, Peter Zijlstra wrote:
> On Thu, May 10, 2018 at 12:42:38PM +0300, Alexey Budankov wrote:
>>> 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.
> 

It looks like Andy confirmed on the questions above.

Is the patch ready to be up streamed now? 

Thanks,
Alexey

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

[Peter Zijlstra]

On Wed, May 23, 2018 at 01:06:58PM +0300, Alexey Budankov wrote:

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

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

[Alexey Budankov]

Hi,

On 23.05.2018 16:09, Peter Zijlstra wrote:
> On Wed, May 23, 2018 at 01:06:58PM +0300, Alexey Budankov wrote:
> 
>> 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.

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

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

[Andy Lutomirski]

On Thu, May 24, 2018 at 7:37 AM Alexey Budankov <
alexey.budankov@linux.intel.com> wrote:

> Hi,

> On 23.05.2018 16:09, Peter Zijlstra wrote:
> > On Wed, May 23, 2018 at 01:06:58PM +0300, Alexey Budankov wrote:
> >
> >> 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.

> >
> > 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;
>           ...
>           */


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

> Thanks,
> Alexey
> >
> >