Re: [PATCH v3 0/9] klp-convert livepatch build tooling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 4/24/19 3:19 PM, Miroslav Benes wrote:
[...]

Result: a small tweak to sympos_sanity_check() to relax its symbol
uniqueness verification:  allow for duplicate <object, name, position>
instances.  Now it will only complain when a supplied symbol references
the same <object, name> but a different <position>.

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 '\<state_show\>'
      30: 0000000000000000      0 NOTYPE  GLOBAL DEFAULT    UNDEF state_show

   % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_b.o | grep '\<state_show\>'
      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 <sympos> 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 '\<state_show\>'
      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 <object, name>
symbols with unique <position> 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.

I think this could work well, but (sorry if I understood Joe's idea wrongly) not as a linker step. Instead of modifying the linker, I think we could create another tool and plug it into the kbuild pipeline prior to the livepatch module linking. This way, we would parse the .o elf files, check for homonyms and rename them based on a convention that is later understood by klp-convert, as suggested.

If I am not missing something, this would fix the case where we have homonyms pointing to the same or different positions, without additional user intervention other then adding the SYMPOS annotations.

If you consider this to be useful I can start experiencing.

Miroslav




[Index of Archives]     [Linux&nblp;USB Development]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite Secrets]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux