Re: [PATCH v11 4/5] arm64: Introduce stack trace reliability checks in the unwinder

[Madhavan T. Venkataraman]

On 11/25/21 8:56 AM, Mark Brown wrote:
> On Tue, Nov 23, 2021 at 01:37:22PM -0600, madvenka@xxxxxxxxxxxxxxxxxxx wrote:
> 
>> Introduce arch_stack_walk_reliable() for ARM64. This works like
>> arch_stack_walk() except that it returns -EINVAL if the stack trace is not
>> reliable.
> 
>> Until all the reliability checks are in place, arch_stack_walk_reliable()
>> may not be used by livepatch. But it may be used by debug and test code.
> 
> Probably also worth noting that this doesn't select
> HAVE_RELIABLE_STACKTRACE which is what any actual users are going to use
> to identify if the architecture has the feature.  I would have been
> tempted to add arch_stack_walk() as a separate patch but equally having
> the user code there (even if it itself can't yet be used...) helps with
> reviewing the actual unwinder so I don't mind.
> 

I did not select HAVE_RELIABLE_STACKTRACE just in case we think that some
more reliability checks need to be added. But if reviewers agree
that this patch series contains all the reliability checks we need, I
will add a patch to select HAVE_RELIABLE_STACKTRACE to the series.

>> +static void unwind_check_reliability(struct task_struct *task,
>> +				     struct stackframe *frame)
>> +{
>> +	if (frame->fp == (unsigned long)task_pt_regs(task)->stackframe) {
>> +		/* Final frame; no more unwind, no need to check reliability */
>> +		return;
>> +	}
> 
> If the unwinder carries on for some reason (the code for that is
> elsewhere and may be updated separately...) then this will start
> checking again.  I'm not sure if this is a *problem* as such but the
> thing about this being the final frame coupled with not actually
> explicitly stopping the unwind here makes me think this should at least
> be clearer, the comment begs the question about what happens if
> something decides it is not in fact the final frame.
> 

I can address this by adding an explicit comment to that effect.
For example, define a separate function to check for the final frame:

/*
 * Check if this is the final frame. Unwind must stop at the final
 * frame.
 */
static inline bool unwind_is_final_frame(struct task_struct *task,
                                         struct stackframe *frame)
{
	return frame->fp == (unsigned long)task_pt_regs(task)->stackframe;
}

Then, use this function in unwind_check_reliability() and unwind_continue().

Is this acceptable?

Madhavan