All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: broonie@kernel.org, jpoimboe@redhat.com, ardb@kernel.org,
	nobuta.keiya@fujitsu.com, sjitindarsingh@gmail.com,
	catalin.marinas@arm.com, will@kernel.org, jmorris@namei.org,
	pasha.tatashin@soleen.com, jthierry@redhat.com,
	linux-arm-kernel@lists.infradead.org,
	live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v8 1/4] arm64: Make all stack walking functions use arch_stack_walk()
Date: Tue, 24 Aug 2021 12:38:25 -0500	[thread overview]
Message-ID: <66d0ff83-bf67-5576-4c74-10f825855091@linux.microsoft.com> (raw)
In-Reply-To: <da2bb980-09c3-5a39-73cd-ca4de4c38d51@linux.microsoft.com>



>>> diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
>>> index a6d18755652f..92a0f4d434e4 100644
>>> --- a/arch/arm64/kernel/return_address.c
>>> +++ b/arch/arm64/kernel/return_address.c
>>> @@ -35,15 +35,11 @@ NOKPROBE_SYMBOL(save_return_addr);
>>>  void *return_address(unsigned int level)
>>>  {
>>>  	struct return_address_data data;
>>> -	struct stackframe frame;
>>>  
>>>  	data.level = level + 2;
>>>  	data.addr = NULL;
>>>  
>>> -	start_backtrace(&frame,
>>> -			(unsigned long)__builtin_frame_address(0),
>>> -			(unsigned long)return_address);
>>> -	walk_stackframe(current, &frame, save_return_addr, &data);
>>> +	arch_stack_walk(save_return_addr, &data, current, NULL);
>>>  
>>>  	if (!data.level)
>>>  		return data.addr;
>>
>> Nor that arch_stack_walk() will start with it's caller, so
>> return_address() will be included in the trace where it wasn't
>> previously, which implies we need to skip an additional level.
>>
> 
> You are correct. I will fix this. Thanks for catching this.
> 
>> That said, I'm not entirely sure why we need to skip 2 levels today; it
>> might be worth checking that's correct.
>>
> 
> AFAICT, return_address() acts like builtin_return_address(). That is, it
> returns the address of the caller. If func() calls return_address(),
> func() wants its caller's address. So, return_address() and func() need to
> be skipped.
> 
> I will change it to skip 3 levels instead of 2.
> 

Actually, I take that back. I remember now. return_address() used to start
with PC=return_address(). That is, it used to start with itself. arch_stack_walk()
starts with its caller which, in this case, is return_address(). So, I don't need
to change anything.

Do you agree?

Madhavan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: broonie@kernel.org, jpoimboe@redhat.com, ardb@kernel.org,
	nobuta.keiya@fujitsu.com, sjitindarsingh@gmail.com,
	catalin.marinas@arm.com, will@kernel.org, jmorris@namei.org,
	pasha.tatashin@soleen.com, jthierry@redhat.com,
	linux-arm-kernel@lists.infradead.org,
	live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v8 1/4] arm64: Make all stack walking functions use arch_stack_walk()
Date: Tue, 24 Aug 2021 12:38:25 -0500	[thread overview]
Message-ID: <66d0ff83-bf67-5576-4c74-10f825855091@linux.microsoft.com> (raw)
In-Reply-To: <da2bb980-09c3-5a39-73cd-ca4de4c38d51@linux.microsoft.com>



