live-patching.vger.kernel.org archive mirror
 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, jthierry@redhat.com,
	linux-arm-kernel@lists.infradead.org,
	live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v1 1/1] arm64: Unwinder enhancements for reliable stack trace
Date: Mon, 1 Mar 2021 10:58:51 -0600	[thread overview]
Message-ID: <0227d386-c392-eb5a-3f52-621a637e46a8@linux.microsoft.com> (raw)
In-Reply-To: <20210225115825.GB37015@C02TD0UTHF1T.local>



On 2/25/21 5:58 AM, Mark Rutland wrote:
> On Wed, Feb 24, 2021 at 01:34:09PM -0600, Madhavan T. Venkataraman wrote:
>> On 2/24/21 6:17 AM, Mark Rutland wrote:
>>> On Tue, Feb 23, 2021 at 12:12:43PM -0600, madvenka@linux.microsoft.com wrote:
>>>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>>> 	Termination
>>>> 	===========
>>>>
>>>> 	Currently, the unwinder terminates when both the FP (frame pointer)
>>>> 	and the PC (return address) of a frame are 0. But a frame could get
>>>> 	corrupted and zeroed. There needs to be a better check.
>>>>
>>>> 	The following special terminating frame and function have been
>>>> 	defined for this purpose:
>>>>
>>>> 	const u64    arm64_last_frame[2] __attribute__ ((aligned (16)));
>>>>
>>>> 	void arm64_last_func(void)
>>>> 	{
>>>> 	}
>>>>
>>>> 	So, set the FP to arm64_last_frame and the PC to arm64_last_func in
>>>> 	the bottom most frame.
>>>
>>> My expectation was that we'd do this per-task, creating an empty frame
>>> record (i.e. with fp=NULL and lr=NULL) on the task's stack at the
>>> instant it was created, and chaining this into x29. That way the address
>>> is known (since it can be derived from the task), and the frame will
>>> also implicitly check that the callchain terminates on the task stack
>>> without loops. That also means that we can use it to detect the entry
>>> code going wrong (e.g. if the SP gets corrupted), since in that case the
>>> entry code would place the record at a different location.
>>
>> That is exactly what this is doing. arm64_last_frame[] is a marker frame
>> that contains fp=0 and pc=0.
> 
> Almost! What I meant was that rather that each task should have its own
> final/marker frame record on its task task rather than sharing a common
> global variable.
> 
> That way a check for that frame record implicitly checks that a task
> started at the expected location *on that stack*, which catches
> additional stack corruption cases (e.g. if we left data on the stack
> prior to an EL0 entry).
> 

Ok. I will think about this.

> [...]
> 
>>> ... I reckon once we've moved the last of the exception triage out to C
>>> this will be relatively simple, since all of the exception handlers will
>>> look like:
>>>
>>> | SYM_CODE_START_LOCAL(elX_exception)
>>> | 	kernel_entry X
>>> | 	mov	x0, sp
>>> | 	bl	elX_exception_handler
>>> | 	kernel_exit X
>>> | SYM_CODE_END(elX_exception)
>>>
>>> ... and so we just need to identify the set of elX_exception functions
>>> (which we'll never expect to take exceptions from directly). We could be
>>> strict and reject unwinding into arbitrary bits of the entry code (e.g.
>>> if we took an unexpected exception), and only permit unwinding to the
>>> BL.
>>>
>>>> 	It can do this if the FP and PC are also recorded elsewhere in the
>>>> 	pt_regs for comparison. Currently, the FP is also stored in
>>>> 	regs->regs[29]. The PC is stored in regs->pc. However, regs->pc can
>>>> 	be changed by lower level functions.
>>>>
>>>> 	Create a new field, pt_regs->orig_pc, and record the return address
>>>> 	PC there. With this, the unwinder can validate the exception frame
>>>> 	and set a flag so that the caller of the unwinder can know when
>>>> 	an exception frame is encountered.
>>>
>>> I don't understand the case you're trying to solve here. When is
>>> regs->pc changed in a way that's problematic?
>>>
>>
>> For instance, I used a test driver in which the driver calls a function
>> pointer which is NULL. The low level fault handler sends a signal to the
>> task. Looks like it changes regs->pc for this.
> 
> I'm struggling to follow what you mean here.
> 
> If the kernel branches to NULL, the CPU should raise an instruction
> abort from the current EL, and our handling of that should terminate the
> thread via die_kernel_fault(), without returning to the faulting
> context, and without altering the PC in the faulting context.
> 
> Signals are delivered to userspace and alter the userspace PC, not a
> kernel PC, so this doesn't seem relevant. Do you mean an exception fixup
> handler rather than a signal?
> 
>> When I dump the stack from the low level handler, the comparison with
>> regs->pc does not work.  But comparison with regs->orig_pc works.
> 
> As above, I'm struggling with this; could you share a concrete example? 
> 

Actually, my bad. I needed the orig_pc because of something that my test
driver did. And, it slipped my mind entirely.

Thanks for pointing it out. I will fix this in my resend.

Thanks again for your comments.

I am currently studying probing/tracing. As soon as I have a solution for that,
I will send out the next version. I look forward to the in-depth review.

Thanks,

Madhavan

      reply	other threads:[~2021-03-01 17:01 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <bc4761a47ad08ab7fdd555fc8094beb8fc758d33>
2021-02-23 18:12 ` [RFC PATCH v1 0/1] " madvenka
2021-02-23 18:12   ` [RFC PATCH v1 1/1] " madvenka
2021-02-23 19:02     ` Mark Brown
2021-02-23 19:20       ` Madhavan T. Venkataraman
2021-02-24 12:33         ` Mark Brown
2021-02-24 19:26           ` Madhavan T. Venkataraman
2021-02-24 12:17     ` Mark Rutland
2021-02-24 19:34       ` Madhavan T. Venkataraman
2021-02-25 11:58         ` Mark Rutland
2021-03-01 16:58           ` Madhavan T. Venkataraman [this message]

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=0227d386-c392-eb5a-3f52-621a637e46a8@linux.microsoft.com \
    --to=madvenka@linux.microsoft.com \
    --cc=broonie@kernel.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 \
    --subject='Re: [RFC PATCH v1 1/1] arm64: Unwinder enhancements for reliable stack trace' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).