Re: [RFC][PATCH 4/7] objtool: Add support for intra-function calls

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Thu, Apr 16, 2020 at 05:07:56PM +0200, Peter Zijlstra wrote:
> From: Alexandre Chartre <alexandre.chartre@oracle.com>
> 
> Change objtool to support intra-function calls. On x86, an intra-function
> call is represented in objtool as a push onto the stack (of the return
> address), and a jump to the destination address. That way the stack
> information is correctly updated and the call flow is still accurate.
> 
> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: https://lkml.kernel.org/r/20200414103618.12657-4-alexandre.chartre@oracle.com
> ---
>  include/linux/frame.h                            |   11 +
>  tools/objtool/Documentation/stack-validation.txt |    8 +
>  tools/objtool/arch/x86/decode.c                  |   17 ++-
>  tools/objtool/check.c                            |  129 ++++++++++++++++++-----
>  tools/objtool/check.h                            |    1 
>  5 files changed, 140 insertions(+), 26 deletions(-)
> 
> --- a/include/linux/frame.h
> +++ b/include/linux/frame.h
> @@ -15,9 +15,20 @@
>  	static void __used __section(.discard.func_stack_frame_non_standard) \
>  		*__func_stack_frame_non_standard_##func = func
>  
> +/*
> + * This macro indicates that the following intra-function call is valid.
> + * Any non-annotated intra-function call will cause objtool to issue warning.

issue *a* warning

> +static int setup_call_dest(struct objtool_file *file, struct instruction *insn)
> +{
> +	unsigned long dest_off;
> +
> +	dest_off = insn->offset + insn->len + insn->immediate;
> +	insn->call_dest = find_func_by_offset(insn->sec, dest_off);
> +	if (!insn->call_dest)
> +		insn->call_dest = find_symbol_by_offset(insn->sec, dest_off);
> +
> +	if (!insn->call_dest) {
> +		/* intra-function call */
> +		if (insn->intra_function_call)
> +			return 0;
> +
> +		WARN_FUNC("intra-function call", insn->sec, insn->offset);
> +		return -1;
> +	}
> +
> +	/* regular call */
> +	if (insn->func && insn->call_dest->type != STT_FUNC) {
> +		WARN_FUNC("unsupported call to non-function",
> +			  insn->sec, insn->offset);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +

This function should first be added in a separate patch which doesn't
change functionality.

> @@ -2269,6 +2327,29 @@ static int validate_branch(struct objtoo
>  			if (dead_end_function(file, insn->call_dest))
>  				return 0;
>  
> +			if (insn->intra_function_call) {
> +				/*
> +				 * The call instruction can update the stack
> +				 * state. Then make the intra-function call
> +				 * behaves like and unconditional jump.

grammar fix: "behave like an unconditional jump".

> +				 */
> +				ret = handle_insn_ops(insn, &state);
> +				if (ret)
> +					return ret;

validate_branch's callers aren't currently able to handle a negative
return code.

> +
> +				ret = validate_branch(file, func, insn,
> +						      insn->jump_dest, state);
> +				if (ret) {
> +					if (backtrace) {
> +						BT_FUNC("(intra-function call)",
> +							insn);
> +					}
> +					return ret;
> +				}
> +
> +				return 0;
> +			}
> +

Could this be cleaner if the insn->type were just changed to
INSN_JUMP_UNCONDITIONAL?  Then it could share the normal jump logic.

Also, now that more instructions are getting stack ops, I wonder if the
ops can just be handled generically for every instruction, with a call
to handle_insn_ops() above, before the switch statement.

-- 
Josh