linux-kernel - Re: [PATCH v2 02/24] objtool: Introduce CFI hash

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YSYX18wjVceB2iPJ@hirez.programming.kicks-ass.net>
Date:   Wed, 25 Aug 2021 12:13:43 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     tglx@...utronix.de, linux-kernel@...r.kernel.org, joro@...tes.org,
        boris.ostrovsky@...cle.com, jgross@...e.com, x86@...nel.org,
        mbenes@...e.com, rostedt@...dmis.org, dvyukov@...gle.com,
        elver@...gle.com, Andi Kleen <andi@...stfloor.org>
Subject: Re: [PATCH v2 02/24] objtool: Introduce CFI hash

On Fri, Aug 20, 2021 at 03:27:55PM -0700, Josh Poimboeuf wrote:
> > +static struct cfi_state *cfi_alloc(void)
> > +{
> > +	struct cfi_state *cfi = calloc(sizeof(struct cfi_state), 1);
> > +	if (!cfi) {
> > +		WARN("calloc failed");
> > +		exit(1);
> > +	}
> > +	nr_cfi++;
> > +	init_cfi_state(cfi);
> > +	return cfi;
> > +}
> 
> I'm thinking this should also add it to the hash.  i.e. I don't think
> there's a scenario where you'd alloc a cfi and not want to add it to the
> hash.  The more sharing the better.

Right, changed it like below.

> > +
> > +struct cfi_state *insn_get_cfi(struct instruction *insn)
> > +{
> > +	if (!insn->cfip)
> > +		insn->cfip = cfi_alloc();
> > +
> > +	return insn->cfip;
> > +}
> 
> Call it something like insn_get_or_alloc_cfi()?
> 
> Also, the function can be static.

It's gone now.

> > +static struct cfi_state *cfi_hash_find(struct cfi_state *cfi)
> > +{
> > +	struct hlist_head *head = &cfi_hash[hash_min(cfi_key(cfi), cfi_bits)];
> > +	struct cfi_state *obj;
> > +
> > +	hlist_for_each_entry(obj, head, hash) {
> > +		if (!cficmp(cfi, obj)) {
> > +			nr_cfi_cache++;
> > +			return obj;
> > +		}
> > +	}
> > +
> > +	obj = cfi_alloc();
> > +	*obj = *cfi;
> > +	hlist_add_head(&obj->hash, head);
> > +
> > +	return obj;
> 
> cfi_hash_find_or_alloc_cfi()?


Made that cfi_hash_find_or_add()

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -30,6 +30,7 @@ struct alternative {
 static unsigned long nr_cfi, nr_cfi_reused, nr_cfi_cache;
 
 static struct cfi_init_state initial_func_cfi;
+static struct cfi_state init_cfi;
 static struct cfi_state func_cfi;
 
 struct instruction *find_insn(struct objtool_file *file,
@@ -278,7 +279,6 @@ static struct cfi_state *cfi_alloc(void)
 		exit(1);
 	}
 	nr_cfi++;
-	init_cfi_state(cfi);
 	return cfi;
 }
 
@@ -298,7 +298,7 @@ static inline u32 cfi_key(struct cfi_sta
 		     sizeof(*cfi) - sizeof(cfi->hash), 0);
 }
 
-static struct cfi_state *cfi_hash_find(struct cfi_state *cfi)
+static struct cfi_state *cfi_hash_find_or_add(struct cfi_state *cfi)
 {
 	struct hlist_head *head = &cfi_hash[hash_min(cfi_key(cfi), cfi_bits)];
 	struct cfi_state *obj;
@@ -1620,10 +1620,10 @@ static void set_func_state(struct cfi_st
 
 static int read_unwind_hints(struct objtool_file *file)
 {
+	struct cfi_state cfi = init_cfi;
 	struct section *sec, *relocsec;
 	struct unwind_hint *hint;
 	struct instruction *insn;
-	struct cfi_state *cfi;
 	struct reloc *reloc;
 	int i;
 
@@ -1666,9 +1666,8 @@ static int read_unwind_hints(struct objt
 			continue;
 		}
 
-		cfi = insn->cfip;
-		if (!cfi)
-			cfi = insn->cfip = cfi_alloc();
+		if (insn->cfip)
+			cfi = *(insn->cfip);
 
 		if (arch_decode_hint_reg(insn, hint->sp_reg)) {
 			WARN_FUNC("unsupported unwind_hint sp base reg %d",
@@ -1679,6 +1678,8 @@ static int read_unwind_hints(struct objt
 		cfi->cfa.offset = bswap_if_needed(hint->sp_offset);
 		cfi->type = hint->type;
 		cfi->end = hint->end;
+
+		insn->cfip = cfi_hash_find_or_add(&cfi);
 	}
 
 	return 0;
@@ -2831,7 +2832,7 @@ static int validate_branch(struct objtoo
 				insn->cfip = prev_insn->cfip;
 				nr_cfi_reused++;
 			} else {
-				insn->cfip = cfi_hash_find(&state.cfi);
+				insn->cfip = cfi_hash_find_or_add(&state.cfi);
 			}
 		}
 
@@ -3239,6 +3240,7 @@ int check(struct objtool_file *file)
 	int ret, warnings = 0;
 
 	arch_initial_func_cfi_state(&initial_func_cfi);
+	init_cfi_state(&init_cfi);
 	init_cfi_state(&func_cfi);
 	set_func_state(&func_cfi);
 
@@ -3250,6 +3252,7 @@ int check(struct objtool_file *file)
 	if (!cfi_hash_alloc(1UL << (file->elf->symbol_bits - 3)))
 		goto out;
 
+	cfi_hash_add(&init_cfi);
 	cfi_hash_add(&func_cfi);
 
 	if (list_empty(&file->insn_list))