>>> diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
>>> index a6d18755652f..92a0f4d434e4 100644
>>> --- a/arch/arm64/kernel/return_address.c
>>> +++ b/arch/arm64/kernel/return_address.c
>>> @@ -35,15 +35,11 @@ NOKPROBE_SYMBOL(save_return_addr);
>>>  void *return_address(unsigned int level)
>>>  {
>>>  	struct return_address_data data;
>>> -	struct stackframe frame;
>>>  
>>>  	data.level = level + 2;
>>>  	data.addr = NULL;
>>>  
>>> -	start_backtrace(&frame,
>>> -			(unsigned long)__builtin_frame_address(0),
>>> -			(unsigned long)return_address);
>>> -	walk_stackframe(current, &frame, save_return_addr, &data);
>>> +	arch_stack_walk(save_return_addr, &data, current, NULL);
>>>  
>>>  	if (!data.level)
>>>  		return data.addr;
>>
>> Nor that arch_stack_walk() will start with it's caller, so
>> return_address() will be included in the trace where it wasn't
>> previously, which implies we need to skip an additional level.
>>
> 
> You are correct. I will fix this. Thanks for catching this.
> 
>> That said, I'm not entirely sure why we need to skip 2 levels today; it
>> might be worth checking that's correct.
>>
> 
> AFAICT, return_address() acts like builtin_return_address(). That is, it
> returns the address of the caller. If func() calls return_address(),
> func() wants its caller's address. So, return_address() and func() need to
> be skipped.
> 
> I will change it to skip 3 levels instead of 2.
> 

Actually, I take that back. I remember now. return_address() used to start
with PC=return_address(). That is, it used to start with itself. arch_stack_walk()
starts with its caller which, in this case, is return_address(). So, I don't need
to change anything.

Do you agree?

Madhavan

  reply	other threads:[~2021-08-24 17:39 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <b45aac2843f16ca759e065ea547ab0afff8c0f01>
2021-08-12 19:05 ` [RFC PATCH v8 0/4] arm64: Reorganize the unwinder and implement stack trace reliability checks madvenka
2021-08-12 19:05   ` madvenka
2021-08-12 19:06   ` [RFC PATCH v8 1/4] arm64: Make all stack walking functions use arch_stack_walk() madvenka
2021-08-12 19:06     ` madvenka
2021-08-24 13:13     ` Mark Rutland
2021-08-24 13:13       ` Mark Rutland
2021-08-24 17:21       ` Madhavan T. Venkataraman
2021-08-24 17:21         ` Madhavan T. Venkataraman
2021-08-24 17:38         ` Madhavan T. Venkataraman [this message]
2021-08-24 17:38           ` Madhavan T. Venkataraman
2021-08-24 17:38         ` Mark Brown
2021-08-24 17:38           ` Mark Brown
2021-08-24 17:40           ` Madhavan T. Venkataraman
2021-08-24 17:40             ` Madhavan T. Venkataraman
2021-08-26  4:52       ` Madhavan T. Venkataraman
2021-08-26  4:52         ` Madhavan T. Venkataraman
2021-10-09 23:41       ` Madhavan T. Venkataraman
2021-10-09 23:41         ` Madhavan T. Venkataraman
2021-08-12 19:06   ` [RFC PATCH v8 2/4] arm64: Reorganize the unwinder code for better consistency and maintenance madvenka
2021-08-12 19:06     ` madvenka
2021-08-26 15:46     ` Mark Brown
2021-08-26 15:46       ` Mark Brown
2021-08-26 23:19       ` Madhavan T. Venkataraman
2021-08-26 23:19         ` Madhavan T. Venkataraman
2021-09-01 16:20         ` Mark Brown
2021-09-01 16:20           ` Mark Brown
2021-09-02  7:10           ` Madhavan T. Venkataraman
2021-09-02  7:10             ` Madhavan T. Venkataraman
2021-08-12 19:06   ` [RFC PATCH v8 3/4] arm64: Introduce stack trace reliability checks in the unwinder madvenka
2021-08-12 19:06     ` madvenka
2021-08-24  5:55     ` nobuta.keiya
2021-08-24  5:55       ` nobuta.keiya
2021-08-24 12:19       ` Madhavan T. Venkataraman
2021-08-24 12:19         ` Madhavan T. Venkataraman
2021-08-25  0:01         ` nobuta.keiya
2021-08-25  0:01           ` nobuta.keiya
2021-08-26 15:57     ` Mark Brown
2021-08-26 15:57       ` Mark Brown
2021-08-26 23:31       ` Madhavan T. Venkataraman
2021-08-26 23:31         ` Madhavan T. Venkataraman
2021-08-12 19:06   ` [RFC PATCH v8 4/4] arm64: Create a list of SYM_CODE functions, check return PC against list madvenka
2021-08-12 19:06     ` madvenka
2021-08-12 19:17   ` [RFC PATCH v8 0/4] arm64: Reorganize the unwinder and implement stack trace reliability checks Madhavan T. Venkataraman
2021-08-12 19:17     ` Madhavan T. Venkataraman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=66d0ff83-bf67-5576-4c74-10f825855091@linux.microsoft.com \
    --to=madvenka@linux.microsoft.com \
    --cc=ardb@kernel.org \
    --cc=broonie@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=jmorris@namei.org \
    --cc=jpoimboe@redhat.com \
    --cc=jthierry@redhat.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=nobuta.keiya@fujitsu.com \
    --cc=pasha.tatashin@soleen.com \
    --cc=sjitindarsingh@gmail.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.