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

From: Julien Thierry
Date: Thu Mar 26 2020 - 04:01:27 EST




On 3/25/20 4:50 PM, Peter Zijlstra wrote:
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 :-)


I appreciate the concideration for my patches but, since your patches have been posted a few times already and were already reviewed, you might as well commit them. I'll rebase my patches on top and see the state of things (I'll need to do that with the whole arm64 series anyway).

I'll try to post the new version fast enough so I'm not behind some other major objtool changes :) .

In the mean time I'll have a look at this series and see what I might have to change for the rest of the arm64-objtool set!

Thanks,

--
Julien Thierry