Messages in this thread | | | Subject | Re: [PATCH v2 3/8] livepatch: Add klp-convert tool | From | Joao Moreira <> | Date | Tue, 26 Mar 2019 17:13:57 -0300 |
| |
On 3/20/19 4:36 PM, Miroslav Benes wrote: >>> [ ... snip ... ] >>> + >>> +/* Checks if sympos is valid, otherwise prints valid sympos list */ >>> +static bool valid_sympos(struct sympos *sp) >>> +{ >>> + struct symbol_entry *e; >>> + int counter = 0; >>> + >>> + list_for_each_entry(e, &symbols, list) { >>> + if ((strcmp(e->symbol_name, sp->symbol_name) == 0) && >>> + (strcmp(e->object_name, sp->object_name) == 0)) { >>> + if (counter == sp->pos) >>> + return true; >>> + counter++; >>> + } >>> + } >>> + >>> + WARN("Provided KLP_SYMPOS does not match a symbol: %s.%s,%d", >>> + sp->object_name, sp->symbol_name, sp->pos); >>> + print_valid_module_relocs(sp->symbol_name); >>> + >>> + return false; >>> +} >> >> I believe there is an off-by-one error condition either here, or in >> livepatch kernel core sympos disambiguator code (in which case, external >> tools like kpatch-build would also need to be adjusted). >> >> The scenarios that I've observed using klp-convert and kpatch-build: >> >> sympos == 0, uniquely named symbol >> sympos == 1, non-unique symbol name, first instance >> sympos == 2, non-unique symbol name, second instance >> ... > > Yes, that is exactly how we defined it back then. > >> When I built a klp-convert selftest, I made sure that the target module >> contained multiple symbols of the same name across two .o files. >> However, when I tried to use a KLP_MODULE_RELOC annotation in the >> livepatch to resolve to the second (ie, last) symbol, using a sympos=2, >> klp-convert kept telling me that the only valid KLP_SYMPOS were 0 and 1. >> >> The following code adjusts klp-convert to match the sympos logic, as I >> understand it in livepatch/core.c and kpatch-build; >> >> [squash] livepatch/klp-convert: fix klp-convert off-by-one sympos >> https://github.com/torvalds/linux/commit/1ed8e5baa98f7920bbbaa562278b3ed44552e01f >> >> This change also adds a check that sympos == 0 is in fact a uniquely >> named symbol. >
Nice catch, thanks for it. All looks good to me.
> Looks good to me. Maybe we can even make it simpler and use similar > approach as in klp_find_callback() and klp_find_object_symbol() from > kernel/livepatch/core.c. On the other hand, given that we want to print > useful output in all cases, the code might happen to be more complicated > and definitely ugly.
I don't have an opinion here.
> >>> [ ... snip ... ] >>> + >>> +int main(int argc, const char **argv) >>> +{ >>> + const char *klp_in_module, *klp_out_module, *symbols_list; >>> + struct rela *rela, *tmprela; >>> + struct section *sec, *aux; >>> + struct sympos sp; >>> + struct elf *klp_elf; >>> + >>> + if (argc != 4) { >>> + WARN("Usage: %s <Symbols.list> <input.ko> <out.ko>", argv[0]); >>> + return -1; >>> + } >>> + >>> + symbols_list = argv[1]; >>> + klp_in_module = argv[2]; >>> + klp_out_module = argv[3]; >>> + >>> + klp_elf = elf_open(klp_in_module); >>> + if (!klp_elf) { >>> + WARN("Unable to read elf file %s\n", klp_in_module); >>> + return -1; >>> + } >>> + >>> + if (!load_syms_lists(symbols_list)) >>> + return -1; >>> + >>> + if (!load_usr_symbols(klp_elf)) { >>> + WARN("Unable to load user-provided sympos"); >>> + return -1; >>> + } >>> + >>> + 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"); >> >> A really minor suggestion, but I found it useful to tell the user >> exactly which symbol was missing: >> >> [squash] livepatch/klp-convert: add missing symbol name to warning >> https://github.com/torvalds/linux/commit/7a41a8f3d9b9e11f0c75914cb925af1486c1ecc6 > > I think that Joao has exactly the same hunk staged somewhere > already. You are right. The message is not much informative. >
Yes, someone had the same idea for the klp-convert which we use inside SUSE. But it did not make it to this patch-set.
Nice that Joe added it here :)
> Miroslav >
| |