Re: [PATCH v2 01/14] objtool: Allow branches within the same alternative.

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Tue, Apr 28, 2020 at 09:11:02PM +0200, Peter Zijlstra wrote:
> From: Alexandre Chartre <alexandre.chartre@oracle.com>
> 
> Currently objtool prevents any branch to an alternative. While preventing
> branching from the outside to the middle of an alternative makes perfect
> sense, branching within the same alternative should be allowed. To do so,
> identify each alternative and check that a branch to an alternative comes
> from the same alternative.
> 
> 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-3-alexandre.chartre@oracle.com

I still don't like this patch, other than the "alt_group =
alt_group_next_index++" thing as a prerequisite for the next patch.

Instead of all this, let's just get rid of the 

  "don't know how to handle branch to middle of alternative instruction group"

warning, which I think we decided is useless and not worth the effort
anyway.

> ---
>  tools/objtool/check.c |   26 ++++++++++++++++++++------
>  tools/objtool/check.h |    3 ++-
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -770,7 +770,9 @@ static int handle_group_alt(struct objto
>  			    struct instruction *orig_insn,
>  			    struct instruction **new_insn)
>  {
> +	static unsigned int alt_group_next_index = 1;
>  	struct instruction *last_orig_insn, *last_new_insn, *insn, *fake_jump = NULL;
> +	unsigned int alt_group = alt_group_next_index++;
>  	unsigned long dest_off;
>  
>  	last_orig_insn = NULL;
> @@ -779,7 +781,7 @@ static int handle_group_alt(struct objto
>  		if (insn->offset >= special_alt->orig_off + special_alt->orig_len)
>  			break;
>  
> -		insn->alt_group = true;
> +		insn->alt_group = alt_group;
>  		last_orig_insn = insn;
>  	}
>  
> @@ -813,6 +815,7 @@ static int handle_group_alt(struct objto
>  	}
>  
>  	last_new_insn = NULL;
> +	alt_group = alt_group_next_index++;
>  	insn = *new_insn;
>  	sec_for_each_insn_from(file, insn) {
>  		if (insn->offset >= special_alt->new_off + special_alt->new_len)
> @@ -822,6 +825,7 @@ static int handle_group_alt(struct objto
>  
>  		insn->ignore = orig_insn->ignore_alts;
>  		insn->func = orig_insn->func;
> +		insn->alt_group = alt_group;
>  
>  		/*
>  		 * Since alternative replacement code is copy/pasted by the
> @@ -2163,6 +2167,15 @@ static int validate_return(struct symbol
>  	return 0;
>  }
>  
> +static bool is_branch_to_alternative(struct instruction *from,
> +				     struct instruction *to)
> +{
> +	if (!from || !to->alt_group || !list_empty(&to->alts))
> +		return false;
> +
> +	return (from->alt_group != to->alt_group);
> +}
> +
>  /*
>   * Follow the branch starting at the given instruction, and recursively follow
>   * any other branches (jumps).  Meanwhile, track the frame pointer state at
> @@ -2170,6 +2183,7 @@ static int validate_return(struct symbol
>   * tools/objtool/Documentation/stack-validation.txt.
>   */
>  static int validate_branch(struct objtool_file *file, struct symbol *func,
> +			   struct instruction *from,
>  			   struct instruction *insn, struct insn_state state)
>  {
>  	struct alternative *alt;
> @@ -2180,7 +2194,7 @@ static int validate_branch(struct objtoo
>  
>  	sec = insn->sec;
>  
> -	if (insn->alt_group && list_empty(&insn->alts)) {
> +	if (is_branch_to_alternative(from, insn)) {
>  		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
>  			  sec, insn->offset);
>  		return 1;
> @@ -2227,7 +2241,7 @@ static int validate_branch(struct objtoo
>  				if (alt->skip_orig)
>  					skip_orig = true;
>  
> -				ret = validate_branch(file, func, alt->insn, state);
> +				ret = validate_branch(file, func, NULL, alt->insn, state);
>  				if (ret) {
>  					if (backtrace)
>  						BT_FUNC("(alt)", insn);
> @@ -2271,7 +2285,7 @@ static int validate_branch(struct objtoo
>  
>  			} else if (insn->jump_dest) {
>  				ret = validate_branch(file, func,
> -						      insn->jump_dest, state);
> +						      insn, insn->jump_dest, state);
>  				if (ret) {
>  					if (backtrace)
>  						BT_FUNC("(branch)", insn);
> @@ -2402,7 +2416,7 @@ static int validate_unwind_hints(struct
>  
>  	while (&insn->list != &file->insn_list && (!sec || insn->sec == sec)) {
>  		if (insn->hint && !insn->visited) {
> -			ret = validate_branch(file, insn->func, insn, state);
> +			ret = validate_branch(file, insn->func, NULL, insn, state);
>  			if (ret && backtrace)
>  				BT_FUNC("<=== (hint)", insn);
>  			warnings += ret;
> @@ -2543,7 +2557,7 @@ static int validate_symbol(struct objtoo
>  
>  	state->uaccess = sym->uaccess_safe;
>  
> -	ret = validate_branch(file, insn->func, insn, *state);
> +	ret = validate_branch(file, insn->func, NULL, insn, *state);
>  	if (ret && backtrace)
>  		BT_FUNC("<=== (sym)", insn);
>  	return ret;
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -30,12 +30,13 @@ struct instruction {
>  	unsigned int len;
>  	enum insn_type type;
>  	unsigned long immediate;
> -	bool alt_group, dead_end, ignore, ignore_alts;
> +	bool dead_end, ignore, ignore_alts;
>  	bool hint;
>  	bool retpoline_safe;
>  	s8 instr;
>  	u8 visited;
>  	u8 ret_offset;
> +	int alt_group;
>  	struct symbol *call_dest;
>  	struct instruction *jump_dest;
>  	struct instruction *first_jump_src;
> 
> 

-- 
Josh