From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6113EC10F11 for ; Wed, 24 Apr 2019 18:19:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3C5AA20685 for ; Wed, 24 Apr 2019 18:19:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1733276AbfDXSTL (ORCPT ); Wed, 24 Apr 2019 14:19:11 -0400 Received: from mx2.suse.de ([195.135.220.15]:51408 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1731658AbfDXSTK (ORCPT ); Wed, 24 Apr 2019 14:19:10 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 650A9AEA1; Wed, 24 Apr 2019 18:19:08 +0000 (UTC) Date: Wed, 24 Apr 2019 20:19:07 +0200 (CEST) From: Miroslav Benes To: Joe Lawrence cc: linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, linux-kbuild@vger.kernel.org, Jessica Yu , Jiri Kosina , Joao Moreira , Josh Poimboeuf , Konstantin Khlebnikov , Masahiro Yamada , Michael Matz , Nicolai Stange , Petr Mladek Subject: Re: [PATCH v3 0/9] klp-convert livepatch build tooling In-Reply-To: <20190417201316.GA690@redhat.com> Message-ID: References: <20190410155058.9437-1-joe.lawrence@redhat.com> <20190417201316.GA690@redhat.com> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org [...] > Result: a small tweak to sympos_sanity_check() to relax its symbol > uniqueness verification: allow for duplicate > instances. Now it will only complain when a supplied symbol references > the same but a different . > > diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c > index 82c27d219372..713835dfc9ec 100644 > --- a/scripts/livepatch/klp-convert.c > +++ b/scripts/livepatch/klp-convert.c > @@ -194,7 +194,10 @@ static void clear_sympos_annontations(struct elf *klp_elf) > } > } > > -/* Checks if two or more elements in usr_symbols have the same name */ > +/* > + * Checks if two or more elements in usr_symbols have the same > + * object and name, but different symbol position > + */ > static bool sympos_sanity_check(void) > { > bool sane = true; > @@ -203,7 +206,9 @@ static bool sympos_sanity_check(void) > list_for_each_entry(sp, &usr_symbols, list) { > aux = list_next_entry(sp, list); > list_for_each_entry_from(aux, &usr_symbols, list) { > - if (strcmp(sp->symbol_name, aux->symbol_name) == 0) { > + if (sp->pos != aux->pos && > + strcmp(sp->object_name, aux->object_name) == 0 && > + strcmp(sp->symbol_name, aux->symbol_name) == 0) > WARN("Conflicting KLP_SYMPOS definition: %s.%s,%d vs. %s.%s,%d.", > sp->object_name, sp->symbol_name, sp->pos, > aux->object_name, aux->symbol_name, aux->pos); Looks good and I'd definitely include it in v4. > Unique sympos > ------------- > > But even with the above workaround, specifying unique symbol positions > will (and should) result in a klp-convert complaint. > > When modifying the test module with unique symbol position annotation > values (1 and 2 respectively): > > test_klp_convert_multi_objs_a.c: > > extern void *state_show; > ... > KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = { > KLP_SYMPOS(state_show, 1) > }; > > test_klp_convert_multi_objs_b.c: > > extern void *state_show; > ... > KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = { > KLP_SYMPOS(state_show, 2) > }; > > > Each object file's symbol table contains a "state_show" instance: > > % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_a.o | grep '\' > 30: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show > > % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_b.o | grep '\' > 20: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show > > and the intermediate test_klp_convert_multi_objs.klp.o file contains a > combined .klp.module_relocs.vmlinux relocation section with two > KLP_SYMPOS structures, each with a unique value: > > % objcopy -O binary --only-section=.klp.module_relocs.vmlinux \ > lib/livepatch/test_klp_convert_multi_objs.klp.o >(hexdump) > > 0000000 0000 0000 0000 0000 0001 0000 0000 0000 > 0000010 0000 0000 0002 0000 > > but the symbol tables were merged, sorted and non-unique symbols > removed, leaving a *single* unresolved "state_show" instance: > > % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs.klp.o | grep '\' > 53: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show > > which means that even though we have separate relocations for each > "state_show" instance: > > Relocation section [ 6] '.rela.text.unlikely' for section [ 5] '.text.unlikely' at offset 0x44510 contains 11 entries: > Offset Type Value Addend Name > 0x0000000000000003 X86_64_32S 000000000000000000 +0 state_show > ... > 0x0000000000000034 X86_64_32S 000000000000000000 +0 state_show > ... > > they share the same rela->sym and there is no way to distinguish which > one correlates to which KLP_SYMPOS structure. > > > Future Work > ----------- > > I don't see an easy way to support multiple homonym > symbols with unique values in the current livepatch module > Elf format. The only solutions that come to mind right now include > renaming homonym symbols somehow to retain the relocation->symbol > relationship when separate object files are combined. Perhaps an > intermediate linker step could make annotated symbols unique in some way > to achieve this. /thinking out loud I'd set this aside for now and we can return to it later. I think it could be quite rare in practice. I was thinking about renaming the symbol too. We can extend the symbol naming convention we have now and deal with it in klp_resolve_symbols(), but maybe Josh will come up with something clever and cleaner. Miroslav