[RFC,v2,3/8] arm64: Terminate the stack trace at TASK_FRAME and EL0_FRAME

Commit Message

Madhavan T. Venkataraman March 15, 2021, 4:57 p.m. UTC

From: "Madhavan T. Venkataraman" <[e-mail]>

Implement the following checks in the unwinder to detect the terminating
frame reliably:

	- The frame must end in task_pt_regs(task)->stackframe.

	- The frame type must be either TASK_FRAME or EL0_FRAME.

Signed-off-by: Madhavan T. Venkataraman <[e-mail]>
---
 arch/arm64/kernel/stacktrace.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Mark Brown March 18, 2021, 6:26 p.m. UTC | #1

On Mon, Mar 15, 2021 at 11:57:55AM -0500, [e-mail] wrote:

> +	/* Terminal record, nothing to unwind */
> +	if (fp == (unsigned long) regs->stackframe) {
> +		if (regs->frame_type == TASK_FRAME ||
> +		    regs->frame_type == EL0_FRAME)
> +			return -ENOENT;
>  		return -EINVAL;
> +	}

This is conflating the reliable stacktrace checks (which your series
will later flag up with frame->reliable) with verifying that we found
the bottom of the stack by looking for this terminal stack frame record.
For the purposes of determining if the unwinder got to the bottom of the
stack we don't care what stack type we're looking at, we just care if it
managed to walk to this defined final record.  

At the minute nothing except reliable stack trace has any intention of
checking the specific return code but it's clearer to be consistent.

Madhavan T. Venkataraman March 18, 2021, 8:29 p.m. UTC | #2

On 3/18/21 1:26 PM, Mark Brown wrote:
> On Mon, Mar 15, 2021 at 11:57:55AM -0500, [e-mail] wrote:
> 
>> +	/* Terminal record, nothing to unwind */
>> +	if (fp == (unsigned long) regs->stackframe) {
>> +		if (regs->frame_type == TASK_FRAME ||
>> +		    regs->frame_type == EL0_FRAME)
>> +			return -ENOENT;
>>  		return -EINVAL;
>> +	}
> 
> This is conflating the reliable stacktrace checks (which your series
> will later flag up with frame->reliable) with verifying that we found
> the bottom of the stack by looking for this terminal stack frame record.
> For the purposes of determining if the unwinder got to the bottom of the
> stack we don't care what stack type we're looking at, we just care if it
> managed to walk to this defined final record.  
> 
> At the minute nothing except reliable stack trace has any intention of
> checking the specific return code but it's clearer to be consistent.
> 

So, you are saying that the type check is redundant. OK. I will remove it
and just return -ENOENT on reaching the final record.

Madhavan

Mark Rutland March 23, 2021, 10:36 a.m. UTC | #3

On Thu, Mar 18, 2021 at 03:29:19PM -0500, Madhavan T. Venkataraman wrote:
> 
> 
> On 3/18/21 1:26 PM, Mark Brown wrote:
> > On Mon, Mar 15, 2021 at 11:57:55AM -0500, [e-mail] wrote:
> > 
> >> +	/* Terminal record, nothing to unwind */
> >> +	if (fp == (unsigned long) regs->stackframe) {
> >> +		if (regs->frame_type == TASK_FRAME ||
> >> +		    regs->frame_type == EL0_FRAME)
> >> +			return -ENOENT;
> >>  		return -EINVAL;
> >> +	}
> > 
> > This is conflating the reliable stacktrace checks (which your series
> > will later flag up with frame->reliable) with verifying that we found
> > the bottom of the stack by looking for this terminal stack frame record.
> > For the purposes of determining if the unwinder got to the bottom of the
> > stack we don't care what stack type we're looking at, we just care if it
> > managed to walk to this defined final record.  
> > 
> > At the minute nothing except reliable stack trace has any intention of
> > checking the specific return code but it's clearer to be consistent.
> > 
> 
> So, you are saying that the type check is redundant. OK. I will remove it
> and just return -ENOENT on reaching the final record.

Yes please; and please fold that into the same patch that adds the final
records.

Thanks,
Mark.

Madhavan T. Venkataraman March 23, 2021, 12:40 p.m. UTC | #4

On 3/23/21 5:36 AM, Mark Rutland wrote:
> On Thu, Mar 18, 2021 at 03:29:19PM -0500, Madhavan T. Venkataraman wrote:
>>
>>
>> On 3/18/21 1:26 PM, Mark Brown wrote:
>>> On Mon, Mar 15, 2021 at 11:57:55AM -0500, [e-mail] wrote:
>>>
>>>> +	/* Terminal record, nothing to unwind */
>>>> +	if (fp == (unsigned long) regs->stackframe) {
>>>> +		if (regs->frame_type == TASK_FRAME ||
>>>> +		    regs->frame_type == EL0_FRAME)
>>>> +			return -ENOENT;
>>>>  		return -EINVAL;
>>>> +	}
>>>
>>> This is conflating the reliable stacktrace checks (which your series
>>> will later flag up with frame->reliable) with verifying that we found
>>> the bottom of the stack by looking for this terminal stack frame record.
>>> For the purposes of determining if the unwinder got to the bottom of the
>>> stack we don't care what stack type we're looking at, we just care if it
>>> managed to walk to this defined final record.  
>>>
>>> At the minute nothing except reliable stack trace has any intention of
>>> checking the specific return code but it's clearer to be consistent.
>>>
>>
>> So, you are saying that the type check is redundant. OK. I will remove it
>> and just return -ENOENT on reaching the final record.
> 
> Yes please; and please fold that into the same patch that adds the final
> records.
> 

Will do.

Thanks.

Madhavan

Patch

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index ad20981dfda4..504cd161339d 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -43,16 +43,22 @@  int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 {
 	unsigned long fp = frame->fp;
 	struct stack_info info;
+	struct pt_regs *regs;
 
-	/* Terminal record; nothing to unwind */
-	if (!fp)
-		return -ENOENT;
+	if (!tsk)
+		tsk = current;
+	regs = task_pt_regs(tsk);
 
-	if (fp & 0xf)
+	/* Terminal record, nothing to unwind */
+	if (fp == (unsigned long) regs->stackframe) {
+		if (regs->frame_type == TASK_FRAME ||
+		    regs->frame_type == EL0_FRAME)
+			return -ENOENT;
 		return -EINVAL;
+	}
 
-	if (!tsk)
-		tsk = current;
+	if (!fp || fp & 0xf)
+		return -EINVAL;
 
 	if (!on_accessible_stack(tsk, fp, &info))
 		return -EINVAL;