On Tue, 23 Apr 2019, Joe Lawrence wrote: > On Wed, Apr 10, 2019 at 11:50:52AM -0400, Joe Lawrence wrote: > > > > [ ... snip ... ] > > > > +static bool convert_rela(struct section *oldsec, struct rela *r, > > + struct sympos *sp, struct elf *klp_elf) > > +{ > > + struct section *sec; > > + struct rela *r1, *r2; > > + > > + sec = get_or_create_klp_rela_section(oldsec, sp, klp_elf); > > + if (!sec) { > > + WARN("Can't create or access klp.rela section (%s.%s)\n", > > + sp->object_name, sp->symbol_name); > > + return false; > > + } > > + > > + if (!convert_klp_symbol(r->sym, sp)) { > > + WARN("Unable to convert symbol name (%s.%s)\n", sec->name, > > + r->sym->name); > > + return false; > > + } > > + > > + /* Move the converted rela to klp rela section */ > > + list_for_each_entry_safe(r1, r2, &oldsec->relas, list) { > > + if (r1->sym->name == r->sym->name) { > > + list_del(&r1->list); > > + list_add(&r1->list, &sec->relas); > > + } > > + } > > + return true; > > +} > > This one took a while to find and debug, but I believe that > convert_rela()'s list removal is not as safe as it thinks it is. > > Start with its calling context from main() below: > > > + list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) { > > + if (!is_rela_section(sec)) > > + continue; > > + > > + list_for_each_entry_safe(rela, tmprela, &sec->relas, list) { > > + if (!must_convert(rela->sym)) > > + continue; > > + > > + if (!find_missing_position(rela->sym, &sp)) { > > + WARN("Unable to find missing symbol: %s", > > + rela->sym->name); > > + return -1; > > + } > > + if (!convert_rela(sec, rela, &sp, klp_elf)) { > > + WARN("Unable to convert relocation: %s", > > + rela->sym->name); > > + return -1; > > + } > > + } > > + } > > AFAIK the *_safe list traversals, they cache the ->next value at the > beginning of each iteration, so that one could blow the current element > in position away. The cached ->next value is then assigned when moving > to the next iteration. > > But notice how convert_rela() looks through the entire list of > relocations, moving each rela with a matching symbol? > > Consider a slight tweak to samples/livepatch-annotated.c: > > > static int livepatch_cmdline_proc_show(struct seq_file *m, void *v) > { > + if (saved_command_line) > + saved_command_line[0] = '\0'; > + > seq_printf(m, "%s livepatch=1\n", saved_command_line); > return 0; > } > > > On my system, this generates relocations like this: > > % eu-readelf --relocs samples/livepatch/livepatch-annotated-sample.o > Relocation section [ 2] '.rela.text' for section [ 1] '.text' at offset 0x98 contains 9 entries: > Offset Type Value Addend Name > 0x0000000000000001 X86_64_PC32 000000000000000000 -4 __fentry__ > 0x0000000000000008 X86_64_PC32 000000000000000000 -4 saved_command_line > 0x0000000000000017 X86_64_PC32 000000000000000000 -4 saved_command_line > 0x000000000000001e X86_64_32S 000000000000000000 +0 .rodata.str1.1 > 0x0000000000000023 X86_64_PC32 000000000000000000 -4 seq_printf > 0x0000000000000031 X86_64_PC32 000000000000000000 -4 __fentry__ > 0x0000000000000038 X86_64_32S 000000000000000000 +0 .data > 0x0000000000000051 X86_64_PC32 000000000000000000 -4 __fentry__ > 0x000000000000003d X86_64_PC32 000000000000000000 -4 klp_enable_patch > > We now have back-to-back rela's with sym->name = "saved_command_line". > When the first is converted, convert_rela() will move both of them to > the klp rela section. The linked list values may be consistent, but the > cached ->next value will be bogus and the in-flight-traversal will run > off the rails. Yep, valid, if I'm reading the code correctly. > I think we can work around it with a combination of 1) only moving a > single rela symbol at a time in convert_rela and 2) processing the > second (third, etc.) a little bit more so that they are moved > individually. > > I hacked this together and it works against the livepatch-annotate.c > test above so far... > > -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- > > diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c > index 82c27d219372..126395f1c0cd 100644 > --- a/scripts/livepatch/klp-convert.c > +++ b/scripts/livepatch/klp-convert.c > @@ -517,6 +517,7 @@ static bool convert_rela(struct section *oldsec, struct rela *r, > if (r1->sym->name == r->sym->name) { > list_del(&r1->list); > list_add(&r1->list, &sec->relas); > + break; > } > } > return true; Couldn't we remove the loop all together instead of breaking it? list_del(&r->list); list_add(&r->list, &sec->relas); could be sufficient. > @@ -549,8 +550,8 @@ static bool is_converted(char *sname) > } > > /* > - * Checks if symbol must be converted (conditions): > - * not resolved, not already converted or isn't an exported symbol > + * Checks if symbol must be or was already converted (conditions): > + * not resolved or isn't an exported symbol > */ > static bool must_convert(struct symbol *sym) > { > @@ -566,7 +567,7 @@ static bool must_convert(struct symbol *sym) > if (strcmp(sym->name, ".TOC.") == 0) > return false; > > - return (!(is_converted(sym->name) || is_exported(sym->name))); > + return (!is_exported(sym->name)); > } > > /* Checks if a section is a klp rela section */ > @@ -640,7 +641,8 @@ int main(int argc, const char **argv) > if (!must_convert(rela->sym)) > continue; > > - if (!find_missing_position(rela->sym, &sp)) { > + if (!is_converted(rela->sym->name) && > + !find_missing_position(rela->sym, &sp)) { > WARN("Unable to find missing symbol: %s", > rela->sym->name); > return -1; Looks good. Miroslav