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;
Subject: Re: [PATCH 2/7] objtool: Allow branches within the same alternative.
To:     Alexandre Chartre <alexandre.chartre@oracle.com>, x86@kernel.org
Cc:     linux-kernel@vger.kernel.org, jpoimboe@redhat.com,
        peterz@infradead.org, tglx@linutronix.de
References: <20200402082220.808-1-alexandre.chartre@oracle.com>
 <20200402082220.808-3-alexandre.chartre@oracle.com>
From:   Julien Thierry <jthierry@redhat.com>
Message-ID: <50e8a5d8-7cb4-f25c-9657-eb11038bd0b5@redhat.com>
Date:   Thu, 2 Apr 2020 13:03:30 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <20200402082220.808-3-alexandre.chartre@oracle.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Alexandre,

I ran into the same issue for the arm64 work:
https://lkml.org/lkml/2020/1/9/656

Your solution seems nicer however.

On 4/2/20 9:22 AM, Alexandre Chartre wrote:
> 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>
> ---
>   tools/objtool/check.c | 19 +++++++++++++------
>   tools/objtool/check.h |  3 ++-
>   2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 708562fb89e1..214809ac2776 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -712,7 +712,9 @@ static int handle_group_alt(struct objtool_file *file,
>   			    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;
> @@ -721,7 +723,7 @@ static int handle_group_alt(struct objtool_file *file,
>   		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;
>   	}
>   
> @@ -1942,6 +1944,7 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
>    * tools/objtool/Documentation/stack-validation.txt.
>    */
>   static int validate_branch(struct objtool_file *file, struct symbol *func,
> +			   struct instruction *from,

Maybe instead of passing a new instruction pointer, just the alt_group 
could be passed? 0 Meaning it was not in an alt group.

>   			   struct instruction *first, struct insn_state state)
>   {
>   	struct alternative *alt;
> @@ -1953,7 +1956,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   	insn = first;
>   	sec = insn->sec;
>   
> -	if (insn->alt_group && list_empty(&insn->alts)) {
> +	if (insn->alt_group &&
> +	    (!from || from->alt_group != insn->alt_group) &&
> +	    list_empty(&insn->alts)) {

This would become

	if (insn->alt_group != alt_group && list_empty(&insn->alts))

And the recursive validate_branch() calls would just take 
insn->alt_group as parameter (and the calls in validate_functions() and 
validate_unwind_hints() would take 0).

Any opinions on that?

>   		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
>   			  sec, insn->offset);
>   		return 1;
> @@ -2035,7 +2040,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   				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);
> @@ -2105,7 +2111,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   					return ret;
>   
>   			} else if (insn->jump_dest) {
> -				ret = validate_branch(file, func,
> +				ret = validate_branch(file, func, insn,
>   						      insn->jump_dest, state);
>   				if (ret) {
>   					if (backtrace)
> @@ -2236,7 +2242,8 @@ static int validate_unwind_hints(struct objtool_file *file)
>   
>   	for_each_insn(file, insn) {
>   		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;
> @@ -2377,7 +2384,7 @@ static int validate_functions(struct objtool_file *file)
>   
>   			state.uaccess = func->uaccess_safe;
>   
> -			ret = validate_branch(file, func, insn, state);
> +			ret = validate_branch(file, func, NULL, insn, state);
>   			if (ret && backtrace)
>   				BT_FUNC("<=== (func)", insn);
>   			warnings += ret;
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index 6d875ca6fce0..cffb23d81782 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -33,7 +33,8 @@ struct instruction {
>   	unsigned int len;
>   	enum insn_type type;
>   	unsigned long immediate;
> -	bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
> +	unsigned int alt_group;
> +	bool dead_end, ignore, hint, save, restore, ignore_alts;
>   	bool retpoline_safe;
>   	u8 visited;
>   	struct symbol *call_dest;
> 

Cheers,

-- 
Julien Thierry