Re: [RFC v4 05/18] objtool: special: Adapt special section handling

[Julien <julien.thierry.kdev@gmail.com>]

Hi Raphaël,

On 16/08/19 13:23, Raphael Gault wrote:
> This patch abstracts the few architecture dependent tests that are
> perform when handling special section and switch tables. It enables any
> architecture to ignore a particular CPU feature or not to handle switch
> tables.

I think it would be better if this patch only focused on CPU features 
and leave dealing with switch table to subsequent patches.

> 
> Signed-off-by: Raphael Gault <raphael.gault@arm.com>
> ---
>   tools/objtool/arch/arm64/Build                |  1 +
>   tools/objtool/arch/arm64/arch_special.c       | 22 +++++++++++++++
>   .../objtool/arch/arm64/include/arch_special.h | 10 +++++--
>   tools/objtool/arch/x86/Build                  |  1 +
>   tools/objtool/arch/x86/arch_special.c         | 28 +++++++++++++++++++
>   tools/objtool/arch/x86/include/arch_special.h |  9 ++++++
>   tools/objtool/check.c                         | 24 ++++++++++++++--
>   tools/objtool/special.c                       |  9 ++----
>   tools/objtool/special.h                       |  3 ++
>   9 files changed, 96 insertions(+), 11 deletions(-)
>   create mode 100644 tools/objtool/arch/arm64/arch_special.c
>   create mode 100644 tools/objtool/arch/x86/arch_special.c
> 
> diff --git a/tools/objtool/arch/arm64/Build b/tools/objtool/arch/arm64/Build
> index bf7a32c2b9e9..3d09be745a84 100644
> --- a/tools/objtool/arch/arm64/Build
> +++ b/tools/objtool/arch/arm64/Build
> @@ -1,3 +1,4 @@
> +objtool-y += arch_special.o
>   objtool-y += decode.o
>   objtool-y += orc_dump.o
>   objtool-y += orc_gen.o
> diff --git a/tools/objtool/arch/arm64/arch_special.c b/tools/objtool/arch/arm64/arch_special.c
> new file mode 100644
> index 000000000000..a21d28876317
> --- /dev/null
> +++ b/tools/objtool/arch/arm64/arch_special.c
> @@ -0,0 +1,22 @@
> +/*
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +#include "../../special.h"
> +#include "arch_special.h"
> +
> +void arch_force_alt_path(unsigned short feature,
> +			 bool uaccess,
> +			 struct special_alt *alt)
> +{
> +}
> diff --git a/tools/objtool/arch/arm64/include/arch_special.h b/tools/objtool/arch/arm64/include/arch_special.h
> index 63da775d0581..185103be8a51 100644
> --- a/tools/objtool/arch/arm64/include/arch_special.h
> +++ b/tools/objtool/arch/arm64/include/arch_special.h
> @@ -30,7 +30,13 @@
>   #define ALT_ORIG_LEN_OFFSET	10
>   #define ALT_NEW_LEN_OFFSET	11
>   
> -#define X86_FEATURE_POPCNT (4 * 32 + 23)
> -#define X86_FEATURE_SMAP   (9 * 32 + 20)
> +static inline bool arch_should_ignore_feature(unsigned short feature)
> +{
> +	return false;
> +}
>   
> +static inline bool arch_support_switch_table(void)
> +{
> +	return false;

If I understand correctly , this gets later (patch 8) replaced by 
arch_find_switch_table() which can return NULL if unable to find a 
switch table.

So, is it necessary to introduce this function at this stage of the 
patchset? Can't we just have directly arch_find_switch_table() ?

Also, I believe that in the end you keep the two 
arch_support_switch_table() implementations (x86 and arm64) despite 
getting rid of the only caller (in patch 8).


> +}
>   #endif /* _ARM64_ARCH_SPECIAL_H */
> diff --git a/tools/objtool/arch/x86/Build b/tools/objtool/arch/x86/Build
> index 1f11b45999d0..63e167775bc8 100644
> --- a/tools/objtool/arch/x86/Build
> +++ b/tools/objtool/arch/x86/Build
> @@ -1,3 +1,4 @@
> +objtool-y += arch_special.o
>   objtool-y += decode.o
>   objtool-y += orc_dump.o
>   objtool-y += orc_gen.o
> diff --git a/tools/objtool/arch/x86/arch_special.c b/tools/objtool/arch/x86/arch_special.c
> new file mode 100644
> index 000000000000..6583a1770bb2
> --- /dev/null
> +++ b/tools/objtool/arch/x86/arch_special.c
> @@ -0,0 +1,28 @@
> +/*
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +#include "../../special.h"
> +#include "arch_special.h"
> +
> +void arch_force_alt_path(unsigned short feature,
> +			 bool uaccess,

Maybe you could have something like:

void arch_select_alt_path(unsigned short feature, struct special_alt *alt);

or arch_process_alt_inst(...);

This way you just let the arch decide what to do for alternative 
instructions associated with their features, and call it in the "if 
(entry->feature)" branch of get_alt_entry().

You don't need to pass the "uaccess" as that info is globally accessible 
from builtin.h.

> +			 struct special_alt *alt)
> +{
> +		if (feature == X86_FEATURE_SMAP) {
> +			if (uaccess)
> +				alt->skip_orig = true;
> +			else
> +				alt->skip_alt = true;
> +		}
> +}
> diff --git a/tools/objtool/arch/x86/include/arch_special.h b/tools/objtool/arch/x86/include/arch_special.h
> index 424ce47013e3..fce2b1193194 100644
> --- a/tools/objtool/arch/x86/include/arch_special.h
> +++ b/tools/objtool/arch/x86/include/arch_special.h
> @@ -33,4 +33,13 @@
>   #define X86_FEATURE_POPCNT (4 * 32 + 23)
>   #define X86_FEATURE_SMAP   (9 * 32 + 20)
>   
> +static inline bool arch_should_ignore_feature(unsigned short feature)
> +{
> +	return feature == X86_FEATURE_POPCNT;
> +}

With my above suggestion you shouldn't need that and just use the arch 
specific alternative handling set the alt->skip_orig.

> +
> +static inline bool arch_support_switch_table(void)
> +{
> +	return true;
> +}
>   #endif /* _X86_ARCH_SPECIAL_H */
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 30e147391dcb..4af6422d3428 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -729,7 +729,7 @@ static int handle_group_alt(struct objtool_file *file,
>   		last_orig_insn = insn;
>   	}
>   
> -	if (next_insn_same_sec(file, last_orig_insn)) {
> +	if (last_orig_insn && next_insn_same_sec(file, last_orig_insn)) {
>   		fake_jump = malloc(sizeof(*fake_jump));
>   		if (!fake_jump) {
>   			WARN("malloc failed");
> @@ -1061,6 +1061,26 @@ static struct rela *find_jump_table(struct objtool_file *file,
>   		table_rela = find_rela_by_dest(table_sec, table_offset);
>   		if (!table_rela)
>   			continue;
> +		/*
> +		 * If we are on arm64 architecture, we now that we
> +		 * are in presence of a switch table thanks to
> +		 * the `br <Xn>` insn. but we can't retrieve it yet.
> +		 * So we just ignore unreachable for this file.
> +		 */
> +		if (!arch_support_switch_table()) {
> +			file->ignore_unreachables = true;
> +			return NULL;
> +		}
> +
> +		rodata_rela = find_rela_by_dest(rodata_sec, table_offset);
> +		if (rodata_rela) {
> +			/*
> +			 * Use of RIP-relative switch jumps is quite rare, and
> +			 * indicates a rare GCC quirk/bug which can leave dead
> +			 * code behind.
> +			 */
> +			if (text_rela->type == R_X86_64_PC32)
> +				file->ignore_unreachables = true;
>   
>   		/*
>   		 * Use of RIP-relative switch jumps is quite rare, and
> @@ -1864,7 +1884,7 @@ 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->visited && insn->alt_group && list_empty(&insn->alts)) {
>   		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
>   			  sec, insn->offset);
>   		return 1;
> diff --git a/tools/objtool/special.c b/tools/objtool/special.c
> index b8ccee1b5382..7a0092d6e5b3 100644
> --- a/tools/objtool/special.c
> +++ b/tools/objtool/special.c
> @@ -81,7 +81,7 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry,
>   		 * feature path which is a "very very small percentage of
>   		 * machines".
>   		 */
> -		if (feature == X86_FEATURE_POPCNT)
> +		if (arch_should_ignore_feature(feature))
>   			alt->skip_orig = true;
>   
>   		/*
> @@ -93,12 +93,7 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry,
>   		 * find paths that see the STAC but take the NOP instead of
>   		 * CLAC and the other way around.
>   		 */
> -		if (feature == X86_FEATURE_SMAP) {
> -			if (uaccess)
> -				alt->skip_orig = true;
> -			else
> -				alt->skip_alt = true;
> -		}
> +		arch_force_alt_path(feature, uaccess, alt);
>   	}
>   
>   	orig_rela = find_rela_by_dest(sec, offset + entry->orig);
> diff --git a/tools/objtool/special.h b/tools/objtool/special.h
> index 35061530e46e..90626a7e41cf 100644
> --- a/tools/objtool/special.h
> +++ b/tools/objtool/special.h
> @@ -27,5 +27,8 @@ struct special_alt {
>   };
>   
>   int special_get_alts(struct elf *elf, struct list_head *alts);
> +void arch_force_alt_path(unsigned short feature,
> +			 bool uaccess,
> +			 struct special_alt *alt);
>   
>   #endif /* _SPECIAL_H */
> 

CHeers,

-- 
Julien Thierry