Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 18 Jan 2018 11:09:31 -0800
From:   Guenter Roeck <linux@roeck-us.net>
To:     "Woodhouse, David" <dwmw@amazon.co.uk>
Cc:     Andi Kleen <ak@linux.intel.com>, Paul Turner <pjt@google.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Greg Kroah-Hartman <gregkh@linux-foundation.org>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        Dave Hansen <dave.hansen@intel.com>, tglx@linutronix.de,
        Kees Cook <keescook@google.com>,
        Rik van Riel <riel@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Andy Lutomirski <luto@amacapital.net>,
        Jiri Kosina <jikos@kernel.org>, gnomes@lxorguk.ukuu.org.uk,
        x86@kernel.org, thomas.lendacky@amd.com,
        Josh Poimboeuf <jpoimboe@redhat.com>
Subject: Re: [v8,02/12] objtool: Allow alternatives to be ignored
Message-ID: <20180118190931.GA27143@roeck-us.net>
References: <1515707194-20531-3-git-send-email-dwmw@amazon.co.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1515707194-20531-3-git-send-email-dwmw@amazon.co.uk>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi folks,

On Thu, Jan 11, 2018 at 09:46:24PM +0000, Woodhouse, David wrote:
> Getting objtool to understand retpolines is going to be a bit of a
> challenge.  For now, take advantage of the fact that retpolines are
> patched in with alternatives.  Just read the original (sane)
> non-alternative instruction, and ignore the patched-in retpoline.
> 
> This allows objtool to understand the control flow *around* the
> retpoline, even if it can't yet follow what's inside.  This means the
> ORC unwinder will fail to unwind from inside a retpoline, but will work
> fine otherwise.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  tools/objtool/check.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  tools/objtool/check.h |  2 +-
>  2 files changed, 57 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index de053fb..f40d46e 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -428,6 +428,40 @@ static void add_ignores(struct objtool_file *file)
>  }
>  
>  /*
> + * FIXME: For now, just ignore any alternatives which add retpolines.  This is
> + * a temporary hack, as it doesn't allow ORC to unwind from inside a retpoline.
> + * But it at least allows objtool to understand the control flow *around* the
> + * retpoline.
> + */
> +static int add_nospec_ignores(struct objtool_file *file)
> +{
> +	struct section *sec;
> +	struct rela *rela;
> +	struct instruction *insn;
> +
> +	sec = find_section_by_name(file->elf, ".rela.discard.nospec");
> +	if (!sec)
> +		return 0;
> +
> +	list_for_each_entry(rela, &sec->rela_list, list) {
> +		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.nospec entry");
> +			return -1;
> +		}
> +
> +		insn->ignore_alts = true;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
>   * Find the destination instructions for all jumps.
>   */
>  static int add_jump_destinations(struct objtool_file *file)
> @@ -509,11 +543,18 @@ static int add_call_destinations(struct objtool_file *file)
>  			dest_off = insn->offset + insn->len + insn->immediate;
>  			insn->call_dest = find_symbol_by_offset(insn->sec,
>  								dest_off);
> +			/*
> +			 * FIXME: Thanks to retpolines, it's now considered
> +			 * normal for a function to call within itself.  So
> +			 * disable this warning for now.
> +			 */
> +#if 0
>  			if (!insn->call_dest) {
>  				WARN_FUNC("can't find call dest symbol at offset 0x%lx",
>  					  insn->sec, insn->offset, dest_off);
>  				return -1;
>  			}
> +#endif

This crashes for me in is_fentry_call().

Program received signal SIGSEGV, Segmentation fault.
is_fentry_call (insn=<optimized out>, insn=<optimized out>) at check.c:1113
1113		if (insn->type == INSN_CALL &&
(gdb) info stack
#0  is_fentry_call (insn=<optimized out>, insn=<optimized out>) at check.c:1113
#1  validate_branch (file=0x7ffffff7e440, first=0x7ffffff7e128, state=...) at check.c:1747
#2  0x0000000000404bd3 in validate_branch (file=0x7ffffff7e440, first=0x7ffffff7e128, state=...) at check.c:1770
#3  0x0000000000406783 in validate_functions (file=<optimized out>) at check.c:1933
#4  check (_objname=0x6bb9d0 "", _no_fp=40, no_unreachable=4, orc=false) at check.c:2006
#5  0x00000000004021c1 in handle_internal_command (argv=0x7fffffffe5c0, argc=4) at objtool.c:108
#6  main (argc=4, argv=0x7fffffffe5c0) at objtool.c:131

This is not entirely surprising, since insn->call_dest is NULL and
is_fentry_call() doesn't expect that.

How is this supposed to work ? What am I missing ?

Guenter

>  		} else if (rela->sym->type == STT_SECTION) {
>  			insn->call_dest = find_symbol_by_offset(rela->sym->sec,
>  								rela->addend+4);
> @@ -678,12 +719,6 @@ static int add_special_section_alts(struct objtool_file *file)
>  		return ret;
>  
>  	list_for_each_entry_safe(special_alt, tmp, &special_alts, list) {
> -		alt = malloc(sizeof(*alt));
> -		if (!alt) {
> -			WARN("malloc failed");
> -			ret = -1;
> -			goto out;
> -		}
>  
>  		orig_insn = find_insn(file, special_alt->orig_sec,
>  				      special_alt->orig_off);
> @@ -694,6 +729,10 @@ static int add_special_section_alts(struct objtool_file *file)
>  			goto out;
>  		}
>  
> +		/* Ignore retpoline alternatives. */
> +		if (orig_insn->ignore_alts)
> +			continue;
> +
>  		new_insn = NULL;
>  		if (!special_alt->group || special_alt->new_len) {
>  			new_insn = find_insn(file, special_alt->new_sec,
> @@ -719,6 +758,13 @@ static int add_special_section_alts(struct objtool_file *file)
>  				goto out;
>  		}
>  
> +		alt = malloc(sizeof(*alt));
> +		if (!alt) {
> +			WARN("malloc failed");
> +			ret = -1;
> +			goto out;
> +		}
> +
>  		alt->insn = new_insn;
>  		list_add_tail(&alt->list, &orig_insn->alts);
>  
> @@ -1035,6 +1081,10 @@ static int decode_sections(struct objtool_file *file)
>  
>  	add_ignores(file);
>  
> +	ret = add_nospec_ignores(file);
> +	if (ret)
> +		return ret;
> +
>  	ret = add_jump_destinations(file);
>  	if (ret)
>  		return ret;
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index 47d9ea7..dbadb30 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -44,7 +44,7 @@ struct instruction {
>  	unsigned int len;
>  	unsigned char type;
>  	unsigned long immediate;
> -	bool alt_group, visited, dead_end, ignore, hint, save, restore;
> +	bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts;
>  	struct symbol *call_dest;
>  	struct instruction *jump_dest;
>  	struct list_head alts;