Re: [RFC PATCH 16/21] objtool: Add support for CONFIG_CFI_CLANG

[Peter Zijlstra]

On Fri, Apr 29, 2022 at 01:36:39PM -0700, Sami Tolvanen wrote:

> +static void *kcfi_alloc_hash(unsigned long size)
> +{
> +	kcfi_bits = max(10, ilog2(size));
> +	kcfi_hash = mmap(NULL, sizeof(struct hlist_head) << kcfi_bits,
> +			PROT_READ|PROT_WRITE,
> +			MAP_PRIVATE|MAP_ANON, -1, 0);
> +	if (kcfi_hash == (void *)-1L) {
> +		WARN("mmap fail kcfi_hash");
> +		kcfi_hash = NULL;
> +	}  else if (stats) {
> +		printf("kcfi_bits: %d\n", kcfi_bits);
> +	}
> +
> +	return kcfi_hash;
> +}
> +
> +static void add_kcfi_type(struct kcfi_type *type)
> +{
> +	hlist_add_head(&type->hash,
> +		&kcfi_hash[hash_min(
> +			sec_offset_hash(type->sec, type->offset),
> +			kcfi_bits)]);

:se cino=(0:0

Also, I'm thinking you can unwrap some lines at least.

> +}
> +
> +static bool is_kcfi_typeid(struct elf *elf, struct instruction *insn)
> +{
> +	struct hlist_head *head;
> +	struct kcfi_type *type;
> +	struct reloc *reloc;
> +
> +	if (!kcfi)
> +		return false;
> +
> +	/* Compiler-generated annotation in .kcfi_types. */
> +	head = &kcfi_hash[hash_min(sec_offset_hash(insn->sec, insn->offset), kcfi_bits)];
> +
> +	hlist_for_each_entry(type, head, hash)
> +		if (type->sec == insn->sec && type->offset == insn->offset)
> +			return true;

missing { }

> +
> +	/* Manual annotation (in assembly code). */
> +	reloc = find_reloc_by_dest(elf, insn->sec, insn->offset);
> +
> +	if (reloc && !strncmp(reloc->sym->name, "__kcfi_typeid_", 14))
> +		return true;
> +
> +	return false;
> +}
> +
>  /*
>   * This checks to see if the given function is a "noreturn" function.
>   *
> @@ -388,13 +487,18 @@ static int decode_instructions(struct objtool_file *file)
>  			insn->sec = sec;
>  			insn->offset = offset;
>  
> -			ret = arch_decode_instruction(file, sec, offset,
> -						      sec->sh.sh_size - offset,
> -						      &insn->len, &insn->type,
> -						      &insn->immediate,
> -						      &insn->stack_ops);
> -			if (ret)
> -				goto err;
> +			if (is_kcfi_typeid(file->elf, insn)) {
> +				insn->type = INSN_KCFI_TYPEID;
> +				insn->len = KCFI_TYPEID_LEN;

Urgh, what does this do for decode speed? This is a hash-lookup for
every single instruction.

Is that kcfi location array sorted by the compiler? Because then you can
keep a running iterator and replace the whole lookup with a simple
equality comparison.

> +			} else {
> +				ret = arch_decode_instruction(file, sec, offset,
> +							      sec->sh.sh_size - offset,
> +							      &insn->len, &insn->type,
> +							      &insn->immediate,
> +							      &insn->stack_ops);
> +				if (ret)
> +					goto err;
> +			}
>  
>  			/*
>  			 * By default, "ud2" is a dead end unless otherwise