Messages in this thread | | | Date | Thu, 14 Apr 2022 08:44:49 -0700 | From | Josh Poimboeuf <> | Subject | Re: [PATCH 10/18] objtool: Extricate ibt from stack validation |
| |
On Thu, Apr 14, 2022 at 09:53:18AM +0200, Peter Zijlstra wrote: > On Wed, Apr 13, 2022 at 04:19:45PM -0700, Josh Poimboeuf wrote: > > Extricate ibt from validate_branch() in preparation for making stack > > validation optional. > > It does a bit more..
Indeed.
> > - /* already done in validate_branch() */ > > - if (sec->sh.sh_flags & SHF_EXECINSTR) > > - continue; > > > > - if (!sec->reloc) > > continue; > > > > - if (!strncmp(sec->name, ".orc", 4)) > > - continue; > > > > - if (!strncmp(sec->name, ".discard", 8)) > > continue; > > > > - if (!strncmp(sec->name, ".debug", 6)) > > continue; > > > > - if (!strcmp(sec->name, "_error_injection_whitelist")) > > continue; > > > > - if (!strcmp(sec->name, "_kprobe_blacklist")) > > continue; > > > > - is_data = strstr(sec->name, ".data") || strstr(sec->name, ".rodata"); > > > > - list_for_each_entry(reloc, &sec->reloc->reloc_list, list) { > > - struct instruction *dest; > > > > - dest = validate_ibt_reloc(file, reloc); > > - if (is_data && dest && !dest->noendbr) > > - warn_noendbr("data ", sec, reloc->offset, dest); > > - } > > So this iterates all sections and excludes a bunch, and only reports > fail for .data/.rodata.
Oops.
> > +static int validate_ibt(struct objtool_file *file) > > +{ > > + struct section *sec; > > + struct reloc *reloc; > > + struct instruction *insn; > > + int warnings = 0; > > + > > + for_each_insn(file, insn) > > + warnings += validate_ibt_insn(file, insn); > > So I specifically didn't do this because I wanted to reduce the amount > of loops we do over those instructions. But yeah, if you really want to > allow --ibt without --stack-validate (but why?) then I suppose so. > > Esp. for the vmlinux.o case, iterating all insn can quickly add up to > significant time.
I didn't look at the performance, but if it's a problem then we can eventually look at combining several of the for_each_insn() features into a single loop: retpolines, ibt, reachability check, maybe even some of decode_sections().
> > > + for_each_sec(file, sec) { > > + > > + if (!strstr(sec->name, ".data") && !strstr(sec->name, ".rodata")) > > + continue; > > But this only iterates .data/.rodata. > > That's not the same, specifically, it'll not iterate stuff like ksymtab > that contains the EXPORT_SYMBOL* crud. The result being that we can now > seal EXPORT'ed symbols, which will make modules really sad. > > There's also the .initcall sections, sealing initcalls typcally ends really > badly. > > And there might be a few others I forgot about.
Ok. That was subtle, it needs a comment or two. I had the distinct feeling I was introducing a bug, then I got distracted ;-)
Doesn't the compiler give those special cases ENDBR anyway? Just wondering why we avoid the warning for those.
-- Josh
| |