Re: [PATCH v3 26/26] objtool: Add STT_NOTYPE noinstr validation

[Peter Zijlstra]

On Wed, Mar 25, 2020 at 11:40:46AM -0500, Josh Poimboeuf wrote:
> On Wed, Mar 25, 2020 at 04:53:48PM +0100, Peter Zijlstra wrote:
> > On Wed, Mar 25, 2020 at 09:42:11AM -0500, Josh Poimboeuf wrote:
> > > Sure, but couldn't validate_unwind_hints() and
> > > validate_reachable_instructions() be changed to *only* run on
> > > .noinstr.text, for the vmlinux case?  That might help converge the
> > > vmlinux and !vmlinux paths.
> > 
> > You're thinking something like so then?
> 
> Not exactly.  But I don't want to keep churning this patch set.  I can
> add more patches later, so don't worry about it.
> 
> But I was thinking it would eventually be good to have the top-level
> check() be like
> 
> 	sec = NULL;
> 	if (!validate_all)
> 		sec = find_section_by_name(file->elf, ".noinstr.text");
> 
> 	ret = validate_functions(&file, sec);
> 	if (ret < 0)
> 		goto out;
> 	warnings += ret;
> 
> 	ret = validate_unwind_hints(&file, sec);
> 	if (ret < 0)
> 		goto out;
> 	warnings += ret;
> 
> 	if (!warnings) {
> 		ret = validate_reachable_instructions(&file, sec);
> 		if (ret < 0)
> 			goto out;
> 		warnings += ret;
> 	}
> 
> 	if (!validate_all)
> 		goto out;
> 
> 	ret = validate_retpoline(&file);
> 	....
> 
> that way the general flow is the same regardless...

Ah,... I see what you mean, there's two little wrinkles with that:

 - validate_reachable_instructions() is strictly superluous, it does no
   additional validation between the !vmlinux and vmlinux mode, so I'd
   put that if (!validate_all) goto out, one up.

 - tglx has a patch adding .entry.text to be considered as noinstr as a
   whole, which has:


@@ -2636,11 +2637,16 @@ static int validate_vmlinux_functions(st
 	int warnings = 0;
 
 	sec = find_section_by_name(file->elf, ".noinstr.text");
-	if (!sec)
-		return 0;
+	if (sec) {
+		warnings += validate_section(file, sec);
+		warnings += validate_unwind_hints(file, sec);
+	}
 
-	warnings += validate_section(file, sec);
-	warnings += validate_unwind_hints(file, sec);
+	sec = find_section_by_name(file->elf, ".entry.text");
+	if (sec) {
+		warnings += validate_section(file, sec);
+		warnings += validate_unwind_hints(file, sec);
+	}
 
 	return warnings;
 }

Anyway, yes, we can do this on top.

I was going to commit the first 17 patches to tip/core/objtool and
repost the remaining 13 once more. And then see how much pain I did to
Julien's patches :-)