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

From: Miroslav Benes
Date: Thu Apr 23 2020 - 10:34:26 EST


> /*
> * Find the destination instructions for all calls.
> */
> @@ -715,10 +725,7 @@ static int add_call_destinations(struct
> continue;
>
> if (!insn->call_dest) {
> - WARN_FUNC("unsupported intra-function call",
> - insn->sec, insn->offset);
> - if (retpoline)
> - WARN("If this is a retpoline, please patch it in with alternatives and annotate it with ANNOTATE_NOSPEC_ALTERNATIVE.");
> + WARN_FUNC("intra-function call", insn->sec, insn->offset);

"unsupported intra-function call"?

> return -1;
> }
>
> @@ -741,6 +748,12 @@ static int add_call_destinations(struct
> }
> } else
> insn->call_dest = rela->sym;
> +
> + /*
> + * Whatever stack impact regular CALLs have, should be
> + * undone by the RETURN of the called function.

* Annotated intra-function CALLs are treated as JMPs with a stack_op.
* See read_intra_function_calls().

would make it a bit clearer.

> + */
> + remove_insn_ops(insn);
> }
>
> return 0;
> @@ -1416,6 +1429,57 @@ static int read_instr_hints(struct objto
> return 0;
> }
>
> +static int read_intra_function_calls(struct objtool_file *file)
> +{
> + struct instruction *insn;
> + struct section *sec;
> + struct rela *rela;
> +
> + sec = find_section_by_name(file->elf, ".rela.discard.intra_function_calls");
> + if (!sec)
> + return 0;
> +
> + list_for_each_entry(rela, &sec->rela_list, list) {
> + unsigned long dest_off;
> +
> + if (rela->sym->type != STT_SECTION) {
> + WARN("unexpected relocation symbol type in %s",
> + sec->name);
> + return -1;
> + }
> +
> + insn = find_insn(file, rela->sym->sec, rela->addend);
> + if (!insn) {
> + WARN("bad .discard.intra_function_call entry");
> + return -1;
> + }
> +
> + if (insn->type != INSN_CALL) {
> + WARN_FUNC("intra_function_call not a direct call",
> + insn->sec, insn->offset);
> + return -1;
> + }
> +
> + /*
> + * Treat intra-function CALLs as JMPs, but with a stack_op.
> + * Also see how setup_call_dest() strips stack_ops from normal
> + * CALLs.

/*
* Treat annotated intra-function CALLs as JMPs, but with a stack_op.
* Also see how add_call_destinations() strips stack_ops from normal
* CALLs.
*/

? (note added "annotated" and s/setup_call_dest/add_call_destinations/)

> + */
> + insn->type = INSN_JUMP_UNCONDITIONAL;

[...]

> @@ -2245,6 +2313,9 @@ static int validate_branch(struct objtoo
> return 0;
> }
>
> + if (handle_insn_ops(insn, &state))
> + return 1;
> +
> switch (insn->type) {
>
> case INSN_RETURN:
> @@ -2304,9 +2375,6 @@ static int validate_branch(struct objtoo
> break;
>
> case INSN_EXCEPTION_RETURN:
> - if (handle_insn_ops(insn, &state))
> - return 1;
> -
> /*
> * This handles x86's sync_core() case, where we use an
> * IRET to self. All 'normal' IRET instructions are in
> @@ -2326,8 +2394,6 @@ static int validate_branch(struct objtoo
> return 0;
>
> case INSN_STACK:
> - if (handle_insn_ops(insn, &state))
> - return 1;
> break;

So we could get rid of INSN_STACK now as Julien proposed, couldn't we? If
I am not missing something. handle_insn_ops() is called unconditionally
here for all insn types and you remove stack_ops when unneeded.

We could also go ahead with Julien's proposal to remove
INSN_EXCEPTION_RETURN hack and move it to tools/objtool/arch/x86/decode.c.

Miroslav