live-patching.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
To: Mark Brown <broonie@kernel.org>
Cc: mark.rutland@arm.com, 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: Tue, 23 Feb 2021 13:20:49 -0600	[thread overview]
Message-ID: <08e8e02c-8ef0-26bb-1d0d-7dda54b5fefd@linux.microsoft.com> (raw)
In-Reply-To: <20210223190240.GK5116@sirena.org.uk>



On 2/23/21 1:02 PM, Mark Brown wrote:
> On Tue, Feb 23, 2021 at 12:12:43PM -0600, madvenka@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>
>> Unwinder changes
>> ================
> 
> This is making several different changes so should be split into a patch
> series - for example the change to terminate on a specific function
> pointer rather than NULL and the changes to the exception/interupt
> detection should be split.  Please see submitting-patches.rst for some
> discussion about how to split things up.  In general if you've got a
> changelog enumerating a number of different changes in a patch that's a
> warning sign that it might be good split things up.
> 

Will do.

> You should also copy the architecture maintainers (Catalin and Will) on
> any arch/arm64 submissions.
> 

Will do when I resubmit.

>> 	Unwinder return value
>> 	=====================
>>
>> 	Currently, the unwinder returns -EINVAL for stack trace termination
>> 	as well as stack trace error. Return -ENOENT for stack trace
>> 	termination and -EINVAL for error to disambiguate. This idea has
>> 	been borrowed from Mark Brown.
> 
> You could just include my patch for this in your series.
> 

OK.

>> Reliable stack trace function
>> =============================
>>
>> Implement arch_stack_walk_reliable(). This function walks the stack like
>> the existing stack trace functions with a couple of additional checks:
>>
>> 	Return address check
>> 	--------------------
>>
>> 	For each frame, check the return address to see if it is a
>> 	proper kernel text address. If not, return -EINVAL.
>>
>> 	Exception frame check
>> 	---------------------
>>
>> 	Check each frame to see if it is an EL1 exception frame. If it is,
>> 	return -EINVAL.
> 
> Again, this should be at least one separate patch.  How does this ensure
> that we don't have any issues with any of the various probe mechanisms?
> If there's no need to explicitly check anything that should be called
> out in the changelog.
> 

I am trying to do this in an incremental fashion. I have to study the probe
mechanisms a little bit more before I can come up with a solution. But
if you want to see that addressed in this patch set, I could do that.
It will take a little bit of time. That is all.

> Since all these changes are mixed up this is a fairly superficial
> review of the actual code.
> 

Understood. I will split things up and we can take it from there.

>> +static notrace struct pt_regs *get_frame_regs(struct task_struct *task,
>> +					      struct stackframe *frame)
>> +{
>> +	unsigned long stackframe, regs_start, regs_end;
>> +	struct stack_info info;
>> +
>> +	stackframe = frame->prev_fp;
>> +	if (!stackframe)
>> +		return NULL;
>> +
>> +	(void) on_accessible_stack(task, stackframe, &info);
> 
> Shouldn't we return NULL if we are not on an accessible stack?
> 

The prev_fp has already been checked by the unwinder in the previous
frame. That is why I don't check the return value. If that is acceptable,
I will add a comment.

>> +static notrace int update_frame(struct task_struct *task,
>> +				struct stackframe *frame)
> 
> This function really needs some documentation, the function is just
> called update_frame() which doesn't say what sort of updates it's
> supposed to do and most of the checks aren't explained, not all of them
> are super obvious.
> 

I will add the documentation as well as try think of a better name.

>> +{
>> +	unsigned long lsb = frame->fp & 0xf;
>> +	unsigned long fp = frame->fp & ~lsb;
>> +	unsigned long pc = frame->pc;
>> +	struct pt_regs *regs;
>> +
>> +	frame->exception_frame = false;
>> +
>> +	if (fp == (unsigned long) arm64_last_frame &&
>> +	    pc == (unsigned long) arm64_last_func)
>> +		return -ENOENT;
>> +
>> +	if (!lsb)
>> +		return 0;
>> +	if (lsb != 1)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * This looks like an EL1 exception frame.
> 
> For clarity it would be good to spell out the properties of an EL1
> exception frame.  It is not clear to me why we don't reference the frame
> type information the unwinder already records as part of these checks.
> 
> In general, especially for the bits specific to reliable stack trace, I
> think we want to err on the side of verbosity here so that it is crystal
> clear what all the checks are supposed to be doing and it's that much
> easier to tie everything through to the requirements document.

OK. I will improve the documentation.

Madhavan

  reply	other threads:[~2021-02-23 19:23 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 [this message]
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

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=08e8e02c-8ef0-26bb-1d0d-7dda54b5fefd@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).