| Message ID | 20190410155058.9437-1-joe.lawrence@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | klp-convert livepatch build tooling | expand |
On Wed, Apr 10, 2019 at 11:50:49AM -0400, Joe Lawrence wrote: > Hi folks, > > This is the third installment of the klp-convert tool for generating and > processing livepatch symbols for livepatch module builds. For those > following along at home, archive links to previous versions: > > RFC: > https://lore.kernel.org/lkml/cover.1477578530.git.jpoimboe@redhat.com/ > v2: > https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878faf2@redhat.com/ > > (Note that I don't see v2 archived at lore, but that is a link to the > most recent subthread that lore did catch.) > > > Livepatches may use symbols which are not contained in its own scope, > and, because of that, may end up compiled with relocations that will > only be resolved during module load. Yet, when the referenced symbols are > not exported, solving this relocation requires information on the object > that holds the symbol (either vmlinux or modules) and its position inside > the object, as an object may contain multiple symbols with the same name. > Providing such information must be done accordingly to what is specified > in Documentation/livepatch/module-elf-format.txt. > > Currently, there is no trivial way to embed the required information as > requested in the final livepatch elf object. klp-convert solves this > problem in two different forms: (i) by relying on a symbol map, which is > built during kernel compilation, to automatically infer the relocation > targeted symbol, and, when such inference is not possible (ii) by using > annotations in the elf object to convert the relocation accordingly to > the specification, enabling it to be handled by the livepatch loader. > > Given the above, add support for symbol mapping in the form of > Symbols.list file; add klp-convert tool; integrate klp-convert tool into > kbuild; make livepatch modules discernible during kernel compilation > pipeline; add data-structure and macros to enable users to annotate > livepatch source code; make modpost stage compatible with livepatches; > update livepatch-sample and update documentation. > > The patch was tested under three use-cases: > > use-case 1: There is a relocation in the lp that can be automatically > resolved by klp-convert. For example. see the saved_command_line > variable in lib/livepatch/test_klp_convert2.c. > > use-case 2: There is a relocation in the lp that cannot be automatically > resolved, as the name of the respective symbol appears in multiple > objects. The livepatch contains an annotation to enable a correct > relocation. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections > in lib/livepatch/test_klp_convert{1,2}.c. > > use-case 3: There is a relocation in the lp that cannot be automatically > resolved similarly as 2, but no annotation was provided in the > livepatch, triggering an error during compilation. Reproducible by > removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in > lib/livepatch/test_klp_convert{1,2}.c. > > > Branches > -------- > > Since I spent some time tinkering with v2 and accumulated a bunch of > fixes, I collected them up and am posting this new version. For review > purposes, I posted two branches up to github: > > 1 - an expanded branch that with changes separate from the original > https://github.com/joe-lawrence/linux/commits/klp-convert-v3-expanded > > 2 - a squashed branch of (1) that comprises v3: > https://github.com/joe-lawrence/linux/commits/klp-convert-v3 > > Non-trivial commits in the expanded branch have some extra commentary > and details for debugging in the commit message that were dropped when > squashing into their respective parent commits. > > > TODO > ---- > > There are a few outstanding items that I came across while reviewing v2, > but as changes started accumulating, it made sense to defer those to a > later version. I'll reply to this thread individual topics to start > discussion sub-threads for those. > > > Changelogs > ---------- > > The commit changelogs were getting long, so I've moved them here for > posterity and review purposes: > > livepatch: Create and include UAPI headers > v2: > - jmoreira: split up and changelog > v3: > - joe: convert to SPDX license tags > > kbuild: Support for Symbols.list creation > v3: > - jmoreira: adjust for multiobject livepatch > - joe: add klpclean to PHONY > - joe: align KLP prefix > > livepatch: Add klp-convert tool > v2: > - khlebnikov: use HOSTLOADLIBES_ instead of HOSTLDFLAGS: -lelf must be > at the end > - jmoreira: add support to automatic relocation conversion in > klp-convert.c, changelog > v3: > - joe: convert to SPDX license tags > - jmoreira: add rela symbol name to WARNs > - jmoreira: ignore relocations to .TOC and symbols with index 0 > - joe: separate and fix valid_sympos() sympos=0 and sympos=1..n checks > - joe: fix symbol use-after-frees > - joe: fix remaining valgrind leak complaints (non-error paths only) > - joe: checkpatch nits > > livepatch: Add klp-convert annotation helpers > v2: > - jmoreira: split up: move KLP_MODULE_RELOC from previous patch to > here, add KLP_SYMPOS, move macros from include/uapi/livepatch.h to > include/linux/livepatch.h > v3: > - joe: suggested by jpoimboe, KLP_MODULE_RELOC macro should 4-byte > align klp_module_reloc structures > > modpost: Integrate klp-convert > v2: > - khlebnikov: save cmd_ld_ko_o into .module.cmd, if_changed_rule > doesn't do that.f > - khlebnikov: fix bashisms for debian where /bin/sh is a symlink to > /bin/dash > - khlebnikov: rename rule_link_module to rule_ld_ko_o, otherwise > arg-check inside if_changed_rule compares cmd_link_module and > cmd_ld_ko_o. > - khlebnikov: check modinfo -F livepatch only if CONFIG_LIVEPATCH is > true. > - mbenes: remove modinfo call. LIVEPATCH_ in Makefile > - jmoreira: split up: move the .livepatch file-based scheme for > identifying livepatches to a previous patch, as it was required for > correctly building Symbols.list there. > v3: > - joe: adjust rule_ld_ko_o to call echo-cmd > - joe: rebase for v5.1 > - joe: align KLP prefix > > modpost: Add modinfo flag to livepatch modules > v2: > - jmoreira: fix modpost.c (add_livepatch_flag) to update module > structure with livepatch flag and prevent modpost from breaking due to > unresolved symbols > v3: > - joe: adjust modpost.c::get_modinfo() call for v5.0 version > > livepatch: Add sample livepatch module > v3: > - joe: update all in-tree livepatches with LIVEPATCH_* modinfo flag > > documentation: Update on livepatch elf format > v2: > - jmoreira: update module to use KLP_SYMPOS > - jmoreira: Comments on symbol resolution scheme > - jmoreira: Update Makefile > - jmoreira: Remove MODULE_INFO statement > - jmoreira: Changelog > v3: > - joe: rebase for v5.1 > - joe: code shuffle to better match original source file > > livepatch/selftests: add klp-convert > v3: > - joe: clarify sympos=0 and sympos=1..n > > > And now the usual git cover-letter boilerplate... > > Joao Moreira (2): > kbuild: Support for Symbols.list creation > documentation: Update on livepatch elf format > > Joe Lawrence (1): > livepatch/selftests: add klp-convert > > Josh Poimboeuf (5): > livepatch: Create and include UAPI headers > livepatch: Add klp-convert tool > livepatch: Add klp-convert annotation helpers > modpost: Integrate klp-convert > livepatch: Add sample livepatch module > > Miroslav Benes (1): > modpost: Add modinfo flag to livepatch modules > > .gitignore | 1 + > Documentation/livepatch/livepatch.txt | 3 + > Documentation/livepatch/module-elf-format.txt | 50 +- > MAINTAINERS | 2 + > Makefile | 30 +- > include/linux/livepatch.h | 13 + > include/uapi/linux/livepatch.h | 22 + > kernel/livepatch/core.c | 4 +- > lib/livepatch/Makefile | 15 + > lib/livepatch/test_klp_atomic_replace.c | 1 - > lib/livepatch/test_klp_callbacks_demo.c | 1 - > lib/livepatch/test_klp_callbacks_demo2.c | 1 - > lib/livepatch/test_klp_convert1.c | 106 +++ > lib/livepatch/test_klp_convert2.c | 103 +++ > lib/livepatch/test_klp_convert_mod_a.c | 25 + > lib/livepatch/test_klp_convert_mod_b.c | 13 + > lib/livepatch/test_klp_livepatch.c | 1 - > samples/livepatch/Makefile | 7 + > .../livepatch/livepatch-annotated-sample.c | 102 +++ > samples/livepatch/livepatch-sample.c | 1 - > scripts/Kbuild.include | 4 +- > scripts/Makefile | 1 + > scripts/Makefile.build | 7 + > scripts/Makefile.modpost | 24 +- > scripts/livepatch/.gitignore | 1 + > scripts/livepatch/Makefile | 7 + > scripts/livepatch/elf.c | 753 ++++++++++++++++++ > scripts/livepatch/elf.h | 72 ++ > scripts/livepatch/klp-convert.c | 692 ++++++++++++++++ > scripts/livepatch/klp-convert.h | 39 + > scripts/livepatch/list.h | 391 +++++++++ > scripts/mod/modpost.c | 82 +- > scripts/mod/modpost.h | 1 + > .../selftests/livepatch/test-livepatch.sh | 64 ++ > 34 files changed, 2616 insertions(+), 23 deletions(-) > create mode 100644 include/uapi/linux/livepatch.h > create mode 100644 lib/livepatch/test_klp_convert1.c > create mode 100644 lib/livepatch/test_klp_convert2.c > create mode 100644 lib/livepatch/test_klp_convert_mod_a.c > create mode 100644 lib/livepatch/test_klp_convert_mod_b.c > create mode 100644 samples/livepatch/livepatch-annotated-sample.c > create mode 100644 scripts/livepatch/.gitignore > create mode 100644 scripts/livepatch/Makefile > create mode 100644 scripts/livepatch/elf.c > create mode 100644 scripts/livepatch/elf.h > create mode 100644 scripts/livepatch/klp-convert.c > create mode 100644 scripts/livepatch/klp-convert.h > create mode 100644 scripts/livepatch/list.h > > -- > 2.20.1 [ cc += Jason and Evgenii ] Apologies for the long brain dump, but I promised to reply to this thread with some of the larger TODO issues I came across with this patchset. Static key support is probably the most complicated item, so there is a lot of debugging output I can provide. See the tl;dr and Future Work sections for the executive summary. Static key support ================== tl;dr ----- Livepatch symbols are created when building livepatch modules that reference the (non-exported) static keys of a target object. When loading a livepatch that contains klp-converted static key symbols, the module loader crashes. Testing setup ------------- The easiest way to see the source and repro is to grab this branch: https://github.com/joe-lawrence/linux/commits/klp-convert-v4-static-keys and note these last two commits: - livepatch/klp-convert: abort on static key conversion: this will prevent klp-convert from converting any relocations to the __jump_table. Revert this commit to see the crash. If we don't have a fix before merging, I would suggest a temporary abort like this to avoid silently creating dangerous livepatches. - livepatch/selftests: add livepatch static keys: this adds the self-test which will repro the crash. I added a new livepatch selftest to verify klp-convert and static key behavior: it consists of two kernel modules: test_klp_static_keys_mod.ko and livepatch module that patches it, test_klp_static_keys.ko. The base module contains a few different types of static keys: default true / false, exported / not-exported, and one created by the trace event macro infrastructure. The livepatch module references each of these, relying upon klp-convert. This module also provides its own static key for reference. test_klp_static_keys_mod.ko --------------------------- module_key (false) - exported module_key2 (true) <---- TRACE_EVENT(test_klp_static_keys) <-- | | | | | klp-convert resolves test_klp_static_keys.ko - livepatch | | these references ----------------------------------- | | klp_key (true) | | test_klp_static_keys ----------------- | module_key2 ---------------------------- module_key Currently .klp symbols are created as well as a relocation section for those symbols in the their corresponding .text locations. test_klp_static_keys_mod.ko --------------------------- % eu-readelf --symbols lib/livepatch/test_klp_static_keys_mod.ko | \ grep -e '__tracepoint_test_klp_static_keys' -e '\<module_key[2]*\>' 49: 0000000000000010 16 OBJECT LOCAL DEFAULT 44 module_key 65: 0000000000000010 16 OBJECT LOCAL DEFAULT 32 module_key2 96: 0000000000000000 48 OBJECT GLOBAL DEFAULT 40 __tracepoint_test_klp_static_keys test_klp_static_keys.klp.o -------------------------- % eu-readelf --symbols lib/livepatch/test_klp_static_keys.klp.o | \ grep -e '__tracepoint_test_klp_static_keys' -e '\<module_key[2]*\>' 71: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF __tracepoint_test_klp_static_keys 78: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF module_key2 84: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF module_key Lots of __tracepoint_test_klp_static_keys relocations since each module function registers an event when it is called: % eu-readelf --reloc lib/livepatch/test_klp_static_keys.klp.o Relocation section [ 4] '.rela.text' for section [ 3] '.text' at offset 0x2e7d8 contains 88 entries: Offset Type Value Addend Name ... 0x000000000000002c X86_64_32S 000000000000000000 +0 module_key2 ... 0x000000000000007c X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys ... 0x00000000000000d9 X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys ... 0x0000000000000169 X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys ... 0x00000000000001f9 X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys ... 0x000000000000028c X86_64_32S 000000000000000000 +0 module_key ... 0x00000000000002dc X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys ... 0x000000000000038c X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys ... 0x00000000000003eb X86_64_PC32 000000000000000000 +36 __tracepoint_test_klp_static_keys Relocations generated for the __jump_table itself, note that I grouped each jump entry <code, target, key> relocation set: Relocation section [19] '.rela__jump_table' for section [18] '__jump_table' at offset 0x2fb28 contains 30 entries: Offset Type Value Addend Name 000000000000000000 X86_64_PC32 000000000000000000 +12 .text 0x0000000000000004 X86_64_PC32 000000000000000000 +102 .text 0x0000000000000008 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys 0x0000000000000010 X86_64_PC32 000000000000000000 +188 .text 0x0000000000000014 X86_64_PC32 000000000000000000 +195 .text 0x0000000000000018 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys 0x0000000000000020 X86_64_PC32 000000000000000000 +304 .text 0x0000000000000024 X86_64_PC32 000000000000000000 +0 .text.unlikely 0x0000000000000028 X86_64_PC64 000000000000000000 +16 .bss 0x0000000000000030 X86_64_PC32 000000000000000000 +332 .text 0x0000000000000034 X86_64_PC32 000000000000000000 +339 .text 0x0000000000000038 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys 0x0000000000000040 X86_64_PC32 000000000000000000 +448 .text 0x0000000000000044 X86_64_PC32 000000000000000000 +52 .text.unlikely 0x0000000000000048 X86_64_PC64 000000000000000000 +0 module_key 0x0000000000000050 X86_64_PC32 000000000000000000 +476 .text 0x0000000000000054 X86_64_PC32 000000000000000000 +483 .text 0x0000000000000058 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys 0x0000000000000060 X86_64_PC32 000000000000000000 +592 .text 0x0000000000000064 X86_64_PC32 000000000000000000 +104 .text.unlikely 0x0000000000000068 X86_64_PC64 000000000000000000 +1 module_key2 0x0000000000000070 X86_64_PC32 000000000000000000 +620 .text 0x0000000000000074 X86_64_PC32 000000000000000000 +710 .text 0x0000000000000078 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys 0x0000000000000080 X86_64_PC32 000000000000000000 +796 .text 0x0000000000000084 X86_64_PC32 000000000000000000 +886 .text 0x0000000000000088 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys 0x0000000000000090 X86_64_PC32 000000000000000000 +962 .text 0x0000000000000094 X86_64_PC32 000000000000000000 +981 .text 0x0000000000000098 X86_64_PC64 000000000000000000 +8 __tracepoint_test_klp_static_keys test_klp_static_keys.ko ----------------------- Finally klp-convert has transformed a bunch of symbols for us: % eu-readelf --symbols lib/livepatch/test_klp_static_keys.ko | \ grep -e '__tracepoint_test_klp_static_keys' -e '\<module_key[2]*\>' 71: 0000000000000000 0 NOTYPE GLOBAL DEFAULT LOOS+0 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 78: 0000000000000000 0 NOTYPE GLOBAL DEFAULT LOOS+0 .klp.sym.test_klp_static_keys_mod.module_key2,0 84: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF module_key % eu-readelf --reloc lib/livepatch/test_klp_static_keys.ko Relocation section [19] '.rela__jump_table' for section [18] '__jump_table' at offset 0x2480 contains 30 entries: Offset Type Value Addend Name 000000000000000000 X86_64_PC32 000000000000000000 +12 .text 0x0000000000000004 X86_64_PC32 000000000000000000 +102 .text 0x0000000000000008 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 0x0000000000000010 X86_64_PC32 000000000000000000 +188 .text 0x0000000000000014 X86_64_PC32 000000000000000000 +195 .text 0x0000000000000018 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 0x0000000000000020 X86_64_PC32 000000000000000000 +304 .text 0x0000000000000024 X86_64_PC32 000000000000000000 +0 .text.unlikely 0x0000000000000028 X86_64_PC64 000000000000000000 +16 .bss 0x0000000000000030 X86_64_PC32 000000000000000000 +332 .text 0x0000000000000034 X86_64_PC32 000000000000000000 +339 .text 0x0000000000000038 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 0x0000000000000040 X86_64_PC32 000000000000000000 +448 .text 0x0000000000000044 X86_64_PC32 000000000000000000 +52 .text.unlikely 0x0000000000000048 X86_64_PC64 000000000000000000 +0 module_key 0x0000000000000050 X86_64_PC32 000000000000000000 +476 .text 0x0000000000000054 X86_64_PC32 000000000000000000 +483 .text 0x0000000000000058 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 0x0000000000000060 X86_64_PC32 000000000000000000 +592 .text 0x0000000000000064 X86_64_PC32 000000000000000000 +104 .text.unlikely 0x0000000000000068 X86_64_PC64 000000000000000000 +1 .klp.sym.test_klp_static_keys_mod.module_key2,0 0x0000000000000070 X86_64_PC32 000000000000000000 +620 .text 0x0000000000000074 X86_64_PC32 000000000000000000 +710 .text 0x0000000000000078 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 0x0000000000000080 X86_64_PC32 000000000000000000 +796 .text 0x0000000000000084 X86_64_PC32 000000000000000000 +886 .text 0x0000000000000088 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 0x0000000000000090 X86_64_PC32 000000000000000000 +962 .text 0x0000000000000094 X86_64_PC32 000000000000000000 +981 .text 0x0000000000000098 X86_64_PC64 000000000000000000 +8 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 And here's our final set of klp-converted relocations which include the unexported trace point symbol and module_key2 symbols: Relocation section [44] '.klp.rela.test_klp_static_keys_mod..text' for section [ 3] '.text' at offset 0x55c18 contains 12 entries: Offset Type Value Addend Name ... 0x00000000000003eb X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 0x000000000000038c X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 0x00000000000002dc X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 0x00000000000001f9 X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 0x0000000000000169 X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 0x00000000000000d9 X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 0x000000000000007c X86_64_PC32 000000000000000000 +36 .klp.sym.test_klp_static_keys_mod.__tracepoint_test_klp_static_keys,0 0x000000000000002c X86_64_32S 000000000000000000 +0 .klp.sym.test_klp_static_keys_mod.module_key2,0 ... Current behavior ---------------- Not good. The livepatch successfully builds but crashes on load: % insmod lib/livepatch/test_klp_static_keys_mod.ko % insmod lib/livepatch/test_klp_static_keys.ko BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 #PF error: [normal kernel read fault] PGD 0 P4D 0 Oops: 0000 [#1] SMP PTI CPU: 3 PID: 9367 Comm: insmod Tainted: G E K 5.1.0-rc4+ #4 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20180724_192412-buildhw-07.phx2.fedoraproject.org-1.fc29 04/01/2014 RIP: 0010:jump_label_apply_nops+0x3b/0x60 Code: 02 00 00 48 c1 e5 04 48 01 dd 48 39 eb 74 3a 72 0b eb 36 48 83 c3 10 48 39 dd 76 2d 48 8b 43 08 48 89 c2 83 e0 01 48 83 e2 fc <48> 8b 54 13 10 83 e2 01 38 c2 75 dd 48 89 df 31 f6 48 83 c3 10 e8 RSP: 0018:ffffa8874068fcf8 EFLAGS: 00010206 RAX: 0000000000000000 RBX: ffffffffc07fd000 RCX: 000000000000000d RDX: 000000003f803000 RSI: ffffffffa5077be0 RDI: ffffffffc07fe540 RBP: ffffffffc07fd0a0 R08: ffffa88740f43878 R09: ffffa88740eed000 R10: 0000000000055a4b R11: ffffa88740f43878 R12: ffffa88740f430b8 R13: 0000000000000000 R14: ffffa88740f42df8 R15: 0000000000042b01 FS: 00007f4f1dafb740(0000) GS:ffff9a81fbb80000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000010 CR3: 00000000b5d8a000 CR4: 00000000000006e0 Call Trace: module_finalize+0x184/0x1c0 load_module+0x1400/0x1910 ? kernel_read_file+0x18d/0x1c0 ? __do_sys_finit_module+0xa8/0x110 __do_sys_finit_module+0xa8/0x110 do_syscall_64+0x55/0x1a0 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x7f4f1cae82bd Future work ----------- At the very least, I think this call-chain ordering is wrong for livepatch static key symbols: load_module apply_relocations post_relocation module_finalize jump_label_apply_nops << ... prepare_coming_module blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_COMING, mod); jump_label_module_notify case MODULE_STATE_COMING jump_label_add_module << do_init_module do_one_initcall(mod->init) __init patch_init [kpatch-patch] klp_register_patch klp_init_patch klp_for_each_object(patch, obj) klp_init_object klp_init_object_loaded klp_write_object_relocations << blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_LIVE, mod); jump_label_module_notify case MODULE_STATE_LIVE jump_label_invalidate_module_init where klp_write_object_relocations() is called way *after* jump_label_apply_nops() and jump_label_add_module(). Detection --------- I have been tinkering with some prototype code to defer jump_label_apply_nops() and jump_label_add_module(), but it has been slow going. I think the jist of it is that we're going to need to call these dynamically when individual klp_objects are patched, not when the livepatch module itself loads. If anyone with static key expertise wants to jump in here, let me know. In the meantime, I cooked up a potential followup commit to detect conversion of static key symbols and klp-convert failure. It basically runs through the output .ko's ELF symbols and verifies that none of the converted ones can be found as a .rela__jump_table relocated symbol. It accurately catches the problematic references in test_klp_static_keys.ko thus far. This was based on a similar issue reported as a bug against kpatch-build, in which Josh wrote code to detect this scenario: https://github.com/dynup/kpatch/issues/946 https://github.com/jpoimboe/kpatch/commit/2cd2d27607566aee9590c367e615207ce1ce24c6 I can post ("livepatch/klp-convert: abort on static key conversion") here as a follow commit if it looks reasonable and folks wish to review it... or we can try and tackle static keys before merging klp-convert. -- Joe
On Wed, Apr 10, 2019 at 11:50:49AM -0400, Joe Lawrence wrote: > Hi folks, > > This is the third installment of the klp-convert tool for generating and > processing livepatch symbols for livepatch module builds. For those > following along at home, archive links to previous versions: > > RFC: > https://lore.kernel.org/lkml/cover.1477578530.git.jpoimboe@redhat.com/ > v2: > https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878faf2@redhat.com/ > > (Note that I don't see v2 archived at lore, but that is a link to the > most recent subthread that lore did catch.) > > > Livepatches may use symbols which are not contained in its own scope, > and, because of that, may end up compiled with relocations that will > only be resolved during module load. Yet, when the referenced symbols are > not exported, solving this relocation requires information on the object > that holds the symbol (either vmlinux or modules) and its position inside > the object, as an object may contain multiple symbols with the same name. > Providing such information must be done accordingly to what is specified > in Documentation/livepatch/module-elf-format.txt. > Could we get some details with examples or a sample, sorry I might be dense and missing simple information. The problem being solved is not clear to me from the changelog. > Currently, there is no trivial way to embed the required information as > requested in the final livepatch elf object. klp-convert solves this > problem in two different forms: (i) by relying on a symbol map, which is > built during kernel compilation, to automatically infer the relocation > targeted symbol, and, when such inference is not possible (ii) by using > annotations in the elf object to convert the relocation accordingly to > the specification, enabling it to be handled by the livepatch loader. > > Given the above, add support for symbol mapping in the form of > Symbols.list file; add klp-convert tool; integrate klp-convert tool into > kbuild; make livepatch modules discernible during kernel compilation > pipeline; add data-structure and macros to enable users to annotate > livepatch source code; make modpost stage compatible with livepatches; > update livepatch-sample and update documentation. > > The patch was tested under three use-cases: > > use-case 1: There is a relocation in the lp that can be automatically > resolved by klp-convert. For example. see the saved_command_line > variable in lib/livepatch/test_klp_convert2.c. > > use-case 2: There is a relocation in the lp that cannot be automatically > resolved, as the name of the respective symbol appears in multiple > objects. The livepatch contains an annotation to enable a correct > relocation. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections > in lib/livepatch/test_klp_convert{1,2}.c. > > use-case 3: There is a relocation in the lp that cannot be automatically > resolved similarly as 2, but no annotation was provided in the > livepatch, triggering an error during compilation. Reproducible by > removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in > lib/livepatch/test_klp_convert{1,2}.c. > Are these a part of the series? Balbir Singh > > Branches > -------- > > Since I spent some time tinkering with v2 and accumulated a bunch of > fixes, I collected them up and am posting this new version. For review > purposes, I posted two branches up to github: > > 1 - an expanded branch that with changes separate from the original > https://github.com/joe-lawrence/linux/commits/klp-convert-v3-expanded > > 2 - a squashed branch of (1) that comprises v3: > https://github.com/joe-lawrence/linux/commits/klp-convert-v3 > > Non-trivial commits in the expanded branch have some extra commentary > and details for debugging in the commit message that were dropped when > squashing into their respective parent commits. > > > TODO > ---- > > There are a few outstanding items that I came across while reviewing v2, > but as changes started accumulating, it made sense to defer those to a > later version. I'll reply to this thread individual topics to start > discussion sub-threads for those. > > > Changelogs > ---------- > > The commit changelogs were getting long, so I've moved them here for > posterity and review purposes: > > livepatch: Create and include UAPI headers > v2: > - jmoreira: split up and changelog > v3: > - joe: convert to SPDX license tags > > kbuild: Support for Symbols.list creation > v3: > - jmoreira: adjust for multiobject livepatch > - joe: add klpclean to PHONY > - joe: align KLP prefix > > livepatch: Add klp-convert tool > v2: > - khlebnikov: use HOSTLOADLIBES_ instead of HOSTLDFLAGS: -lelf must be > at the end > - jmoreira: add support to automatic relocation conversion in > klp-convert.c, changelog > v3: > - joe: convert to SPDX license tags > - jmoreira: add rela symbol name to WARNs > - jmoreira: ignore relocations to .TOC and symbols with index 0 > - joe: separate and fix valid_sympos() sympos=0 and sympos=1..n checks > - joe: fix symbol use-after-frees > - joe: fix remaining valgrind leak complaints (non-error paths only) > - joe: checkpatch nits > > livepatch: Add klp-convert annotation helpers > v2: > - jmoreira: split up: move KLP_MODULE_RELOC from previous patch to > here, add KLP_SYMPOS, move macros from include/uapi/livepatch.h to > include/linux/livepatch.h > v3: > - joe: suggested by jpoimboe, KLP_MODULE_RELOC macro should 4-byte > align klp_module_reloc structures > > modpost: Integrate klp-convert > v2: > - khlebnikov: save cmd_ld_ko_o into .module.cmd, if_changed_rule > doesn't do that.f > - khlebnikov: fix bashisms for debian where /bin/sh is a symlink to > /bin/dash > - khlebnikov: rename rule_link_module to rule_ld_ko_o, otherwise > arg-check inside if_changed_rule compares cmd_link_module and > cmd_ld_ko_o. > - khlebnikov: check modinfo -F livepatch only if CONFIG_LIVEPATCH is > true. > - mbenes: remove modinfo call. LIVEPATCH_ in Makefile > - jmoreira: split up: move the .livepatch file-based scheme for > identifying livepatches to a previous patch, as it was required for > correctly building Symbols.list there. > v3: > - joe: adjust rule_ld_ko_o to call echo-cmd > - joe: rebase for v5.1 > - joe: align KLP prefix > > modpost: Add modinfo flag to livepatch modules > v2: > - jmoreira: fix modpost.c (add_livepatch_flag) to update module > structure with livepatch flag and prevent modpost from breaking due to > unresolved symbols > v3: > - joe: adjust modpost.c::get_modinfo() call for v5.0 version > > livepatch: Add sample livepatch module > v3: > - joe: update all in-tree livepatches with LIVEPATCH_* modinfo flag > > documentation: Update on livepatch elf format > v2: > - jmoreira: update module to use KLP_SYMPOS > - jmoreira: Comments on symbol resolution scheme > - jmoreira: Update Makefile > - jmoreira: Remove MODULE_INFO statement > - jmoreira: Changelog > v3: > - joe: rebase for v5.1 > - joe: code shuffle to better match original source file > > livepatch/selftests: add klp-convert > v3: > - joe: clarify sympos=0 and sympos=1..n > > > And now the usual git cover-letter boilerplate... > > Joao Moreira (2): > kbuild: Support for Symbols.list creation > documentation: Update on livepatch elf format > > Joe Lawrence (1): > livepatch/selftests: add klp-convert > > Josh Poimboeuf (5): > livepatch: Create and include UAPI headers > livepatch: Add klp-convert tool > livepatch: Add klp-convert annotation helpers > modpost: Integrate klp-convert > livepatch: Add sample livepatch module > > Miroslav Benes (1): > modpost: Add modinfo flag to livepatch modules > > .gitignore | 1 + > Documentation/livepatch/livepatch.txt | 3 + > Documentation/livepatch/module-elf-format.txt | 50 +- > MAINTAINERS | 2 + > Makefile | 30 +- > include/linux/livepatch.h | 13 + > include/uapi/linux/livepatch.h | 22 + > kernel/livepatch/core.c | 4 +- > lib/livepatch/Makefile | 15 + > lib/livepatch/test_klp_atomic_replace.c | 1 - > lib/livepatch/test_klp_callbacks_demo.c | 1 - > lib/livepatch/test_klp_callbacks_demo2.c | 1 - > lib/livepatch/test_klp_convert1.c | 106 +++ > lib/livepatch/test_klp_convert2.c | 103 +++ > lib/livepatch/test_klp_convert_mod_a.c | 25 + > lib/livepatch/test_klp_convert_mod_b.c | 13 + > lib/livepatch/test_klp_livepatch.c | 1 - > samples/livepatch/Makefile | 7 + > .../livepatch/livepatch-annotated-sample.c | 102 +++ > samples/livepatch/livepatch-sample.c | 1 - > scripts/Kbuild.include | 4 +- > scripts/Makefile | 1 + > scripts/Makefile.build | 7 + > scripts/Makefile.modpost | 24 +- > scripts/livepatch/.gitignore | 1 + > scripts/livepatch/Makefile | 7 + > scripts/livepatch/elf.c | 753 ++++++++++++++++++ > scripts/livepatch/elf.h | 72 ++ > scripts/livepatch/klp-convert.c | 692 ++++++++++++++++ > scripts/livepatch/klp-convert.h | 39 + > scripts/livepatch/list.h | 391 +++++++++ > scripts/mod/modpost.c | 82 +- > scripts/mod/modpost.h | 1 + > .../selftests/livepatch/test-livepatch.sh | 64 ++ > 34 files changed, 2616 insertions(+), 23 deletions(-) > create mode 100644 include/uapi/linux/livepatch.h > create mode 100644 lib/livepatch/test_klp_convert1.c > create mode 100644 lib/livepatch/test_klp_convert2.c > create mode 100644 lib/livepatch/test_klp_convert_mod_a.c > create mode 100644 lib/livepatch/test_klp_convert_mod_b.c > create mode 100644 samples/livepatch/livepatch-annotated-sample.c > create mode 100644 scripts/livepatch/.gitignore > create mode 100644 scripts/livepatch/Makefile > create mode 100644 scripts/livepatch/elf.c > create mode 100644 scripts/livepatch/elf.h > create mode 100644 scripts/livepatch/klp-convert.c > create mode 100644 scripts/livepatch/klp-convert.h > create mode 100644 scripts/livepatch/list.h > > -- > 2.20.1
On Tue, 16 Apr 2019, Balbir Singh wrote: > On Wed, Apr 10, 2019 at 11:50:49AM -0400, Joe Lawrence wrote: > > Hi folks, > > > > This is the third installment of the klp-convert tool for generating and > > processing livepatch symbols for livepatch module builds. For those > > following along at home, archive links to previous versions: > > > > RFC: > > https://lore.kernel.org/lkml/cover.1477578530.git.jpoimboe@redhat.com/ > > v2: > > https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878faf2@redhat.com/ > > > > (Note that I don't see v2 archived at lore, but that is a link to the > > most recent subthread that lore did catch.) > > > > > > Livepatches may use symbols which are not contained in its own scope, > > and, because of that, may end up compiled with relocations that will > > only be resolved during module load. Yet, when the referenced symbols are > > not exported, solving this relocation requires information on the object > > that holds the symbol (either vmlinux or modules) and its position inside > > the object, as an object may contain multiple symbols with the same name. > > Providing such information must be done accordingly to what is specified > > in Documentation/livepatch/module-elf-format.txt. > > > > > Could we get some details with examples or a sample, sorry I might be dense > and missing simple information. The problem being solved is not clear to > me from the changelog. The patch set tries to solve the problem described in Documentation/livepatch/module-elf-format.txt. Currently, there is no tool in upstream to automatically generate the relocation records. kpatch-build can do it and we'd like to have it as well. klp-convert should thus do the job. The sample module is introduced in patch 7 and you can look at the selftests in patch 9 too. Maybe we can describe the problem better in the cover letter, but on the other hand patch changelogs are more important and they are good (well, in my opinion of course). > > Currently, there is no trivial way to embed the required information as > > requested in the final livepatch elf object. klp-convert solves this > > problem in two different forms: (i) by relying on a symbol map, which is > > built during kernel compilation, to automatically infer the relocation > > targeted symbol, and, when such inference is not possible (ii) by using > > annotations in the elf object to convert the relocation accordingly to > > the specification, enabling it to be handled by the livepatch loader. > > > > Given the above, add support for symbol mapping in the form of > > Symbols.list file; add klp-convert tool; integrate klp-convert tool into > > kbuild; make livepatch modules discernible during kernel compilation > > pipeline; add data-structure and macros to enable users to annotate > > livepatch source code; make modpost stage compatible with livepatches; > > update livepatch-sample and update documentation. > > > > The patch was tested under three use-cases: > > > > use-case 1: There is a relocation in the lp that can be automatically > > resolved by klp-convert. For example. see the saved_command_line > > variable in lib/livepatch/test_klp_convert2.c. > > > > use-case 2: There is a relocation in the lp that cannot be automatically > > resolved, as the name of the respective symbol appears in multiple > > objects. The livepatch contains an annotation to enable a correct > > relocation. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections > > in lib/livepatch/test_klp_convert{1,2}.c. > > > > use-case 3: There is a relocation in the lp that cannot be automatically > > resolved similarly as 2, but no annotation was provided in the > > livepatch, triggering an error during compilation. Reproducible by > > removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in > > lib/livepatch/test_klp_convert{1,2}.c. > > > > > Are these a part of the series? Yes, see 9/9. Regards, Miroslav
[...] > Current behavior > ---------------- > > Not good. The livepatch successfully builds but crashes on load: > > % insmod lib/livepatch/test_klp_static_keys_mod.ko > % insmod lib/livepatch/test_klp_static_keys.ko > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 > #PF error: [normal kernel read fault] > PGD 0 P4D 0 > Oops: 0000 [#1] SMP PTI > CPU: 3 PID: 9367 Comm: insmod Tainted: G E K 5.1.0-rc4+ #4 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20180724_192412-buildhw-07.phx2.fedoraproject.org-1.fc29 04/01/2014 > RIP: 0010:jump_label_apply_nops+0x3b/0x60 > Code: 02 00 00 48 c1 e5 04 48 01 dd 48 39 eb 74 3a 72 0b eb 36 48 83 c3 10 48 39 dd 76 2d 48 8b 43 08 48 89 c2 83 e0 01 48 83 e2 fc <48> 8b 54 13 10 83 e2 01 38 c2 75 dd 48 89 df 31 f6 48 83 c3 10 e8 > RSP: 0018:ffffa8874068fcf8 EFLAGS: 00010206 > RAX: 0000000000000000 RBX: ffffffffc07fd000 RCX: 000000000000000d > RDX: 000000003f803000 RSI: ffffffffa5077be0 RDI: ffffffffc07fe540 > RBP: ffffffffc07fd0a0 R08: ffffa88740f43878 R09: ffffa88740eed000 > R10: 0000000000055a4b R11: ffffa88740f43878 R12: ffffa88740f430b8 > R13: 0000000000000000 R14: ffffa88740f42df8 R15: 0000000000042b01 > FS: 00007f4f1dafb740(0000) GS:ffff9a81fbb80000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000000000000010 CR3: 00000000b5d8a000 CR4: 00000000000006e0 > Call Trace: > module_finalize+0x184/0x1c0 > load_module+0x1400/0x1910 > ? kernel_read_file+0x18d/0x1c0 > ? __do_sys_finit_module+0xa8/0x110 > __do_sys_finit_module+0xa8/0x110 > do_syscall_64+0x55/0x1a0 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > RIP: 0033:0x7f4f1cae82bd > > > Future work > ----------- > > At the very least, I think this call-chain ordering is wrong for > livepatch static key symbols: > > load_module > > apply_relocations > > post_relocation > module_finalize > jump_label_apply_nops << > > ... > > prepare_coming_module > blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_COMING, mod); > jump_label_module_notify > case MODULE_STATE_COMING > jump_label_add_module << > > do_init_module > > do_one_initcall(mod->init) > __init patch_init [kpatch-patch] > klp_register_patch > klp_init_patch > klp_for_each_object(patch, obj) > klp_init_object > klp_init_object_loaded > klp_write_object_relocations << > > blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_LIVE, mod); > jump_label_module_notify > case MODULE_STATE_LIVE > jump_label_invalidate_module_init > > where klp_write_object_relocations() is called way *after* > jump_label_apply_nops() and jump_label_add_module(). Quick look, but it seems quite similar to the problem we had with apply_alternatives(). See arch/x86/kernel/livepatch.c and the commit which introduced it. I think, we should do the same for jump labels. Add jump_label_apply_nops() from module_finalize() to arch_klp_init_object_loaded() and convert jump_table ELF section so its processing is delayed. Which leads me another TODO... klp-convert does not convert even .altinstructions and .parainstructions sections, so it has that problem as well. If I remember, it was on Josh's TODO list when he first introduced klp-convert. See cover.1477578530.git.jpoimboe@redhat.com. The selftest for the alternatives would be appreciated too. One day. And of course we should look at the other supported architectures and their module_finalize() functions. I have it on my TODO list somewhere, but you know how it works with those :/. I am sure there are more hidden surprises there. > Detection > --------- > > I have been tinkering with some prototype code to defer > jump_label_apply_nops() and jump_label_add_module(), but it has been > slow going. I think the jist of it is that we're going to need to call > these dynamically when individual klp_objects are patched, not when the > livepatch module itself loads. If anyone with static key expertise > wants to jump in here, let me know. > > In the meantime, I cooked up a potential followup commit to detect > conversion of static key symbols and klp-convert failure. It basically > runs through the output .ko's ELF symbols and verifies that none of the > converted ones can be found as a .rela__jump_table relocated symbol. It > accurately catches the problematic references in test_klp_static_keys.ko > thus far. > > This was based on a similar issue reported as a bug against > kpatch-build, in which Josh wrote code to detect this scenario: > > https://github.com/dynup/kpatch/issues/946 > https://github.com/jpoimboe/kpatch/commit/2cd2d27607566aee9590c367e615207ce1ce24c6 > > I can post ("livepatch/klp-convert: abort on static key conversion") > here as a follow commit if it looks reasonable and folks wish to review > it... or we can try and tackle static keys before merging klp-convert. Good idea. I'd rather fix it, but I think it could be a lot of work, so something like this patch seems to be a good idea. Thanks Miroslav
On 4/16/19 1:24 AM, Balbir Singh wrote: > > Could we get some details with examples or a sample, sorry I might be dense > and missing simple information. The problem being solved is not clear to > me from the changelog. > Hi Balbir, I see that Miroslav has already pointed to documentation, samples and self-tests for the patchset, but here is a summary in my own words: kpatch-build constructs livepatch kernel modules by comparing a reference build with a patched build, combing through ELF object sections and extracting new and changed sections that include patched code. An alternative approach is "source-based", in which a livepatch kernel module is built (mostly entirely) using the kernel's build infrastructure. Sources for a livepatch are gathered ahead of time and then built like an ordinary kernel module. In either approach, there lies the problem of symbol visibility: how can a livepatch resolve symbols that a kernel module ordinarily can't. For example, file local or simply unexported symbols in across the kernel and other modules. Enter the concept of "livepatch symbols" described in module-elf-format.txt. kpatch-build already creates such "livepatch symbols" (see its create_klp_relasecs_and_syms()) and the livepatching core kernel code already knows how resolve such symbols at klp_object patch time (see klp_write_object_relocations()). The klp-convert tool and this supporting patchset would empower source-based-constructed livepatch modules to do the same. -- Joe
On 4/10/19 11:50 AM, Joe Lawrence wrote: > Hi folks, > > This is the third installment of the klp-convert tool for generating and > processing livepatch symbols for livepatch module builds. For those > following along at home, archive links to previous versions: > > RFC: > https://lore.kernel.org/lkml/cover.1477578530.git.jpoimboe@redhat.com/ > v2: > https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878faf2@redhat.com/ > > (Note that I don't see v2 archived at lore, but that is a link to the > most recent subthread that lore did catch.) > > > Livepatches may use symbols which are not contained in its own scope, > and, because of that, may end up compiled with relocations that will > only be resolved during module load. Yet, when the referenced symbols are > not exported, solving this relocation requires information on the object > that holds the symbol (either vmlinux or modules) and its position inside > the object, as an object may contain multiple symbols with the same name. > Providing such information must be done accordingly to what is specified > in Documentation/livepatch/module-elf-format.txt. Hi Miroslav, I noticed that some binutils programs like gdb, objdump, etc. don't like the .ko kernel objects that we're generating from this patchset, specifically those with the additional '.klp.rela.<obj>..text' livepatch symbol relocation sections. For reference, I opened a new bugzilla with more details here: https://sourceware.org/bugzilla/show_bug.cgi?id=24456 And was about to ping the binutils mailing list about the assertion that is tripping in bfd/elf.c. The thought occurred to me that you guys might already be carrying a patch to workaround this issue? -- Joe
On Tue, 16 Apr 2019, Joe Lawrence wrote: > On 4/10/19 11:50 AM, Joe Lawrence wrote: > > Hi folks, > > > > This is the third installment of the klp-convert tool for generating and > > processing livepatch symbols for livepatch module builds. For those > > following along at home, archive links to previous versions: > > > > RFC: > > https://lore.kernel.org/lkml/cover.1477578530.git.jpoimboe@redhat.com/ > > v2: > > https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878faf2@redhat.com/ > > > > (Note that I don't see v2 archived at lore, but that is a link to the > > most recent subthread that lore did catch.) > > > > > > Livepatches may use symbols which are not contained in its own scope, > > and, because of that, may end up compiled with relocations that will > > only be resolved during module load. Yet, when the referenced symbols are > > not exported, solving this relocation requires information on the object > > that holds the symbol (either vmlinux or modules) and its position inside > > the object, as an object may contain multiple symbols with the same name. > > Providing such information must be done accordingly to what is specified > > in Documentation/livepatch/module-elf-format.txt. > > Hi Miroslav, > > I noticed that some binutils programs like gdb, objdump, etc. don't like the > .ko kernel objects that we're generating from this patchset, specifically > those with the additional '.klp.rela.<obj>..text' livepatch symbol relocation > sections. > > For reference, I opened a new bugzilla with more details here: > https://sourceware.org/bugzilla/show_bug.cgi?id=24456 Another great catch. > And was about to ping the binutils mailing list about the assertion that is > tripping in bfd/elf.c. The thought occurred to me that you guys might already > be carrying a patch to workaround this issue? No, unfortunately we don't. At least I don't know about anything and a quick test on openSUSE Leap 15.0 confirms it (objdump -D gives "bad value"). Thanks Miroslav
On Wed, Apr 10, 2019 at 11:50:49AM -0400, Joe Lawrence wrote: > > [ ... snip ... ] > > TODO > ---- > > There are a few outstanding items that I came across while reviewing v2, > but as changes started accumulating, it made sense to defer those to a > later version. I'll reply to this thread individual topics to start > discussion sub-threads for those. Multiple object files ===================== For v3, we tweaked the build scripts so that we could successfully build a klp-converted livepatch module from multiple object files. One interesting use-case is supporting two livepatch symbols of the same name, but different object/position values. An example target kernel module might be layed out like this: test_klp_convert_mod.ko << target module is comprised of two object files, each defining test_klp_convert_mod_a.o their own local get_homonym_string() get_homonym_string() function and homonym_string[] homonym_string[] character arrays. test_klp_convert_mod_b.o get_homonym_string() homonym_string[] A look at interesting parts of the the module's symbol table: % eu-readelf --symbols lib/livepatch/test_klp_convert_mod.ko | \ grep -e 'homonym' -e test_klp_convert_mod_ 29: 0000000000000000 0 FILE LOCAL DEFAULT ABS test_klp_convert_mod_a.c 32: 0000000000000010 8 FUNC LOCAL DEFAULT 3 get_homonym_string 33: 0000000000000000 17 OBJECT LOCAL DEFAULT 5 homonym_string 37: 0000000000000000 0 FILE LOCAL DEFAULT ABS test_klp_convert_mod_b.c 38: 0000000000000020 8 FUNC LOCAL DEFAULT 3 get_homonym_string 39: 0000000000000000 17 OBJECT LOCAL DEFAULT 11 homonym_string suggests that any livepatch module that wishes to resolve to test_klp_convert_mod_a.c :: get_homonym_string() should include an annotation like: file_a.c: KLP_MODULE_RELOC(test_klp_convert_mod) relocs_a[] = { KLP_SYMPOS(get_homonym_string, 1), }; and to resolve test_klp_convert_mod_b.c :: get_homonym_string(): file_b.c: KLP_MODULE_RELOC(test_klp_convert_mod) relocs_b[] = { KLP_SYMPOS(get_homonym_string, 2), }; Unfortunately klp-convert v3 will fail to build such livepatch module, regardless of sympos values: klp-convert: Conflicting KLP_SYMPOS definition: vmlinux.state_show,2 vs. vmlinux.state_show,1. klp-convert: Unable to load user-provided sympos make[1]: *** [scripts/Makefile.modpost:148: lib/livepatch/test_klp_convert_multi_objs.ko] Error 255 This abort message can be traced to sympos_sanity_check() as it verifies that there no duplicate symbol names found in its list of user specified symbols (ie, those needing to be converted). Common sympos ------------- New test case and related fixup can be found here: https://github.com/joe-lawrence/linux/commits/klp-convert-v4-multiple-objs To better debug this issue, I added another selftest that demonstrates this configuration in isolation. "show_state" is a popular kernel function name (my VM reported 5 instances in kallsyms) so I chose that symbol name. Initially I specified the *same* symbol position (1) in both .c file KLP_SYMPOS annotations. At the very least, we can trivially patch klp-convert v3 to support annotations for the the same symbol <object, name, position> value across object files. 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); 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 In the meantime, the unique symbol <position> case is already detected and with a little tweaking we could support multiple common symbol <position> values. -- Joe
[...] > 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. Miroslav
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 >
On Wed, Apr 24, 2019 at 04:13:59PM -0300, Joao Moreira wrote: > > > 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. I tend to agree with Miroslav that it's probably ok to ignore this issue for now, as long as klp-convert can detect it and spit out an error. I assume the patch author could work around it with a kallsyms hack. If we encounter it much in the real world then we could figure out a solution at that point.
On 4/24/19 3:13 PM, Joao Moreira wrote: >>> 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 agree, especially since we can detect this corner case and abort the translation. >> 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. My knowledge of the build tools is limited, so there was a bunch of hand-waving you couldn't see when I wrote that paragraph :) But yes, that is basically the idea: plugging into the kbuild pipeline to give these some kinda of .o-unique prefix that klp-convert would interpret and strip accordingly. > 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. > It's not the highest priority, but even a prototype of how to insert a script into the pipeline to achieve this would be massively time saving for myself. If renaming looks easy, we could try to work into the initial klp-convert patchset... if not, save it for a follow up enhancement. Thanks, -- Joe
On Tue, Apr 16, 2019 at 01:37:13PM +0200, Miroslav Benes wrote: > > [ ... snip ... ] > > Quick look, but it seems quite similar to the problem we had with > apply_alternatives(). See arch/x86/kernel/livepatch.c and the commit which > introduced it. That was an interesting diversion :) I think I grok the idea as: The kernel supports a few different code-patching methods: - SMP locks - alternatives - paravirt - jump labels and we need to ensure that they do not prematurely operate on unresolved klp-relocations. The solution that led to arch/x86/kernel/livepatch.c introduces "klp.arch" sections that rename such klp-relocations *and* their associated special section data structures. Processing is then deferred until after a relevant klp_object is loaded. > I think, we should do the same for jump labels. Add > jump_label_apply_nops() from module_finalize() to > arch_klp_init_object_loaded() and convert jump_table ELF section so its > processing is delayed. Nod. Tthat sounds about right. There may be some more work yet in the static keys API as well, but I'm not 100%. > Which leads me another TODO... klp-convert does not convert even > .altinstructions and .parainstructions sections, so it has that problem as > well. If I remember, it was on Josh's TODO list when he first introduced > klp-convert. See cover.1477578530.git.jpoimboe@redhat.com. In the RFC, Josh highlights a somewhat difficult problem regarding these special sections -- how to associate these special section data structures and their relocations to a specific klp_object. If I understand his suggestion, he proposed annotating livepatch module replacement functions as to stuff them into specially named ELF sections (which would include the klp_object name) and then bypass the existing livepatch registration API. No minor change. With that in mind, I'm starting to think of a game plan for klp-convert like: - phase 1: detect /abort unsupported sections - phase 2: manual annotations in livepatch modules (like KLP_MODULE_RELOC / SYMPOS, but for special sections) so that klp-convert can start building "klp.arch" sections - phase 3: livepatch API change above to support somewhat more automatic generation of phase 2 annotations > The selftest for the alternatives would be appreciated too. One day. In the course of understanding the background behind arch/x86/kernel/livepatch.c, I wrote a bunch of livepatch selftests that try out simple examples of those special sections. For alternatives, I did something like: /* TODO: find reliably true/false features */ #define TRUE_FEATURE (X86_FEATURE_FPU) #define FALSE_FEATURE (X86_FEATURE_VME) ... klp_function1() klp_function2() klp_new_function() asm (ALTERNATIVE("call klp_function1", "call klp_function2", TRUE_FEATURE)); asm (ALTERNATIVE("call klp_function1", "call klp_function2", FALSE_FEATURE)); asm (ALTERNATIVE("call mod_function1", "call mod_function2", TRUE_FEATURE)); asm (ALTERNATIVE("call mod_function1", "call mod_function2", FALSE_FEATURE)); asm (ALTERNATIVE("call mod_function2", "call mod_function1", TRUE_FEATURE)); asm (ALTERNATIVE("call mod_function2", "call mod_function1", FALSE_FEATURE)); so that I could see what kind of relocations were generated for default and non-default instructions as well as module-local and then unexported-extern functions. Once we have klp-convert supporting these conversions, I think something like that would suffice. In the meantime, I'm not sure how to create "klp.arch" sectioned ELFs without something like kpatch-build. > And of course we should look at the other supported architectures and > their module_finalize() functions. I have it on my TODO list somewhere, > but you know how it works with those :/. I am sure there are more hidden > surprises there. Hmm, well powerpc and s390 do appear to have processing for special sections as well ... but for the moment, I'm going to focus on x86 as that seems like enough work for now :) > > Detection > > --------- > > > > I can post ("livepatch/klp-convert: abort on static key conversion") > > here as a follow commit if it looks reasonable and folks wish to review > > it... or we can try and tackle static keys before merging klp-convert. > > Good idea. I'd rather fix it, but I think it could be a lot of work, so > something like this patch seems to be a good idea. I'm thinking of adding this in a commit so klp-convert can intercept these sections: static bool is_section_supported(char *sname) { if (strcmp(sname, ".rela.altinstructions") == 0) return false; if (strcmp(sname, ".rela.parainstructions") == 0) return false; if (strcmp(sname, ".rela__jump_table") == 0) return false; return true; } Right now my v4 collection has a bunch of small fixups and nitpick corrections. It feels like a good resting place for now before embarking on special section support, what do you think? -- Joe
On Fri, 3 May 2019, Joe Lawrence wrote: > On Tue, Apr 16, 2019 at 01:37:13PM +0200, Miroslav Benes wrote: > > > > [ ... snip ... ] > > > > Quick look, but it seems quite similar to the problem we had with > > apply_alternatives(). See arch/x86/kernel/livepatch.c and the commit which > > introduced it. > > That was an interesting diversion :) I think I grok the idea as: > > The kernel supports a few different code-patching methods: > > - SMP locks > - alternatives > - paravirt > - jump labels > > and we need to ensure that they do not prematurely operate on unresolved > klp-relocations. The solution that led to arch/x86/kernel/livepatch.c > introduces "klp.arch" sections that rename such klp-relocations *and* > their associated special section data structures. Processing is then > deferred until after a relevant klp_object is loaded. Correct. > > I think, we should do the same for jump labels. Add > > jump_label_apply_nops() from module_finalize() to > > arch_klp_init_object_loaded() and convert jump_table ELF section so its > > processing is delayed. > > Nod. Tthat sounds about right. There may be some more work yet in the > static keys API as well, but I'm not 100%. > > > Which leads me another TODO... klp-convert does not convert even > > .altinstructions and .parainstructions sections, so it has that problem as > > well. If I remember, it was on Josh's TODO list when he first introduced > > klp-convert. See cover.1477578530.git.jpoimboe@redhat.com. > > In the RFC, Josh highlights a somewhat difficult problem regarding these > special sections -- how to associate these special section data > structures and their relocations to a specific klp_object. > > If I understand his suggestion, he proposed annotating livepatch module > replacement functions as to stuff them into specially named ELF sections > (which would include the klp_object name) and then bypass the existing > livepatch registration API. No minor change. > > With that in mind, I'm starting to think of a game plan for klp-convert > like: > > - phase 1: detect /abort unsupported sections > > - phase 2: manual annotations in livepatch modules (like > KLP_MODULE_RELOC / SYMPOS, but for special sections) so > that klp-convert can start building "klp.arch" sections > > - phase 3: livepatch API change above to support somewhat more > automatic generation of phase 2 annotations Looks good to me. First, I'd focus on something which covers (hopefully) a vast majority cases and then we can do the rest. So yes, this seems to be a good plan. > > And of course we should look at the other supported architectures and > > their module_finalize() functions. I have it on my TODO list somewhere, > > but you know how it works with those :/. I am sure there are more hidden > > surprises there. > > Hmm, well powerpc and s390 do appear to have processing for special > sections as well ... but for the moment, I'm going to focus on x86 as > that seems like enough work for now :) Yes, please :). > > > Detection > > > --------- > > > > > > I can post ("livepatch/klp-convert: abort on static key conversion") > > > here as a follow commit if it looks reasonable and folks wish to review > > > it... or we can try and tackle static keys before merging klp-convert. > > > > Good idea. I'd rather fix it, but I think it could be a lot of work, so > > something like this patch seems to be a good idea. > > I'm thinking of adding this in a commit so klp-convert can intercept > these sections: > > static bool is_section_supported(char *sname) > { > if (strcmp(sname, ".rela.altinstructions") == 0) > return false; > if (strcmp(sname, ".rela.parainstructions") == 0) > return false; > if (strcmp(sname, ".rela__jump_table") == 0) > return false; > return true; > } > > Right now my v4 collection has a bunch of small fixups and nitpick > corrections. It feels like a good resting place for now before > embarking on special section support, what do you think? Yes. Thanks Miroslav
Hi folks, This is the third installment of the klp-convert tool for generating and processing livepatch symbols for livepatch module builds. For those following along at home, archive links to previous versions: RFC: https://lore.kernel.org/lkml/cover.1477578530.git.jpoimboe@redhat.com/ v2: https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878faf2@redhat.com/ (Note that I don't see v2 archived at lore, but that is a link to the most recent subthread that lore did catch.) Livepatches may use symbols which are not contained in its own scope, and, because of that, may end up compiled with relocations that will only be resolved during module load. Yet, when the referenced symbols are not exported, solving this relocation requires information on the object that holds the symbol (either vmlinux or modules) and its position inside the object, as an object may contain multiple symbols with the same name. Providing such information must be done accordingly to what is specified in Documentation/livepatch/module-elf-format.txt. Currently, there is no trivial way to embed the required information as requested in the final livepatch elf object. klp-convert solves this problem in two different forms: (i) by relying on a symbol map, which is built during kernel compilation, to automatically infer the relocation targeted symbol, and, when such inference is not possible (ii) by using annotations in the elf object to convert the relocation accordingly to the specification, enabling it to be handled by the livepatch loader. Given the above, add support for symbol mapping in the form of Symbols.list file; add klp-convert tool; integrate klp-convert tool into kbuild; make livepatch modules discernible during kernel compilation pipeline; add data-structure and macros to enable users to annotate livepatch source code; make modpost stage compatible with livepatches; update livepatch-sample and update documentation. The patch was tested under three use-cases: use-case 1: There is a relocation in the lp that can be automatically resolved by klp-convert. For example. see the saved_command_line variable in lib/livepatch/test_klp_convert2.c. use-case 2: There is a relocation in the lp that cannot be automatically resolved, as the name of the respective symbol appears in multiple objects. The livepatch contains an annotation to enable a correct relocation. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in lib/livepatch/test_klp_convert{1,2}.c. use-case 3: There is a relocation in the lp that cannot be automatically resolved similarly as 2, but no annotation was provided in the livepatch, triggering an error during compilation. Reproducible by removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in lib/livepatch/test_klp_convert{1,2}.c. Branches -------- Since I spent some time tinkering with v2 and accumulated a bunch of fixes, I collected them up and am posting this new version. For review purposes, I posted two branches up to github: 1 - an expanded branch that with changes separate from the original https://github.com/joe-lawrence/linux/commits/klp-convert-v3-expanded 2 - a squashed branch of (1) that comprises v3: https://github.com/joe-lawrence/linux/commits/klp-convert-v3 Non-trivial commits in the expanded branch have some extra commentary and details for debugging in the commit message that were dropped when squashing into their respective parent commits. TODO ---- There are a few outstanding items that I came across while reviewing v2, but as changes started accumulating, it made sense to defer those to a later version. I'll reply to this thread individual topics to start discussion sub-threads for those. Changelogs ---------- The commit changelogs were getting long, so I've moved them here for posterity and review purposes: livepatch: Create and include UAPI headers v2: - jmoreira: split up and changelog v3: - joe: convert to SPDX license tags kbuild: Support for Symbols.list creation v3: - jmoreira: adjust for multiobject livepatch - joe: add klpclean to PHONY - joe: align KLP prefix livepatch: Add klp-convert tool v2: - khlebnikov: use HOSTLOADLIBES_ instead of HOSTLDFLAGS: -lelf must be at the end - jmoreira: add support to automatic relocation conversion in klp-convert.c, changelog v3: - joe: convert to SPDX license tags - jmoreira: add rela symbol name to WARNs - jmoreira: ignore relocations to .TOC and symbols with index 0 - joe: separate and fix valid_sympos() sympos=0 and sympos=1..n checks - joe: fix symbol use-after-frees - joe: fix remaining valgrind leak complaints (non-error paths only) - joe: checkpatch nits livepatch: Add klp-convert annotation helpers v2: - jmoreira: split up: move KLP_MODULE_RELOC from previous patch to here, add KLP_SYMPOS, move macros from include/uapi/livepatch.h to include/linux/livepatch.h v3: - joe: suggested by jpoimboe, KLP_MODULE_RELOC macro should 4-byte align klp_module_reloc structures modpost: Integrate klp-convert v2: - khlebnikov: save cmd_ld_ko_o into .module.cmd, if_changed_rule doesn't do that.f - khlebnikov: fix bashisms for debian where /bin/sh is a symlink to /bin/dash - khlebnikov: rename rule_link_module to rule_ld_ko_o, otherwise arg-check inside if_changed_rule compares cmd_link_module and cmd_ld_ko_o. - khlebnikov: check modinfo -F livepatch only if CONFIG_LIVEPATCH is true. - mbenes: remove modinfo call. LIVEPATCH_ in Makefile - jmoreira: split up: move the .livepatch file-based scheme for identifying livepatches to a previous patch, as it was required for correctly building Symbols.list there. v3: - joe: adjust rule_ld_ko_o to call echo-cmd - joe: rebase for v5.1 - joe: align KLP prefix modpost: Add modinfo flag to livepatch modules v2: - jmoreira: fix modpost.c (add_livepatch_flag) to update module structure with livepatch flag and prevent modpost from breaking due to unresolved symbols v3: - joe: adjust modpost.c::get_modinfo() call for v5.0 version livepatch: Add sample livepatch module v3: - joe: update all in-tree livepatches with LIVEPATCH_* modinfo flag documentation: Update on livepatch elf format v2: - jmoreira: update module to use KLP_SYMPOS - jmoreira: Comments on symbol resolution scheme - jmoreira: Update Makefile - jmoreira: Remove MODULE_INFO statement - jmoreira: Changelog v3: - joe: rebase for v5.1 - joe: code shuffle to better match original source file livepatch/selftests: add klp-convert v3: - joe: clarify sympos=0 and sympos=1..n And now the usual git cover-letter boilerplate... Joao Moreira (2): kbuild: Support for Symbols.list creation documentation: Update on livepatch elf format Joe Lawrence (1): livepatch/selftests: add klp-convert Josh Poimboeuf (5): livepatch: Create and include UAPI headers livepatch: Add klp-convert tool livepatch: Add klp-convert annotation helpers modpost: Integrate klp-convert livepatch: Add sample livepatch module Miroslav Benes (1): modpost: Add modinfo flag to livepatch modules .gitignore | 1 + Documentation/livepatch/livepatch.txt | 3 + Documentation/livepatch/module-elf-format.txt | 50 +- MAINTAINERS | 2 + Makefile | 30 +- include/linux/livepatch.h | 13 + include/uapi/linux/livepatch.h | 22 + kernel/livepatch/core.c | 4 +- lib/livepatch/Makefile | 15 + lib/livepatch/test_klp_atomic_replace.c | 1 - lib/livepatch/test_klp_callbacks_demo.c | 1 - lib/livepatch/test_klp_callbacks_demo2.c | 1 - lib/livepatch/test_klp_convert1.c | 106 +++ lib/livepatch/test_klp_convert2.c | 103 +++ lib/livepatch/test_klp_convert_mod_a.c | 25 + lib/livepatch/test_klp_convert_mod_b.c | 13 + lib/livepatch/test_klp_livepatch.c | 1 - samples/livepatch/Makefile | 7 + .../livepatch/livepatch-annotated-sample.c | 102 +++ samples/livepatch/livepatch-sample.c | 1 - scripts/Kbuild.include | 4 +- scripts/Makefile | 1 + scripts/Makefile.build | 7 + scripts/Makefile.modpost | 24 +- scripts/livepatch/.gitignore | 1 + scripts/livepatch/Makefile | 7 + scripts/livepatch/elf.c | 753 ++++++++++++++++++ scripts/livepatch/elf.h | 72 ++ scripts/livepatch/klp-convert.c | 692 ++++++++++++++++ scripts/livepatch/klp-convert.h | 39 + scripts/livepatch/list.h | 391 +++++++++ scripts/mod/modpost.c | 82 +- scripts/mod/modpost.h | 1 + .../selftests/livepatch/test-livepatch.sh | 64 ++ 34 files changed, 2616 insertions(+), 23 deletions(-) create mode 100644 include/uapi/linux/livepatch.h create mode 100644 lib/livepatch/test_klp_convert1.c create mode 100644 lib/livepatch/test_klp_convert2.c create mode 100644 lib/livepatch/test_klp_convert_mod_a.c create mode 100644 lib/livepatch/test_klp_convert_mod_b.c create mode 100644 samples/livepatch/livepatch-annotated-sample.c create mode 100644 scripts/livepatch/.gitignore create mode 100644 scripts/livepatch/Makefile create mode 100644 scripts/livepatch/elf.c create mode 100644 scripts/livepatch/elf.h create mode 100644 scripts/livepatch/klp-convert.c create mode 100644 scripts/livepatch/klp-convert.h create mode 100644 scripts/livepatch/list.h