On 5/5/21 11:46 AM, Mark Brown wrote:

On Tue, May 04, 2021 at 02:32:35PM -0500, Madhavan T. Venkataraman wrote:

quoted

If you prefer, I could do something like this:

check_pc:
	if (!__kernel_text_address(frame->pc))
		frame->reliable = false;

	range = lookup_range(frame->pc);

#ifdef CONFIG_FUNCTION_GRAPH_TRACER
	if (tsk->ret_stack &&
		frame->pc == (unsigned long)return_to_handler) {
		...
		frame->pc = ret_stack->ret;
		frame->pc = ptrauth_strip_insn_pac(frame->pc);
		goto check_pc;
	}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */

quoted

Is that acceptable?

I think that works even if it's hard to love the goto, might want some
defensiveness to ensure we can't somehow end up in an infinite loop with
a sufficiently badly formed stack.

I could do something like this:

- Move all frame->pc checking code into a function called check_frame_pc().

	bool	check_frame_pc(frame)
	{
		Do all the checks including function graph
		return frame->pc changed
	}

- Then, in unwind_frame()

unwind_frame()
{
	int	i;
	...

	for (i = 0; i < MAX_CHECKS; i++) {
		if (!check_frame(tsk, frame))
			break;
	}

	if (i == MAX_CHECKS)
		frame->reliable = false;
	return 0;
}

The above would take care of future cases like kretprobe_trampoline().

If this is acceptable, then the only question is - what should be the value of
MAX_CHECKS (I will rename it to something more appropriate)?

Madhavan