On 5/4/21 11:05 AM, Mark Brown wrote:

On Mon, May 03, 2021 at 12:36:13PM -0500, madvenka@linux.microsoft.com wrote:

quoted

From: "Madhavan T. Venkataraman" <redacted>

Create a sym_code_ranges[] array to cover the following text sections that
contain functions defined as SYM_CODE_*(). These functions are low-level

This makes sense to me - a few of bikesheddy comments below but nothing
really substantive.

OK.

quoted

+static struct code_range *lookup_range(unsigned long pc)

This feels like it should have a prefix on the name (eg, unwinder_)
since it looks collision prone.  Or lookup_code_range() rather than just
plain lookup_range().

I will add the prefix.

quoted

+{

+       struct code_range *range;
+         
+       for (range = sym_code_ranges; range->start; range++) {

It seems more idiomatic to use ARRAY_SIZE() rather than a sentinel here,
the array can't be empty.

If there is a match, I return the matched range. Else, I return the sentinel.
This is just so I don't have to check for range == NULL after calling
lookup_range().

I will change it to what you have suggested and check for NULL explicitly.
It is not a problem.

quoted

+	range = lookup_range(frame->pc);
+
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	if (tsk->ret_stack &&
 		frame->pc == (unsigned long)return_to_handler) {

@@ -118,9 +160,21 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 			return -EINVAL;
 		frame->pc = ret_stack->ret;
 		frame->pc = ptrauth_strip_insn_pac(frame->pc);
+		return 0;
 	}

Do we not need to look up the range of the restored pc and validate
what's being pointed to here?  It's not immediately obvious why we do
the lookup before handling the function graph tracer, especially given
that we never look at the result and there's now a return added skipping
further reliability checks.  At the very least I think this needs some
additional comments so the code is more obvious.

I want sym_code_ranges[] to contain both unwindable and non-unwindable ranges.
Unwindable ranges will be special ranges such as the return_to_handler() and
kretprobe_trampoline() functions for which the unwinder has (or will have)
special code to unwind. So, the lookup_range() has to happen before the
function graph code. Please look at the last patch in the series for
the fix for the above function graph code.

On the question of "should the original return address be checked against
sym_code_ranges[]?" - I assumed that if there is a function graph trace on a
function, it had to be an ftraceable function. It would not be a part
of sym_code_ranges[]. Is that a wrong assumption on my part?

Madhavan