Re: [PATCH v5 0/3] selftests: livepatch: test livepatching a kprobed function
On 10/17/24 16:01, Michael Vetter wrote: > Thanks for all the reviews. > > V5: > Replace /sys/kernel/livepatch also in other/already existing tests. > Improve commit message of 3rd patch. > > V4: > Use variable for /sys/kernel/debug. > Be consistent with "" around variables. > Fix path in commit message to /sys/kernel/debug/kprobes/enabled. > > V3: > Save and restore kprobe state also when test fails, by integrating it > into setup_config() and cleanup(). > Rename SYSFS variables in a more logical way. > Sort test modules in alphabetical order. > Rename module description. > > V2: > Save and restore kprobe state. > > Michael Vetter (3): > selftests: livepatch: rename KLP_SYSFS_DIR to SYSFS_KLP_DIR > selftests: livepatch: save and restore kprobe state > selftests: livepatch: test livepatching a kprobed function > > tools/testing/selftests/livepatch/Makefile| 3 +- > .../testing/selftests/livepatch/functions.sh | 29 + > .../selftests/livepatch/test-callbacks.sh | 24 +++ > .../selftests/livepatch/test-ftrace.sh| 2 +- > .../selftests/livepatch/test-kprobe.sh| 62 +++ > .../selftests/livepatch/test-livepatch.sh | 12 ++-- > .../testing/selftests/livepatch/test-state.sh | 8 +-- > .../selftests/livepatch/test-syscall.sh | 2 +- > .../testing/selftests/livepatch/test-sysfs.sh | 8 +-- > .../selftests/livepatch/test_modules/Makefile | 3 +- > .../livepatch/test_modules/test_klp_kprobe.c | 38 > 11 files changed, 150 insertions(+), 41 deletions(-) > create mode 100755 tools/testing/selftests/livepatch/test-kprobe.sh > create mode 100644 > tools/testing/selftests/livepatch/test_modules/test_klp_kprobe.c > With the small syntax error fixed in unload_lp(), Reviewed-by: Joe Lawrence Thanks, Michael, this is a good test to add to the suite. -- Joe
Re: [PATCH v5 1/3] selftests: livepatch: rename KLP_SYSFS_DIR to SYSFS_KLP_DIR
On 10/17/24 16:01, Michael Vetter wrote:
> @@ -246,12 +246,12 @@ function unload_lp() {
> function disable_lp() {
> local mod="$1"
>
> - log "% echo 0 > /sys/kernel/livepatch/$mod/enabled"
> - echo 0 > /sys/kernel/livepatch/"$mod"/enabled
> + log "% echo 0 > $SYSFS_KLP_DIR/$mod/enabled"
> + echo 0 > "$SYSFS_KLP_DIR"/mod"/enabled
Nit: syntax error here, should be (quotation fix and $mod is a variable):
echo 0 > "$SYSFS_KLP_DIR/$mod/enabled"
With that, the test works for me.
--
Joe
Re: [RFC 00/31] objtool, livepatch: Livepatch module generation
On Thu, Sep 12, 2024 at 09:44:04AM -0400, Joe Lawrence wrote:
> On Wed, Sep 11, 2024 at 12:39:42AM -0700, Josh Poimboeuf wrote:
> > On Mon, Sep 02, 2024 at 08:59:43PM -0700, Josh Poimboeuf wrote:
> > > Hi,
> > >
> > > Here's a new way to build livepatch modules called klp-build.
> > >
> > > I started working on it when I realized that objtool already does 99% of
> > > the work needed for detecting function changes.
> > >
> > > This is similar in concept to kpatch-build, but the implementation is
> > > much cleaner.
> > >
> > > Personally I still have reservations about the "source-based" approach
> > > (klp-convert and friends), including the fragility and performance
> > > concerns of -flive-patching. I would submit that klp-build might be
> > > considered the "official" way to make livepatch modules.
> > >
> > > Please try it out and let me know what you think. Based on v6.10.
> > >
> > > Also avaiable at:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git
> > > klp-build-rfc
> >
> > Here's an updated branch with a bunch of fixes. It's still incompatible
> > with BTF at the moment, otherwise it should (hopefully) fix the rest of
> > the issues reported so far.
> >
> > While the known bugs are fixed, I haven't finished processing all the
> > review comments yet. Once that happens I'll post a proper v2.
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git
> > klp-build-v1.5
>
> Hi Josh,
>
> I've had much better results with v1.5, thanks for collecting up those
> fixes in a branch.
>
Today's experiment used the centos-stream-10's kernel config with
CONFIG_MODULE_ALLOW_BTF_MISMATCH=y and cs-10's gcc (GCC) 14.2.1 20240801
(Red Hat 14.2.1-1).
First, more gcc nits (running top-level `make`):
check.c: In function ‘decode_instructions’:
check.c:410:54: error: ‘calloc’ sizes specified with ‘sizeof’ in the earlier
argument and not in the later argument [-Werror=calloc-transposed-args]
410 | insns = calloc(sizeof(*insn),
INSN_CHUNK_SIZE);
| ^
check.c:410:54: note: earlier argument should specify number of elements,
later size of each element
check.c: In function ‘init_pv_ops’:
check.c:551:38: error: ‘calloc’ sizes specified with ‘sizeof’ in the earlier
argument and not in the later argument [-Werror=calloc-transposed-args]
551 | file->pv_ops = calloc(sizeof(struct pv_state), nr);
| ^~
check.c:551:38: note: earlier argument should specify number of elements,
later size of each element
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 63c2d6c06..c6f192859 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -407,7 +407,7 @@ static void decode_instructions(struct objtool_file *file)
for (offset = 0; offset < sec_size(sec); offset += insn->len) {
if (!insns || idx == INSN_CHUNK_MAX) {
- insns = calloc(sizeof(*insn), INSN_CHUNK_SIZE);
+ insns = calloc(INSN_CHUNK_SIZE, sizeof(*insn));
ERROR_ON(!insns, "calloc");
idx = 0;
@@ -548,7 +548,7 @@ static void init_pv_ops(struct objtool_file *file)
return;
nr = sym->len / sizeof(unsigned long);
- file->pv_ops = calloc(sizeof(struct pv_state), nr);
+ file->pv_ops = calloc(nr, sizeof(struct pv_state));
ERROR_ON(!file->pv_ops, "calloc");
for (idx = 0; idx < nr; idx++)
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
and now a happy build of objtool.
The top-level `make` moves onto building all the kernel objects, but
then objtool vmlinux.o crashes:
$ gdb --args ./tools/objtool/objtool --sym-checksum --hacks=jump_label
--hacks=noinstr --hacks=skylake --ibt --orc --retpoline --rethunk --static-call
--uaccess --prefix=16 --link vmlinux.o
Program received signal SIGSEGV, Segmentation fault.
ignore_unreachable_insn (file=0x435ea0 , insn=0x1cd928c0) at
check.c:3980
3980if (prev_insn->dead_end &&
(gdb) bt
#0 ignore_unreachable_insn (file=0x435ea0 , insn=0x1cd928c0) at
check.c:3980
#1 validate_reachable_instructions (file=0x435ea0 ) at check.c:4452
#2 check (file=file@entry=0x435ea0 ) at check.c:4610
#3 0x00
Re: [PATCH v3 0/6] livepatch: klp-convert tool - Minimal version
On Tue, Aug 27, 2024 at 02:30:45PM +0200, Lukas Hruska wrote: > Summary > --- > > This is a significantly simplified version of the original klp-convert tool. > The klp-convert code has never got a proper review and also clean ups > were not easy. The last version was v7, see > https://lore.kernel.org/r/20230306140824.3858543-1-joe.lawre...@redhat.com > > The main change is that the tool does not longer search for the > symbols which would need the livepatch specific relocation entry. > Also klp.symbols file is not longer needed. > > Instead, the needed information is appended to the symbol declaration > via a new macro KLP_RELOC_SYMBOL(). It creates symbol with all needed > metadata. For example: > > extern char *saved_command_line \ > KLP_RELOC_SYMBOL(vmlinux, vmlinux, saved_command_line, 0); > > would create symbol > > $>readelf -r -W : > Relocation section '.rela.text' at offset 0x32e60 contains 10 entries: > Offset Info Type Symbol's Value > Symbol's Name + Addend > [...] > 0068 003c0002 R_X86_64_PC32 > .klp.sym.rela.vmlinux.vmlinux.saved_command_line,0 - 4 > [...] > > > The simplified klp-convert tool just transforms symbols > created by KLP_RELOC_SYMBOL() to object specific rela sections > and rela entries which would later be proceed when the livepatch > or the livepatched object is loaded. > > For example, klp-convert would replace the above symbols with: > > $> readelf -r -W > Relocation section '.klp.rela.vmlinux.text' at offset 0x5cb60 contains 1 > entry: > Offset Info Type Symbol's Value > Symbol's Name + Addend > 0068 003c0002 R_X86_64_PC32 > .klp.sym.vmlinux.saved_command_line,0 - 4 > > > Note that similar macro was needed also in the original version > to handle more symbols of the same name (sympos). > > Given the above, add klp-convert tool; integrate klp-convert tool into > kbuild; 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. > > > Testing > --- > > The patchset selftests build and execute on x86_64, s390x, and ppc64le > for both default config (with added livepatch dependencies) and a larger > SLE-15-ish config. > > > Summary of changes in this minimal version v3 > > > - klp-convert: symbol format changes (suggested by jlawrence) > - samples: fixed name of added sample in Makefile (suggested by pmladek) > - selftests: added ibt test case as an example (DON'T MERGE) > - fixed all suggested small changes in v2 > > Previous versions > - > > RFC: > https://lore.kernel.org/r/cover.1477578530.git.jpoim...@redhat.com/ > v2: > https://lore.kernel.org/r/f52d29f7-7d1b-ad3d-050b-a9fa8878f...@redhat.com/ > v3: > https://lore.kernel.org/r/20190410155058.9437-1-joe.lawre...@redhat.com/ > v4: > https://lore.kernel.org/r/20190509143859.9050-1-joe.lawre...@redhat.com/ > v5: > (not posted) > https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v5-devel > v6: > https://lore.kernel.org/r/20220216163940.228309-1-joe.lawre...@redhat.com/ > v7: > https://lore.kernel.org/r/20230306140824.3858543-1-joe.lawre...@redhat.com/ > v1 minimal: > https://lore.kernel.org/r/20231106162513.17556-1-lhru...@suse.cz/ > v2 minimal: > https://lore.kernel.org/r/20240516133009.20224-1-lhru...@suse.cz/ > Hi Lukas, Thanks again for posting the patchset and trying a simpler approach. I tested with latest kpatch-build tree with no ill effects -- essentially klp-convert is safe to run against .ko files that already contain klp-relocations. I would prefer more extensive selftests for various klp-relocation types (as well as symbol position), however I believe wasn't the point of the minimal version of this patchset. We can add more tests later. Anyway, now we have two RFC / patchsets supporting in-tree creation of klp-relocations (klp-convert and Josh's objtool patchset). I think we need to figure out whether one precludes the other, can they co-exist, or does that even make sense. Since LPC is right around the corner, does it make sense for folks to sync up at some point and talk pros/cons to various approaches? We don't have a microconference this year, but perhaps over lunch or beers? -- Joe
Re: [RFC 00/31] objtool, livepatch: Livepatch module generation
On Wed, Sep 11, 2024 at 12:39:42AM -0700, Josh Poimboeuf wrote: > On Mon, Sep 02, 2024 at 08:59:43PM -0700, Josh Poimboeuf wrote: > > Hi, > > > > Here's a new way to build livepatch modules called klp-build. > > > > I started working on it when I realized that objtool already does 99% of > > the work needed for detecting function changes. > > > > This is similar in concept to kpatch-build, but the implementation is > > much cleaner. > > > > Personally I still have reservations about the "source-based" approach > > (klp-convert and friends), including the fragility and performance > > concerns of -flive-patching. I would submit that klp-build might be > > considered the "official" way to make livepatch modules. > > > > Please try it out and let me know what you think. Based on v6.10. > > > > Also avaiable at: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git > > klp-build-rfc > > Here's an updated branch with a bunch of fixes. It's still incompatible > with BTF at the moment, otherwise it should (hopefully) fix the rest of > the issues reported so far. > > While the known bugs are fixed, I haven't finished processing all the > review comments yet. Once that happens I'll post a proper v2. > > git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git > klp-build-v1.5 Hi Josh, I've had much better results with v1.5, thanks for collecting up those fixes in a branch. One new thing to report: depending on the optimzation of drivers/gpu/drm/vmwgfx/vmwgfx.o, objtool may throw a complaint about an unexpected relocation symbol type: $ make -j$(nproc) drivers/gpu/drm/vmwgfx/vmwgfx.o drivers/gpu/drm/vmwgfx/vmwgfx.o: error: objtool [check.c:1048]: unexpected relocation symbol type in .rela.discard.func_stack_frame_non_standard: 0 I modified check.c to print the reloc->sym->name in this case and it reports, "vmw_recv_msg". If I recreate vmwgfx.o and dump the symbol table, I notice that this is a NOTYPE symbol (probably because of vmw_recv_msg.constprop.0?) $ ld -m elf_x86_64 -z noexecstack -r -o drivers/gpu/drm/vmwgfx/vmwgfx.o @drivers/gpu/drm/vmwgfx/vmwgfx.mod $ readelf --wide --symbols drivers/gpu/drm/vmwgfx/vmwgfx.o | grep -b -e 'vmw_recv_msg' -e 'vmw_send_msg' 148334: 2198: 0010 183 FUNCLOCAL DEFAULT 1255 vmw_send_msg 151116: 2234: 0010 409 FUNCLOCAL DEFAULT 1251 vmw_recv_msg.constprop.0 180895: 2615: 0 NOTYPE GLOBAL DEFAULT UND vmw_recv_msg I don't think the config matters (I used the centos-stream-10 config) as long as the driver builds. I only saw this with a rhel-9 gcc version 11.5.0 20240719 (Red Hat 11.5.0-2) and not fedora gcc version 12.3.1 20230508 (Red Hat 12.3.1-1), which kept vmw_recv_msg w/o constprop. -- Joe
Re: [RFC 00/31] objtool, livepatch: Livepatch module generation
On Fri, Sep 06, 2024 at 06:47:06PM -0700, Josh Poimboeuf wrote: > > Normally I build objtool with > > make tools/objtool > > or just > > make > > Those use the objtool Makefile without all the extra kernel flags. > > How do you normally build objtool? > Usually as part of the kernel build, for example: $ git clone \ git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git \ --branch klp-build-rfc $ cd linux && make -s defconfig && make -j$(nproc) Results in the same implicit function declaration and uninitialized variables errors. (Thanks to tools/objtool/Makefile's OBJTOOL_CFLAGS := -Werror I believe.) Re-reading my report, I thought building the two object files (check.o and klp-diff.o) individually would report their respective problems, but I see now that the invocation seems to want to build all the .o's, so disregard that build wrinkle. I almost always build objtool by a top-level `make` or `make tools/objtool`, so sorry for any confusion. -- Joe
Re: [RFC 00/31] objtool, livepatch: Livepatch module generation
On Fri, Sep 06, 2024 at 10:00:08AM -0700, Josh Poimboeuf wrote:
> On Fri, Sep 06, 2024 at 09:56:06AM -0400, Joe Lawrence wrote:
> > In the case of klp-diff.c, adding #include will provide the
> > memmem prototype. For both files, I needed to #define _GNU_SOURCE for
> > that prototype though.
> >
> > For the other complaint, I just set struct instruction *dest_insn = NULL
> > at the top of the for loop, but perhaps the code could be refactored to
> > clarify the situation to the compiler if you prefer not to do that.
>
> Thanks! I'll get these fixed up.
>
Also, with the workarounds mentioned above, the two you sent to Song,
and the same .config I attached in the first email, I get the following
error when trying to build the canonical /proc/cmdline example:
$ cat cmdline-string.patch
diff --git a/fs/proc/cmdline.c b/fs/proc/cmdline.c
index a6f76121955f..2bcaf9ec6f78 100644
--- a/fs/proc/cmdline.c
+++ b/fs/proc/cmdline.c
@@ -7,8 +7,7 @@
static int cmdline_proc_show(struct seq_file *m, void *v)
{
- seq_puts(m, saved_command_line);
- seq_putc(m, '\n');
+ seq_printf(m, "%s kpatch=1", saved_command_line);
return 0;
}
$ ./scripts/livepatch/klp-build ./cmdline-string.patch 2>&1 | tee build2.out
- klp-build: building original kernel
vmlinux.o: warning: objtool: init_espfix_bsp+0xab: unreachable instruction
vmlinux.o: warning: objtool: init_espfix_ap+0x50: unreachable instruction
vmlinux.o: warning: objtool: syscall_init+0xca: unreachable instruction
vmlinux.o: warning: objtool: sync_core_before_usermode+0xf: unreachable
instruction
vmlinux.o: warning: objtool: sync_core_before_usermode+0xf: unreachable
instruction
vmlinux.o: warning: objtool: tc_wrapper_init+0x16: unreachable instruction
vmlinux.o: warning: objtool: pvh_start_xen+0x50: relocation to !ENDBR:
pvh_start_xen+0x57
- klp-build: building patched kernel
vmlinux.o: warning: objtool: init_espfix_bsp+0xab: unreachable instruction
vmlinux.o: warning: objtool: init_espfix_ap+0x50: unreachable instruction
vmlinux.o: warning: objtool: syscall_init+0xca: unreachable instruction
vmlinux.o: warning: objtool: sync_core_before_usermode+0xf: unreachable
instruction
vmlinux.o: warning: objtool: sync_core_before_usermode+0xf: unreachable
instruction
vmlinux.o: warning: objtool: tc_wrapper_init+0x16: unreachable instruction
vmlinux.o: warning: objtool: pvh_start_xen+0x50: relocation to !ENDBR:
pvh_start_xen+0x57
- klp-build: diffing objects
kvm.o: added: __UNIQUE_ID_nop_644
kvm.o: added: __UNIQUE_ID_nop_645
kvm.o: added: __UNIQUE_ID_nop_646
kvm.o: added: __UNIQUE_ID_nop_647
kvm.o: added: __UNIQUE_ID_nop_648
kvm.o: added: __UNIQUE_ID_nop_649
kvm.o: added: __UNIQUE_ID_nop_650
kvm.o: added: __UNIQUE_ID_nop_651
kvm.o: added: __UNIQUE_ID_nop_652
vmlinux.o: changed: cmdline_proc_show
- klp-build: building patch module
make[2]: /bin/sh: Argument list too long
make[2]: *** [scripts/Makefile.build:253:
/home/jolawren/src/linux/klp-tmp/out/livepatch.mod] Error 127
make[1]: *** [/home/jolawren/src/linux/Makefile:1943:
/home/jolawren/src/linux/klp-tmp/out] Error 2
make: *** [Makefile:240: __sub-make] Error 2
klp-build: error: module build failed
I'm guessing that this happens because of the huge dependency line in
klp-tmp/out/Kbuild for livepatch-y, it includes over 2000 object files
(that I'm pretty sure didn't change :)
I can set this up on a live machine next week if you want to investigate
further.
--
Joe
Re: [PATCH] livepatch: introduce klp_func called interface
Hi Wardenjohn, To follow up, Red Hat kpatch QE pointed me toward this test: https://gitlab.com/redhat/centos-stream/tests/kernel/kernel-tests/-/tree/main/general/kpatch/kpatch-trace?ref_type=heads which reports a few interesting things via systemd service and ftrace: - Patched functions - Traced functions - Code that was modified, but didn't run - Other code that ftrace saw, but is not listed in the sysfs directory The code looks to be built on top of the same basic ftrace commands that I sent the other day. You may be able to reuse/copy/etc bits from the scripts if they are handy. -- Joe
Re: [PATCH] livepatch: introduce klp_func called interface
On Tue, Jun 04, 2024 at 04:14:51PM +0800, zhang warden wrote: > > > > On Jun 1, 2024, at 03:16, Joe Lawrence wrote: > > > > Adding these attributes to livepatch sysfs would be expedient and > > probably easier for us to use, but imposes a recurring burden on us to > > maintain and test (where is the documentation and kselftest for this new > > interface?). Or, we could let the other tools handle all of that for > > us. > How this attribute imposes a recurring burden to maintain and test? > Perhaps "responsibility" is a better description. This would introduce an attribute that someone's userspace utility is relying on. It should at least have a kselftest to ensure a random patch in 2027 doesn't break it. > > Perhaps if someone already has an off-the-shelf script that is using > > ftrace to monitor livepatched code, it could be donated to > > Documentation/livepatch/? I can ask our QE folks if they have something > > like this. > > My intention to introduce this attitude to sysfs is that user who what to see > if this function is called can just need to show this function attribute in > the livepatch sysfs interface. > > User who have no experience of using ftrace will have problems to get the > calling state of the patched function. After all, ftrace is a professional > kernel tracing tools. > > Adding this attribute will be more easier for us to show if this patched > function is called. Actually, I have never try to use ftrace to trace a > patched function. Is it OK of using ftrace for a livepatched function? > If you build with CONFIG_SAMPLE_LIVEPATCH=m, you can try it out (or with one of your own livepatches): # Convenience variable $ SYSFS=/sys/kernel/debug/tracing # Install the livepatch sample demo module $ insmod samples/livepatch/livepatch-sample.ko # Verify that ftrace can filter on our functions $ grep cmdline_proc_show $SYSFS/available_filter_functions cmdline_proc_show livepatch_cmdline_proc_show [livepatch_sample] # Turn off any existing tracing and filter functions $ echo 0 > $SYSFS/tracing_on $ echo > $SYSFS/set_ftrace_filter # Set up the function tracer and add the kernel's cmdline_proc_show() # and livepatch-sample's livepatch_cmdline_proc_show() $ echo function > $SYSFS/current_tracer $ echo cmdline_proc_show >> $SYSFS/set_ftrace_filter $ echo livepatch_cmdline_proc_show >> $SYSFS/set_ftrace_filter $ cat $SYSFS/set_ftrace_filter cmdline_proc_show livepatch_cmdline_proc_show [livepatch_sample] # Turn on the ftracing and force execution of the original and # livepatched functions $ echo 1 > $SYSFS/tracing_on $ cat /proc/cmdline this has been live patched # Checkout out the trace file results $ cat $SYSFS/trace # tracer: function # # entries-in-buffer/entries-written: 2/2 #P:8 # #_-=> irqs-off/BH-disabled # / _=> need-resched # | / _---=> hardirq/softirq # || / _--=> preempt-depth # ||| / _-=> migrate-disable # / delay # TASK-PID CPU# | TIMESTAMP FUNCTION # | | | | | | cat-254 [002] ...2. 363.043498: cmdline_proc_show <-seq_read_iter cat-254 [002] ...1. 363.043501: livepatch_cmdline_proc_show <-seq_read_iter The kernel docs provide a lot of explanation of the complete ftracing interface. It's pretty power stuff, though you may also go the other direction and look into using the trace-cmd front end to simplify all of the sysfs manipulation. Julia Evans wrote a blog [1] a while back that provides a some more examples. [1] https://jvns.ca/blog/2017/03/19/getting-started-with-ftrace/ -- Joe
Re: [PATCH 0/2] livepatch: Add compiler optimization disclaimer/docs
On 5/31/24 07:23, Miroslav Benes wrote: > Hi, > > On Tue, 21 Jul 2020, Joe Lawrence wrote: > >> In light of [PATCH] Revert "kbuild: use -flive-patching when >> CONFIG_LIVEPATCH is enabled" [1], we should add some loud disclaimers >> and explanation of the impact compiler optimizations have on >> livepatching. >> >> The first commit provides detailed explanations and examples. The list >> was taken mostly from Miroslav's LPC talk a few years back. This is a >> bit rough, so corrections and additional suggestions welcome. Expanding >> upon the source-based patching approach would be helpful, too. >> >> The second commit adds a small README.rst file in the livepatch samples >> directory pointing the reader to the doc introduced in the first commit. >> >> I didn't touch the livepatch kselftests yet as I'm still unsure about >> how to best account for IPA here. We could add the same README.rst >> disclaimer here, too, but perhaps we have a chance to do something more. >> Possibilities range from checking for renamed functions as part of their >> build, or the selftest scripts, or even adding something to the kernel >> API. I think we'll have a better idea after reviewing the compiler >> considerations doc. > > thanks to Marcos for resurrecting this. > > Joe, do you have an updated version by any chance? Some things have > changed since July 2020 so it calls for a new review. If there was an > improved version, it would be easier. If not, no problem at all. > Yea, it's been a little while :) I don't have any newer version than this one. I can rebase, apply all of the v1 suggestions, and see where it stands. LMK if you can think of any specifics that could be added. For example, CONFIG_KERNEL_IBT will be driving some changes soon, whether it be klp-convert for source-based patches or vmlinux.o binary comparison for kpatch-build. I can push a v2 with a few changes, but IIRC, last time we reviewed this, it kinda begged the question of how someone is creating the livepatch in the first place. As long as we're fine holding that thought for a while longer, this doc may still be useful by itself. -- Joe
Re: [PATCH] livepatch: introduce klp_func called interface
On Tue, May 21, 2024 at 08:34:46AM +0200, Miroslav Benes wrote: > Hello, > > On Mon, 20 May 2024, zhang warden wrote: > > > > > > > > On May 20, 2024, at 14:46, Miroslav Benes wrote: > > > > > > Hi, > > > > > > On Mon, 20 May 2024, Wardenjohn wrote: > > > > > >> Livepatch module usually used to modify kernel functions. > > >> If the patched function have bug, it may cause serious result > > >> such as kernel crash. > > >> > > >> This is a kobject attribute of klp_func. Sysfs interface named > > >> "called" is introduced to livepatch which will be set as true > > >> if the patched function is called. > > >> > > >> /sys/kernel/livepatchcalled > > >> > > >> This value "called" is quite necessary for kernel stability > > >> assurance for livepatching module of a running system. > > >> Testing process is important before a livepatch module apply to > > >> a production system. With this interface, testing process can > > >> easily find out which function is successfully called. > > >> Any testing process can make sure they have successfully cover > > >> all the patched function that changed with the help of this interface. > > > > > > Even easier is to use the existing tracing infrastructure in the kernel > > > (ftrace for example) to track the new function. You can obtain much more > > > information with that than the new attribute provides. > > > > > > Regards, > > > Miroslav > > Hi Miroslav > > > > First, in most cases, testing process is should be automated, which make > > using existing tracing infrastructure inconvenient. > > could you elaborate, please? We use ftrace exactly for this purpose and > our testing process is also more or less automated. > > > Second, livepatch is > > already use ftrace for functional replacement, I don’t think it is a > > good choice of using kernel tracing tool to trace a patched function. > > Why? > > > At last, this attribute can be thought of as a state of a livepatch > > function. It is a state, like the "patched" "transition" state of a > > klp_patch. Adding this state will not break the state consistency of > > livepatch. > > Yes, but the information you get is limited compared to what is available > now. You would obtain the information that a patched function was called > but ftrace could also give you the context and more. > > It would not hurt to add a new attribute per se but I am wondering about > the reason and its usage. Once we have it, the commit message should be > improved based on that. > I agree with Miroslav about using ftrace, or Dan in the other thread about using gcov if even more granular coverage is needed. Admittedly, I would have to go find documentation to remember how to do either, but at least I'd be using well-tested facilities designed for this exact purpose. Adding these attributes to livepatch sysfs would be expedient and probably easier for us to use, but imposes a recurring burden on us to maintain and test (where is the documentation and kselftest for this new interface?). Or, we could let the other tools handle all of that for us. Perhaps if someone already has an off-the-shelf script that is using ftrace to monitor livepatched code, it could be donated to Documentation/livepatch/? I can ask our QE folks if they have something like this. Regards, -- Joe
Re: [PATCH v2 2/6] livepatch: Add klp-convert tool
On Thu, May 16, 2024 at 03:30:05PM +0200, Lukas Hruska wrote: > Livepatches need to access external symbols which can't be handled > by the normal relocation mechanism. It is needed for two types > of symbols: > > + Symbols which can be local for the original livepatched function. > The alternative implementation in the livepatch sees them > as external symbols. > > + Symbols in modules which are exported via EXPORT_SYMBOL*(). They > must be handled special way otherwise the livepatch module would > depend on the livepatched one. Loading such livepatch would cause > loading the other module as well. > > The address of these symbols can be found via kallsyms. Or they can > be relocated using livepatch specific relocation sections as 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 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, create scripts/livepatch to hold tools developed for > livepatches and add source files for klp-convert there. > > Allow to annotate such external symbols in the livepatch by a macro > KLP_RELOC_SYMBOL(). It will create symbol with all needed > metadata. For example: > > extern char *saved_command_line \ > KLP_RELOC_SYMBOL(vmlinux, vmlinux, saved_command_line, 0); > > would create symbol > > $>readelf -r -W : > Relocation section '.rela.text' at offset 0x32e60 contains 10 entries: > Offset Info Type Symbol's Value > Symbol's Name + Addend > [...] > 0068 003c0002 R_X86_64_PC32 > .klp.sym.rela.vmlinux.vmlinux.saved_command_line,0 - 4 > [...] > > Also add scripts/livepatch/klp-convert. The tool transforms symbols > created by KLP_RELOC_SYMBOL() to object specific rela sections > and rela entries which would later be proceed when the livepatch > or the livepatched object is loaded. > > For example, klp-convert would replace the above symbol with: > > $> readelf -r -W > Relocation section '.klp.rela.vmlinux.text' at offset 0x5cb60 contains 1 > entry: > Offset Info Type Symbol's Value > Symbol's Name + Addend > 0068 003c0002 R_X86_64_PC32 > .klp.sym.vmlinux.saved_command_line,0 - 4 > > klp-convert relies on libelf and on a list implementation. Add files > scripts/livepatch/elf.c and scripts/livepatch/elf.h, which are a libelf > interfacing layer and scripts/livepatch/list.h, which is a list > implementation. > > Update Makefiles to correctly support the compilation of the new tool, > update MAINTAINERS file and add a .gitignore file. > > [jpoim...@redhat.com: initial version] > Signed-off-by: Josh Poimboeuf > [joe.lawre...@redhat.com: clean-up and fixes] > Signed-off-by: Joe Lawrence > [lhru...@suse.cz: klp-convert code, minimal approach] > Signed-off-by: Lukas Hruska > Reviewed-by: Marcos Paulo de Souza > --- > MAINTAINERS | 1 + > include/linux/livepatch.h | 19 + > scripts/Makefile| 1 + > scripts/livepatch/.gitignore| 1 + > scripts/livepatch/Makefile | 5 + > scripts/livepatch/elf.c | 817 > scripts/livepatch/elf.h | 73 +++ > scripts/livepatch/klp-convert.c | 284 +++ > scripts/livepatch/klp-convert.h | 23 + > scripts/livepatch/list.h| 391 +++ > 10 files changed, 1615 insertions(+) > 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 > > diff --git a/MAINTAINERS b/MAINTAINERS > index 130b8b0bd4f7..d2facc1f4e15 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12618,6 +12618,7 @@ F:include/uapi/linux/livepatch.h > F: kernel/livepatch/ > F: kernel/module/livepatch.c > F: samples/livepatch/ > +F: scripts/livepatch/ > F: tools/testing/selftests/livepatch/ > > LLC (802.2) > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index 9b9b38e89563..83bbcd1c43fd 100644 > --- a/include/linux/livepatch.h > +++ b/i
Re: [PATCH v2 2/6] livepatch: Add klp-convert tool
Hi Lukas,
As mentioned offlist, reviewing and testing this is on my TODO list, but
here are some early notes ...
On Thu, May 16, 2024 at 03:30:05PM +0200, Lukas Hruska wrote:
> Livepatches need to access external symbols which can't be handled
> by the normal relocation mechanism. It is needed for two types
> of symbols:
>
> + Symbols which can be local for the original livepatched function.
> The alternative implementation in the livepatch sees them
> as external symbols.
>
> + Symbols in modules which are exported via EXPORT_SYMBOL*(). They
> must be handled special way otherwise the livepatch module would
> depend on the livepatched one. Loading such livepatch would cause
> loading the other module as well.
>
> The address of these symbols can be found via kallsyms. Or they can
> be relocated using livepatch specific relocation sections as 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 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, create scripts/livepatch to hold tools developed for
> livepatches and add source files for klp-convert there.
>
> Allow to annotate such external symbols in the livepatch by a macro
> KLP_RELOC_SYMBOL(). It will create symbol with all needed
> metadata. For example:
>
> extern char *saved_command_line \
> KLP_RELOC_SYMBOL(vmlinux, vmlinux, saved_command_line, 0);
Nit: should this be KLP_RELOC_SYMBOL_POS if it including the 0 position?
(Or KLP_RELOC_SYMBOL and omit the implied 0-th position.)
> diff --git a/scripts/livepatch/elf.c b/scripts/livepatch/elf.c
> --- /dev/null
> +++ b/scripts/livepatch/elf.c
> +static int update_shstrtab(struct elf *elf)
> +{
> + struct section *shstrtab, *sec;
> + size_t orig_size, new_size = 0, offset, len;
> + char *buf;
> +
> + shstrtab = find_section_by_name(elf, ".shstrtab");
> + if (!shstrtab) {
> + WARN("can't find .shstrtab");
> + return -1;
> + }
> +
> + orig_size = new_size = shstrtab->size;
> +
> + list_for_each_entry(sec, &elf->sections, list) {
> + if (sec->sh.sh_name != ~0U)
> + continue;
> + new_size += strlen(sec->name) + 1;
> + }
> +
> + if (new_size == orig_size)
> + return 0;
> +
> + buf = malloc(new_size);
> + if (!buf) {
> + WARN("malloc failed");
> + return -1;
> + }
> + memcpy(buf, (void *)shstrtab->data, orig_size);
While reviewing klp-convert v7 [1], Alexey Dobriyan notes here,
"This code is called realloc(). :-)"
[1]
https://lore.kernel.org/live-patching/4ce29654-4e1e-4680-9c25-715823ff5e02@p183/
> +static int update_strtab(struct elf *elf)
> +{
> + struct section *strtab;
> + struct symbol *sym;
> + size_t orig_size, new_size = 0, offset, len;
> + char *buf;
> +
> + strtab = find_section_by_name(elf, ".strtab");
> + if (!strtab) {
> + WARN("can't find .strtab");
> + return -1;
> + }
> +
> + orig_size = new_size = strtab->size;
> +
> + list_for_each_entry(sym, &elf->symbols, list) {
> + if (sym->sym.st_name != ~0U)
> + continue;
> + new_size += strlen(sym->name) + 1;
> + }
> +
> + if (new_size == orig_size)
> + return 0;
> +
> + buf = malloc(new_size);
> + if (!buf) {
> + WARN("malloc failed");
> + return -1;
> + }
> + memcpy(buf, (void *)strtab->data, orig_size);
I think Alexey's realloc suggestion would apply here, too.
> +static int write_file(struct elf *elf, const char *file)
> +{
> + int fd;
> + Elf *e;
> + GElf_Ehdr eh, ehout;
> + Elf_Scn *scn;
> + Elf_Data *data;
> + GElf_Shdr sh;
> + struct section *sec;
> +
> + fd = creat(file, 0664);
> + if (fd == -1) {
> + WARN("couldn't create %s", file);
> + return -1;
> + }
> +
> + e = elf_begin(fd, ELF_C_WRITE, NULL);
Alexy also found an ELF coding bug:
"elf_end() doesn't close descriptor, so there is potentially corrupted
data. There is no unlink() call if writes fail as well."
> +void elf_close(struct elf *elf)
> +{
> + struct section *sec, *tmpsec;
> + struct symbol *sym, *tmpsym;
> + struct rela *rela, *tmprela;
> +
> + list_for_each_entry_safe(sym, tmpsym, &elf->symbols, list) {
> + list_del(&sym->list);
> + free(sym);
> + }
> + list_for_each_entry_safe(sec, tmpsec, &elf->sections, list) {
> + list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
> + list_del(&rela->list);
> + free(rela);
> + }
> +
Re: [PATCH] livepatch: Add KLP_IDLE state
On 4/4/24 11:17, Petr Mladek wrote:
> On Tue 2024-04-02 09:52:31, Joe Lawrence wrote:
>> On Tue, Apr 02, 2024 at 11:09:54AM +0800, zhangwar...@gmail.com wrote:
>>> From: Wardenjohn
>>>
>>> In livepatch, using KLP_UNDEFINED is seems to be confused.
>>> When kernel is ready, livepatch is ready too, which state is
>>> idle but not undefined. What's more, if one livepatch process
>>> is finished, the klp state should be idle rather than undefined.
>>>
>>> Therefore, using KLP_IDLE to replace KLP_UNDEFINED is much better
>>> in reading and understanding.
>>> ---
>>> include/linux/livepatch.h | 1 +
>>> kernel/livepatch/patch.c | 2 +-
>>> kernel/livepatch/transition.c | 24
>>> 3 files changed, 14 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>>> index 9b9b38e89563..c1c53cd5b227 100644
>>> --- a/include/linux/livepatch.h
>>> +++ b/include/linux/livepatch.h
>>> @@ -19,6 +19,7 @@
>>>
>>> /* task patch states */
>>> #define KLP_UNDEFINED -1
>>> +#define KLP_IDLE -1
>>
>> Hi Wardenjohn,
>>
>> Quick question, does this patch intend to:
>>
>> - Completely replace KLP_UNDEFINED with KLP_IDLE
>> - Introduce KLP_IDLE as an added, fourth potential state
>> - Introduce KLP_IDLE as synonym of sorts for KLP_UNDEFINED under certain
>> conditions
>>
>> I ask because this patch leaves KLP_UNDEFINED defined and used in other
>> parts of the tree (ie, init/init_task.c), yet KLP_IDLE is added and
>> continues to use the same -1 enumeration.
>
> Having two names for the same state adds more harm than good.
>
> Honestly, neither "task->patch_state == KLP_UNDEFINED" nor "KLP_IDLE"
> make much sense.
>
> The problem is in the variable name. It is not a state of a patch.
> It is the state of the transition. The right solution would be
> something like:
>
> klp_target_state -> klp_transition_target_state
> task->patch_state -> task->klp_transition_state
> KLP_UNKNOWN -> KLP_IDLE
>
Yes, this is exactly how I think of these when reading the code. The
model starts to make a lot more sense once you look at it thru this lens :)
> But it would also require renaming:
>
> /proc//patch_state -> klp_transition_state
>
> which might break userspace tools => likely not acceptable.
>
>
> My opinion:
>
> It would be nice to clean this up but it does not look worth the
> effort.
>
Agreed. Instead of changing code and the sysfs interface, we could
still add comments like:
/* task patch transition target states */
#define KLP_UNDEFINED -1 /* idle, no transition in progress */
#define KLP_UNPATCHED0 /* transitioning to unpatched state */
#define KLP_PATCHED 1 /* transitioning to patched state */
/* klp transition target state */
static int klp_target_state = KLP_UNDEFINED;
struct task_struct = {
.patch_state= KLP_UNDEFINED, /* klp transition state */
Maybe just one comment is enough? Alternatively, we could elaborate in
Documentation/livepatch/livepatch.rst if it's really confusing.
Wardenjohn, since you're probably reading this code with fresh(er) eyes,
would any of the above be helpful?
--
Joe
Re: [PATCH] livepatch: Add KLP_IDLE state
On Tue, Apr 02, 2024 at 11:09:54AM +0800, zhangwar...@gmail.com wrote: > From: Wardenjohn > > In livepatch, using KLP_UNDEFINED is seems to be confused. > When kernel is ready, livepatch is ready too, which state is > idle but not undefined. What's more, if one livepatch process > is finished, the klp state should be idle rather than undefined. > > Therefore, using KLP_IDLE to replace KLP_UNDEFINED is much better > in reading and understanding. > --- > include/linux/livepatch.h | 1 + > kernel/livepatch/patch.c | 2 +- > kernel/livepatch/transition.c | 24 > 3 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index 9b9b38e89563..c1c53cd5b227 100644 > --- a/include/linux/livepatch.h > +++ b/include/linux/livepatch.h > @@ -19,6 +19,7 @@ > > /* task patch states */ > #define KLP_UNDEFINED-1 > +#define KLP_IDLE -1 Hi Wardenjohn, Quick question, does this patch intend to: - Completely replace KLP_UNDEFINED with KLP_IDLE - Introduce KLP_IDLE as an added, fourth potential state - Introduce KLP_IDLE as synonym of sorts for KLP_UNDEFINED under certain conditions I ask because this patch leaves KLP_UNDEFINED defined and used in other parts of the tree (ie, init/init_task.c), yet KLP_IDLE is added and continues to use the same -1 enumeration. -- Joe
Re: [PATCH v2] livepatch: Replace the fake signal sending with TIF_NOTIFY_SIGNAL infrastructure
On 3/29/21 9:28 AM, Miroslav Benes wrote:
Livepatch sends a fake signal to all remaining blocking tasks of a
running transition after a set period of time. It uses TIF_SIGPENDING
flag for the purpose. Commit 12db8b690010 ("entry: Add support for
TIF_NOTIFY_SIGNAL") added a generic infrastructure to achieve the same.
Replace our bespoke solution with the generic one.
Reviewed-by: Jens Axboe
Reviewed-by: Petr Mladek
Signed-off-by: Miroslav Benes
---
v2:
- #include from kernel/signal.c removed [Petr]
kernel/livepatch/transition.c | 5 ++---
kernel/signal.c | 4 +---
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index f6310f848f34..3a4beb9395c4 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -9,6 +9,7 @@
#include
#include
+#include
#include "core.h"
#include "patch.h"
#include "transition.h"
@@ -369,9 +370,7 @@ static void klp_send_signals(void)
* Send fake signal to all non-kthread tasks which are
* still not migrated.
*/
- spin_lock_irq(&task->sighand->siglock);
- signal_wake_up(task, 0);
- spin_unlock_irq(&task->sighand->siglock);
+ set_notify_signal(task);
}
}
read_unlock(&tasklist_lock);
diff --git a/kernel/signal.c b/kernel/signal.c
index f2a1b898da29..604290a8ca89 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -43,7 +43,6 @@
#include
#include
#include
-#include
#include
#include
@@ -181,8 +180,7 @@ void recalc_sigpending_and_wake(struct task_struct *t)
void recalc_sigpending(void)
{
- if (!recalc_sigpending_tsk(current) && !freezing(current) &&
- !klp_patch_pending(current))
+ if (!recalc_sigpending_tsk(current) && !freezing(current))
clear_thread_flag(TIF_SIGPENDING);
}
Looks good to me. Thanks for checking on this and updating.
Acked-by: Joe Lawrence
-- Joe
Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD
On 3/25/21 5:26 AM, Miroslav Benes wrote:
On Thu, 25 Mar 2021, Dong Kai wrote:
commit 15b2219facad ("kernel: freezer should treat PF_IO_WORKER like
PF_KTHREAD for freezing") is to fix the freezeing issue of IO threads
by making the freezer not send them fake signals.
Here live patching consistency model call klp_send_signals to wake up
all tasks by send fake signal to all non-kthread which only check the
PF_KTHREAD flag, so it still send signal to io threads which may lead to
freezeing issue of io threads.
I suppose this could happen, but it will also affect the live patching
transition if the io threads do not react to signals.
Are you able to reproduce it easily? I mean, is there a testcase I could
use to take a closer look?
If repro is only hypothetical at this point, perhaps we can artificially
create it in selftests? And useful to verify the future change you
mentioned in your other reply?
-- Joe
Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD
On 3/24/21 9:48 PM, Dong Kai wrote:
commit 15b2219facad ("kernel: freezer should treat PF_IO_WORKER like
PF_KTHREAD for freezing") is to fix the freezeing issue of IO threads
nit: s/freezeing/freezing
by making the freezer not send them fake signals.
Here live patching consistency model call klp_send_signals to wake up
all tasks by send fake signal to all non-kthread which only check the
PF_KTHREAD flag, so it still send signal to io threads which may lead to
freezeing issue of io threads.
Here we take the same fix action by treating PF_IO_WORKERS as PF_KTHREAD
within klp_send_signal function.
Signed-off-by: Dong Kai
---
note:
the io threads freeze issue links:
[1] https://lore.kernel.org/io-uring/yegnip43%2f6kfn...@kevinlocke.name/
[2]
https://lore.kernel.org/io-uring/d7350ce7-17dc-75d7-611b-27ebf2cb5...@kernel.dk/
kernel/livepatch/transition.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index f6310f848f34..0e1c35c8f4b4 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -358,7 +358,7 @@ static void klp_send_signals(void)
* Meanwhile the task could migrate itself and the action
* would be meaningless. It is not serious though.
*/
- if (task->flags & PF_KTHREAD) {
+ if (task->flags & (PF_KTHREAD | PF_IO_WORKER)) {
/*
* Wake up a kthread which sleeps interruptedly and
* still has not been migrated.
(PF_KTHREAD | PF_IO_WORKER) is open coded in soo many places maybe this
is a silly question, but...
If the livepatch code could use fake_signal_wake_up(), we could
consolidate the pattern in klp_send_signals() with the one in
freeze_task(). Then there would only one place for wake up / fake
signal logic.
I don't fully understand the differences in the freeze_task() version,
so I only pose this as a question and not v2 request.
As it is, this change seems logical to me, so:
Acked-by: Joe Lawrence
Thanks,
-- Joe
Re: [PATCH V2] docs: livepatch: Fix a typo and remove the unnecessary gaps in a sentence
On 3/5/21 8:37 AM, Bhaskar Chowdhury wrote: On 12:56 Fri 05 Mar 2021, Matthew Wilcox wrote: On Fri, Mar 05, 2021 at 03:39:23PM +0530, Bhaskar Chowdhury wrote: s/varibles/variables/ ...and remove leading spaces from a sentence. What do you mean 'leading spaces'? Separating two sentences with one space or two is a matter of personal style, and we do not attempt to enforce a particular style in the kernel. The spaces before the "In" .. nor I am imposing anything , it was peter caught and told me that it is hanging ..move it to the next line ..so I did. .. Initially I thought the same as Matthew, but after inspecting the diff I realized it was just a line wrap. Looks fine to me. Sometimes it may not be convenient or possible to allocate shadow variables alongside their parent objects. Or a livepatch fix may -require shadow varibles to only a subset of parent object instances. In +require shadow variables to only a subset of parent object instances. wrong preposition, s/to/for/..where??? Hi Bhaskar, Thanks for spotting, I'd be happy with v2 as is or a v3 if you want to update s/shadow variables to only/shadow variables for only/ but knowing me, I probably repeated the same phrasing elsewhere. Up to you, thanks. Acked-by: Joe Lawrence -- Joe
Re: [PATCH] kbuild: add extra-y to targets-for-modules
On 12/16/20 1:14 AM, Masahiro Yamada wrote:
On Tue, Dec 8, 2020 at 11:31 PM Artem Savkov wrote:
On Tue, Dec 08, 2020 at 05:20:35PM +0800, WANG Chao wrote:
Sorry for the late reply.
On 11/25/20 at 10:42P, Masahiro Yamada wrote:
On Tue, Nov 24, 2020 at 12:05 AM WANG Chao wrote:
On 11/23/20 at 02:23P, Masahiro Yamada wrote:
On Tue, Nov 3, 2020 at 3:23 PM WANG Chao wrote:
extra-y target doesn't build for 'make M=...' since commit 6212804f2d78
("kbuild: do not create built-in objects for external module builds").
This especially breaks kpatch, which is using 'extra-y := kpatch.lds'
and 'make M=...' to build livepatch patch module.
Add extra-y to targets-for-modules so that such kind of build works
properly.
Signed-off-by: WANG Chao
---
scripts/Makefile.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index ae647379b579..0113a042d643 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -86,7 +86,7 @@ ifdef need-builtin
targets-for-builtin += $(obj)/built-in.a
endif
-targets-for-modules := $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
+targets-for-modules := $(extra-y) $(patsubst %.o, %.mod, $(filter %.o,
$(obj-m)))
ifdef need-modorder
targets-for-modules += $(obj)/modules.order
--
2.29.1
NACK.
Please fix your Makefile.
Hint:
https://patchwork.kernel.org/project/linux-kbuild/patch/20201123045403.63402-6-masahi...@kernel.org/
Probably what you should use is 'targets'.
I tried with 'targets' and 'always-y'. Both doesn't work for me.
I narraw it down to the following example:
cat > Makefile << _EOF_
obj-m += foo.o
ldflags-y += -T $(src)/kpatch.lds
always-y += kpatch.lds
foo-objs += bar.o
all:
make -C /lib/modules/$(shell uname -r)/build M=$(PWD)
clean:
make -C /lib/modules/$(shell uname -r)/build M=$(PWD) clean
_EOF_
Take a look into scripts/Makefile.build:488:
__build: $(if $(KBUILD_BUILTIN), $(targets-for-builtin)) \
$(if $(KBUILD_MODULES), $(targets-for-modules)) \
$(subdir-ym) $(always-y)
@:
'always-y' is built after 'targets-for-modules'. This makes
'targets-for-modules' fails because kpatch.lds isn't there.
Heh, you rely on the targets built from left to right,
and you have never thought Make supports the parallel option -j.
You're right. I missed that.
You need to specify the dependency if you expect objects
are built in the particular order.
However, in this case, using ldflags-y looks wrong
in the first place.
The linker script is used when combining the object
as well as the final link of *.ko
We want linker script to be used on both those steps, otherwise modpost
fails.
In that case, does the following work?
(untested)
diff --git a/kmod/patch/Makefile b/kmod/patch/Makefile
index e017b17..02d4c66 100644
--- a/kmod/patch/Makefile
+++ b/kmod/patch/Makefile
@@ -12,7 +12,9 @@ endif
obj-m += $(KPATCH_NAME).o
ldflags-y += -T $(src)/kpatch.lds
-extra-y := kpatch.lds
+targets += kpatch.lds
+
+$(obj)/$(KPATCH_NAME).o: $(obj)/kpatch.lds
$(KPATCH_NAME)-objs += patch-hook.o output.o
Hi Masahiro,
Yeah this is more or less what Artem came up with:
https://github.com/dynup/kpatch/pull/1149
though we hadn't added kpatch.lds to targets. Is there documentation
somewhere on what effect "targets" has for out-of-tree builds?
Thanks,
-- Joe
Re: [PATCH] selftests/lkdtm: Use "comm" instead of "diff" for dmesg
On 9/9/20 3:49 PM, Kees Cook wrote:
On Fri, Jun 26, 2020 at 01:59:43PM -0700, Kees Cook wrote:
Instead of full GNU diff (which smaller boot environments may not have),
use "comm" which is more available.
Reported-by: Naresh Kamboju
Link:
https://lore.kernel.org/lkml/ca+g9fythp+gg+brr_gkbmxu2ooi-_e9pattpb6tvrswv1g1...@mail.gmail.com
Fixes: f131d9edc29d ("selftests/lkdtm: Don't clear dmesg when running tests")
Signed-off-by: Kees Cook
Shuah, this really needs to land to fix lkdtm tests on busybox. Can
you add this to -next? (Or is it better to direct this to Greg for the
lkdtm tree?)
Thanks!
-Kees
---
tools/testing/selftests/lkdtm/run.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/lkdtm/run.sh
b/tools/testing/selftests/lkdtm/run.sh
index 8383eb89d88a..5fe23009ae13 100755
--- a/tools/testing/selftests/lkdtm/run.sh
+++ b/tools/testing/selftests/lkdtm/run.sh
@@ -82,7 +82,7 @@ dmesg > "$DMESG"
($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true
# Record and dump the results
-dmesg | diff --changed-group-format='%>' --unchanged-group-format='' "$DMESG" - >
"$LOG" || true
+dmesg | comm -13 "$DMESG" - > "$LOG" || true
cat "$LOG"
# Check for expected output
--
2.25.1
--
Kees Cook
Hi Kees,
You may want to consider a similar follow up to the one Miroslav made to
the livepatching equivalent:
https://lore.kernel.org/live-patching/nycvar.yfh.7.76.2008271528000.27...@cbobk.fhfr.pm/T/#m1c17812d2c005dd57e9a299a4a492026a156619e
basically 'comm' will complain if two lines from dmesg have the same
timestamp prefix and their text portions are not sorted.
Regards,
-- Joe
Re: [PATCH] selftests/livepatch: Do not check order when using "comm" for dmesg checking
On Thu, Aug 27, 2020 at 01:07:09PM +0200, Miroslav Benes wrote:
> check_result() uses "comm" to check expected results of selftests output
> in dmesg. Everything works fine if timestamps in dmesg are unique. If
> not, like in this example
>
> [ 86.844422] test_klp_callbacks_demo: pre_unpatch_callback:
> test_klp_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
> [ 86.844422] livepatch: 'test_klp_callbacks_demo': starting unpatching
> transition
>
Heh, our assumption that the timestamps would provide sorting wasn't
true after all.
> , "comm" fails with "comm: file 2 is not in sorted order". Suppress the
> order checking with --nocheck-order option.
>
> Signed-off-by: Miroslav Benes
Acked-by: Joe Lawrence
And not so important for selftests, but helpful for backporting efforts:
Fixes: 2f3f651f3756 ("selftests/livepatch: Use "comm" instead of "diff" for
dmesg")
> ---
>
> The strange thing is, I can reproduce the issue easily and reliably on
> older codestreams (4.12) but not on current upstream in my testing
> environment. I think the change makes sense regardless though.
>
We haven't backported v5.8 changes just yet, so thanks for finding this
one and posting a fix.
-- Joe
> tools/testing/selftests/livepatch/functions.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/livepatch/functions.sh
> b/tools/testing/selftests/livepatch/functions.sh
> index 1aba83c87ad3..846c7ed71556 100644
> --- a/tools/testing/selftests/livepatch/functions.sh
> +++ b/tools/testing/selftests/livepatch/functions.sh
> @@ -278,7 +278,7 @@ function check_result {
> # help differentiate repeated testing runs. Remove them with a
> # post-comparison sed filter.
>
> - result=$(dmesg | comm -13 "$SAVED_DMESG" - | \
> + result=$(dmesg | comm --nocheck-order -13 "$SAVED_DMESG" - | \
>grep -e 'livepatch:' -e 'test_klp' | \
>grep -v '\(tainting\|taints\) kernel' | \
>sed 's/^\[[ 0-9.]*\] //')
> --
> 2.28.0
>
Re: [PATCH v4 00/10] Function Granular KASLR
On 8/21/20 7:02 PM, Kristen Carlson Accardi wrote:
On Wed, 2020-07-22 at 16:33 -0500, Josh Poimboeuf wrote:
On Wed, Jul 22, 2020 at 12:56:10PM -0700, Kristen Carlson Accardi
wrote:
On Wed, 2020-07-22 at 12:42 -0700, Kees Cook wrote:
On Wed, Jul 22, 2020 at 11:07:30AM -0500, Josh Poimboeuf wrote:
On Wed, Jul 22, 2020 at 07:39:55AM -0700, Kees Cook wrote:
On Wed, Jul 22, 2020 at 11:27:30AM +0200, Miroslav Benes
wrote:
Let me CC live-patching ML, because from a quick glance
this is
something
which could impact live patching code. At least it
invalidates
assumptions
which "sympos" is based on.
In a quick skim, it looks like the symbol resolution is using
kallsyms_on_each_symbol(), so I think this is safe? What's a
good
selftest for live-patching?
The problem is duplicate symbols. If there are two static
functions
named 'foo' then livepatch needs a way to distinguish them.
Our current approach to that problem is "sympos". We rely on
the
fact
that the second foo() always comes after the first one in the
symbol
list and kallsyms. So they're referred to as foo,1 and foo,2.
Ah. Fun. In that case, perhaps the LTO series has some solutions.
I
think builds with LTO end up renaming duplicate symbols like
that, so
it'll be back to being unique.
Well, glad to hear there might be some precendence for how to solve
this, as I wasn't able to think of something reasonable off the top
of
my head. Are you speaking of the Clang LTO series?
https://lore.kernel.org/lkml/20200624203200.78870-1-samitolva...@google.com/
I'm not sure how LTO does it, but a few more (half-brained) ideas
that
could work:
1) Add a field in kallsyms to keep track of a symbol's original
offset
before randomization/re-sorting. Livepatch could use that field
to
determine the original sympos.
2) In fgkaslr code, go through all the sections and mark the ones
which
have duplicates (i.e. same name). Then when shuffling the
sections,
skip a shuffle if it involves a duplicate section. That way all
the
duplicates would retain their original sympos.
3) Livepatch could uniquely identify symbols by some feature other
than
sympos. For example:
Symbol/function size - obviously this would only work if
duplicately
named symbols have different sizes.
Checksum - as part of a separate feature we're also looking at
giving
each function its own checksum, calculated based on its
instruction
opcodes. Though calculating checksums at runtime could be
complicated by IP-relative addressing.
I'm thinking #1 or #2 wouldn't be too bad. #3 might be harder.
Hi there! I was trying to find a super easy way to address this, so I
thought the best thing would be if there were a compiler or linker
switch to just eliminate any duplicate symbols at compile time for
vmlinux. I filed this question on the binutils bugzilla looking to see
if there were existing flags that might do this, but H.J. Lu went ahead
and created a new one "-z unique", that seems to do what we would need
it to do.
https://sourceware.org/bugzilla/show_bug.cgi?id=26391
When I use this option, it renames any duplicate symbols with an
extension - for example duplicatefunc.1 or duplicatefunc.2.
I tried out H.J. Lu's branch and built some of the livepatch selftests
with -z unique-symbol and indeed observe the following pattern:
foo, foo.1, foo.2, etc.
for homonym symbol names.
> You could
either match on the full unique name of the specific binary you are
trying to patch, or you match the base name and use the extension to
determine original position. Do you think this solution would work?
I think it could work for klp-relocations.
As a quick test, I was able to hack the WIP klp-convert branch [1] to
generate klp-relocations with the following hack:
const char *foo(void) __asm__("foo.1");
when building foo's target with -z unique-symbol. (The target contained
two static foo() functions.) The asm rename trick exercised the
klp-convert implicit conversion feature, as the symbol was now uniquely
named and included a non-valid C symbol character. User-defined
klp-convert annotation support will require some refactoring, but
shouldn't be too difficult to support as well.
If
so, I can modify livepatch to refuse to patch on duplicated symbols if
CONFIG_FG_KASLR and when this option is merged into the tool chain I
can add it to KBUILD_LDFLAGS when CONFIG_FG_KASLR and livepatching
should work in all cases.
I don't have a grasp on how complicated the alternatives might be, so
I'll let others comment on best paths forward. I just wanted to note
that -z unique-symbol looks like it could reasonable work well for this
niche case.
[1]
https://github.com/joe-lawrence/linux/tree/klp-convert-v5-expanded-v5.8
(not modified for -z unique-symbol, but noted for reference)
-- Joe
refactoring livepatch documentation was Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc
On 8/6/20 8:03 AM, Petr Mladek wrote: On Wed 2020-07-22 15:51:39, Josh Poimboeuf wrote: On Wed, Jul 22, 2020 at 01:03:03PM -0400, Joe Lawrence wrote: On 7/21/20 7:04 PM, Josh Poimboeuf wrote: On Tue, Jul 21, 2020 at 12:14:06PM -0400, Joe Lawrence wrote: Compiler optimizations can have serious implications on livepatching. Create a document that outlines common optimization patterns and safe ways to livepatch them. Signed-off-by: Joe Lawrence There's a lot of good info here, but I wonder if it should be reorganized a bit and instead called "how to create a livepatch module", because that's really the point of it all. That would be nice. Would you consider a stand-alone compiler-optimizations doc an incremental step towards that end? Note that the other files (callbacks, shadow-vars, system-state) in their current form might be as confusing to the newbie. It's an incremental step towards _something_. Whether that's a cohesive patch creation guide, or just a growing hodgepodge of random documents, it may be too early to say :-) Yes, it would be nice to have a cohesive documentation. But scattered pieces are better than nothing. I'm thinking a newcomer reading this might be lost. It's not necessarily clear that there are currently two completely different approaches to creating a livepatch module, each with their own quirks and benefits/drawbacks. There is one mention of a "source-based livepatch author" but no explanation of what that means. Yes, the initial draft was light on source-based patching since I only really tinker with it for samples/kselftests. The doc was the result of an experienced livepatch developer and Sunday afternoon w/the compiler. I'm sure it reads as such. :) Are experienced livepatch developers the intended audience? If so I question what value this document has in its current form. Presumably experienced livepatch developers would already know this stuff. IMHO, this document is useful even for newbies. They might at least get a clue about these catches. It is better than nothing. I do not want to discourage Joe from creating even better documentation. But if he does not have interest or time to work on it, I am happy even for this piece. Hi Petr, Josh, The compiler optimization pitfall document can wait for refactored livepatch documentation if that puts it into better context, particularly for newbies. I don't mind either way. FWIW, I don't profess to be an authoritative source its content -- we've dealt some of these issues in kpatch, so it was interesting to see how they affect livepatches that don't rely on binary comparison. Toward the larger goal, I've changed the thread subject to talk about how we may rearrange and supplement our current documentation. This is a first pass at a possible refactoring... 1. Provide a better index page to connect the other files/docs, like https://www.kernel.org/doc/html/latest/core-api/index.html but obviously not that extensive. Right now we have only a Table of Contents tree without any commentary. 2. Rearrange and refactor sections: livepatch.rst Keep just about everything Add a history section to explain ksplice, kgraft, kpatch for the uninitiated? Add a section on source based vs. binary diff livepatch creation, this may be worth its own top-level section Livepatch API Basic API Callbacks Shadow variables Cumulative patches System state KLP Relocations Right now this is a bit academic AFAIK kpatch is the only tool currently making use of them. So maybe this document becomes a more general purpose doc explaining how to reference unexported symbols? (ie, how does kgraft currently do it, particularly w/kallsyms going unexported?) Eventually this could contain klp-convert howto if it ever gets merged. Compiler considerations TBD I suppose this doesn't create a "Livepatching creation for dummies" guide, but my feeling is that there are so many potential (hidden) pitfalls that such guide would be dangerous. If someone were to ask me today how to start building a livepatch, I would probably point them at the samples to demonstrate the basic concept and API, but then implore them to read through the documentation to understand how quickly complicated it can become. -- Joe
Re: [PATCH v4 00/10] Function Granular KASLR
On Fri, Jul 17, 2020 at 09:59:57AM -0700, Kristen Carlson Accardi wrote: > Function Granular Kernel Address Space Layout Randomization (fgkaslr) > - > > This patch set is an implementation of finer grained kernel address space > randomization. It rearranges your kernel code at load time > on a per-function level granularity, with only around a second added to > boot time. > > Changes in v4: > - > * dropped the patch to split out change to STATIC definition in > x86/boot/compressed/misc.c and replaced with a patch authored > by Kees Cook to avoid the duplicate malloc definitions > * Added a section to Documentation/admin-guide/kernel-parameters.txt > to document the fgkaslr boot option. > * redesigned the patch to hide the new layout when reading > /proc/kallsyms. The previous implementation utilized a dynamically > allocated linked list to display the kernel and module symbols > in alphabetical order. The new implementation uses a randomly > shuffled index array to display the kernel and module symbols > in a random order. > > Changes in v3: > - > * Makefile changes to accommodate CONFIG_LD_DEAD_CODE_DATA_ELIMINATION > * removal of extraneous ALIGN_PAGE from _etext changes > * changed variable names in x86/tools/relocs to be less confusing > * split out change to STATIC definition in x86/boot/compressed/misc.c > * Updates to Documentation to make it more clear what is preserved in .text > * much more detailed commit message for function granular KASLR patch > * minor tweaks and changes that make for more readable code > * this cover letter updated slightly to add additional details > > Changes in v2: > -- > * Fix to address i386 build failure > * Allow module reordering patch to be configured separately so that > arm (or other non-x86_64 arches) can take advantage of module function > reordering. This support has not be tested by me, but smoke tested by > Ard Biesheuvel on arm. > * Fix build issue when building on arm as reported by > Ard Biesheuvel > > Patches to objtool are included because they are dependencies for this > patchset, however they have been submitted by their maintainer separately. > > Background > -- > KASLR was merged into the kernel with the objective of increasing the > difficulty of code reuse attacks. Code reuse attacks reused existing code > snippets to get around existing memory protections. They exploit software bugs > which expose addresses of useful code snippets to control the flow of > execution for their own nefarious purposes. KASLR moves the entire kernel > code text as a unit at boot time in order to make addresses less predictable. > The order of the code within the segment is unchanged - only the base address > is shifted. There are a few shortcomings to this algorithm. > > 1. Low Entropy - there are only so many locations the kernel can fit in. This >means an attacker could guess without too much trouble. > 2. Knowledge of a single address can reveal the offset of the base address, >exposing all other locations for a published/known kernel image. > 3. Info leaks abound. > > Finer grained ASLR has been proposed as a way to make ASLR more resistant > to info leaks. It is not a new concept at all, and there are many variations > possible. Function reordering is an implementation of finer grained ASLR > which randomizes the layout of an address space on a function level > granularity. We use the term "fgkaslr" in this document to refer to the > technique of function reordering when used with KASLR, as well as finer > grained > KASLR in general. > > Proposed Improvement > > This patch set proposes adding function reordering on top of the existing > KASLR base address randomization. The over-arching objective is incremental > improvement over what we already have. It is designed to work in combination > with the existing solution. The implementation is really pretty simple, and > there are 2 main area where changes occur: > > * Build time > > GCC has had an option to place functions into individual .text sections for > many years now. This option can be used to implement function reordering at > load time. The final compiled vmlinux retains all the section headers, which > can be used to help find the address ranges of each function. Using this > information and an expanded table of relocation addresses, individual text > sections can be suffled immediately after decompression. Some data tables > inside the kernel that have assumptions about order require re-sorting > after being updated when applying relocations. In order to modify these > tables, > a few key symbols are excluded from the objcopy symbol stripping process for > use after shuffling the text segments. > > Some highlights from the build time changes to look for: > > The top level kernel Makefile was modified to add the gcc flag if
Re: [PATCH v4 00/10] Function Granular KASLR
On 8/3/20 1:45 PM, Kees Cook wrote: On Mon, Aug 03, 2020 at 02:39:32PM +0300, Evgenii Shatokhin wrote: There are at least 2 places where high-order memory allocations might happen during module loading. Such allocations may fail if memory is fragmented, while physically contiguous memory areas are not really needed there. I suggest to switch to kvmalloc/kvfree there. While this does seem to be the right solution for the extant problem, I do want to take a moment and ask if the function sections need to be exposed at all? What tools use this information, and do they just want to see the bounds of the code region? (i.e. the start/end of all the .text* sections) Perhaps .text.* could be excluded from the sysfs section list? [[cc += FChE, see [0] for Evgenii's full mail ]] It looks like debugging tools like systemtap [1], gdb [2] and its add-symbol-file cmd, etc. peek at the /sys/module//section/ info. But yeah, it would be preferable if we didn't export a long sysfs representation if nobody actually needs it. [0] https://lore.kernel.org/lkml/e9c4d88b-86db-47e9-4299-3fac45a7e...@virtuozzo.com/ [1] https://fossies.org/linux/systemtap/staprun/staprun.c [2] https://www.oreilly.com/library/view/linux-device-drivers/0596005903/ch04.html#linuxdrive3-CHP-4-SECT-6.1 -- Joe
Re: [PATCH 2/7] modules: mark find_symbol static
On 7/29/20 12:24 PM, Greg Kroah-Hartman wrote: On Wed, Jul 29, 2020 at 06:13:18PM +0200, Jessica Yu wrote: +++ Christoph Hellwig [29/07/20 08:27 +0200]: find_symbol is only used in module.c. Signed-off-by: Christoph Hellwig CCing the livepatching ML, as this may or may not impact its users. AFAIK, the out-of-tree kpatch module had used find_symbol() in the past, I am not sure what its current status is. I suspect all of its functionality has been migrated to upstream livepatch already. We still have symbol_get(), which is what I thought they were using. The deprecated (though still in the repo) kpatch.ko still references find_symbol(), but that module is no longer officially supported by the project. Jessica is correct that the functionality has been migrated upstream. I don't see any references to symbol_get() either, so we're good on that front, too. Thanks, -- Joe
Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc
On 7/21/20 7:04 PM, Josh Poimboeuf wrote: On Tue, Jul 21, 2020 at 12:14:06PM -0400, Joe Lawrence wrote: Compiler optimizations can have serious implications on livepatching. Create a document that outlines common optimization patterns and safe ways to livepatch them. Signed-off-by: Joe Lawrence There's a lot of good info here, but I wonder if it should be reorganized a bit and instead called "how to create a livepatch module", because that's really the point of it all. That would be nice. Would you consider a stand-alone compiler-optimizations doc an incremental step towards that end? Note that the other files (callbacks, shadow-vars, system-state) in their current form might be as confusing to the newbie. I'm thinking a newcomer reading this might be lost. It's not necessarily clear that there are currently two completely different approaches to creating a livepatch module, each with their own quirks and benefits/drawbacks. There is one mention of a "source-based livepatch author" but no explanation of what that means. Yes, the initial draft was light on source-based patching since I only really tinker with it for samples/kselftests. The doc was the result of an experienced livepatch developer and Sunday afternoon w/the compiler. I'm sure it reads as such. :) Maybe it could begin with an overview of the two approaches, and then delve more into the details of each approach, and then delve even more into the gory details about compiler optimizations. Up until now, the livepatch documentation has danced around the particular creation method and only described the API in abstract. If a compiler considerations doc needs to have that complete context then I'd suggest we reorganize the entire lot as a prerequisite. Also the kpatch-build section can reference the patch author guide which we have on github. Good point. I think there are a few kpatch-specific implications (sibling call changes maybe) to consider. -- Joe
Re: [PATCH v4 00/10] Function Granular KASLR
On 7/22/20 10:51 AM, Joe Lawrence wrote: On 7/22/20 10:39 AM, Kees Cook wrote: On Wed, Jul 22, 2020 at 11:27:30AM +0200, Miroslav Benes wrote: Let me CC live-patching ML, because from a quick glance this is something which could impact live patching code. At least it invalidates assumptions which "sympos" is based on. In a quick skim, it looks like the symbol resolution is using kallsyms_on_each_symbol(), so I think this is safe? What's a good selftest for live-patching? Hi Kees, I don't think any of the in-tree tests currently exercise the kallsyms/sympos end of livepatching. On second thought, I mispoke.. The general livepatch code does use it: klp_init_object klp_init_object_loaded klp_find_object_symbol in which case any of the current kselftests should exercise that. % make -C tools/testing/selftests/livepatch run_tests -- Joe
Re: [PATCH v4 00/10] Function Granular KASLR
On 7/22/20 10:39 AM, Kees Cook wrote: On Wed, Jul 22, 2020 at 11:27:30AM +0200, Miroslav Benes wrote: Let me CC live-patching ML, because from a quick glance this is something which could impact live patching code. At least it invalidates assumptions which "sympos" is based on. In a quick skim, it looks like the symbol resolution is using kallsyms_on_each_symbol(), so I think this is safe? What's a good selftest for live-patching? Hi Kees, I don't think any of the in-tree tests currently exercise the kallsyms/sympos end of livepatching. I do have a local branch that does facilitate creating klp-relocations that do rely upon this feature -- I'll try to see if I can get those working with this patchset and report back later this week. -- Joe
[PATCH 1/2] docs/livepatch: Add new compiler considerations doc
Compiler optimizations can have serious implications on livepatching.
Create a document that outlines common optimization patterns and safe
ways to livepatch them.
Signed-off-by: Joe Lawrence
---
.../livepatch/compiler-considerations.rst | 220 ++
Documentation/livepatch/index.rst | 1 +
Documentation/livepatch/livepatch.rst | 7 +
3 files changed, 228 insertions(+)
create mode 100644 Documentation/livepatch/compiler-considerations.rst
diff --git a/Documentation/livepatch/compiler-considerations.rst
b/Documentation/livepatch/compiler-considerations.rst
new file mode 100644
index ..23b9cc01bb9c
--- /dev/null
+++ b/Documentation/livepatch/compiler-considerations.rst
@@ -0,0 +1,220 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+===
+Compiler considerations
+===
+
+Creating livepatch modules may seem as straightforward as updating a
+few functions in source code and registering them with the livepatch API.
+This idealized method may produce functional livepatch modules in some
+cases.
+
+.. warning::
+
+ A safe and accurate livepatch **must** take into account compiler
+ optimizations and their effect on the binary code that is executed and
+ ultimately livepatched.
+
+Examples
+
+
+Interprocedural optimization (IPA)
+--
+
+Function inlining is probably the most common compiler optimization that
+affects livepatching. In a simple example, inlining transforms the original
+code::
+
+ foo() { ... [ foo implementation ] ... }
+
+ bar() { ... foo() ... }
+
+to::
+
+ bar() { ... [ foo implementation ] ... }
+
+Inlining is comparable to macro expansion, however the compiler may inline
+cases which it determines worthwhile (while preserving original call/return
+semantics in others) or even partially inline pieces of functions (see cold
+functions in GCC function suffixes section below).
+
+To safely livepatch ``foo()`` from the previous example, all of its callers
+need to be taken into consideration. For those callers that the compiler had
+inlined ``foo()``, a livepatch should include a new version of the calling
+function such that it:
+
+ 1. Calls a new, patched version of the inlined function, or
+ 2. Provides an updated version of the caller that contains its own inlined
+ and updated version of the inlined function
+
+Other interesting IPA examples include:
+
+- *IPA-SRA*: removal of unused parameters, replace parameters passed by
+ referenced by parameters passed by value. This optimization basically
+ violates ABI.
+
+ .. note::
+ GCC changes the name of function. See GCC function suffixes
+ section below.
+
+- *IPA-CP*: find values passed to functions are constants and then optimizes
+ accordingly Several clones of a function are possible if a set is limited.
+
+ .. note::
+ GCC changes the name of function. See GCC function suffixes
+ section below.
+
+- *IPA-PURE-CONST*: discover which functions are pure or constant. GCC can
+ eliminate calls to such functions, memory accesses can be removed etc.
+
+- *IPA-ICF*: perform identical code folding for functions and read-only
+ variables. Replaces a function with an equivalent one.
+
+- *IPA-RA*: optimize saving and restoring registers if the compiler considers
+ it safe.
+
+- *Dead code elimination*: omit unused code paths from the resulting binary.
+
+GCC function suffixes
+-
+
+GCC may rename original, copied, and cloned functions depending upon the
+optimizations applied. Here is a partial list of name suffixes that the
+compiler may apply to kernel functions:
+
+- *Cold subfunctions* : ``.code`` or ``.cold.`` : parts of functions
+ (subfunctions) determined by attribute or optimization to be unlikely
+ executed.
+
+ For example, the unlikely bits of ``irq_do_set_affinity()`` may be moved
+ out to subfunction ``irq_do_set_affinity.cold.49()``. Starting with GCC 9,
+ the numbered suffix has been removed. So in the previous example, the cold
+ subfunction is simply ``irq_do_set_affinity.cold()``.
+
+- *Partial inlining* : ``.part.`` : parts of functions when split from
+ their original function body, improves overall inlining decisions.
+
+ The ``cdev_put()`` function provides an interesting example of a partial
+ clone. GCC builds the source function::
+
+ void cdev_put(struct cdev *p)
+ {
+ if (p) {
+ struct module *owner = p->owner;
+ kobject_put(&p->kobj);
+ module_put(owner);
+ }
+ }
+
+ into two functions, the conditional test in ``cdev_put()`` and the
+ ``kobject_put()`` and ``module_put()`` calls in ``cdev_put.part.0()``::
+
+:
+ e8 bb 60 73 00 callq 81a01a10 <__fentry__>
+ 48 85 fftest %rdi,%rdi
+ 74 05
[PATCH 0/2] livepatch: Add compiler optimization disclaimer/docs
In light of [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled" [1], we should add some loud disclaimers and explanation of the impact compiler optimizations have on livepatching. The first commit provides detailed explanations and examples. The list was taken mostly from Miroslav's LPC talk a few years back. This is a bit rough, so corrections and additional suggestions welcome. Expanding upon the source-based patching approach would be helpful, too. The second commit adds a small README.rst file in the livepatch samples directory pointing the reader to the doc introduced in the first commit. I didn't touch the livepatch kselftests yet as I'm still unsure about how to best account for IPA here. We could add the same README.rst disclaimer here, too, but perhaps we have a chance to do something more. Possibilities range from checking for renamed functions as part of their build, or the selftest scripts, or even adding something to the kernel API. I think we'll have a better idea after reviewing the compiler considerations doc. [1] https://lore.kernel.org/lkml/696262e997359666afa053fe7d1a9fb2bb373964.1595010490.git.jpoim...@redhat.com/ Joe Lawrence (2): docs/livepatch: Add new compiler considerations doc samples/livepatch: Add README.rst disclaimer .../livepatch/compiler-considerations.rst | 220 ++ Documentation/livepatch/index.rst | 1 + Documentation/livepatch/livepatch.rst | 7 + samples/livepatch/README.rst | 15 ++ 4 files changed, 243 insertions(+) create mode 100644 Documentation/livepatch/compiler-considerations.rst create mode 100644 samples/livepatch/README.rst -- 2.21.3
[PATCH 2/2] samples/livepatch: Add README.rst disclaimer
The livepatch samples aren't very careful with respect to compiler IPA-optimization of target kernel functions. Add a quick disclaimer and pointer to the compiler-considerations.rst file to warn readers. Suggested-by: Josh Poimboeuf Signed-off-by: Joe Lawrence --- samples/livepatch/README.rst | 15 +++ 1 file changed, 15 insertions(+) create mode 100644 samples/livepatch/README.rst diff --git a/samples/livepatch/README.rst b/samples/livepatch/README.rst new file mode 100644 index ..2f8ef945f2ac --- /dev/null +++ b/samples/livepatch/README.rst @@ -0,0 +1,15 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +== +Disclaimer +== + +The livepatch sample programs were written with idealized compiler +output in mind. That is to say that they do not consider ways in which +optimization may transform target kernel functions. + +The samples present only a simple API demonstration and should not be +considered completely safe. + +See the Documentation/livepatching/compiler-considerations.rst file for +more details on compiler optimizations and how they affect livepatching. -- 2.21.3
Re: [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"
On 7/20/20 4:50 AM, Kamalesh Babulal wrote:
On 20/07/20 9:05 am, Joe Lawrence wrote:
On 7/17/20 2:29 PM, Josh Poimboeuf wrote:
Use of the new -flive-patching flag was introduced with the following
commit:
43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is
enabled")
This flag has several drawbacks:
[ ... snip ... ]
- While there *is* a distro which relies on this flag for their distro
livepatch module builds, there's not a publicly documented way to
create safe livepatch modules with it. Its use seems to be based on
tribal knowledge. It serves no benefit to those who don't know how to
use it.
(In fact, I believe the current livepatch documentation and samples
are misleading and dangerous, and should be corrected. Or at least
amended with a disclaimer. But I don't feel qualified to make such
changes.)
FWIW, I'm not exactly qualified to document source-based creation either,
however I have written a few of the samples and obviously the kselftest modules.
The samples should certainly include a disclaimer (ie, they are only for API
demonstration purposes!) and eventually it would be great if the kselftest
modules could guarantee their safety as well. I don't know quite yet how we
can automate that, but perhaps some kind of post-build sanity check could
verify that they are in fact patching what they intend to patch.
As for a more general, long-form warning about optimizations, I grabbed
Miroslav's LPC slides from a few years back and poked around at some
IPA-optimized disassembly... Here are my notes that attempt to capture some
common cases:
http://file.bos.redhat.com/~jolawren/klp-compiler-notes/livepatch/compiler-considerations.html
Hi Joe,
The notes link you shared is not accessible.
Oops, lets try that again:
http://people.redhat.com/~jolawren/klp-compiler-notes/livepatch/compiler-considerations.html
-- Joe
Re: [PATCH] Revert "kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled"
On 7/17/20 2:29 PM, Josh Poimboeuf wrote:
Use of the new -flive-patching flag was introduced with the following
commit:
43bd3a95c98e ("kbuild: use -flive-patching when CONFIG_LIVEPATCH is enabled")
This flag has several drawbacks:
[ ... snip ... ]
- While there *is* a distro which relies on this flag for their distro
livepatch module builds, there's not a publicly documented way to
create safe livepatch modules with it. Its use seems to be based on
tribal knowledge. It serves no benefit to those who don't know how to
use it.
(In fact, I believe the current livepatch documentation and samples
are misleading and dangerous, and should be corrected. Or at least
amended with a disclaimer. But I don't feel qualified to make such
changes.)
FWIW, I'm not exactly qualified to document source-based creation
either, however I have written a few of the samples and obviously the
kselftest modules.
The samples should certainly include a disclaimer (ie, they are only for
API demonstration purposes!) and eventually it would be great if the
kselftest modules could guarantee their safety as well. I don't know
quite yet how we can automate that, but perhaps some kind of post-build
sanity check could verify that they are in fact patching what they
intend to patch.
As for a more general, long-form warning about optimizations, I grabbed
Miroslav's LPC slides from a few years back and poked around at some
IPA-optimized disassembly... Here are my notes that attempt to capture
some common cases:
http://file.bos.redhat.com/~jolawren/klp-compiler-notes/livepatch/compiler-considerations.html
It's not complete and I lost steam about 80% of the way through today.
:) But if it looks useful enough to add to Documentation/livepatch, we
can work on it on-list and try to steer folks into using the automated
kpatch-build, objtool (eventually) or a source-based safety checklist.
The source-based steps have been posted on-list a few times, but I think
it only needs to be formalized in a doc.
-- Joe
Re: [PATCH v2] selftests/livepatch: adopt to newer sysctl error format
On 7/14/20 5:10 AM, Petr Mladek wrote:
With procfs v3.3.16, the sysctl command doesn't print the set key and
value on error. This change breaks livepatch selftest test-ftrace.sh,
that tests the interaction of sysctl ftrace_enabled:
Make it work with all sysctl versions using '-q' option.
Explicitly print the final status on success so that it can be verified
in the log. The error message is enough on failure.
Reported-by: Kamalesh Babulal
Signed-off-by: Petr Mladek
---
The patch has been created against livepatch.git,
branch for-5.9/selftests-cleanup. But it applies also against
the current Linus' tree.
tools/testing/selftests/livepatch/functions.sh | 3 ++-
tools/testing/selftests/livepatch/test-ftrace.sh | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/livepatch/functions.sh
b/tools/testing/selftests/livepatch/functions.sh
index 408529d94ddb..1aba83c87ad3 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -75,7 +75,8 @@ function set_dynamic_debug() {
}
function set_ftrace_enabled() {
- result=$(sysctl kernel.ftrace_enabled="$1" 2>&1 | paste --serial
--delimiters=' ')
+ result=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1 && \
+sysctl kernel.ftrace_enabled 2>&1)
echo "livepatch: $result" > /dev/kmsg
}
diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh
index 9160c9ec3b6f..552e165512f4 100755
--- a/tools/testing/selftests/livepatch/test-ftrace.sh
+++ b/tools/testing/selftests/livepatch/test-ftrace.sh
@@ -51,7 +51,7 @@ livepatch: '$MOD_LIVEPATCH': initializing patching transition
livepatch: '$MOD_LIVEPATCH': starting patching transition
livepatch: '$MOD_LIVEPATCH': completing patching transition
livepatch: '$MOD_LIVEPATCH': patching complete
-livepatch: sysctl: setting key \"kernel.ftrace_enabled\": Device or resource
busy kernel.ftrace_enabled = 0
+livepatch: sysctl: setting key \"kernel.ftrace_enabled\": Device or resource
busy
% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled
livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
livepatch: '$MOD_LIVEPATCH': starting unpatching transition
Looks good, thanks.
Reviewed-by: Joe Lawrence
-- Joe
Re: [PATCH] selftests/livepatch: adopt to newer sysctl error format
On Fri, Jul 10, 2020 at 05:27:35PM +0200, Petr Mladek wrote:
> On Fri 2020-07-10 10:40:43, Kamalesh Babulal wrote:
> > With procfs v3.3.16, the sysctl command doesn't prints the set key and
> > value on error. This change breaks livepatch selftest test-ftrace.sh,
> > that tests the interaction of sysctl ftrace_enabled:
> >
Good catch, it looks like it was this procps commit that modified that
behavior:
commit da82fe49b1476d227874905068adb69577e11d96
Author: Patrick Steinhardt
Date: Tue May 29 13:29:03 2018 +0200
sysctl: do not report set key in case `close_stream` fails
As we're using buffered I/O when writing kernel parameters, write errors
may get delayed until we close the `FILE` stream. As we are currently
outputting the key that is to be set disregarding the return value of
`close_stream`, we may end up in a situation where we report error and
success:
$ sysctl kernel.printk_ratelimit=100
sysctl: setting key "kernel.printk_ratelimit": error code 22
kernel.printk_ratelimit = 100
Fix the issue by only outputting the updated value in case
`close_stream` does not report an error.
Signed-off-by: Patrick Steinhardt
And I'd agree that echoing the failed new value was confusing to see
from a user's perspective.
> > # selftests: livepatch: test-ftrace.sh
> > # TEST: livepatch interaction with ftrace_enabled sysctl ... not ok
> > #
> > # --- expected
> > # +++ result
> > # @@ -16,7 +16,7 @@ livepatch: 'test_klp_livepatch': initial
> > # livepatch: 'test_klp_livepatch': starting patching transition
> > # livepatch: 'test_klp_livepatch': completing patching transition
> > # livepatch: 'test_klp_livepatch': patching complete
> > # -livepatch: sysctl: setting key "kernel.ftrace_enabled": Device or
> > resource busy kernel.ftrace_enabled = 0
> > # +livepatch: sysctl: setting key "kernel.ftrace_enabled": Device or
> > resource busy
> > # % echo 0 > /sys/kernel/livepatch/test_klp_livepatch/enabled
> > # livepatch: 'test_klp_livepatch': initializing unpatching transition
> > # livepatch: 'test_klp_livepatch': starting unpatching transition
> > #
> > # ERROR: livepatch kselftest(s) failed
> >
> > on setting sysctl kernel.ftrace_enabled={0,1} value successfully, the
> > set key and value is displayed.
> >
> > This patch fixes it by limiting the output from both the cases to eight
> > words, that includes the error message or set key and value on failure
> > and success. The upper bound of eight words is enough to display the
> > only tracked error message. Also, adjust the check_result string in
> > test-ftrace.sh to match the expected output.
>
> This looks really tricky.
>
> I wonder if we could use "sysctl -q" to refuse printing the value
> even with older versions. The following patch works here with
> sysctl 3.3.15:
>
FWIW, --quiet was added to procps way back in 2004, so it should be safe
to use... and there's already a bunch of net selftests using it.
> diff --git a/tools/testing/selftests/livepatch/functions.sh
> b/tools/testing/selftests/livepatch/functions.sh
> index 2aab9791791d..47aa4c762bb4 100644
> --- a/tools/testing/selftests/livepatch/functions.sh
> +++ b/tools/testing/selftests/livepatch/functions.sh
> @@ -64,7 +64,8 @@ function set_dynamic_debug() {
> }
>
> function set_ftrace_enabled() {
> - result=$(sysctl kernel.ftrace_enabled="$1" 2>&1 | paste --serial
> --delimiters=' ')
> + result=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1 && \
> + sysctl kernel.ftrace_enabled 2>&1)
> echo "livepatch: $result" > /dev/kmsg
> }
>
> diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh
> b/tools/testing/selftests/livepatch/test-ftrace.sh
> index e2a76887f40a..aa967c5d0558 100755
> --- a/tools/testing/selftests/livepatch/test-ftrace.sh
> +++ b/tools/testing/selftests/livepatch/test-ftrace.sh
> @@ -53,7 +53,7 @@ livepatch: '$MOD_LIVEPATCH': initializing patching
> transition
> livepatch: '$MOD_LIVEPATCH': starting patching transition
> livepatch: '$MOD_LIVEPATCH': completing patching transition
> livepatch: '$MOD_LIVEPATCH': patching complete
> -livepatch: sysctl: setting key \"kernel.ftrace_enabled\": Device or resource
> busy kernel.ftrace_enabled = 0
> +livepatch: sysctl: setting key \"kernel.ftrace_enabled\": Device or resource
> busy
> % echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled
> livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
> livepatch: '$MOD_LIVEPATCH': starting unpatching transition
>
>
I think this method is less fragile than word count / cutting and we get
to drop that strange 'paste' invocation (I had to look that up in the
mapages to remember why we used it).
Regards,
-- Joe
Re: [PATCH] selftests/lkdtm: Use "comm" instead of "diff" for dmesg
On Fri, Jun 26, 2020 at 01:59:43PM -0700, Kees Cook wrote:
> Instead of full GNU diff (which smaller boot environments may not have),
> use "comm" which is more available.
>
> Reported-by: Naresh Kamboju
> Link:
> https://lore.kernel.org/lkml/ca+g9fythp+gg+brr_gkbmxu2ooi-_e9pattpb6tvrswv1g1...@mail.gmail.com
> Fixes: f131d9edc29d ("selftests/lkdtm: Don't clear dmesg when running tests")
> Signed-off-by: Kees Cook
> ---
> tools/testing/selftests/lkdtm/run.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/lkdtm/run.sh
> b/tools/testing/selftests/lkdtm/run.sh
> index 8383eb89d88a..5fe23009ae13 100755
> --- a/tools/testing/selftests/lkdtm/run.sh
> +++ b/tools/testing/selftests/lkdtm/run.sh
> @@ -82,7 +82,7 @@ dmesg > "$DMESG"
> ($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true
>
> # Record and dump the results
> -dmesg | diff --changed-group-format='%>' --unchanged-group-format=''
> "$DMESG" - > "$LOG" || true
> +dmesg | comm -13 "$DMESG" - > "$LOG" || true
>
> cat "$LOG"
> # Check for expected output
I'm not familiar with running lkdtm tests, but I copied the same fixup
for the livepatching selftests and "comm" slides in nicely over there,
so,
Acked-by: Joe Lawrence
-- Joe
Re: [PATCH 1/2] selftests/lkdtm: Don't clear dmesg when running tests
On 6/26/20 4:02 AM, Petr Mladek wrote: On Wed 2020-06-24 16:12:47, Joe Lawrence wrote: On Wed, Jun 24, 2020 at 10:39:55AM +0200, Petr Mladek wrote: On Tue 2020-06-23 23:48:36, Joe Lawrence wrote: On 6/22/20 4:51 AM, Naresh Kamboju wrote: On Fri, 8 May 2020 at 12:23, Michael Ellerman wrote: It is Very Rude to clear dmesg in test scripts. That's because the script may be part of a larger test run, and clearing dmesg potentially destroys the output of other tests. We can avoid using dmesg -c by saving the content of dmesg before the test, and then using diff to compare that to the dmesg afterward, producing a log with just the added lines. diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh index dadf819148a4..0b409e187c7b 100755 --- a/tools/testing/selftests/lkdtm/run.sh +++ b/tools/testing/selftests/lkdtm/run.sh # Record and dump the results -dmesg -c >"$LOG" +dmesg | diff --changed-group-format='%>' --unchanged-group-format='' "$DMESG" - > "$LOG" || true We are facing problems with the diff `=%>` part of the option. This report is from the OpenEmbedded environment. We have the same problem from livepatch_testcases. # selftests lkdtm BUG.sh lkdtm: BUG.sh_ # # diff unrecognized option '--changed-group-format=%>' unrecognized: option_'--changed-group-format=%>' # # BusyBox v1.27.2 (2020-03-30 164108 UTC) multi-call binary. v1.27.2: (2020-03-30_164108 # I did a bit more hacking to work that awk script into the livepatching tests. The changes aren't too bad and coding it ourselves lets us drop the temporary dmesg file business. If this looks good, I can send out as a real patch, but then that raises a few questions: The patch worked and I agree that it is not that bad. Well, what about using "comm" as proposed by Michael in the other mail? It seems to be part of coreutils and should be everywhere. I guess that many people, including me, are not fluent in awk. So, I am slightly in favor of the "comm" approach ;-) comm is definitely simpler and for some reason I forgot about the leading timestamps (again!) dismissing it thinking that the inputs weren't sorted. But luckily they are and if Naresh or anyone can confirm that comm is well supported in the BusyBox testing environment, then using that is fine w/me. -- Joe
Re: [PATCH 1/2] selftests/lkdtm: Don't clear dmesg when running tests
On Wed, Jun 24, 2020 at 10:39:55AM +0200, Petr Mladek wrote:
> On Tue 2020-06-23 23:48:36, Joe Lawrence wrote:
> > On 6/22/20 4:51 AM, Naresh Kamboju wrote:
> > > On Fri, 8 May 2020 at 12:23, Michael Ellerman wrote:
> > > >
> > > > It is Very Rude to clear dmesg in test scripts. That's because the
> > > > script may be part of a larger test run, and clearing dmesg
> > > > potentially destroys the output of other tests.
> > > >
> > > > We can avoid using dmesg -c by saving the content of dmesg before the
> > > > test, and then using diff to compare that to the dmesg afterward,
> > > > producing a log with just the added lines.
> > > >
> > > > > > > diff --git a/tools/testing/selftests/lkdtm/run.sh
> > > > > > > b/tools/testing/selftests/lkdtm/run.sh
> > > > index dadf819148a4..0b409e187c7b 100755
> > > > --- a/tools/testing/selftests/lkdtm/run.sh
> > > > +++ b/tools/testing/selftests/lkdtm/run.sh
> > > > # Record and dump the results
> > > > -dmesg -c >"$LOG"
> > > > +dmesg | diff --changed-group-format='%>' --unchanged-group-format=''
> > > > "$DMESG" - > "$LOG" || true
> > >
> > > We are facing problems with the diff `=%>` part of the option.
> > > This report is from the OpenEmbedded environment.
> > > We have the same problem from livepatch_testcases.
> > >
> > > # selftests lkdtm BUG.sh
> > > lkdtm: BUG.sh_ #
> > > # diff unrecognized option '--changed-group-format=%>'
> > > unrecognized: option_'--changed-group-format=%>' #
> > > # BusyBox v1.27.2 (2020-03-30 164108 UTC) multi-call binary.
> > > v1.27.2: (2020-03-30_164108 #
> > > #
> > > : _ #
> > > # Usage diff [-abBdiNqrTstw] [-L LABEL] [-S FILE] [-U LINES] FILE1 FILE2
> > > diff: [-abBdiNqrTstw]_[-L #
> > > # BUG missing 'kernel BUG at' [FAIL]
> > >
> > > Full test output log,
> > > https://qa-reports.linaro.org/lkft/linux-next-oe/build/next-20200621/testrun/2850083/suite/kselftest/test/lkdtm_BUG.sh/log
> > >
> >
> > D'oh! Using diff's changed/unchanged group format was a nice trick to
> > easily fetch the new kernel log messages.
> >
> > I can't think of any simple alternative off the top of my head, so here's a
> > kludgy tested-once awk script:
> >
> > SAVED_DMESG="$(dmesg | tail -n1)"
> > ... tests ...
> > NEW_DMESG=$(dmesg | awk -v last="$SAVED_DMESG" 'p; $0 == last{p=1}')
> >
> > I think timestamps should make each log line unique, but this probably won't
> > handle kernel log buffer overflow.
>
> The test would fail anyway if there was log buffer overflow.
>
> We could check if the last line was still there and
> suggest to increase the log buffer size in the error message.
>
Hi Petr,
This is a good suggestion and I've worked it into the awk script I
wrote last night, check it out at the bottom ...
> > Maybe it would be easier to log a known unique test delimiter msg and then
> > fetch all new messages after that?
>
> The timestamp should be enough to distinguish any message.
>
> But some visual separator between each test is useful anyway. And
> it might include some random string...
>
Ah yeah, you're right about those timestamps again. :)
As for separators, we've already added the "= TEST: foo =" for
the livepatch ones... so I think with the timestamp prefix, we're good
enough, no?
I did a bit more hacking to work that awk script into the livepatching
tests. The changes aren't too bad and coding it ourselves lets us drop
the temporary dmesg file business. If this looks good, I can send out
as a real patch, but then that raises a few questions:
1 - f131d9edc29d ("selftests/lkdtm: Don't clear dmesg when running
tests") has already merged, updating that file doesn't look too
difficult, but will need a Fixes tag and should probably go
through Shuah's tree.
2 - We haven't actually merged the livepatch copy yet. I can roll
another version of that patchset, substituting a fix for the
problematic patch, or we could just tack this one on at the end.
In fine with either approach.
-- Joe
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
>From 892a651839ac6cdd398d6d9998c0d2e4214b72f7 Mon Sep 17 00:00:00 2001
From: Joe Lawrence
Date: Wed, 24
Re: [PATCH 1/2] selftests/lkdtm: Don't clear dmesg when running tests
On 6/22/20 4:51 AM, Naresh Kamboju wrote:
On Fri, 8 May 2020 at 12:23, Michael Ellerman wrote:
It is Very Rude to clear dmesg in test scripts. That's because the
script may be part of a larger test run, and clearing dmesg
potentially destroys the output of other tests.
We can avoid using dmesg -c by saving the content of dmesg before the
test, and then using diff to compare that to the dmesg afterward,
producing a log with just the added lines.
Signed-off-by: Michael Ellerman
---
tools/testing/selftests/lkdtm/run.sh | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/lkdtm/run.sh
b/tools/testing/selftests/lkdtm/run.sh
index dadf819148a4..0b409e187c7b 100755
--- a/tools/testing/selftests/lkdtm/run.sh
+++ b/tools/testing/selftests/lkdtm/run.sh
@@ -59,23 +59,25 @@ if [ -z "$expect" ]; then
expect="call trace:"
fi
-# Clear out dmesg for output reporting
-dmesg -c >/dev/null
-
# Prepare log for report checking
-LOG=$(mktemp --tmpdir -t lkdtm-XX)
+LOG=$(mktemp --tmpdir -t lkdtm-log-XX)
+DMESG=$(mktemp --tmpdir -t lkdtm-dmesg-XX)
cleanup() {
- rm -f "$LOG"
+ rm -f "$LOG" "$DMESG"
}
trap cleanup EXIT
+# Save existing dmesg so we can detect new content below
+dmesg > "$DMESG"
+
# Most shells yell about signals and we're expecting the "cat" process
# to usually be killed by the kernel. So we have to run it in a sub-shell
# and silence errors.
($SHELL -c 'cat <(echo '"$test"') >'"$TRIGGER" 2>/dev/null) || true
# Record and dump the results
-dmesg -c >"$LOG"
+dmesg | diff --changed-group-format='%>' --unchanged-group-format='' "$DMESG" - >
"$LOG" || true
We are facing problems with the diff `=%>` part of the option.
This report is from the OpenEmbedded environment.
We have the same problem from livepatch_testcases.
# selftests lkdtm BUG.sh
lkdtm: BUG.sh_ #
# diff unrecognized option '--changed-group-format=%>'
unrecognized: option_'--changed-group-format=%>' #
# BusyBox v1.27.2 (2020-03-30 164108 UTC) multi-call binary.
v1.27.2: (2020-03-30_164108 #
#
: _ #
# Usage diff [-abBdiNqrTstw] [-L LABEL] [-S FILE] [-U LINES] FILE1 FILE2
diff: [-abBdiNqrTstw]_[-L #
# BUG missing 'kernel BUG at' [FAIL]
Full test output log,
https://qa-reports.linaro.org/lkft/linux-next-oe/build/next-20200621/testrun/2850083/suite/kselftest/test/lkdtm_BUG.sh/log
D'oh! Using diff's changed/unchanged group format was a nice trick to
easily fetch the new kernel log messages.
I can't think of any simple alternative off the top of my head, so
here's a kludgy tested-once awk script:
SAVED_DMESG="$(dmesg | tail -n1)"
... tests ...
NEW_DMESG=$(dmesg | awk -v last="$SAVED_DMESG" 'p; $0 == last{p=1}')
I think timestamps should make each log line unique, but this probably
won't handle kernel log buffer overflow.
Maybe it would be easier to log a known unique test delimiter msg and
then fetch all new messages after that?
-- Joe
Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
On 4/30/20 12:48 PM, Josh Poimboeuf wrote: On Thu, Apr 30, 2020 at 10:38:21AM -0400, Joe Lawrence wrote: On Thu, Apr 23, 2020 at 01:10:30PM -0500, Josh Poimboeuf wrote: This is more of note for the future, but when/if we add livepatch support on arm64 we'll need to make the very same adjustment there as well. See the following pattern: arch/arm64/kernel/module.c: reloc_insn_movw() reloc_insn_imm() reloc_insn_adrp() *place = cpu_to_le32(insn); maybe something like aarch64_insn_patch_text_nosync() could be used there, I dunno. (It looks like ftrace and jump_labels are using that interface.) This is outside the scope of the patchset, but I thought I'd mention it as I was curious to see how other arches were currently handling their relocation updates. True... I suspect your klp-convert selftests will catch that? Indeed. Actually I had hacked enough livepatch code support on ARM to see what happened when converting and loading the test patches :) -- Joe
Re: [PATCH v2 6/9] s390/module: Use s390_kernel_write() for late relocations
On Thu, Apr 23, 2020 at 01:10:30PM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 23, 2020 at 09:12:28AM -0500, Josh Poimboeuf wrote:
> > > > this is strange. While I would have expected an exception similar to
> > > > this, it really should have happened on the "sturg" instruction which
> > > > does the DAT-off store in s390_kernel_write(), and certainly not with
> > > > an ID of 0004 (protection). However, in your case, it happens on a
> > > > normal store instruction, with 0004 indicating a protection exception.
> > > >
> > > > This is more like what I would expect e.g. in the case where you do
> > > > _not_ use the s390_kernel_write() function for RO module text patching,
> > > > but rather normal memory access. So I am pretty sure that this is not
> > > > related to the s390_kernel_write(), but some other issue, maybe some
> > > > place left where you still use normal memory access?
> > >
> > > The call trace above also suggests that it is not a late relocation, no?
> > > The path is from KLP module init function through klp_enable_patch. It
> > > should
> > > mean that the to-be-patched object is loaded (it must be a module thanks
> > > to a check klp_init_object_loaded(), vmlinux relocations were processed
> > > earlier in apply_relocations()).
> > >
> > > However, the KLP module state here must be COMING, so s390_kernel_write()
> > > should be used. What are we missing?
> >
> > I'm also scratching my head. It _should_ be using s390_kernel_write()
> > based on the module state, but I don't see that on the stack trace.
> >
> > This trace (and Gerald's comment) seem to imply it's using
> > __builtin_memcpy(), which might expected for UNFORMED state.
> >
> > Weird...
>
> Mystery solved:
>
> $ CROSS_COMPILE=s390x-linux-gnu- scripts/faddr2line vmlinux
> apply_rela+0x16a/0x520
> apply_rela+0x16a/0x520:
> apply_rela at arch/s390/kernel/module.c:336
>
> which corresponds to the following code in apply_rela():
>
>
> case R_390_PLTOFF64:/* 16 bit offset from GOT to PLT. */
> if (info->plt_initialized == 0) {
> unsigned int *ip;
> ip = me->core_layout.base + me->arch.plt_offset +
> info->plt_offset;
> ip[0] = 0x0d10e310; /* basr 1,0 */
> ip[1] = 0x100a0004; /* lg 1,10(1) */
>
>
> Notice how it's writing directly to text... oops.
>
This is more of note for the future, but when/if we add livepatch
support on arm64 we'll need to make the very same adjustment there as
well. See the following pattern:
arch/arm64/kernel/module.c:
reloc_insn_movw()
reloc_insn_imm()
reloc_insn_adrp()
*place = cpu_to_le32(insn);
maybe something like aarch64_insn_patch_text_nosync() could be used
there, I dunno. (It looks like ftrace and jump_labels are using that
interface.)
This is outside the scope of the patchset, but I thought I'd mention it
as I was curious to see how other arches were currently handling their
relocation updates.
-- Joe
Re: [PATCH v3 3/3] selftests/livepatch: Test interaction with ftrace_enabled
On 10/16/19 1:06 PM, Kamalesh Babulal wrote: On 10/16/19 5:03 PM, Miroslav Benes wrote: From: Joe Lawrence Since livepatching depends upon ftrace handlers to implement "patched" code functionality, verify that the ftrace_enabled sysctl value interacts with livepatch registration as expected. At the same time, ensure that ftrace_enabled is set and part of the test environment configuration that is saved and restored when running the selftests. Signed-off-by: Joe Lawrence Signed-off-by: Miroslav Benes [...] diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh new file mode 100755 index ..e2a76887f40a --- /dev/null +++ b/tools/testing/selftests/livepatch/test-ftrace.sh This test fails due to wrong file permissions, with the warning: # Warning: file test-ftrace.sh is not executable, correct this. not ok 4 selftests: livepatch: test-ftrace.sh Hi Kamalesh, Thanks for testing. This error is weird as the git log says new file mode: 100755. 'git am' of Miroslav's patchset gives me a new test-ftrace.sh with "Access: (0775/-rwxrwxr-x)" Did you apply the mbox directly or.. ? -- Joe
Re: [PATCH v3 2/3] selftests/livepatch: Make dynamic debug setup and restore generic
On 10/16/19 1:10 PM, Kamalesh Babulal wrote: On 10/16/19 5:03 PM, Miroslav Benes wrote: From: Joe Lawrence Livepatch selftests currently save the current dynamic debug config and tweak it for the selftests. The config is restored at the end. Make the infrastructure generic, so that more variables can be saved and restored. Signed-off-by: Joe Lawrence Signed-off-by: Miroslav Benes --- .../testing/selftests/livepatch/functions.sh | 22 +++ .../selftests/livepatch/test-callbacks.sh | 2 +- .../selftests/livepatch/test-livepatch.sh | 2 +- .../selftests/livepatch/test-shadow-vars.sh | 2 +- A minor nit pick, should the README also updated with the setup_config()? Yup, good eye. I think it should be a simple matter of s/set_dynamic_debug/setup_config/g -- Joe
Re: [PATCH v3 5/6] x86/ftrace: Use text_poke()
On 10/15/19 11:31 AM, Jessica Yu wrote: +++ Joe Lawrence [15/10/19 11:06 -0400]: On 10/15/19 10:13 AM, Miroslav Benes wrote: Yes, it does. klp_module_coming() calls module_disable_ro() on all patching modules which patch the coming module in order to call apply_relocate_add(). New (patching) code for a module can be relocated only when the relevant module is loaded. FWIW, would the LPC blue-sky2 model (ie, Steve's suggestion @ plumber's where livepatches only patch a single object and updates are kept on disk to handle coming module updates as they are loaded) eliminate those outstanding relocations and the need to perform this late permission flipping? I wasn't at Plumbers sadly, was this idea documented/talked about in detail somewhere? (recording, slides, etherpad, etc?). What do you mean by updates are kept on disk? Maybe someone can explain it more in detail? :) Livepatching folks -- I don't have the LPC summary link (etherpad?) that Jiri put together. Does someone have that handy for Jessica? Thanks, -- Joe
Re: [PATCH v3 5/6] x86/ftrace: Use text_poke()
On 10/15/19 10:13 AM, Miroslav Benes wrote: Yes, it does. klp_module_coming() calls module_disable_ro() on all patching modules which patch the coming module in order to call apply_relocate_add(). New (patching) code for a module can be relocated only when the relevant module is loaded. FWIW, would the LPC blue-sky2 model (ie, Steve's suggestion @ plumber's where livepatches only patch a single object and updates are kept on disk to handle coming module updates as they are loaded) eliminate those outstanding relocations and the need to perform this late permission flipping? -- Joe
Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag
On Tue, Oct 15, 2019 at 01:23:10PM +0200, Miroslav Benes wrote:
> > Hi Miroslav,
> >
> > Maybe we should add a test to verify this new behavior? See sample
> > version below (lightly tested). We can add to this one, or patch
> > seperately if you prefer.
>
> Thanks a lot, Joe. It looks nice. I'll include it in v3. One question
> below.
>
> [ ... snip ... ]
>
> Shouldn't we call push_ftrace_enabled() somewhere at the beginning of the
> test script? set_dynamic_debug() calls its push_dynamic_debug() directly,
> but set_ftrace_enabled() is different, because it is called more than once
> in the script.
>
> One could argue that ftrace_enabled has to be true at the beginning of
> testing anyway, but I think it would be cleaner. Btw, we should probably
> guarantee that ftrace_enabled is true when livepatch selftests are
> invoked.
>
Here's a new version that combines the ftrace_enabled configuration with
the debugfs stuff (so its only pushed/pop once per test) and updates all
the tests accordingly. Feel free to any tweaks or flourishes when
adding to v3.
Thanks,
-- Joe
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
>From a22ca55b3f429b7c9ceed6be87a571f77520994c Mon Sep 17 00:00:00 2001
From: Joe Lawrence
Date: Tue, 15 Oct 2019 10:33:18 -0400
Subject: [PATCH] selftests/livepatch: test interaction with ftrace_enabled
Since livepatching depends upon ftrace handlers to implement "patched"
code functionality, verify that the ftrace_enabled sysctl value
interacts with livepatch registration as expected. At the same time,
ensure that ftrace_enabled is set and part of the test environment
configuration that is saved and restored when running the selftests.
Signed-off-by: Joe Lawrence
---
tools/testing/selftests/livepatch/Makefile| 3 +-
.../testing/selftests/livepatch/functions.sh | 34 +++---
.../selftests/livepatch/test-callbacks.sh | 2 +-
.../selftests/livepatch/test-ftrace.sh| 65 +++
.../selftests/livepatch/test-livepatch.sh | 2 +-
.../selftests/livepatch/test-shadow-vars.sh | 2 +-
6 files changed, 95 insertions(+), 13 deletions(-)
create mode 100755 tools/testing/selftests/livepatch/test-ftrace.sh
diff --git a/tools/testing/selftests/livepatch/Makefile
b/tools/testing/selftests/livepatch/Makefile
index fd405402c3ff..1886d9d94b88 100644
--- a/tools/testing/selftests/livepatch/Makefile
+++ b/tools/testing/selftests/livepatch/Makefile
@@ -4,6 +4,7 @@ TEST_PROGS_EXTENDED := functions.sh
TEST_PROGS := \
test-livepatch.sh \
test-callbacks.sh \
- test-shadow-vars.sh
+ test-shadow-vars.sh \
+ test-ftrace.sh
include ../lib.mk
diff --git a/tools/testing/selftests/livepatch/functions.sh
b/tools/testing/selftests/livepatch/functions.sh
index 79b0affd21fb..31eb09e38729 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -29,29 +29,45 @@ function die() {
exit 1
}
-function push_dynamic_debug() {
-DYNAMIC_DEBUG=$(grep '^kernel/livepatch'
/sys/kernel/debug/dynamic_debug/control | \
-awk -F'[: ]' '{print "file " $1 " line " $2 " " $4}')
+function push_config() {
+ DYNAMIC_DEBUG=$(grep '^kernel/livepatch'
/sys/kernel/debug/dynamic_debug/control | \
+ awk -F'[: ]' '{print "file " $1 " line " $2 " " $4}')
+ FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled)
}
-function pop_dynamic_debug() {
+function pop_config() {
if [[ -n "$DYNAMIC_DEBUG" ]]; then
echo -n "$DYNAMIC_DEBUG" >
/sys/kernel/debug/dynamic_debug/control
fi
+ if [[ -n "$FTRACE_ENABLED" ]]; then
+ sysctl kernel.ftrace_enabled="$FTRACE_ENABLED" &> /dev/null
+ fi
}
-# set_dynamic_debug() - save the current dynamic debug config and tweak
-# it for the self-tests. Set a script exit trap
-# that restores the original config.
function set_dynamic_debug() {
-push_dynamic_debug
-trap pop_dynamic_debug EXIT INT TERM HUP
cat <<-EOF > /sys/kernel/debug/dynamic_debug/control
file kernel/livepatch/* +p
func klp_try_switch_task -p
EOF
}
+function set_ftrace_enabled() {
+ local sysctl="$1"
+ result=$(sysctl kernel.ftrace_enabled="$1" 2>&1 | paste --serial
--delimiters=' ')
+ echo "livepatch: $result" > /dev/kmsg
+}
+
+# setup_config - save the current config and set a script exit trap that
+# restores the original config. Setup the dyna
Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag
On 10/15/19 7:23 AM, Miroslav Benes wrote:
Hi Miroslav,
Maybe we should add a test to verify this new behavior? See sample
version below (lightly tested). We can add to this one, or patch
seperately if you prefer.
Thanks a lot, Joe. It looks nice. I'll include it in v3. One question
below.
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
>From c8c9f22e3816ca4c90ab7e7159d2ce536eaa5fad Mon Sep 17 00:00:00 2001
From: Joe Lawrence
Date: Mon, 14 Oct 2019 18:25:01 -0400
Subject: [PATCH] selftests/livepatch: test interaction with ftrace_enabled
Since livepatching depends upon ftrace handlers to implement "patched"
functionality, verify that the ftrace_enabled sysctl value interacts
with livepatch registration as expected.
Signed-off-by: Joe Lawrence
---
tools/testing/selftests/livepatch/Makefile| 3 +-
.../testing/selftests/livepatch/functions.sh | 18 +
.../selftests/livepatch/test-ftrace.sh| 65 +++
3 files changed, 85 insertions(+), 1 deletion(-)
create mode 100755 tools/testing/selftests/livepatch/test-ftrace.sh
diff --git a/tools/testing/selftests/livepatch/Makefile
b/tools/testing/selftests/livepatch/Makefile
index fd405402c3ff..1886d9d94b88 100644
--- a/tools/testing/selftests/livepatch/Makefile
+++ b/tools/testing/selftests/livepatch/Makefile
@@ -4,6 +4,7 @@ TEST_PROGS_EXTENDED := functions.sh
TEST_PROGS := \
test-livepatch.sh \
test-callbacks.sh \
- test-shadow-vars.sh
+ test-shadow-vars.sh \
+ test-ftrace.sh
include ../lib.mk
diff --git a/tools/testing/selftests/livepatch/functions.sh
b/tools/testing/selftests/livepatch/functions.sh
index 79b0affd21fb..556252efece0 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -52,6 +52,24 @@ function set_dynamic_debug() {
EOF
}
+function push_ftrace_enabled() {
+ FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled)
+}
Shouldn't we call push_ftrace_enabled() somewhere at the beginning of the
test script? set_dynamic_debug() calls its push_dynamic_debug() directly,
but set_ftrace_enabled() is different, because it is called more than once
in the script.
One could argue that ftrace_enabled has to be true at the beginning of
testing anyway, but I think it would be cleaner. Btw, we should probably
guarantee that ftrace_enabled is true when livepatch selftests are
invoked.
Ah yes, that occurred to me while creating that piece of the patch.
Something like setup_test_config() that pushes both ftrace and the
debugfs, etc. would be cleaner for all scripts. If you're onboard with
that idea, I can make that revision.
-- Joe
Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag
On Mon, Oct 14, 2019 at 12:59:23PM +0200, Miroslav Benes wrote:
> Livepatch uses ftrace for redirection to new patched functions. It means
> that if ftrace is disabled, all live patched functions are disabled as
> well. Toggling global 'ftrace_enabled' sysctl thus affect it directly.
> It is not a problem per se, because only administrator can set sysctl
> values, but it still may be surprising.
>
> Introduce PERMANENT ftrace_ops flag to amend this. If the
> FTRACE_OPS_FL_PERMANENT is set on any ftrace ops, the tracing cannot be
> disabled by disabling ftrace_enabled. Equally, a callback with the flag
> set cannot be registered if ftrace_enabled is disabled.
>
> Signed-off-by: Miroslav Benes
> ---
> v1->v2:
> - different logic, proposed by Joe Lawrence
>
> Two things I am not sure about much:
>
> - return codes. I chose EBUSY, because it seemed the least
> inappropriate. I usually pick the wrong one, so suggestions are
> welcome.
> - I did not add any pr_* reporting the problem to make it consistent
> with the existing code.
>
> Documentation/trace/ftrace-uses.rst | 8
> Documentation/trace/ftrace.rst | 4 +++-
> include/linux/ftrace.h | 3 +++
> kernel/livepatch/patch.c| 3 ++-
> kernel/trace/ftrace.c | 23 +--
> 5 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/trace/ftrace-uses.rst
> b/Documentation/trace/ftrace-uses.rst
> index 1fbc69894eed..740bd0224d35 100644
> --- a/Documentation/trace/ftrace-uses.rst
> +++ b/Documentation/trace/ftrace-uses.rst
> @@ -170,6 +170,14 @@ FTRACE_OPS_FL_RCU
> a callback may be executed and RCU synchronization will not protect
> it.
>
> +FTRACE_OPS_FL_PERMANENT
> +If this is set on any ftrace ops, then the tracing cannot disabled by
> +writing 0 to the proc sysctl ftrace_enabled. Equally, a callback with
> +the flag set cannot be registered if ftrace_enabled is 0.
> +
> +Livepatch uses it not to lose the function redirection, so the system
> +stays protected.
> +
>
> Filtering which functions to trace
> ==
> diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
> index e3060eedb22d..d2b5657ed33e 100644
> --- a/Documentation/trace/ftrace.rst
> +++ b/Documentation/trace/ftrace.rst
> @@ -2976,7 +2976,9 @@ Note, the proc sysctl ftrace_enable is a big on/off
> switch for the
> function tracer. By default it is enabled (when function tracing is
> enabled in the kernel). If it is disabled, all function tracing is
> disabled. This includes not only the function tracers for ftrace, but
> -also for any other uses (perf, kprobes, stack tracing, profiling, etc).
> +also for any other uses (perf, kprobes, stack tracing, profiling, etc). It
> +cannot be disabled if there is a callback with FTRACE_OPS_FL_PERMANENT set
> +registered.
>
> Please disable this with care.
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 8a8cb3c401b2..c2cad29dc557 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -142,6 +142,8 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
> * PID - Is affected by set_ftrace_pid (allows filtering on those pids)
> * RCU - Set when the ops can only be called when RCU is watching.
> * TRACE_ARRAY - The ops->private points to a trace_array descriptor.
> + * PERMAMENT - Set when the ops is permanent and should not be affected by
> + * ftrace_enabled.
> */
> enum {
> FTRACE_OPS_FL_ENABLED = 1 << 0,
> @@ -160,6 +162,7 @@ enum {
> FTRACE_OPS_FL_PID = 1 << 13,
> FTRACE_OPS_FL_RCU = 1 << 14,
> FTRACE_OPS_FL_TRACE_ARRAY = 1 << 15,
> + FTRACE_OPS_FL_PERMANENT = 1 << 16,
> };
>
> #ifdef CONFIG_DYNAMIC_FTRACE
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index bd43537702bd..b552cf2d85f8 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -196,7 +196,8 @@ static int klp_patch_func(struct klp_func *func)
> ops->fops.func = klp_ftrace_handler;
> ops->fops.flags = FTRACE_OPS_FL_SAVE_REGS |
> FTRACE_OPS_FL_DYNAMIC |
> - FTRACE_OPS_FL_IPMODIFY;
> + FTRACE_OPS_FL_IPMODIFY |
> + FTRACE_OPS_FL_PERMANENT;
>
> list_add(&ops->node, &klp_ops);
>
> diff --git a/kernel/t
Re: [PATCH v3 5/5] livepatch: Selftests of the API for tracking system state changes
On Wed, Oct 09, 2019 at 04:18:59PM +0200, Petr Mladek wrote:
> On Fri 2019-10-04 10:47:42, Joe Lawrence wrote:
> > On Thu, Oct 03, 2019 at 11:01:37AM +0200, Petr Mladek wrote:
> > > Four selftests for the new API.
> > >
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/livepatch/test-state.sh
> > > @@ -0,0 +1,180 @@
> > > +#!/bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (C) 2018 Joe Lawrence
> > > +
> >
> > nit: this should probably read as:
> > # Copyright (C) 2019 Petr Mladek
> >
> > > +
> > > +load_lp $MOD_LIVEPATCH
> > > +disable_lp $MOD_LIVEPATCH
> > > +unload_lp $MOD_LIVEPATCH
> > > +
> > > +check_result "% modprobe test_klp_state
> > > +livepatch: enabling patch 'test_klp_state'
> > > +livepatch: 'test_klp_state': initializing patching transition
> > > +test_klp_state: pre_patch_callback: vmlinux
> > > +test_klp_state: allocate_loglevel_state: allocating space to store
> > > console_loglevel
> > > +livepatch: 'test_klp_state': starting patching transition
> > > +livepatch: 'test_klp_state': completing patching transition
> > > +test_klp_state: post_patch_callback: vmlinux
> > > +test_klp_state: fix_console_loglevel: fixing console_loglevel
> > > +livepatch: 'test_klp_state': patching complete
> > > +% echo 0 > /sys/kernel/livepatch/test_klp_state/enabled
> > > +livepatch: 'test_klp_state': initializing unpatching transition
> > > +test_klp_state: pre_unpatch_callback: vmlinux
> > > +test_klp_state: restore_console_loglevel: restoring console_loglevel
> > > +livepatch: 'test_klp_state': starting unpatching transition
> > > +livepatch: 'test_klp_state': completing unpatching transition
> > > +test_klp_state: post_unpatch_callback: vmlinux
> > > +test_klp_state: free_loglevel_state: freeing space for the stored
> > > console_loglevel
> > > +livepatch: 'test_klp_state': unpatching complete
> > > +% rmmod test_klp_state"
> > > +
> >
> > nit: a matter of style, and I don't mind either, but the other test
> > scripts used $MOD_LIVEPATCH{2,3} variable replacement in the
> > check_result string. I think I originally did that when we were
> > reviewing the first self-test patchset and the filenames may or may not
> > have changed. Those names are stable now, so I don't have a strong
> > opinion either way.
>
> Please, find below and updated version of the patch that fixes
> the two above problems and also the kbuild robot failure.
>
> The build failure was fixed by #include
>
> Here we go:
>
> From 206e0a53de25ddb042e5947c2a736e33ca4a2b4c Mon Sep 17 00:00:00 2001
> From: Petr Mladek
> Date: Mon, 10 Jun 2019 14:02:42 +0200
> Subject: [PATCH v3.1 5/5] livepatch: Selftests of the API for tracking system
> state
> changes
>
> Four selftests for the new API.
>
> Signed-off-by: Petr Mladek
> Acked-by: Miroslav Benes
> ---
> lib/livepatch/Makefile | 5 +-
> lib/livepatch/test_klp_state.c | 162
> lib/livepatch/test_klp_state2.c | 191
>
> lib/livepatch/test_klp_state3.c | 5 +
> tools/testing/selftests/livepatch/Makefile | 3 +-
> tools/testing/selftests/livepatch/test-state.sh | 180 ++
> 6 files changed, 544 insertions(+), 2 deletions(-)
> create mode 100644 lib/livepatch/test_klp_state.c
> create mode 100644 lib/livepatch/test_klp_state2.c
> create mode 100644 lib/livepatch/test_klp_state3.c
> create mode 100755 tools/testing/selftests/livepatch/test-state.sh
>
> diff --git a/lib/livepatch/Makefile b/lib/livepatch/Makefile
> index 26900ddaef82..295b94bff370 100644
> --- a/lib/livepatch/Makefile
> +++ b/lib/livepatch/Makefile
> @@ -8,7 +8,10 @@ obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
> test_klp_callbacks_busy.o \
> test_klp_callbacks_mod.o \
> test_klp_livepatch.o \
> - test_klp_shadow_vars.o
> + test_klp_shadow_vars.o \
> + test_klp_state.o \
> + test_klp_state2.o \
> + test_klp_state3.o
>
> # Target modules to be livepatched require CC_FLAGS_FTRACE
> CFLAGS_test_klp_callbacks_busy.o += $
Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag
On Wed, Oct 09, 2019 at 01:22:34PM +0200, Petr Mladek wrote: > On Tue 2019-10-08 15:35:34, Joe Lawrence wrote: > > On Mon, Oct 07, 2019 at 10:17:11AM +0200, Miroslav Benes wrote: > > > Livepatch uses ftrace for redirection to new patched functions. It is > > > thus directly affected by ftrace sysctl knobs such as ftrace_enabled. > > > Setting ftrace_enabled to 0 also disables all live patched functions. It > > > is not a problem per se, because only administrator can set sysctl > > > values, but it still may be surprising. > > > > > > Introduce PERMANENT ftrace_ops flag to amend this. If the > > > FTRACE_OPS_FL_PERMANENT is set, the tracing of the function is not > > > disabled. Such ftrace_ops can still be unregistered in a standard way. > > > > > > The patch set passes ftrace and livepatch kselftests. > > > > > > Miroslav Benes (3): > > > ftrace: Make test_rec_ops_needs_regs() generic > > > ftrace: Introduce PERMANENT ftrace_ops flag > > > livepatch: Use FTRACE_OPS_FL_PERMANENT > > > > > > Documentation/trace/ftrace-uses.rst | 6 > > > Documentation/trace/ftrace.rst | 2 ++ > > > include/linux/ftrace.h | 8 +++-- > > > kernel/livepatch/patch.c| 3 +- > > > kernel/trace/ftrace.c | 47 - > > > 5 files changed, 55 insertions(+), 11 deletions(-) > > > > > > -- > > > 2.23.0 > > > > > > > Hi Miroslav, > > > > I wonder if the opposite would be more intuitive: when ftrace_enabled is > > not set, don't allow livepatches to register ftrace filters and > > likewise, don't allow ftrace_enabled to be unset if any livepatches are > > already registered. I guess you could make an argument either way, but > > just offering another option. Perhaps livepatches should follow similar > > behavior of other ftrace clients (like perf probes?) > > I am not sure that I understand it correctly. > > ftrace_enables is a global flag. My expectation is that it can be > manipulated at any time. But it should affect only ftrace handlers > without FTRACE_OPS_FL_PERMANENT flag. > > By other words, the handlers with FTRACE_OPS_FL_PERMANENT flag and > only these handlers should ignore the global flag. > > To be even more precise. If a function has registered more ftrace > handlers then the global ftrace_enable setting shold affect only > the handlers without the flag. > Petr, I believe this is more or less what the patchset implemented. I pointed out a small inconsistency in that livepatches loaded after ftrace_enable=0 successfully transitioned despite the ftrace handlers not being enabled for that livepatch. Toggling ftrace_enable would have the side effect of enabling those handlers. >From the POV of ftrace I suppose this may be expected behavior and nobody should be mucking with sysctl's that they don't fully understand. However if I put on my sysadmin hat, I think I would find this slightly confusing. At the very least, the livepatch load should make some mention that its replacement functions aren't actually live. Shoring up this quirk so that the FTRACE_OPS_FL_PERMANENT always registers and fires might be simple enough solution... On the other hand, I suggested that the presence of FTRACE_OPS_FL_PERMANENT flags could prevent turning off ftrace_enable and vice versa. That would offer the user immediate feedback without introducing potentially unexpected (and silent) behavior. I'm happy with either solution as long as it's consistent for the user and easy to maintain :) -- Joe
Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag
On Mon, Oct 07, 2019 at 10:17:11AM +0200, Miroslav Benes wrote: > Livepatch uses ftrace for redirection to new patched functions. It is > thus directly affected by ftrace sysctl knobs such as ftrace_enabled. > Setting ftrace_enabled to 0 also disables all live patched functions. It > is not a problem per se, because only administrator can set sysctl > values, but it still may be surprising. > > Introduce PERMANENT ftrace_ops flag to amend this. If the > FTRACE_OPS_FL_PERMANENT is set, the tracing of the function is not > disabled. Such ftrace_ops can still be unregistered in a standard way. > > The patch set passes ftrace and livepatch kselftests. > > Miroslav Benes (3): > ftrace: Make test_rec_ops_needs_regs() generic > ftrace: Introduce PERMANENT ftrace_ops flag > livepatch: Use FTRACE_OPS_FL_PERMANENT > > Documentation/trace/ftrace-uses.rst | 6 > Documentation/trace/ftrace.rst | 2 ++ > include/linux/ftrace.h | 8 +++-- > kernel/livepatch/patch.c| 3 +- > kernel/trace/ftrace.c | 47 - > 5 files changed, 55 insertions(+), 11 deletions(-) > > -- > 2.23.0 > Hi Miroslav, I wonder if the opposite would be more intuitive: when ftrace_enabled is not set, don't allow livepatches to register ftrace filters and likewise, don't allow ftrace_enabled to be unset if any livepatches are already registered. I guess you could make an argument either way, but just offering another option. Perhaps livepatches should follow similar behavior of other ftrace clients (like perf probes?) As for the approach in this patchset, is it consistent that livepatches loaded after setting ftrace_enabled to 0 will successfully load, but not execute their new code... but then when ftrace_enabled is toggled, the new livepatch code remains on? For example: 1 - Turn ftrace_enabled off and load the /proc/cmdline livepatch test case, note that it reports a success patching transition, but doesn't run new its code: % dmesg -C % sysctl kernel.ftrace_enabled=0 kernel.ftrace_enabled = 0 % insmod lib/livepatch/test_klp_livepatch.ko % echo $? 0 % dmesg [ 450.579980] livepatch: enabling patch 'test_klp_livepatch' [ 450.581243] livepatch: 'test_klp_livepatch': starting patching transition [ 451.942971] livepatch: 'test_klp_livepatch': patching complete % cat /proc/cmdline BOOT_IMAGE=(hd0,msdos1)/boot/vmlinuz-5.4.0-rc2+ root=UUID=c42bb089-b5c1-4e17-82bd-132f55bee54c ro console=ttyS0 console=ttyS0,115200n8 no_timer_check net.ifnames=0 crashkernel=auto 2 - Turn ftrace_enabled on and see that the livepatch now works: % sysctl kernel.ftrace_enabled=1 kernel.ftrace_enabled = 1 % cat /proc/cmdline test_klp_livepatch: this has been live patched 3 - Turn ftrace_enabled off and see that it's still enabled: % sysctl kernel.ftrace_enabled=0 kernel.ftrace_enabled = 0 % cat /proc/cmdline test_klp_livepatch: this has been live patched Steps 2 and 3 match the behavior described by the patchset, but I was particularly wondering what you thought about step 1. IMHO, I would expect step 1 to fully enable the livepatch, or at the very least, not report a patch transition (though that may confuse userspace tools waiting for that report). Thanks, -- Joe
Re: [PATCH v3 5/5] livepatch: Selftests of the API for tracking system state changes
On Thu, Oct 03, 2019 at 11:01:37AM +0200, Petr Mladek wrote:
> Four selftests for the new API.
>
> Signed-off-by: Petr Mladek
> Acked-by: Miroslav Benes
> ---
> lib/livepatch/Makefile | 5 +-
> lib/livepatch/test_klp_state.c | 161
> lib/livepatch/test_klp_state2.c | 190
>
> lib/livepatch/test_klp_state3.c | 5 +
> tools/testing/selftests/livepatch/Makefile | 3 +-
> tools/testing/selftests/livepatch/test-state.sh | 180 ++
> 6 files changed, 542 insertions(+), 2 deletions(-)
> create mode 100644 lib/livepatch/test_klp_state.c
> create mode 100644 lib/livepatch/test_klp_state2.c
> create mode 100644 lib/livepatch/test_klp_state3.c
> create mode 100755 tools/testing/selftests/livepatch/test-state.sh
>
> [ ... snip ... ]
> diff --git a/tools/testing/selftests/livepatch/test-state.sh
> b/tools/testing/selftests/livepatch/test-state.sh
> new file mode 100755
> index ..1139c664c11c
> --- /dev/null
> +++ b/tools/testing/selftests/livepatch/test-state.sh
> @@ -0,0 +1,180 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2018 Joe Lawrence
> +
nit: this should probably read as:
# Copyright (C) 2019 Petr Mladek
> +. $(dirname $0)/functions.sh
> +
> +MOD_LIVEPATCH=test_klp_state
> +MOD_LIVEPATCH2=test_klp_state2
> +MOD_LIVEPATCH3=test_klp_state3
> +
> +set_dynamic_debug
> +
> +
> +# TEST: Loading and removing a module that modifies the system state
> +
> +echo -n "TEST: system state modification ... "
> +dmesg -C
> +
> +load_lp $MOD_LIVEPATCH
> +disable_lp $MOD_LIVEPATCH
> +unload_lp $MOD_LIVEPATCH
> +
> +check_result "% modprobe test_klp_state
> +livepatch: enabling patch 'test_klp_state'
> +livepatch: 'test_klp_state': initializing patching transition
> +test_klp_state: pre_patch_callback: vmlinux
> +test_klp_state: allocate_loglevel_state: allocating space to store
> console_loglevel
> +livepatch: 'test_klp_state': starting patching transition
> +livepatch: 'test_klp_state': completing patching transition
> +test_klp_state: post_patch_callback: vmlinux
> +test_klp_state: fix_console_loglevel: fixing console_loglevel
> +livepatch: 'test_klp_state': patching complete
> +% echo 0 > /sys/kernel/livepatch/test_klp_state/enabled
> +livepatch: 'test_klp_state': initializing unpatching transition
> +test_klp_state: pre_unpatch_callback: vmlinux
> +test_klp_state: restore_console_loglevel: restoring console_loglevel
> +livepatch: 'test_klp_state': starting unpatching transition
> +livepatch: 'test_klp_state': completing unpatching transition
> +test_klp_state: post_unpatch_callback: vmlinux
> +test_klp_state: free_loglevel_state: freeing space for the stored
> console_loglevel
> +livepatch: 'test_klp_state': unpatching complete
> +% rmmod test_klp_state"
> +
nit: a matter of style, and I don't mind either, but the other test
scripts used $MOD_LIVEPATCH{2,3} variable replacement in the
check_result string. I think I originally did that when we were
reviewing the first self-test patchset and the filenames may or may not
have changed. Those names are stable now, so I don't have a strong
opinion either way.
FWIW, if someone wants to make the copyright change on merge, I'm cool
with this version.
-- Joe
Re: [PATCH v3 0/5] livepatch: new API to track system state changes
On Thu, Oct 03, 2019 at 11:01:32AM +0200, Petr Mladek wrote: > Hi, > > this is another piece in the puzzle that helps to maintain more > livepatches. > > Especially pre/post (un)patch callbacks might change a system state. > Any newly installed livepatch has to somehow deal with system state > modifications done be already installed livepatches. > > This patchset provides a simple and generic API that > helps to keep and pass information between the livepatches. > It is also usable to prevent loading incompatible livepatches. > > Changes since v2: > > + Typo fixes [Miroslav] > + Move the documentation at the end of the list [Miroslav] > + Add Miroslav's acks > > Changes since v1: > > + Use "unsigned long" instead of "int" for "state.id" [Nicolai] > + Use "unsigned int" instead of "int" for "state.version [Petr] > + Include "state.h" to avoid warning about non-static func [Miroslav] > + Simplify logic in klp_is_state_compatible() [Miroslav] > + Document how livepatches should handle the state [Nicolai] > + Fix some typos, formulation, module metadata [Joe, Miroslav] > > Petr Mladek (5): > livepatch: Keep replaced patches until post_patch callback is called > livepatch: Basic API to track system state changes > livepatch: Allow to distinguish different version of system state > changes > livepatch: Documentation of the new API for tracking system state > changes > livepatch: Selftests of the API for tracking system state changes > > Documentation/livepatch/index.rst | 1 + > Documentation/livepatch/system-state.rst| 167 + > include/linux/livepatch.h | 17 +++ > kernel/livepatch/Makefile | 2 +- > kernel/livepatch/core.c | 44 -- > kernel/livepatch/core.h | 5 +- > kernel/livepatch/state.c| 122 +++ > kernel/livepatch/state.h| 9 ++ > kernel/livepatch/transition.c | 12 +- > lib/livepatch/Makefile | 5 +- > lib/livepatch/test_klp_state.c | 161 > lib/livepatch/test_klp_state2.c | 190 > > lib/livepatch/test_klp_state3.c | 5 + > tools/testing/selftests/livepatch/Makefile | 3 +- > tools/testing/selftests/livepatch/test-state.sh | 180 ++ > 15 files changed, 902 insertions(+), 21 deletions(-) > create mode 100644 Documentation/livepatch/system-state.rst > create mode 100644 kernel/livepatch/state.c > create mode 100644 kernel/livepatch/state.h > create mode 100644 lib/livepatch/test_klp_state.c > create mode 100644 lib/livepatch/test_klp_state2.c > create mode 100644 lib/livepatch/test_klp_state3.c > create mode 100755 tools/testing/selftests/livepatch/test-state.sh > > -- > 2.16.4 > Hi Petr, Thanks for respinning this one with the latest updates. The implementation looks fine to me. I have two really minor nits for the selftest (I'll reply to that commit), but I wouldn't hold up the series for them. Acked-by: Joe Lawrence -- Joe
Re: [RFC PATCH 2/2] livepatch: Clear relocation targets on a module removal
On 9/6/19 1:51 PM, Miroslav Benes wrote:
Now, I don't think that replacing .ko on disk is a good idea. We've
already discussed it. It would lead to a maintenance/packaging problem,
because you never know which version of the module is loaded in the
system. The state space grows rather rapidly there.
What exactly are your concerns?
Either the old version of the module is loaded, and it's livepatched; or
the new version of the module is loaded, and it's not livepatched.
Let's have module foo.ko with function a().
Live patch 1 (LP1) fixes it to a'(), which calls new function b() (present
in LP1). LP1 is used only if foo.ko is loaded. foo.ko is replaced with
foo'.ko on disk. It contains both a'() (fixed a() to be precise) and new
b().
Now there is LP2 with new function c() (or c'(), it does not matter)
calling b(). Either foo.ko or foo'.ko can be loaded and you don't know
which one. The implementation LP2 would be different in both cases.
You could say that it does not matter. If LP2 is implemented for foo.ko,
the same could work for foo'.ko (b() would be a part of LP2 and would not
be called directly from foo'.ko). LP2 would only be necessarily larger. It
is true in case of functions, but if symbol b is not a function but a
global variable, it is different then.
Moreover, in this case foo'.ko is "LP superset". Meaning that it contains
only fixes which are present in LP1. What if it is not. We usually
preserve kABI, so there could be a module in two or more versions compiled
from slightly different code (older/newer and so on) and you don't know
which one is loaded. To be fair we don't allow it (I think) at SUSE except
for KMPs (kernel module packages) (the issue of course exists even now
and we haven't solved it yet, because it is rare) and out of tree modules
which we don't support with LP. It could be solved with srcversion, but it
complicates things a lot. "blue sky" idea could extend the issue to all
modules given the above is real.
Does it make sense?
If I understand correctly, you're saying that this would add another
dimension to the potential system state that livepatches need to
consider? e.g. when updating a livepatch to v3, a v2 patched module may
or may not be loaded. So are we updating livepatch v2 code or module v2
code...
I agree that for functions, we could probably get away with repeating
code, but not necessarily for new global variables.
Then there's the question of:
module v3 == module v{1,2} + livepatch v3?
Is this scenario similar to one where a customer somehow finds and loads
module v3 before loading livepatch v3? Livepatch doesn't have a
srcversion whitelist so this should be entirely possible. I suppose it
is a bit different in that module v3 would be starting from a fresh load
and not something that livepatch v3 has hotpatched from an unknown
source/base.
-- Joe
Re: [RFC PATCH 2/2] livepatch: Clear relocation targets on a module removal
On 9/5/19 7:09 AM, Petr Mladek wrote: On Wed 2019-09-04 21:50:55, Josh Poimboeuf wrote: On Wed, Sep 04, 2019 at 10:49:32AM +0200, Petr Mladek wrote: I wonder what is necessary for a productive discussion on Plumbers: + Josh would like to see what code can get removed when late handling of modules gets removed. I think that it might be partially visible from Joe's blue-sky patches. Yes, and I like what I see. Especially the removal of the .klp.arch nastiness! Could we get rid of it? Is there any other way to get access to static variables and functions from the livepatched code? Hi Petr, I think the question is whether .klp (not-arch specific) relocations would be sufficient (without late module patching). If it would a great simplification if those were all we needed. I'm not 100% sure about this quite yet, but am hoping that is the case. Anyway, it might rule out some variants so that we could better concentrate on the acceptable ones. Or come with yet another proposal that would avoid the real blockers. I'd like to hear more specific negatives about Joe's recent patches, which IMO, are the best option we've discussed so far. I discussed this approach with our project manager. He was not much excited about this solution. His first idea was that it would block attaching USB devices. They are used by admins when taking care of the servers. And there might be other scenarios where a new module might need loading to solve some situation. > Customers understand Livepatching as a way how to secure system without immediate reboot and with minimal (invisible) effect on the workload. They might get pretty surprised when the system > suddenly blocks their "normal" workflow. FWIW the complete blue-sky idea was that the package delivered to the customer (RPM, deb, whatever) would provide: - livepatch .ko, blacklists known vulnerable srcversions - updated .ko's for the blacklisted modules The second part would maintain module loading workflow, albeit with a new set .ko files. As Miroslav said. No solution is perfect. We need to find the most acceptable compromise. It seems that you are more concerned about saving code, reducing complexity and risk. I am more concerned about user satisfaction. It is almost impossible to predict effects on user satisfaction because they have different workflow, use case, expectation, and tolerance. We could better estimate the technical side of each solution: + implementation cost + maintenance cost + risks + possible improvements and hardening + user visible effects + complication and limits with creating livepatches From my POV, the most problematic is the arch-specific code. It is hard to maintain and we do not have it fully under control. And I do not believe that we could remove all arch specific code when we do not allow delayed livepatching of modules. No doubt there will probably always be some arch-specific code, and even my blue-sky branch didn't move all that much. But I think the idea could be a bigger simplification in terms of the mental model, should the solution be acceptable by criteria you mention above. -- Joe
Re: [RFC PATCH 2/2] livepatch: Clear relocation targets on a module removal
On 9/4/19 4:49 AM, Petr Mladek wrote: On Tue 2019-09-03 15:02:34, Miroslav Benes wrote: On Mon, 2 Sep 2019, Joe Lawrence wrote: On 9/2/19 12:13 PM, Miroslav Benes wrote: I can easily foresee more problems like those in the future. Going forward we have to always keep track of which special sections are needed for which architectures. Those special sections can change over time, or can simply be overlooked for a given architecture. It's fragile. Indeed. It bothers me a lot. Even x86 "port" is not feature complete in this regard (jump labels, alternatives,...) and who knows what lurks in the corners of the other architectures we support. So it is in itself reason enough to do something about late module patching. Hi Miroslav, I was tinkering with the "blue-sky" ideas that I mentioned to Josh the other day. I dunno if you had a chance to look at what removing that code looks like, but I can continue to flesh out that idea if it looks interesting: Unfortunately no and I don't think I'll come up with something useful before LPC, so anything is really welcome. https://github.com/joe-lawrence/linux/tree/blue-sky A full demo would require packaging up replacement .ko's with a livepatch, as well as "blacklisting" those deprecated .kos, etc. But that's all I had time to cook up last week before our holiday weekend here. Frankly, I'm not sure about this approach. I'm kind of torn. The current solution is far from ideal, but I'm not excited about the other options either. It seems like the choice is basically between "general but technically complicated fragile solution with nontrivial maintenance burden", or "something safer and maybe cleaner, but limiting for users/distros". Of course it depends on whether the limitation is even real and how big it is. Unfortunately we cannot quantify it much and that is probably why our opinions (in the email thread) differ. I wonder what is necessary for a productive discussion on Plumbers: Pre-planning this part of the miniconf is a great idea. + Josh would like to see what code can get removed when late handling of modules gets removed. I think that it might be partially visible from Joe's blue-sky patches. + I would like to better understand the scope of the current problems. It is about modifying code in the livepatch that depends on position of the related code: + relocations are rather clear; we will need them anyway to access non-public (static) API from the original code. + What are the other changes? + Do we use them in livepatches? How often? + How often new problematic features appear? + Would be possible to detect potential problems, for example by comparing the code in the binary and in memory when the module is loaded the normal way? + Would be possible to reset the livepatch code in memory when the related module is unloaded and safe us half of the troubles? + It might be useful to prepare overview of the existing proposals and agree on the positives and negatives. I am afraid that some of them might depend on the customer base and use cases. Sometimes we might not have enough information. But it might be good to get on the same page where possible. Anyway, it might rule out some variants so that we could better concentrate on the acceptable ones. Or come with yet another proposal that would avoid the real blockers. Any other ideas? I'll just add to your list that late module patching introduces complexity for klp-convert / livepatch style relocation support. Without worrying about unloaded modules, I *think* klp-convert might already be able to handle relocations in special sections (altinsts, parainst, etc.). I've put the current klp-convert patchset on top of the blue-sky branch to see if this indeed the case, but I'm not sure if I'll get through that experiment before LPC. Would it be better to discuss this in a separate room with a whiteboard or paperboard? Whiteboard would probably be ideal, but paper would work and be more transportable than the former. -- Joe
Re: [RFC PATCH 2/2] livepatch: Clear relocation targets on a module removal
On 9/2/19 12:13 PM, Miroslav Benes wrote: I can easily foresee more problems like those in the future. Going forward we have to always keep track of which special sections are needed for which architectures. Those special sections can change over time, or can simply be overlooked for a given architecture. It's fragile. Indeed. It bothers me a lot. Even x86 "port" is not feature complete in this regard (jump labels, alternatives,...) and who knows what lurks in the corners of the other architectures we support. So it is in itself reason enough to do something about late module patching. Hi Miroslav, I was tinkering with the "blue-sky" ideas that I mentioned to Josh the other day. I dunno if you had a chance to look at what removing that code looks like, but I can continue to flesh out that idea if it looks interesting: https://github.com/joe-lawrence/linux/tree/blue-sky A full demo would require packaging up replacement .ko's with a livepatch, as well as "blacklisting" those deprecated .kos, etc. But that's all I had time to cook up last week before our holiday weekend here. Regards, -- Joe
Re: [RFC PATCH 2/2] livepatch: Clear relocation targets on a module removal
On 8/26/19 10:54 AM, Josh Poimboeuf wrote: On Fri, Aug 23, 2019 at 10:13:06AM +0200, Petr Mladek wrote: On Thu 2019-08-22 17:36:49, Josh Poimboeuf wrote: On Fri, Aug 16, 2019 at 11:46:08AM +0200, Petr Mladek wrote: On Wed 2019-08-14 10:12:44, Josh Poimboeuf wrote: On Wed, Aug 14, 2019 at 01:06:09PM +0200, Miroslav Benes wrote: Really, we should be going in the opposite direction, by creating module dependencies, like all other kernel modules do, ensuring that a module is loaded *before* we patch it. That would also eliminate this bug. We should look at whether it makes sense to destabilize live patching for everybody, for a small minority of people who care about a small minority of edge cases. I do not see it that simple. Forcing livepatched modules to be loaded would mean loading "random" new modules when updating livepatches: I don't want to start a long debate on this, because this idea isn't even my first choice. But we shouldn't dismiss it outright. I am glad to hear that this is not your first choice. + It means more actions and higher risk to destabilize the system. Different modules have different quality. Maybe the distro shouldn't ship modules which would destabilize the system. Is this realistic? Even the best QA could not check all scenarios. My point is that the more actions we do the bigger the risk is. Sure, it introduces risk. But we have to compare that risk (which only affects rare edge cases) with the ones introduced by the late module patching code. I get the feeling that "late module patching" introduces risk to a broader range of use cases than "occasional loading of unused modules". The latter risk could be minimized by introducing a disabled state for modules - load it in memory, but don't expose it to users until explicitly loaded. Just a brainstormed idea; not sure whether it would work in practice. Interesting idea. We would need to ensure consistency between the loaded-but-not-enabled module and the version on disk. Does module init run when it's enabled? Etc. What about folding this the other way? ie, if a livepatch targets unloaded module foo, loaded module bar, and vmlinux ... it effectively patches bar and vmlinux, but the foo changes are dropped. Responsibility is placed on the admin to install an updated foo before loading it (in which case, livepatching core will again ignore foo.) Building on this idea, perhaps loading that livepatch would also blacklist specific, known vulnerable (unloaded) module versions. If the admin tries to load one, a debug msg is generated explaining why it can't be loaded by default. + It might open more security holes that are not fixed by the livepatch. Following the same line of thinking, the livepatch infrastructure might open security holes because of the inherent complexity of late module patching. Could you be more specific, please? Has there been any known security hole in the late module livepatching code? Just off the top of my head, I can think of two recent bugs which can be blamed on late module patching: 1) There was a RHEL-only bug which caused arch_klp_init_object_loaded() to not be loaded. This resulted in a panic when certain patched code was executed. 2) arch_klp_init_object_loaded() currently doesn't have any jump label specific code. This has recently caused panics for patched code which relies on static keys. The workaround is to not use jump labels in patched code. The real fix is to add support for them in arch_klp_init_object_loaded(). I can easily foresee more problems like those in the future. Going forward we have to always keep track of which special sections are needed for which architectures. Those special sections can change over time, or can simply be overlooked for a given architecture. It's fragile. FWIW, the static keys case is more involved than simple deferred relocations -- those keys are added to lists and then the static key code futzes with them when it needs to update code sites. That means the code managing the data structures, kernel/jump_label.c, will need to understand livepatch's strangely loaded-but-not-initialized variants. I don't think the other special sections will require such invasive changes, but it's something to keep in mind with respect to late module patching. -- Joe
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
On 8/19/19 3:31 AM, Miroslav Benes wrote: On Mon, 19 Aug 2019, Masahiro Yamada wrote: I can review this series from the build system point of view, but I am not familiar enough with live-patching itself. Some possibilities: [1] Merge this series thru the live-patch tree after the kbuild base patches land. This requires one extra development cycle (targeting for 5.5-rc1) but I think this is the official way if you do not rush into it. I'd prefer this option. There is no real rush and I think we can wait one extra development cycle. Agreed. I'm in no hurry and was only curious about the kbuild changes that this patchset is now dependent on -- how to note them for other reviewers or anyone wishing to test. Joe, could you submit one more revision with all the recent changes (once kbuild improvements settle down), please? We should take a look at the whole thing one more time? What do you think? Definitely, yes. I occasionally force a push to: https://github.com/joe-lawrence/linux/tree/klp-convert-v5-expanded as I've been updating and collecting feedback from v4. Once updates settle, I'll send out a new v5 set. -- Joe
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
On 8/18/19 11:50 PM, Masahiro Yamada wrote: Hi Joe, On Sat, Aug 17, 2019 at 4:01 AM Joe Lawrence wrote: I didn't realize that we're supposed to be able to still build external modules after "make clean". If that's the case, then one might want to build an external klp-module after doing that. Yes. 'make clean' must keep all the build artifacts needed for building external modules. With that in mind, shouldn't Symbols.list to persist until mrproper? And I think modules-livepatch could go away during clean, what do you think? -- Joe Symbols.list should be kept by the time mrproper is run. So, please add it to MRROPER_FILES instead of CLEAN_FILES. modules.livepatch is a temporary file, so you can add it to CLEAN_FILES. OK, I'll add those to their respective lists. How is this feature supposed to work for external modules? klp-convert receives: "symbols from vmlinux" + "symbols from no-klp in-tree modules" + "symbols from no-klp external modules" ?? I don't think that this use-case has been previously thought out (Miroslav, correct me if I'm wrong here.) I did just run an external build of a copy of samples/livepatch/livepatch-annotated-sample.c: - modules.livepatch is generated in external dir - klp-convert is invoked for the livepatch module - the external livepatch module successfully loads But that was only testing external livepatch modules. I don't know if we need/want to support general external modules supplementing Symbols.list, at least for the initial klp-convert commit. I suppose external livepatch modules would then need to specify additional Symbols.list(s) files somehow as well. BTW, 'Symbols.list' sounds like a file to list out symbols for generic purposes, but in fact, the file format is very specific for the klp-convert tool. Perhaps, is it better to rename it so it infers this is for livepatching? What do you think? I don't know if the "Symbols.list" name and leading uppercase was based on any convention, but something like symbols.klp would be fine with me. Thanks, -- Joe
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
On 8/16/19 8:43 AM, Joe Lawrence wrote: On 8/16/19 4:19 AM, Miroslav Benes wrote: Hi, I cleaned up the build system, and pushed it based on my kbuild tree. Please see: git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git klp-cleanup This indeed looks much simpler and cleaner (as far as I can judge with my limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch, "Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy module which is then livepatched by lib/livepatch/test_klp_convert1.c). Yeah, Masahiro this is great, thanks for reworking this! I did tweak one module like Miroslav mentioned and I think a few of the newly generated files need to be cleaned up as part of "make clean", but all said, this is a nice improvement. Well actually, now I see this comment in the top-level Makefile: # Cleaning is done on three levels. # make clean Delete most generated files #Leave enough to build external modules # make mrproper Delete the current configuration, and all generated files # make distclean Remove editor backup files, patch leftover files and the like I didn't realize that we're supposed to be able to still build external modules after "make clean". If that's the case, then one might want to build an external klp-module after doing that. With that in mind, shouldn't Symbols.list to persist until mrproper? And I think modules-livepatch could go away during clean, what do you think? -- Joe
Re: [PATCH v4 07/10] livepatch: Add sample livepatch module
On 8/16/19 7:35 AM, Masahiro Yamada wrote:
Hi Joe,
On Thu, May 9, 2019 at 11:39 PM Joe Lawrence wrote:
--- /dev/null
+++ b/samples/livepatch/livepatch-annotated-sample.c
@@ -0,0 +1,102 @@
Please use SPDX instead of the license boilerplate.
Thanks.
Good eye, this revision was spun before ("treewide: Replace GPLv2
boilerplate/reference with SPDX - rule 13"), so I will add this to the
v5 TODO list.
Thanks,
-- Joe
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
On 8/16/19 4:19 AM, Miroslav Benes wrote: Hi, I cleaned up the build system, and pushed it based on my kbuild tree. Please see: git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git klp-cleanup This indeed looks much simpler and cleaner (as far as I can judge with my limited kbuild knowledge). We just need to remove MODULE_INFO(livepatch, "Y") from lib/livepatch/test_klp_convert_mod_a.c to make it compile and work (test_klp_convert_mod_a is not a livepatch module, it is just a dummy module which is then livepatched by lib/livepatch/test_klp_convert1.c). Yeah, Masahiro this is great, thanks for reworking this! I did tweak one module like Miroslav mentioned and I think a few of the newly generated files need to be cleaned up as part of "make clean", but all said, this is a nice improvement. Are you targeting the next merge window for your kbuild branch? (This appears to be what the klp-cleanup branch was based on.) Thanks, -- Joe
Re: [PATCH] selftests: livepatch: add missing fragments to config
On 8/14/19 7:16 AM, Anders Roxell wrote:
When generating config with 'make defconfig kselftest-merge' fragment
CONFIG_TEST_LIVEPATCH=m isn't set.
Rework to enable CONFIG_LIVEPATCH and CONFIG_DYNAMIC_DEBUG as well.
Signed-off-by: Anders Roxell
---
tools/testing/selftests/livepatch/config | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/livepatch/config
b/tools/testing/selftests/livepatch/config
index 0dd7700464a8..ad23100cb27c 100644
--- a/tools/testing/selftests/livepatch/config
+++ b/tools/testing/selftests/livepatch/config
@@ -1 +1,3 @@
+CONFIG_LIVEPATCH=y
+CONFIG_DYNAMIC_DEBUG=y
CONFIG_TEST_LIVEPATCH=m
Cool, I didn't know about that make target when doing commit
bae054372aba ("selftests/livepatch: add DYNAMIC_DEBUG config dependency")
How does kselftest-merge verify dependencies?
CONFIG_LIVEPATCH has its own list of configuration dependencies (see
kernel/livepatch/Kconfig) but we don't list all of those in this config
file. Just curious.
Thanks,
-- Joe
Re: [PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
On Wed, Jul 31, 2019 at 02:58:27PM +0900, Masahiro Yamada wrote: > Hi Joe, > > > On Thu, May 9, 2019 at 11:39 PM Joe Lawrence wrote: > > > > From: Miroslav Benes > > > > Currently, livepatch infrastructure in the kernel relies on > > MODULE_INFO(livepatch, "Y") statement in a livepatch module. Then the > > kernel module loader knows a module is indeed livepatch module and can > > behave accordingly. > > > > klp-convert, on the other hand relies on LIVEPATCH_* statement in the > > module's Makefile for exactly the same reason. > > > > Remove dependency on modinfo and generate MODULE_INFO flag > > automatically in modpost when LIVEPATCH_* is defined in the module's > > Makefile. Generate a list of all built livepatch modules based on > > the .livepatch file and store it in (MODVERDIR)/livepatchmods. Give > > this list as an argument for modpost which will use it to identify > > livepatch modules. > > > > As MODULE_INFO is no longer needed, remove it. > > > I do not understand this patch. > This makes the implementation so complicated. > > I think MODULE_INFO(livepatch, "Y") is cleaner than > LIVEPATCH_* in Makefile. > > > How about this approach? > > > [1] Make modpost generate the list of livepatch modules. > (livepatch-modules) > > [2] Generate Symbols.list in scripts/Makefile.modpost > (vmlinux + modules excluding livepatch-modules) > > [3] Run klp-convert for modules in livepatch-modules. > > > If you do this, you can remove most of the build system hacks > can't you? > > > I attached an example implementation for [1]. > > Please check whether this works. > Hi Masahiro, I tested and step [1] that you attached did create the livepatch-modules as expected. Thanks for that example, it does look cleaner that what we had in the patchset. I'm admittedly out of my element with kbuild changes, but here are my naive attempts at steps [2] and [3]... [step 2] generate Symbols.list - I tacked this on as a dependency of the $(modules:.ko=.mod.o), but there is probably a better more logical place to put it. Also used grep -Fxv to exclude the livepatch-modules list from the modules.order list of modules to process. -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost index 3eca7fccadd4..5409bbc212bb 100644 --- a/scripts/Makefile.modpost +++ b/scripts/Makefile.modpost @@ -111,7 +111,23 @@ quiet_cmd_cc_o_c = CC $@ cmd_cc_o_c = $(CC) $(c_flags) $(KBUILD_CFLAGS_MODULE) $(CFLAGS_MODULE) \ -c -o $@ $< -$(modules:.ko=.mod.o): %.mod.o: %.mod.c FORCE +quiet_cmd_klp_map = KLP Symbols.list +SLIST = $(objtree)/Symbols.list + +define cmd_symbols_list + $(shell echo "klp-convert-symbol-data.0.1" > $(objtree)/Symbols.list) \ + $(shell echo "*vmlinux" >> $(objtree)/Symbols.list) \ + $(shell nm -f posix $(objtree)/vmlinux | cut -d\ -f1 >> $(objtree)/Symbols.list) \ + $(foreach ko, $(sort $(shell grep -Fxv -f livepatch-modules modules.order)),\ + $(shell echo "*$(shell basename -s .ko $(ko))" >> $(objtree)/Symbols.list) \ + $(shell nm -f posix $(patsubst %.ko,%.o,$(ko)) | cut -d\ -f1 >> $(objtree)/Symbols.list)) +endef + +Symbols.list: __modpost + $(if $(CONFIG_LIVEPATCH), $(call cmd,symbols_list)) + + +$(modules:.ko=.mod.o): %.mod.o: %.mod.c Symbols.list FORCE $(call if_changed_dep,cc_o_c) targets += $(modules:.ko=.mod.o) -- 2.18.1 -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- [step 3] klp-convert the livepatch-modules - more or less what existed in the patchset already, however used the grep -Fx trick to process only modules found in livepatch-modules file: -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include index 73e80b917f12..f085644c2b97 100644 --- a/scripts/Kbuild.include +++ b/scripts/Kbuild.include @@ -223,6 +223,8 @@ endif # (needed for the shell) make-cmd = $(call escsq,$(subst $(pound),$$(pound),$(subst $$,,$(cmd_$(1) +save-cmd = printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd + # Find any prerequisites that is newer than target or that does not exist. # PHONY targets skipped in both cases. any-prereq = $(filter-out $(PHONY),$?)$(filter-out $(PHONY) $(wildcard $^),$^) @@ -230,7 +232,7 @@ any-prereq = $(filter-out $(PHONY),$?)$(filter-out $(PHONY) $(wildcard $^),$^) #
Re: [PATCH v4 03/10] livepatch: Add klp-convert tool
On Wed, Jul 31, 2019 at 12:36:05PM +0900, Masahiro Yamada wrote: > On Wed, Jul 31, 2019 at 11:50 AM Masahiro Yamada > wrote: > > > > On Thu, May 9, 2019 at 11:39 PM Joe Lawrence > > wrote: > > > > > > From: Josh Poimboeuf > > > > > > 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 Symbols.list, > > > 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, create scripts/livepatch to hold tools developed for > > > livepatches and add source files for klp-convert there. > > > > > > The core file of klp-convert is scripts/livepatch/klp-convert.c, which > > > implements the heuristics used to solve the relocations and the > > > conversion of unresolved symbols into the expected format, as defined > > > in [1]. > > > > > > klp-convert receives as arguments the Symbols.list file, an input > > > livepatch module to be converted and the output name for the converted > > > livepatch. When it starts running, klp-convert parses Symbols.list and > > > builds two internal lists of symbols, one containing the exported and > > > another containing the non-exported symbols. Then, by parsing the rela > > > sections in the elf object, klp-convert identifies which symbols must > > > be converted, which are those unresolved and that do not have a > > > corresponding exported symbol, and attempts to convert them > > > accordingly to the specification. > > > > > > By using Symbols.list, klp-convert identifies which symbols have names > > > that only appear in a single kernel object, thus being capable of > > > resolving these cases without the intervention of the developer. When > > > various homonymous symbols exist through kernel objects, it is not > > > possible to infer the right one, thus klp-convert falls back into > > > using developer annotations. If these were not provided, then the tool > > > will print a list with all acceptable targets for the symbol being > > > processed. > > > > > > Annotations in the context of klp-convert are accessible as struct > > > klp_module_reloc entries in sections named > > > .klp.module_relocs.. These entries are pairs of symbol > > > references and positions which are to be resolved against definitions > > > in . > > > > > > Define the structure klp_module_reloc in > > > include/linux/uapi/livepatch.h allowing developers to annotate the > > > livepatch source code with it. > > > > > > klp-convert relies on libelf and on a list implementation. Add files > > > scripts/livepatch/elf.c and scripts/livepatch/elf.h, which are a > > > libelf interfacing layer and scripts/livepatch/list.h, which is a > > > list implementation. > > > > > > Update Makefiles to correctly support the compilation of the new tool, > > > update MAINTAINERS file and add a .gitignore file. > > > > > > [1] - Documentation/livepatch/module-elf-format.txt > > > > > > Signed-off-by: Josh Poimboeuf > > > Signed-off-by: Konstantin Khlebnikov > > > Signed-off-by: Joao Moreira > > > Signed-off-by: Joe Lawrence > > > --- > > > MAINTAINERS | 1 + > > > include/uapi/linux/livepatch.h | 5 + > > > scripts/Makefile| 1 + > > > scripts/livepatch/.gitignore| 1 + > > > scripts/livepatch/Makefile | 7
Re: [PATCH] kprobes: Allow kprobes coexist with livepatch
On 7/26/19 12:14 PM, Steven Rostedt wrote: On Thu, 25 Jul 2019 22:07:52 -0400 Joe Lawrence wrote: These results reflect my underestanding of FTRACE_OPS_FL_IPMODIFY in light of your changes, so feel free to add my: Acked-by: Joe Lawrence Is this an urgent patch (needs to go in now and not wait for the next merge window) and if so, should it be marked for stable? Hi Steve, IMHO, it's not urgent.. so unless Josh or other livepatch folks say otherwise, I'm ok with waiting for next merge window. Given summer holiday schedules, that would give them more time to comment if they like. Thanks, -- Joe
Re: [PATCH] kprobes: Allow kprobes coexist with livepatch
probe_ftrace_ops, > + ipmodify ? &kprobe_ipmodify_enabled : &kprobe_ftrace_enabled); > +} > #else/* !CONFIG_KPROBES_ON_FTRACE */ > #define prepare_kprobe(p)arch_prepare_kprobe(p) > #define arm_kprobe_ftrace(p) (-ENODEV) > Thanks for the quick patch, Masami! I gave it a spin and here are my new testing results: perf probe, then livepatch -- % perf probe --add cmdline_proc_show Added new event: probe:cmdline_proc_show (on cmdline_proc_show) You can now use it in all perf tools, such as: perf record -e probe:cmdline_proc_show -aR sleep 1 % perf record -e probe:cmdline_proc_show -aR sleep 30 & [1] 980 % insmod samples/livepatch/livepatch-sample.ko % cat /proc/cmdline this has been live patched % fg perf record -e probe:cmdline_proc_show -aR sleep 30 [ perf record: Woken up 1 times to write data ] [ perf record: Captured and wrote 0.177 MB perf.data (2 samples) ] % perf script insmod 983 [000] 157.126556: probe:cmdline_proc_show: (9bf74890) cat 985 [000] 162.304028: probe:cmdline_proc_show: (9bf74890) livepatch, then perf probe -- % insmod samples/livepatch/livepatch-sample.ko % cat /proc/cmdline this has been live patched % perf record -e probe:cmdline_proc_show -aR sleep 30 event syntax error: 'probe:cmdline_proc_show' \___ unknown tracepoint Error: File /sys/kernel/debug/tracing/events/probe/cmdline_proc_show not found. Hint: Perhaps this kernel misses some CONFIG_ setting to enable this feature?. Run 'perf list' for a list of valid events Usage: perf record [] [] or: perf record [] -- [] -e, --eventevent selector. use 'perf list' to list available events These results reflect my underestanding of FTRACE_OPS_FL_IPMODIFY in light of your changes, so feel free to add my: Acked-by: Joe Lawrence -- Joe
kprobes, livepatch and FTRACE_OPS_FL_IPMODIFY
Hi Masami, I wanted to revisit FTRACE_OPS_FL_IPMODIFY blocking of kprobes and livepatch, at least in cases where kprobe pre_handlers don't modify regs->ip. (We've discussed this previously at part of a kpatch github issue #47: https://github.com/dynup/kpatch/issues/47) The particular use case I was wondering about was perf probing a particular function, then attempting to livepatch that same function: % uname -r 5.3.0-rc1+ % dmesg -C % perf probe --add cmdline_proc_show Added new event: probe:cmdline_proc_show (on cmdline_proc_show) You can now use it in all perf tools, such as: perf record -e probe:cmdline_proc_show -aR sleep 1 % perf record -e probe:cmdline_proc_show -aR sleep 30 & [1] 1007 % insmod samples/livepatch/livepatch-sample.ko insmod: ERROR: could not insert module samples/livepatch/livepatch-sample.ko: Device or resource busy % dmesg [ 440.913962] livepatch_sample: tainting kernel with TAINT_LIVEPATCH [ 440.917123] livepatch_sample: module verification failed: signature and/or required key missing - tainting kernel [ 440.942493] livepatch: enabling patch 'livepatch_sample' [ 440.943445] livepatch: failed to register ftrace handler for function 'cmdline_proc_show' (-16) [ 440.944576] livepatch: failed to patch object 'vmlinux' [ 440.945270] livepatch: failed to enable patch 'livepatch_sample' [ 440.946085] livepatch: 'livepatch_sample': unpatching complete This same behavior holds in reverse, if we want to probe a livepatched function: % insmod samples/livepatch/livepatch-sample.ko % perf probe --add cmdline_proc_show Added new event: probe:cmdline_proc_show (on cmdline_proc_show) You can now use it in all perf tools, such as: perf record -e probe:cmdline_proc_show -aR sleep 1 % perf record -e probe:cmdline_proc_show -aR sleep 30 Error: The sys_perf_event_open() syscall returned with 16 (Device or resource busy) for event (probe:cmdline_proc_show). /bin/dmesg | grep -i perf may provide additional information. Now, if I read kernel/trace/trace_kprobe.c :: kprobe_dispatcher() correctly, it's only going to return !0 (indicating a modified regs->ip) when kprobe_perf_func() returns !0, i.e. regs->ip changes over a call to trace_call_bpf(). Aside: should kprobe_ftrace_handler() check that FTRACE_OPS_FL_IPMODIFY is set when a pre_handler returns !0? In kpatch #47, Josh suggested: - If a kprobe handler needs to modify IP, user sets KPROBE_FLAG_IPMODIFY flag to register_kprobe, and then kprobes sets FTRACE_OPS_FL_IPMODIFY when registering with ftrace for that probe. - If KPROBE_FLAG_IPMODIFY is not used, kprobe_ftrace_handler() can detect when a kprobe handler changes regs->ip and restore it to its original value (regs->ip = ip). Is this something that could still be supported? In cases like perf probe, could we get away with not setting FTRACE_OPS_FL_IPMODIFY? The current way that we're applying that flag, kprobes and livepatch are mutually exclusive (for the same function). Regards, -- Joe
Re: [PATCH] s390/livepatch: Implement reliable stack tracing for the consistency model
On Wed, Jul 10, 2019 at 12:59:18PM +0200, Miroslav Benes wrote: > The livepatch consistency model requires reliable stack tracing > architecture support in order to work properly. In order to achieve > this, two main issues have to be solved. First, reliable and consistent > call chain backtracing has to be ensured. Second, the unwinder needs to > be able to detect stack corruptions and return errors. > > The "zSeries ELF Application Binary Interface Supplement" says: > > "The stack pointer points to the first word of the lowest allocated > stack frame. If the "back chain" is implemented this word will point to > the previously allocated stack frame (towards higher addresses), except > for the first stack frame, which shall have a back chain of zero (NULL). > The stack shall grow downwards, in other words towards lower addresses." > > "back chain" is optional. GCC option -mbackchain enables it. Quoting > Martin Schwidefsky [1]: > > "The compiler is called with the -mbackchain option, all normal C > function will store the backchain in the function prologue. All > functions written in assembler code should do the same, if you find one > that does not we should fix that. The end result is that a task that > *voluntarily* called schedule() should have a proper backchain at all > times. > > Dependent on the use case this may or may not be enough. Asynchronous > interrupts may stop the CPU at the beginning of a function, if kernel > preemption is enabled we can end up with a broken backchain. The > production kernels for IBM Z are all compiled *without* kernel > preemption. So yes, we might get away without the objtool support. > > On a side-note, we do have a line item to implement the ORC unwinder for > the kernel, that includes the objtool support. Once we have that we can > drop the -mbackchain option for the kernel build. That gives us a nice > little performance benefit. I hope that the change from backchain to the > ORC unwinder will not be too hard to implement in the livepatch tools." > > Thus, the call chain backtracing should be currently ensured and objtool > should not be necessary for livepatch purposes. Hi Miroslav, Should there be a CONFIG? dependency on -mbackchain and/or kernel preemption, or does the following ensure that we don't need a explicit build time checks? > Regarding the second issue, stack corruptions and non-reliable states > have to be recognized by the unwinder. Mainly it means to detect > preemption or page faults, the end of the task stack must be reached, > return addresses must be valid text addresses and hacks like function > graph tracing and kretprobes must be properly detected. > > Unwinding a running task's stack is not a problem, because there is a > livepatch requirement that every checked task is blocked, except for the > current task. Due to that, the implementation can be much simpler > compared to the existing non-reliable infrastructure. We can consider a > task's kernel/thread stack only and skip the other stacks. > > Idle tasks are a bit special. Their final back chains point to no_dat > stacks. See for reference CALL_ON_STACK() in smp_start_secondary() > callback used in __cpu_up(). The unwinding is stopped there and it is > not considered to be a stack corruption. > > Signed-off-by: Miroslav Benes > --- > - based on Linus' master > - passes livepatch kselftests > - passes tests from https://github.com/lpechacek/qa_test_klp, which > stress the consistency model and the unwinder a bit more > > arch/s390/Kconfig | 1 + > arch/s390/include/asm/stacktrace.h | 5 ++ > arch/s390/include/asm/unwind.h | 19 ++ > arch/s390/kernel/dumpstack.c | 28 + > arch/s390/kernel/stacktrace.c | 78 + > arch/s390/kernel/unwind_bc.c | 93 ++ > 6 files changed, 224 insertions(+) > > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig > index fdb4246265a5..ea73e555063d 100644 > --- a/arch/s390/Kconfig > +++ b/arch/s390/Kconfig > @@ -170,6 +170,7 @@ config S390 > select HAVE_PERF_EVENTS > select HAVE_RCU_TABLE_FREE > select HAVE_REGS_AND_STACK_ACCESS_API > + select HAVE_RELIABLE_STACKTRACE > select HAVE_RSEQ > select HAVE_SYSCALL_TRACEPOINTS > select HAVE_VIRT_CPU_ACCOUNTING > diff --git a/arch/s390/include/asm/stacktrace.h > b/arch/s390/include/asm/stacktrace.h > index 0ae4bbf7779c..2b5c913c408f 100644 > --- a/arch/s390/include/asm/stacktrace.h > +++ b/arch/s390/include/asm/stacktrace.h > @@ -23,6 +23,11 @@ const char *stack_type_name(enum stack_type type); > int get_stack_info(unsigned long sp, struct task_struct *task, > struct stack_info *info, unsigned long *visit_mask); > > +#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE > +int get_stack_info_reliable(unsigned long sp, struct task_struct *task, > + struct stack_info *info); > +#endif > + > s
Re: [PATCH v4 00/10] klp-convert livepatch build tooling
On Tue, Jun 25, 2019 at 01:36:37PM +0200, Miroslav Benes wrote:
>
> [ ... snip ... ]
>
> If I revert commit d59cadc0a8f8 ("[squash] klp-convert: make
> convert_rela() list-safe") (from Joe's expanded github tree), the problem
> disappears.
>
> I haven't spotted any problem in the code and I cannot explain a
> dependency on GCC version. Any ideas?
>
I can confirm that test_klp_convert1.ko crashes with RHEL-7 and its
older gcc. I added some debugging printf's to klp-convert and see:
% ./scripts/livepatch/klp-convert \
./Symbols.list \
lib/livepatch/test_klp_convert1.klp.o \
lib/livepatch/test_klp_convert1.ko | \
grep saved_command_line
convert_rela: oldsec: .rela.text rela @ 0x1279670 rela->sym @ 0x12791f0
(.klp.sym.vmlinux.saved_command_line,0) offset: 0x3
convert_rela: oldsec: .rela.text rela @ 0x1279cd0 rela->sym @ 0x12791f0
(.klp.sym.vmlinux.saved_command_line,0) offset: 0x9a
move_rela: rela @ 0x1279670 rela->sym @ 0x12791f0
(.klp.sym.vmlinux.saved_command_line,0) offset: 0x3
main: skipping rela @ 0x1279cd0 rela->sym @ 0x12791f0
(.klp.sym.vmlinux.saved_command_line,0) (!must_convert)
I think the problem is:
- Relas at different offsets, but for the same symbol may share symbol
storage. Note the same rela->sym value above.
- Before d59cadc0a8f8 ("[squash] klp-convert: make convert_rela()
list-safe"), convert_rela() iterated through the entire section's
relas, moving any of the same name. This was determined not to be
list safe when moving consecutive relas in the linked list.
- After d59cadc0a8f8 ("[squash] klp-convert: make convert_rela()
list-safe"), convert_rela() still iterates through the section relas,
but only updates r1->sym->klp_rela_sec instead of moving them.
move_rela() was added to be called by the for-each-rela loop in
main().
- Bug 1: klp_rela_sec probably belongs in struct rela and not struct
symbol
- Bug 2: the main loop skips over second, third, etc. matching relas
anyway as the shared symbol name will have already been converted
The following fix might not be elegant, but I can't think of a clever
way to handle the original issue d59cadc0a8f8 ("[squash] klp-convert:
make convert_rela() list-safe") as well as these resulting regressions.
So I broke out the moving of relas to a seperate loop. That is probably
worth a comment and at the same time we might be able to drop some of
these other "safe" loop traversals for ordinary list_for_each_entry.
-- Joe
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
diff --git a/scripts/livepatch/elf.h b/scripts/livepatch/elf.h
index 7551409..57a8242 100644
--- a/scripts/livepatch/elf.h
+++ b/scripts/livepatch/elf.h
@@ -33,7 +33,6 @@ struct symbol {
struct list_head list;
GElf_Sym sym;
struct section *sec;
- struct section *klp_rela_sec;
char *name;
unsigned int idx;
unsigned char bind, type;
@@ -45,6 +44,7 @@ struct rela {
struct list_head list;
GElf_Rela rela;
struct symbol *sym;
+ struct section *klp_rela_sec;
unsigned int type;
unsigned long offset;
int addend;
diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
index b5873ab..50d6471 100644
--- a/scripts/livepatch/klp-convert.c
+++ b/scripts/livepatch/klp-convert.c
@@ -525,7 +525,7 @@ static bool convert_rela(struct section *oldsec, struct
rela *r,
list_for_each_entry_safe(r1, r2, &oldsec->relas, list) {
if (r1->sym->name == r->sym->name) {
- r1->sym->klp_rela_sec = sec;
+ r1->klp_rela_sec = sec;
}
}
return true;
@@ -535,7 +535,7 @@ static void move_rela(struct rela *r)
{
/* Move the converted rela to klp rela section */
list_del(&r->list);
- list_add(&r->list, &r->sym->klp_rela_sec->relas);
+ list_add(&r->list, &r->klp_rela_sec->relas);
}
/* Checks if given symbol name matches a symbol in exp_symbols */
@@ -687,8 +687,11 @@ int main(int argc, const char **argv)
return -1;
}
}
+ }
- move_rela(rela);
+ list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
+ if (is_converted(rela->sym->name))
+ move_rela(rela);
}
}
Re: [PATCH v4 00/10] klp-convert livepatch build tooling
On 6/25/19 7:36 AM, Miroslav Benes wrote:
> [ ... snip ... ]
>
So I made a couple of experiments and found that GCC is somehow involved.
If klp-convert (from scripts/livepatch/) is compiled with our GCC 4.8.5
from SLE12, the output is incorrect. If I compile it with GCC 7.4.0 from
openSUSE Leap 15.1, the output is correct.
If I revert commit d59cadc0a8f8 ("[squash] klp-convert: make
convert_rela() list-safe") (from Joe's expanded github tree), the problem
disappears.
I haven't spotted any problem in the code and I cannot explain a
dependency on GCC version. Any ideas?
Thanks for revisiting and debugging this. Narrowing it down to my "fix"
to convert_rela() should be helpful.
In my case, I was probably testing with RHEL-8, which has gcc 8.2 vs
RHEL-7 which has gcc 4.8. I'll have to make sure to try with a few
different versions for the next round.
-- Joe
Re: [RFC 3/5] livepatch: Allow to distinguish different version of system state changes
On Fri, Jun 21, 2019 at 10:09:11AM -0400, Joe Lawrence wrote: > More word play: would it be any clearer to drop the use of > "modification" when talking about klp_states? Sometimes I read > modification to mean a change to a klp_state itself rather than the > system at large. After reading through the rest of the series, maybe I was premature about this. "System state modification" is used consistently throughout the series, so without having any better suggestion, ignore my comment. -- Joe
Re: [RFC 5/5] livepatch: Selftests of the API for tracking system state changes
On Tue, Jun 11, 2019 at 03:56:27PM +0200, Petr Mladek wrote:
>
> [ ... snip ... ]
>
> diff --git a/lib/livepatch/test_klp_state.c b/lib/livepatch/test_klp_state.c
> new file mode 100644
> index ..c43dc2f2e01d
> --- /dev/null
> +++ b/lib/livepatch/test_klp_state.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2019 SUSE
>
> [ ... snip ... ]
>
> +MODULE_AUTHOR("Joe Lawrence ");
Feel free to update the module author for these.
-- Joe
Re: [RFC 4/5] livepatch: Documentation of the new API for tracking system state changes
On Tue, Jun 11, 2019 at 03:56:26PM +0200, Petr Mladek wrote: > Documentation explaining the motivation, capabilities, and usage > of the new API for tracking system state changes. > > Signed-off-by: Petr Mladek > --- > Documentation/livepatch/index.rst| 1 + > Documentation/livepatch/system-state.rst | 80 > > 2 files changed, 81 insertions(+) > create mode 100644 Documentation/livepatch/system-state.rst > > diff --git a/Documentation/livepatch/index.rst > b/Documentation/livepatch/index.rst > index edd291d51847..94bbbc2c8993 100644 > --- a/Documentation/livepatch/index.rst > +++ b/Documentation/livepatch/index.rst > @@ -9,6 +9,7 @@ Kernel Livepatching > > livepatch > callbacks > +system-state > cumulative-patches > module-elf-format > shadow-vars > diff --git a/Documentation/livepatch/system-state.rst > b/Documentation/livepatch/system-state.rst > new file mode 100644 > index ..3a35073a0b80 > --- /dev/null > +++ b/Documentation/livepatch/system-state.rst > @@ -0,0 +1,80 @@ > [ ... snip ... ] > +1. Livepatch compatibility nit: 2. Livepatch compatibility -- Joe
Re: [RFC 3/5] livepatch: Allow to distinguish different version of system state changes
On Tue, Jun 11, 2019 at 03:56:25PM +0200, Petr Mladek wrote:
> The atomic replace runs pre/post (un)install callbacks only from the new
> livepatch. There are several reasons for this:
>
> + Simplicity: clear ordering of operations, no interactions between
> old and new callbacks.
>
> + Reliability: only new livepatch knows what changes can already be made
> by older livepatches and how to take over the state.
>
> + Testing: the atomic replace can be properly tested only when a newer
> livepatch is available. It might be too late to fix unwanted effect
> of callbacks from older livepatches.
>
> It might happen that an older change is not enough and the same system
> state has to be modified another way. Different changes need to get
> distinguished by a version number added to struct klp_state.
>
> The version can also be used to prevent loading incompatible livepatches.
> The check is done when the livepatch is enabled. The rules are:
>
> + Any completely new system state modification is allowed.
>
> + System state modifications with the same or higher version are allowed
> for already modified system states.
>
More word play: would it be any clearer to drop the use of
"modification" when talking about klp_states? Sometimes I read
modification to mean a change to a klp_state itself rather than the
system at large.
In my mind, "modification" is implied, but I already know where this
patchset is going, so perhaps I'm just trying to be lazy and not type
the whole thing out :) I wish I could come up with a nice, succinct
alternative, but "state" or "klp_state" would work for me. /two cents
> + Cumulative livepatches must handle all system state modifications from
> already installed livepatches.
>
> + Non-cumulative livepatches are allowed to touch already modified
> system states.
>
> Signed-off-by: Petr Mladek
> ---
> include/linux/livepatch.h | 2 ++
> kernel/livepatch/core.c | 8
> kernel/livepatch/state.c | 40 +++-
> kernel/livepatch/state.h | 9 +
> 4 files changed, 58 insertions(+), 1 deletion(-)
> create mode 100644 kernel/livepatch/state.h
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 591abdee30d7..8bc4c6cc3f3f 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -135,10 +135,12 @@ struct klp_object {
> /**
> * struct klp_state - state of the system modified by the livepatch
> * @id: system state identifier (non zero)
> + * @version: version of the change (non-zero)
> * @data:custom data
> */
> struct klp_state {
> int id;
> + int version;
> void *data;
> };
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 24c4a13bd26c..614642719825 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -21,6 +21,7 @@
> #include
> #include "core.h"
> #include "patch.h"
> +#include "state.h"
> #include "transition.h"
>
> /*
> @@ -1003,6 +1004,13 @@ int klp_enable_patch(struct klp_patch *patch)
>
> mutex_lock(&klp_mutex);
>
> + if(!klp_is_patch_compatible(patch)) {
> + pr_err("Livepatch patch (%s) is not compatible with the already
> installed livepatches.\n",
> + patch->mod->name);
> + mutex_unlock(&klp_mutex);
> + return -EINVAL;
> + }
> +
> ret = klp_init_patch_early(patch);
> if (ret) {
> mutex_unlock(&klp_mutex);
> diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
> index f8822b71f96e..b54a69b9e4b4 100644
> --- a/kernel/livepatch/state.c
> +++ b/kernel/livepatch/state.c
> @@ -12,7 +12,9 @@
> #include "transition.h"
>
> #define klp_for_each_state(patch, state) \
> - for (state = patch->states; state && state->id; state++)
> + for (state = patch->states; \
> + state && state->id && state->version; \
> + state++)
Minor git bookkeeping here, but this could be moved to the patch that
introduced the macro.
>
> /**
> * klp_get_state() - get information about system state modified by
> @@ -81,3 +83,39 @@ struct klp_state *klp_get_prev_state(int id)
> return last_state;
> }
> EXPORT_SYMBOL_GPL(klp_get_prev_state);
> +
> +/* Check if the patch is able to deal with the given system state. */
> +static bool klp_is_state_compatible(struct klp_patch *patch,
> + struct klp_state *state)
> +{
> + struct klp_state *new_state;
> +
> + new_state = klp_get_state(patch, state->id);
> +
> + if (new_state)
> + return new_state->version < state->version ? false : true;
> +
> + /* Cumulative livepatch must handle all already modified states. */
> + return patch->replace ? false : true;
> +}
> +
> +/*
> + * Check if the new livepatch will not break the existing system states.
suggestion: "Check
Re: [RFC 2/5] livepatch: Basic API to track system state changes
On Tue, Jun 11, 2019 at 03:56:24PM +0200, Petr Mladek wrote:
> This is another step how to help maintaining more livepatches.
>
> One big help was the atomic replace and cumulative livepatches. These
> livepatches replaces the already installed ones. Therefore it should
nit: s/replaces/replaces
> be enough when each cumulative livepatch is consistent.
>
> The problems might come with shadow variables and callbacks. They might
> change the system behavior or state so that it is not longer safe to
nit: s/not longer safe/no longer safe
> go back and use an older livepatch or the original kernel code. Also
> any new livepatch must be able to detect what changes have already been
> done by the already installed livepatches.
>
> This is where the livepatch system state tracking gets useful. It
> allows to:
>
> - find whether a system state has already been modified by
> previous livepatches
>
> - store data needed to manipulate and restore the system state
>
> The information about the manipulated system states is stored in an
> array of struct klp_state. There are two functions that allow
> to find this structure for a given struct klp_patch or for
> already installed (replaced) livepatches.
>
suggestion: "Two new functions, klp_get_state() and
klp_get_prev_state(), can find this structure ..." or perhaps drop this
part altogether and let the future reader do a 'git show' or 'git log
-p' to see the code changes and the exact function names.
-- Joe
> The dependencies are going to be solved by a version field added later.
> The only important information is that it will be allowed to modify
> the same state by more non-cumulative livepatches. It is the same logic
> as that it is allowed to modify the same function several times.
> The livepatch author is responsible for preventing incompatible
> changes.
>
> Signed-off-by: Petr Mladek
> ---
> include/linux/livepatch.h | 15 +
> kernel/livepatch/Makefile | 2 +-
> kernel/livepatch/state.c | 83
> +++
> 3 files changed, 99 insertions(+), 1 deletion(-)
> create mode 100644 kernel/livepatch/state.c
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index eeba421cc671..591abdee30d7 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -132,10 +132,21 @@ struct klp_object {
> bool patched;
> };
>
> +/**
> + * struct klp_state - state of the system modified by the livepatch
> + * @id: system state identifier (non zero)
> + * @data:custom data
> + */
> +struct klp_state {
> + int id;
> + void *data;
> +};
> +
> /**
> * struct klp_patch - patch structure for live patching
> * @mod: reference to the live patch module
> * @objs:object entries for kernel objects to be patched
> + * @states: system states that can get modified
> * @replace: replace all actively used patches
> * @list:list node for global list of actively used patches
> * @kobj:kobject for sysfs resources
> @@ -150,6 +161,7 @@ struct klp_patch {
> /* external */
> struct module *mod;
> struct klp_object *objs;
> + struct klp_state *states;
> bool replace;
>
> /* internal */
> @@ -220,6 +232,9 @@ void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
> void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
> void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
>
> +struct klp_state *klp_get_state(struct klp_patch *patch, int id);
> +struct klp_state *klp_get_prev_state(int id);
> +
> #else /* !CONFIG_LIVEPATCH */
>
> static inline int klp_module_coming(struct module *mod) { return 0; }
> diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
> index cf9b5bcdb952..cf03d4bdfc66 100644
> --- a/kernel/livepatch/Makefile
> +++ b/kernel/livepatch/Makefile
> @@ -1,4 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0-only
> obj-$(CONFIG_LIVEPATCH) += livepatch.o
>
> -livepatch-objs := core.o patch.o shadow.o transition.o
> +livepatch-objs := core.o patch.o shadow.o state.o transition.o
> diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
> new file mode 100644
> index ..f8822b71f96e
> --- /dev/null
> +++ b/kernel/livepatch/state.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * system_state.c - State of the system modified by livepatches
> + *
> + * Copyright (C) 2019 SUSE
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include
> +#include "core.h"
> +#include "transition.h"
> +
> +#define klp_for_each_state(patch, state) \
> + for (state = patch->states; state && state->id; state++)
> +
> +/**
> + * klp_get_state() - get information about system state modified by
> + * the given patch
> + * @patch: livepatch that modifies the given system state
> + * @id: custom identifier of the modified system state
> + *
> + * Checks whether t
Re: [RFC 0/5] livepatch: new API to track system state changes
On Tue, Jun 11, 2019 at 03:56:22PM +0200, Petr Mladek wrote: > Hi, > > this is another piece in the puzzle that helps to maintain more > livepatches. > > Especially pre/post (un)patch callbacks might change a system state. > Any newly installed livepatch has to somehow deal with system state > modifications done be already installed livepatches. > > This patchset provides, hopefully, a simple and generic API that > helps to keep and pass information between the livepatches. > It is also usable to prevent loading incompatible livepatches. > Thanks for posting, Petr and aplogies for not getting to this RFC earlier. I think this strikes a reasonable balance between the (too) "simplified" versioning scheme that I posted a few weeks back, and what I was afraid might have been too complicated callback-state-version concept. This RFC reads fairly straightforward and especially easy to review given the included documentation and self-tests. I'll add a few comments per patch, but again, I like how this came out. > There was also a related idea to add a sticky flag. It should be > easy to add it later. It would perfectly fit into the new struct > klp_state. I think so, too. It would indicate that the patch is introducing a state which cannot be safely unloaded. But we can talk about that at a later time if/when we want to add that wrinkle to klp_state. -- Joe
Re: [PATCH v4 00/10] klp-convert livepatch build tooling
On 6/14/19 4:34 AM, Petr Mladek wrote:
On Thu 2019-06-13 16:48:02, Joe Lawrence wrote:
On 6/13/19 9:15 AM, Joe Lawrence wrote:
On 6/13/19 9:00 AM, Miroslav Benes wrote:
Hi Joe,
first, I'm sorry for the lack of response so far.
Maybe you've already noticed but the selftests fail. Well, at least in
my VM. When test_klp_convert1.ko is loaded, the process is killed with
[ 518.041826] BUG: kernel NULL pointer dereference, address:
[ 518.042816] #PF: supervisor read access in kernel mode
[ 518.043393] #PF: error_code(0x) - not-present page
[ 518.043981] PGD 0 P4D 0
[ 518.044185] Oops: [#1] SMP PTI
[ 518.044518] CPU: 2 PID: 2255 Comm: insmod Tainted: G O K
5.1.0-klp_convert_v4-193435-g67748576637e #2
[ 518.045784] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.12.1-0-ga5cab58-prebuilt.qemu.org 04/01/2014
[ 518.046940] RIP: 0010:test_klp_convert_init+0x1c/0x40 [test_klp_convert1]
[ 518.047611] Code: 1b a0 48 89 c6 e9 a8 c0 f4 e0 0f 1f 40 00 0f 1f 44 00 00 53 48
c7 c7 00 30 1b a0 e8 5e 33 f6 e0 85 c0 89 c3 74 04 89 d8 5b c3 <48> 8b 35 5d ef
e4 5f 48 c7 c7 28 20 1b a0 e8 75 c0 f4 e0 e8 6c ff
[ 518.049779] RSP: 0018:c9f37cc8 EFLAGS: 00010246
[ 518.050243] RAX: RBX: RCX: 00027de0
[ 518.050922] RDX: RSI: 0006 RDI: 88807ab54f40
[ 518.051619] RBP: a01b1080 R08: 96efde7a R09: 0001
[ 518.052332] R10: R11: R12:
[ 518.053012] R13: R14: 888078b55000 R15: c9f37ea0
[ 518.053714] FS: 7febece1fb80() GS:88807d40()
knlGS:
[ 518.054514] CS: 0010 DS: ES: CR0: 80050033
[ 518.055078] CR2: CR3: 7a56a000 CR4: 06e0
[ 518.055818] Call Trace:
[ 518.056007] do_one_initcall+0x6a/0x2da
[ 518.056340] ? do_init_module+0x22/0x230
[ 518.056702] ? rcu_read_lock_sched_held+0x96/0xa0
[ 518.057125] ? kmem_cache_alloc_trace+0x284/0x2e0
[ 518.057493] do_init_module+0x5a/0x230
[ 518.057900] load_module+0x17bc/0x1f50
[ 518.058214] ? __symbol_put+0x40/0x40
[ 518.058499] ? vfs_read+0x12d/0x160
[ 518.058766] __do_sys_finit_module+0x83/0xc0
[ 518.059122] do_syscall_64+0x57/0x190
[ 518.059407] entry_SYSCALL_64_after_hwframe+0x49/0xbe
...
It crashes right in test_klp_convert_init() when print_*() using
supposed-to-be-converted symbols are called. I'll debug it next week. Can
you reproduce it too?
Hey, thanks for the report..
I don't recall the tests crashing, but I had put this patchset on the
side for a few weeks now. I'll try to fire up a VM and see what happens
today.
Hmm, I haven't been able to reproduce using my original base (Linux 5.1-rc6)
or when rebased ontop of livepatching.git/master + 997a55f3fb6d("stacktrace: Unbreak
stack_trace_save_tsk_reliable()")
I stared into the code a bit but I did not find any bug. Let's hope
that it was just some pre-vacation last minute mistake (system
inconsistency or so ;-)
Anyway, I am curious about one thing. I saw:
function __load_mod() {
local mod="$1"; shift
local msg="% modprobe $mod $*"
log "${msg%% }"
ret=$(modprobe "$mod" "$@" 2>&1)
if [[ "$ret" != "" ]]; then
die "$ret"
fi
# Wait for module in sysfs ...
loop_until '[[ -e "/sys/module/$mod" ]]' ||
die "failed to load module $mod"
}
Is the waiting for sysfs really necessary here?
Note that it is /sys/module and not /sys/kernel/livepatch/.
I can't remember if that was just paranoid-protective-bash coding or
actually required. Libor provided great feedback on the initial patch
series that introduced the self-tests, perhaps he remembers.
My understanding is that modprobe waits until the module succesfully
loaded. mod_sysfs_setup() is called before the module init callback.
Therefore the sysfs interface should be read before modprobe returns.
Do I miss something?
>
If it works different way then there might be some races because
mod_sysfs_setup() is called before the module is alive.
All of this is called from a single bash script function, so in a call
stack fashion, something like this would occur when loading a livepatch
module:
[ mod_sysfs_setup() ]
modprobe waits for: .init complete, MODULE_STATE_LIVE
__load_mod() waits for: /sys/module/$mod
load_lp_nowait() waits for: /sys/kernel/livepatch/$mod
load_lp() waits for:/sys/kernel/livepatch/$mod/transition = 0
test-script.sh
So I would think that by calling modprobe, we ensure that the module
code is ready to go. The /sys/module/$mod check might be redundant as
you say, but because modprobe
Re: [PATCH v4 00/10] klp-convert livepatch build tooling
On 6/13/19 9:15 AM, Joe Lawrence wrote:
> On 6/13/19 9:00 AM, Miroslav Benes wrote:
>> Hi Joe,
>>
>> first, I'm sorry for the lack of response so far.
>>
>> Maybe you've already noticed but the selftests fail. Well, at least in
>> my VM. When test_klp_convert1.ko is loaded, the process is killed with
>>
>> [ 518.041826] BUG: kernel NULL pointer dereference, address:
>>
>> [ 518.042816] #PF: supervisor read access in kernel mode
>> [ 518.043393] #PF: error_code(0x) - not-present page
>> [ 518.043981] PGD 0 P4D 0
>> [ 518.044185] Oops: [#1] SMP PTI
>> [ 518.044518] CPU: 2 PID: 2255 Comm: insmod Tainted: G O K
>> 5.1.0-klp_convert_v4-193435-g67748576637e #2
>> [ 518.045784] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
>> rel-1.12.1-0-ga5cab58-prebuilt.qemu.org 04/01/2014
>> [ 518.046940] RIP: 0010:test_klp_convert_init+0x1c/0x40 [test_klp_convert1]
>> [ 518.047611] Code: 1b a0 48 89 c6 e9 a8 c0 f4 e0 0f 1f 40 00 0f 1f 44 00
>> 00 53 48 c7 c7 00 30 1b a0 e8 5e 33 f6 e0 85 c0 89 c3 74 04 89 d8 5b c3 <48>
>> 8b 35 5d ef e4 5f 48 c7 c7 28 20 1b a0 e8 75 c0 f4 e0 e8 6c ff
>> [ 518.049779] RSP: 0018:c9f37cc8 EFLAGS: 00010246
>> [ 518.050243] RAX: RBX: RCX:
>> 00027de0
>> [ 518.050922] RDX: RSI: 0006 RDI:
>> 88807ab54f40
>> [ 518.051619] RBP: a01b1080 R08: 96efde7a R09:
>> 0001
>> [ 518.052332] R10: R11: R12:
>>
>> [ 518.053012] R13: R14: 888078b55000 R15:
>> c9f37ea0
>> [ 518.053714] FS: 7febece1fb80() GS:88807d40()
>> knlGS:
>> [ 518.054514] CS: 0010 DS: ES: CR0: 80050033
>> [ 518.055078] CR2: CR3: 7a56a000 CR4:
>> 06e0
>> [ 518.055818] Call Trace:
>> [ 518.056007] do_one_initcall+0x6a/0x2da
>> [ 518.056340] ? do_init_module+0x22/0x230
>> [ 518.056702] ? rcu_read_lock_sched_held+0x96/0xa0
>> [ 518.057125] ? kmem_cache_alloc_trace+0x284/0x2e0
>> [ 518.057493] do_init_module+0x5a/0x230
>> [ 518.057900] load_module+0x17bc/0x1f50
>> [ 518.058214] ? __symbol_put+0x40/0x40
>> [ 518.058499] ? vfs_read+0x12d/0x160
>> [ 518.058766] __do_sys_finit_module+0x83/0xc0
>> [ 518.059122] do_syscall_64+0x57/0x190
>> [ 518.059407] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> ...
>>
>> It crashes right in test_klp_convert_init() when print_*() using
>> supposed-to-be-converted symbols are called. I'll debug it next week. Can
>> you reproduce it too?
>
> Hey, thanks for the report..
>
> I don't recall the tests crashing, but I had put this patchset on the
> side for a few weeks now. I'll try to fire up a VM and see what happens
> today.
>
Hmm, I haven't been able to reproduce using my original base (Linux 5.1-rc6)
or when rebased ontop of livepatching.git/master + 997a55f3fb6d("stacktrace:
Unbreak stack_trace_save_tsk_reliable()")
FWIW, my test_klp_convert1.ko has these klp-converted relocations:
Relocation section [36] '.klp.rela.vmlinux..text.unlikely' for section [ 5]
'.text.unlikely' at offset 0x4a6b8 contains 1 entry:
Offset TypeValue Addend Name
0x0003 X86_64_PC32 00 -4
.klp.sym.vmlinux.saved_command_line,0
Relocation section [37] '.klp.rela.test_klp_convert_mod..text.unlikely' for
section [ 5] '.text.unlikely' at offset 0x4a6d0 contains 4 entries:
Offset TypeValue Addend Name
0x004e X86_64_PC32 00 -4
.klp.sym.test_klp_convert_mod.get_homonym_string,1
0x003d X86_64_32S 00 +0
.klp.sym.test_klp_convert_mod.homonym_string,1
0x0027 X86_64_PC32 00 -4
.klp.sym.test_klp_convert_mod.get_driver_name,0
0x0016 X86_64_32S 00 +0
.klp.sym.test_klp_convert_mod.driver_name,0
-- Joe
Re: [PATCH v4 00/10] klp-convert livepatch build tooling
On 6/13/19 9:00 AM, Miroslav Benes wrote: Hi Joe, first, I'm sorry for the lack of response so far. Maybe you've already noticed but the selftests fail. Well, at least in my VM. When test_klp_convert1.ko is loaded, the process is killed with [ 518.041826] BUG: kernel NULL pointer dereference, address: [ 518.042816] #PF: supervisor read access in kernel mode [ 518.043393] #PF: error_code(0x) - not-present page [ 518.043981] PGD 0 P4D 0 [ 518.044185] Oops: [#1] SMP PTI [ 518.044518] CPU: 2 PID: 2255 Comm: insmod Tainted: G O K 5.1.0-klp_convert_v4-193435-g67748576637e #2 [ 518.045784] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58-prebuilt.qemu.org 04/01/2014 [ 518.046940] RIP: 0010:test_klp_convert_init+0x1c/0x40 [test_klp_convert1] [ 518.047611] Code: 1b a0 48 89 c6 e9 a8 c0 f4 e0 0f 1f 40 00 0f 1f 44 00 00 53 48 c7 c7 00 30 1b a0 e8 5e 33 f6 e0 85 c0 89 c3 74 04 89 d8 5b c3 <48> 8b 35 5d ef e4 5f 48 c7 c7 28 20 1b a0 e8 75 c0 f4 e0 e8 6c ff [ 518.049779] RSP: 0018:c9f37cc8 EFLAGS: 00010246 [ 518.050243] RAX: RBX: RCX: 00027de0 [ 518.050922] RDX: RSI: 0006 RDI: 88807ab54f40 [ 518.051619] RBP: a01b1080 R08: 96efde7a R09: 0001 [ 518.052332] R10: R11: R12: [ 518.053012] R13: R14: 888078b55000 R15: c9f37ea0 [ 518.053714] FS: 7febece1fb80() GS:88807d40() knlGS: [ 518.054514] CS: 0010 DS: ES: CR0: 80050033 [ 518.055078] CR2: CR3: 7a56a000 CR4: 06e0 [ 518.055818] Call Trace: [ 518.056007] do_one_initcall+0x6a/0x2da [ 518.056340] ? do_init_module+0x22/0x230 [ 518.056702] ? rcu_read_lock_sched_held+0x96/0xa0 [ 518.057125] ? kmem_cache_alloc_trace+0x284/0x2e0 [ 518.057493] do_init_module+0x5a/0x230 [ 518.057900] load_module+0x17bc/0x1f50 [ 518.058214] ? __symbol_put+0x40/0x40 [ 518.058499] ? vfs_read+0x12d/0x160 [ 518.058766] __do_sys_finit_module+0x83/0xc0 [ 518.059122] do_syscall_64+0x57/0x190 [ 518.059407] entry_SYSCALL_64_after_hwframe+0x49/0xbe ... It crashes right in test_klp_convert_init() when print_*() using supposed-to-be-converted symbols are called. I'll debug it next week. Can you reproduce it too? Hey, thanks for the report.. I don't recall the tests crashing, but I had put this patchset on the side for a few weeks now. I'll try to fire up a VM and see what happens today. Regards, Miroslav PS: it is probably not a coincidence that I come across selftests failures right before I leave for a holiday... Are you volunteering to go on holidays for each patchset until all of the selftests pass? :) -- Joe
Re: Oops caused by race between livepatch and ftrace
On 5/20/19 5:19 PM, Joe Lawrence wrote: On 5/20/19 5:09 PM, Johannes Erdfelt wrote: On Mon, May 20, 2019, Joe Lawrence wrote: These two testing scenarios might be interesting to add to our selftests suite. Can you post or add the source(s) to livepatch-test.ko to the tarball? I made the livepatches using kpatch-build and this simple patch: diff --git a/fs/proc/version.c b/fs/proc/version.c index 94901e8e700d..6b8a3449f455 100644 --- a/fs/proc/version.c +++ b/fs/proc/version.c @@ -12,6 +12,7 @@ static int version_proc_show(struct seq_file *m, void *v) utsname()->sysname, utsname()->release, utsname()->version); + seq_printf(m, "example livepatch\n"); return 0; } I just created enough livepatches with the same source patch so that I could reproduce the issue somewhat reliably. I'll see if I can make something that uses klp directly. Ah ok great, I was hoping it was a relatively simply livepatch. We could probably reuse lib/livepatch/test_klp_livepatch.c to do this (patching cmdline_proc_show instead). The rest of the userspace in the initramfs is really straight forward with the only interesting parts being a couple of shell scripts. Yup. I'll be on PTO later this week, but I'll see about extracting the scripts and building a pile of livepatch .ko's to see how easily it reproduces without qemu. D'oh -- I just remembered that klp doesn't create those klp (arch) relocation sections just yet! Without those, the window for module RO -> RW -> RO in klp_init_object_loaded is going to be really small... at least I can't reproduce it yet without those special sections. So maybe such selftests need to wait post klp-convert. BTW, livepatching folks -- speaking of this window, does it make sense for klp_init_object_loaded() to unconditionally frob the module section permissions? Should it only bother iff it's going to apply relocations/alternatives/paravirt? -- Joe
Re: Oops caused by race between livepatch and ftrace
On 5/20/19 5:09 PM, Johannes Erdfelt wrote: On Mon, May 20, 2019, Joe Lawrence wrote: [ fixed jeyu's email address ] Thank you, the bounce message made it seem like my mail server was blocked and not that the address didn't exist. I think MAINTAINERS needs an update since it still has the @redhat.com address. Here's how it looks on my end: % git describe HEAD v5.1-12317-ga6a4b66bd8f4 % grep M:.*jeyu MAINTAINERS M: Jessica Yu On 5/20/19 3:49 PM, Johannes Erdfelt wrote: [ ... snip ... ] I have put together a test case that can reproduce the crash using KVM. The tarball includes a minimal kernel and initramfs, along with a script to run qemu and the .config used to build the kernel. By default it will attempt to reproduce by loading multiple livepatches at the same time. Passing 'test=ftrace' to the script will attempt to reproduce by racing with ftrace. My test setup reproduces the race and oops more reliably by loading multiple livepatches at the same time than with the ftrace method. It's not 100% reproducible, so the test case may need to be run multiple times. It can be found here (not attached because of its size): http://johannes.erdfelt.com/5.2.0-rc1-a188339ca5-livepatch-race.tar.gz Hi Johannes, This is cool way to distribute the repro kernel, modules, etc! This oops was common in our production environment and was particularly annoying since livepatches would load at boot and early enough to happen before networking and SSH were started. Unfortunately it was difficult to reproduce on other hardware (changing the timing just enough) and our production environment is very complicated. I spent more time than I'd like to admit trying to reproduce this fairly reliably. I knew that I needed to help make it as easy as possible to reproduce to root cause it and for others to take a look at it as well. Thanks for building this test image -- it repro'd on the first try for me. Hmmm, I wonder then how reproducible it would be if we simply extracted the .ko's and test scripts from out of your initramfs and ran it on arbitrary machines. I think the rcutorture self-tests use qemu/kvm to fire up test VMs, but I dunno if livepatch self-tests are ready for level of sophistication yet :) Will need to think on that a bit. These two testing scenarios might be interesting to add to our selftests suite. Can you post or add the source(s) to livepatch-test.ko to the tarball? I made the livepatches using kpatch-build and this simple patch: diff --git a/fs/proc/version.c b/fs/proc/version.c index 94901e8e700d..6b8a3449f455 100644 --- a/fs/proc/version.c +++ b/fs/proc/version.c @@ -12,6 +12,7 @@ static int version_proc_show(struct seq_file *m, void *v) utsname()->sysname, utsname()->release, utsname()->version); + seq_printf(m, "example livepatch\n"); return 0; } I just created enough livepatches with the same source patch so that I could reproduce the issue somewhat reliably. I'll see if I can make something that uses klp directly. Ah ok great, I was hoping it was a relatively simply livepatch. We could probably reuse lib/livepatch/test_klp_livepatch.c to do this (patching cmdline_proc_show instead). The rest of the userspace in the initramfs is really straight forward with the only interesting parts being a couple of shell scripts. Yup. I'll be on PTO later this week, but I'll see about extracting the scripts and building a pile of livepatch .ko's to see how easily it reproduces without qemu. Thanks, -- Joe
Re: Oops caused by race between livepatch and ftrace
[ fixed jeyu's email address ]
On 5/20/19 3:49 PM, Johannes Erdfelt wrote:
[ ... snip ... ]
I have put together a test case that can reproduce the crash using
KVM. The tarball includes a minimal kernel and initramfs, along with
a script to run qemu and the .config used to build the kernel. By
default it will attempt to reproduce by loading multiple livepatches
at the same time. Passing 'test=ftrace' to the script will attempt to
reproduce by racing with ftrace.
My test setup reproduces the race and oops more reliably by loading
multiple livepatches at the same time than with the ftrace method. It's
not 100% reproducible, so the test case may need to be run multiple
times.
It can be found here (not attached because of its size):
http://johannes.erdfelt.com/5.2.0-rc1-a188339ca5-livepatch-race.tar.gz
Hi Johannes,
This is cool way to distribute the repro kernel, modules, etc!
These two testing scenarios might be interesting to add to our selftests
suite. Can you post or add the source(s) to livepatch-test.ko to the
tarball?
The simple patch of extending the module_mutex lock over the entirety
of klp_init_object_loaded fixes it from the livepatch side. This
mostly works because set_all_modules_text_{rw,ro} acquires module_mutex
as well, but it still leaves a hole in the ftrace code. A lock should
probably be held over the entirety of remapping the text sections RW.
This is complicated by the fact that remapping the text section in
ftrace is handled by arch specific code. I'm not sure what a good
solution to this is yet.
A lock or some kind of referencing count.. I'll let other folks comment
on that side of the bug report.
Thanks,
-- Joe
[tip:core/urgent] stacktrace: Unbreak stack_trace_save_tsk_reliable()
Commit-ID: 7eaf51a2e094229b75cc0c315f1cbbe2f3960058
Gitweb: https://git.kernel.org/tip/7eaf51a2e094229b75cc0c315f1cbbe2f3960058
Author: Joe Lawrence
AuthorDate: Fri, 17 May 2019 14:51:17 -0400
Committer: Thomas Gleixner
CommitDate: Sun, 19 May 2019 11:43:22 +0200
stacktrace: Unbreak stack_trace_save_tsk_reliable()
Miroslav reported that the livepatch self-tests were failing, specifically
a case in which the consistency model ensures that a current executing
function is not allowed to be patched, "TEST: busy target module".
Recent renovations of stack_trace_save_tsk_reliable() left it returning
only an -ERRNO success indication in some configuration combinations:
klp_check_stack()
ret = stack_trace_save_tsk_reliable()
#ifdef CONFIG_ARCH_STACKWALK && CONFIG_HAVE_RELIABLE_STACKTRACE
stack_trace_save_tsk_reliable()
ret = arch_stack_walk_reliable()
return 0
return -EINVAL
...
return ret;
...
if (ret < 0)
/* stack_trace_save_tsk_reliable error */
nr_entries = ret; << 0
Previously (and currently for !CONFIG_ARCH_STACKWALK &&
CONFIG_HAVE_RELIABLE_STACKTRACE) stack_trace_save_tsk_reliable() returned
the number of entries that it consumed in the passed storage array.
In the case of the above config and trace, be sure to return the
stacktrace_cookie.len on stack_trace_save_tsk_reliable() success.
Fixes: 25e39e32b0a3f ("livepatch: Simplify stack trace retrieval")
Reported-by: Miroslav Benes
Signed-off-by: Joe Lawrence
Signed-off-by: Thomas Gleixner
Reviewed-by: Kamalesh Babulal
Acked-by: Josh Poimboeuf
Cc: live-patch...@vger.kernel.org
Cc: ji...@kernel.org
Cc: pmla...@suse.com
Link: https://lkml.kernel.org/r/20190517185117.24642-1-joe.lawre...@redhat.com
---
kernel/stacktrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
index 27bafc1e271e..90d3e0bf0302 100644
--- a/kernel/stacktrace.c
+++ b/kernel/stacktrace.c
@@ -206,7 +206,7 @@ int stack_trace_save_tsk_reliable(struct task_struct *tsk,
unsigned long *store,
ret = arch_stack_walk_reliable(consume_entry, &c, tsk);
put_task_stack(tsk);
- return ret;
+ return ret ? ret : c.len;
}
#endif
Re: [PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return
On Fri, May 17, 2019 at 02:51:17PM -0400, Joe Lawrence wrote:
> Miroslav reported that the livepatch self-tests were failing,
> specifically a case in which the consistency model ensures that we do
> not patch a current executing function, "TEST: busy target module".
>
> Recent renovations to stack_trace_save_tsk_reliable() left it returning
> only an -ERRNO success indication in some configuration combinations:
>
> klp_check_stack()
> ret = stack_trace_save_tsk_reliable()
> #ifdef CONFIG_ARCH_STACKWALK && CONFIG_HAVE_RELIABLE_STACKTRACE
> stack_trace_save_tsk_reliable()
> ret = arch_stack_walk_reliable()
> return 0
> return -EINVAL
> ...
> return ret;
> ...
> if (ret < 0)
> /* stack_trace_save_tsk_reliable error */
> nr_entries = ret; << 0
>
> Previously (and currently for !CONFIG_ARCH_STACKWALK &&
> CONFIG_HAVE_RELIABLE_STACKTRACE) stack_trace_save_tsk_reliable()
> returned the number of entries that it consumed in the passed storage
> array.
>
> In the case of the above config and trace, be sure to return the
> stacktrace_cookie.len on stack_trace_save_tsk_reliable() success.
>
> Fixes: 25e39e32b0a3f ("livepatch: Simplify stack trace retrieval")
> Reported-by: Miroslav Benes
> Signed-off-by: Joe Lawrence
> ---
> kernel/stacktrace.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
> index 27bafc1e271e..90d3e0bf0302 100644
> --- a/kernel/stacktrace.c
> +++ b/kernel/stacktrace.c
> @@ -206,7 +206,7 @@ int stack_trace_save_tsk_reliable(struct task_struct
> *tsk, unsigned long *store,
>
> ret = arch_stack_walk_reliable(consume_entry, &c, tsk);
> put_task_stack(tsk);
> - return ret;
> + return ret ? ret : c.len;
> }
> #endif
>
> --
> 2.20.1
>
Hi Thomas,
This change is *very* lightly tested. It passes the livepatch
self-tests and a test driver that I wrote to debug this. If a more
substantial change is needed, feel free to grab this one.
FWIW, here's the debugging module that I used to first verify that the
return code was always 0 (ie, no nr_entries) and then that I was getting
sane values back from the modified version. It's simple, echo in a task
pointer and it will dump the stack trace to dmesg:
% dmesg -C
% echo 0x8a4107082f40 > /sys/module/checkstack/parameters/task_param
% dmesg
[ 1909.546463] checkstack: task @ 8a4107082f40
[ 1909.549280] checkstack: nr_entries = 7
[ 1909.552268] checkstack: [a608fb29] schedule+0x29/0x90
[ 1909.83] checkstack: [a60941ed]
schedule_hrtimeout_range_clock+0x18d/0x1a0
[ 1909.556864] checkstack: [a5b27e38] ep_poll+0x428/0x450
[ 1909.557645] checkstack: [a5b27f10] do_epoll_wait+0xb0/0xd0
[ 1909.558454] checkstack: [a5b27f4a]
__x64_sys_epoll_wait+0x1a/0x20
[ 1909.559354] checkstack: [a58041e5] do_syscall_64+0x55/0x1a0
[ 1909.560233] checkstack: [a620008c]
entry_SYSCALL_64_after_hwframe+0x44/0xa9
-- Joe
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include
#include
#include
#include
#include
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Joe Lawrence ");
#define MAX_STACK_ENTRIES 100
/* No exports, no fun */
static int (*p_stack_trace_save_tsk_reliable)(struct task_struct *tsk, unsigned
long *store, unsigned int size);
int checkstack(struct task_struct *task)
{
static unsigned long entries[MAX_STACK_ENTRIES];
unsigned long address;
int ret, nr_entries, i;
if (!task)
return 0;
pr_info("task @ %lx\n", (unsigned long) task);
ret = p_stack_trace_save_tsk_reliable(task, entries,
ARRAY_SIZE(entries));
WARN_ON_ONCE(ret == -ENOSYS);
if (ret < 0) {
pr_err("%s: %s:%d has an unreliable stack\n",
__func__, task->comm, task->pid);
return ret;
}
nr_entries = ret;
pr_info("nr_entries = %d\n", nr_entries);
for (i = 0; i < nr_entries; i++) {
address = entries[i];
pr_info("\t[%lx] %pS\n", address, (void *) address);
}
return 0;
}
static unsigned long task_param = 0;
static int task_param_set(const char *val, const struct kernel_param *kp)
{
int ret;
ret = param_set_ulong(val, kp);
if (ret < 0)
return ret;
return checkstack((struct task_struct *)task_param);
[PATCH] stacktrace: fix CONFIG_ARCH_STACKWALK stack_trace_save_tsk_reliable return
Miroslav reported that the livepatch self-tests were failing,
specifically a case in which the consistency model ensures that we do
not patch a current executing function, "TEST: busy target module".
Recent renovations to stack_trace_save_tsk_reliable() left it returning
only an -ERRNO success indication in some configuration combinations:
klp_check_stack()
ret = stack_trace_save_tsk_reliable()
#ifdef CONFIG_ARCH_STACKWALK && CONFIG_HAVE_RELIABLE_STACKTRACE
stack_trace_save_tsk_reliable()
ret = arch_stack_walk_reliable()
return 0
return -EINVAL
...
return ret;
...
if (ret < 0)
/* stack_trace_save_tsk_reliable error */
nr_entries = ret; << 0
Previously (and currently for !CONFIG_ARCH_STACKWALK &&
CONFIG_HAVE_RELIABLE_STACKTRACE) stack_trace_save_tsk_reliable()
returned the number of entries that it consumed in the passed storage
array.
In the case of the above config and trace, be sure to return the
stacktrace_cookie.len on stack_trace_save_tsk_reliable() success.
Fixes: 25e39e32b0a3f ("livepatch: Simplify stack trace retrieval")
Reported-by: Miroslav Benes
Signed-off-by: Joe Lawrence
---
kernel/stacktrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
index 27bafc1e271e..90d3e0bf0302 100644
--- a/kernel/stacktrace.c
+++ b/kernel/stacktrace.c
@@ -206,7 +206,7 @@ int stack_trace_save_tsk_reliable(struct task_struct *tsk,
unsigned long *store,
ret = arch_stack_walk_reliable(consume_entry, &c, tsk);
put_task_stack(tsk);
- return ret;
+ return ret ? ret : c.len;
}
#endif
--
2.20.1
Re: livepatching selftests failure on current master branch
On 5/17/19 10:17 AM, Miroslav Benes wrote: Hi, I noticed that livepatching selftests fail on our master branch (https://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git/). ... TEST: busy target module ... not ok --- expected +++ result @@ -7,16 +7,24 @@ livepatch: 'test_klp_callbacks_demo': in test_klp_callbacks_demo: pre_patch_callback: vmlinux test_klp_callbacks_demo: pre_patch_callback: test_klp_callbacks_busy -> [MODULE_STATE_LIVE] Normal state livepatch: 'test_klp_callbacks_demo': starting patching transition +livepatch: 'test_klp_callbacks_demo': completing patching transition +test_klp_callbacks_demo: post_patch_callback: vmlinux +test_klp_callbacks_demo: post_patch_callback: test_klp_callbacks_busy -> [MODULE_STATE_LIVE] Normal state +livepatch: 'test_klp_callbacks_demo': patching complete % modprobe test_klp_callbacks_mod livepatch: applying patch 'test_klp_callbacks_demo' to loading module 'test_klp_callbacks_mod' test_klp_callbacks_demo: pre_patch_callback: test_klp_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init +test_klp_callbacks_demo: post_patch_callback: test_klp_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init test_klp_callbacks_mod: test_klp_callbacks_mod_init % rmmod test_klp_callbacks_mod test_klp_callbacks_mod: test_klp_callbacks_mod_exit +test_klp_callbacks_demo: pre_unpatch_callback: test_klp_callbacks_mod -> [MODULE_STATE_GOING] Going away livepatch: reverting patch 'test_klp_callbacks_demo' on unloading module 'test_klp_callbacks_mod' test_klp_callbacks_demo: post_unpatch_callback: test_klp_callbacks_mod -> [MODULE_STATE_GOING] Going away % echo 0 > /sys/kernel/livepatch/test_klp_callbacks_demo/enabled -livepatch: 'test_klp_callbacks_demo': reversing transition from patching to unpatching +livepatch: 'test_klp_callbacks_demo': initializing unpatching transition +test_klp_callbacks_demo: pre_unpatch_callback: vmlinux +test_klp_callbacks_demo: pre_unpatch_callback: test_klp_callbacks_busy -> [MODULE_STATE_LIVE] Normal state livepatch: 'test_klp_callbacks_demo': starting unpatching transition livepatch: 'test_klp_callbacks_demo': completing unpatching transition test_klp_callbacks_demo: post_unpatch_callback: vmlinux ERROR: livepatch kselftest(s) failed not ok 1..2 selftests: livepatch: test-callbacks.sh [FAIL] which probably means that the consistency model is not in the best shape. There were not many livepatch changes in the latest pull request. Stack unwinder changes may be connected, so adding Thomas to be aware if it leads in this direction. Unfortunately, I'm leaving in a minute and will be gone till Wednesday, so if someone confirms and wants to investigate, definitely feel free to do it. I will take a look today. Thanks for reporting. I hope it's something silly in the tests not the consistency model ... -- Joe
[PATCH v4 03/10] livepatch: Add klp-convert tool
From: Josh Poimboeuf 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 Symbols.list, 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, create scripts/livepatch to hold tools developed for livepatches and add source files for klp-convert there. The core file of klp-convert is scripts/livepatch/klp-convert.c, which implements the heuristics used to solve the relocations and the conversion of unresolved symbols into the expected format, as defined in [1]. klp-convert receives as arguments the Symbols.list file, an input livepatch module to be converted and the output name for the converted livepatch. When it starts running, klp-convert parses Symbols.list and builds two internal lists of symbols, one containing the exported and another containing the non-exported symbols. Then, by parsing the rela sections in the elf object, klp-convert identifies which symbols must be converted, which are those unresolved and that do not have a corresponding exported symbol, and attempts to convert them accordingly to the specification. By using Symbols.list, klp-convert identifies which symbols have names that only appear in a single kernel object, thus being capable of resolving these cases without the intervention of the developer. When various homonymous symbols exist through kernel objects, it is not possible to infer the right one, thus klp-convert falls back into using developer annotations. If these were not provided, then the tool will print a list with all acceptable targets for the symbol being processed. Annotations in the context of klp-convert are accessible as struct klp_module_reloc entries in sections named .klp.module_relocs.. These entries are pairs of symbol references and positions which are to be resolved against definitions in . Define the structure klp_module_reloc in include/linux/uapi/livepatch.h allowing developers to annotate the livepatch source code with it. klp-convert relies on libelf and on a list implementation. Add files scripts/livepatch/elf.c and scripts/livepatch/elf.h, which are a libelf interfacing layer and scripts/livepatch/list.h, which is a list implementation. Update Makefiles to correctly support the compilation of the new tool, update MAINTAINERS file and add a .gitignore file. [1] - Documentation/livepatch/module-elf-format.txt Signed-off-by: Josh Poimboeuf Signed-off-by: Konstantin Khlebnikov Signed-off-by: Joao Moreira Signed-off-by: Joe Lawrence --- MAINTAINERS | 1 + include/uapi/linux/livepatch.h | 5 + scripts/Makefile| 1 + scripts/livepatch/.gitignore| 1 + scripts/livepatch/Makefile | 7 + scripts/livepatch/elf.c | 753 scripts/livepatch/elf.h | 73 scripts/livepatch/klp-convert.c | 713 ++ scripts/livepatch/klp-convert.h | 39 ++ scripts/livepatch/list.h| 391 + 10 files changed, 1984 insertions(+) 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 diff --git a/MAINTAINERS b/MAINTAINERS index 52842fa37261..c1587e1cc385 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9022,6 +9022,7 @@ F:arch/x86/kernel/livepatch.c F: Documentation/livepatch/ F: Documentation/ABI/testing/sysfs-kernel-livepatch F: samples/livepatch/ +F: scripts/livepatch/ F: tools/testing/selftests/livepatch/ L: live-patch...@vger.kernel.org T: git git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching.git diff --git a/include/uapi/linux/livepatch.h b/include/uapi/linux/livepatch.h index e19430918a07..1c364d42d38e 100644 --- a/include/uapi/linux/livepatch.h +++ b/include/uapi/linux/livepatch.h @@ -12,4 +12,9 @@ #define KLP_RELA_PREFIX
[PATCH v4 05/10] modpost: Integrate klp-convert
From: Josh Poimboeuf
Create cmd_klp_convert and hook it into scripts/Makefile.modpost.
cmd_klp_convert invokes klp-convert with the right arguments for the
conversion of unresolved symbols inside a livepatch.
Signed-off-by: Josh Poimboeuf
Signed-off-by: Konstantin Khlebnikov
Signed-off-by: Miroslav Benes
Signed-off-by: Joao Moreira
Signed-off-by: Joe Lawrence
---
scripts/Kbuild.include | 4 +++-
scripts/Makefile.modpost | 16 +++-
scripts/mod/modpost.c| 6 +-
scripts/mod/modpost.h| 1 +
4 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 7484b9d8272f..f8b7d12e44c9 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -236,6 +236,8 @@ endif
# (needed for the shell)
make-cmd = $(call escsq,$(subst $(pound),$$(pound),$(subst
$$,,$(cmd_$(1)
+save-cmd = printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd
+
# Find any prerequisites that is newer than target or that does not exist.
# PHONY targets skipped in both cases.
any-prereq = $(filter-out $(PHONY),$?) $(filter-out $(PHONY) $(wildcard $^),$^)
@@ -243,7 +245,7 @@ any-prereq = $(filter-out $(PHONY),$?) $(filter-out
$(PHONY) $(wildcard $^),$^)
# Execute command if command has changed or prerequisite(s) are updated.
if_changed = $(if $(strip $(any-prereq) $(arg-check)), \
$(cmd); \
- printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd, @:)
+ $(save-cmd), @:)
# Execute the command and also postprocess generated .d dependencies file.
if_changed_dep = $(if $(strip $(any-prereq) $(arg-check)),$(cmd_and_fixdep),@:)
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 6b7f354f189a..1bef611f99b6 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -124,8 +124,22 @@ quiet_cmd_ld_ko_o = LD [M] $@
-o $@ $(real-prereqs) ;\
$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
+SLIST = $(objtree)/Symbols.list
+KLP_CONVERT = scripts/livepatch/klp-convert
+quiet_cmd_klp_convert = KLP $@
+ cmd_klp_convert = mv $@ $(@:.ko=.klp.o); \
+ $(KLP_CONVERT) $(SLIST) $(@:.ko=.klp.o) $@
+
+define rule_ld_ko_o
+ $(Q)$(call echo-cmd,ld_ko_o) $(cmd_ld_ko_o) ; \
+ $(call save-cmd,ld_ko_o) ; \
+ $(if $(CONFIG_LIVEPATCH), \
+ $(if $(wildcard $(MODVERDIR)/$(basetarget).livepatch),\
+ $(call echo-cmd,klp_convert) $(cmd_klp_convert) ))
+endef
+
$(modules): %.ko :%.o %.mod.o FORCE
- +$(call if_changed,ld_ko_o)
+ +$(call if_changed_rule,ld_ko_o)
targets += $(modules)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f277e116e0eb..374b22c76ec5 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1974,6 +1974,10 @@ static void read_symbols(const char *modname)
license = get_next_modinfo(&info, "license", license);
}
+ /* Livepatch modules have unresolved symbols resolved by klp-convert */
+ if (get_modinfo(&info, "livepatch"))
+ mod->livepatch = 1;
+
for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
symname = remove_dot(info.strtab + sym->st_name);
@@ -2101,7 +2105,7 @@ static int check_exports(struct module *mod)
const char *basename;
exp = find_symbol(s->name);
if (!exp || exp->module == mod) {
- if (have_vmlinux && !s->weak) {
+ if (have_vmlinux && !s->weak && !mod->livepatch) {
if (warn_unresolved) {
warn("\"%s\" [%s.ko] undefined!\n",
s->name, mod->name);
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 8453d6ac2f77..2acfaae064ec 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -118,6 +118,7 @@ struct module {
int skip;
int has_init;
int has_cleanup;
+ int livepatch;
struct buffer dev_table_buf;
char srcversion[25];
int is_dot_o;
--
2.20.1
[PATCH v4 04/10] livepatch: Add klp-convert annotation helpers
From: Josh Poimboeuf
Define macros KLP_MODULE_RELOC and KLP_SYMPOS in
include/linux/livepatch.h to improve user-friendliness of the
livepatch annotation process.
Signed-off-by: Josh Poimboeuf
Signed-off-by: Joao Moreira
Signed-off-by: Joe Lawrence
---
include/linux/livepatch.h | 12
1 file changed, 12 insertions(+)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 16b48e8b29a2..3071aec8fd72 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -236,6 +236,18 @@ void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
+/* Used to annotate symbol relocations in livepatches */
+#define KLP_MODULE_RELOC(obj) \
+ struct klp_module_reloc \
+ __attribute__((__section__(".klp.module_relocs." #obj)))\
+ __attribute__((aligned (4)))
+
+#define KLP_SYMPOS(symbol, pos)
\
+ { \
+ .sym = &symbol, \
+ .sympos = pos, \
+ }
+
#else /* !CONFIG_LIVEPATCH */
static inline int klp_module_coming(struct module *mod) { return 0; }
--
2.20.1
[PATCH v4 07/10] livepatch: Add sample livepatch module
From: Josh Poimboeuf
Add a new livepatch sample in samples/livepatch/ to make use of
symbols that must be post-processed to enable load-time relocation
resolution. As the new sample is to be used as an example, it is
annotated with KLP_MODULE_RELOC and with KLP_SYMPOS macros.
The livepatch sample updates the function cmdline_proc_show to
print the string referenced by the symbol saved_command_line
appended by the string "livepatch=1".
Update livepatch-sample.c to remove livepatch MODULE_INFO
statement.
Signed-off-by: Josh Poimboeuf
Signed-off-by: Joao Moreira
Signed-off-by: Joe Lawrence
---
samples/livepatch/Makefile| 2 +
.../livepatch/livepatch-annotated-sample.c| 102 ++
2 files changed, 104 insertions(+)
create mode 100644 samples/livepatch/livepatch-annotated-sample.c
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 514c8156f979..1a92d6b58f33 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -2,6 +2,7 @@ LIVEPATCH_livepatch-sample := y
LIVEPATCH_livepatch-shadow-fix1 := y
LIVEPATCH_livepatch-shadow-fix2 := y
LIVEPATCH_livepatch-callbacks-demo := y
+LIVEPATCH_livepatch-annotated-sample := y
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-mod.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix1.o
@@ -9,3 +10,4 @@ obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-shadow-fix2.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-busymod.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-annotated-sample.o
diff --git a/samples/livepatch/livepatch-annotated-sample.c
b/samples/livepatch/livepatch-annotated-sample.c
new file mode 100644
index ..556ce7e0bdab
--- /dev/null
+++ b/samples/livepatch/livepatch-annotated-sample.c
@@ -0,0 +1,102 @@
+/*
+ * livepatch-annotated-sample.c - Kernel Live Patching Sample Module
+ *
+ * Copyright (C) 2014 Seth Jennings
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+
+/*
+ * This (dumb) live patch overrides the function that prints the
+ * kernel boot cmdline when /proc/cmdline is read.
+ *
+ * This livepatch uses the symbol saved_command_line whose relocation
+ * must be resolved during load time. To enable that, this module
+ * must be post-processed by a tool called klp-convert, which embeds
+ * information to be used by the loader to solve the relocation.
+ *
+ * The module is annotated with KLP_MODULE_RELOC/KLP_SYMPOS macros.
+ * These annotations are used by klp-convert to infer that the symbol
+ * saved_command_line is in the object vmlinux.
+ *
+ * As saved_command_line has no other homonimous symbol across
+ * kernel objects, this annotation is not a requirement, and can be
+ * suppressed with no harm to klp-convert. Yet, it is kept here as an
+ * example on how to annotate livepatch modules that contain symbols
+ * whose names are used in more than one kernel object.
+ *
+ * Example:
+ *
+ * $ cat /proc/cmdline
+ *
+ *
+ * $ insmod livepatch-sample.ko
+ * $ cat /proc/cmdline
+ * livepatch=1
+ *
+ * $ echo 0 > /sys/kernel/livepatch/livepatch_sample/enabled
+ * $ cat /proc/cmdline
+ *
+ */
+
+extern char *saved_command_line;
+
+#include
+static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
+{
+ seq_printf(m, "%s livepatch=1\n", saved_command_line);
+ return 0;
+}
+
+KLP_MODULE_RELOC(vmlinux) vmlinux_relocs[] = {
+ KLP_SYMPOS(saved_command_line, 0)
+};
+
+static struct klp_func funcs[] = {
+ {
+ .old_name = "cmdline_proc_show",
+ .new_func = livepatch_cmdline_proc_show,
+ }, { }
+};
+
+static struct klp_object objs[] = {
+ {
+ /* name being NULL means vmlinux */
+ .funcs = funcs,
+ }, { }
+};
+
+static struct klp_patch patch = {
+ .mod = THIS_MODULE,
+ .objs = objs,
+};
+
+static int livepatch_init(void)
+{
+ return klp_enable_patch(&patch);
+}
+
+static void livepatch_exit(void)
+{
+}
+
+module_init(livepatch_init);
+module_exit(livepatch_exit);
+MODULE_LICENSE("GPL");
--
2.20.1
[PATCH v4 10/10] livepatch/klp-convert: abort on special sections
To properly convert alternatives, paravirt ops, and static keys,
klp-convert needs to implement "klp.arch" sections as supported by
d4c3e6e1b193 (“livepatch/x86: apply alternatives and paravirt patches
after relocations”).
There is some amount of ELF section bookkeeping required for this (ie,
moving data structure entries and relocations to their ".klp.arch"
equivalents) but the hardest part will be determining klp_object
relationships. This may require some larger changes to livepatch API,
so defer support for now.
Signed-off-by: Joe Lawrence
---
scripts/livepatch/klp-convert.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
index 72b65428e738..b5873abecefb 100644
--- a/scripts/livepatch/klp-convert.c
+++ b/scripts/livepatch/klp-convert.c
@@ -617,6 +617,18 @@ static void free_converted_resources(struct elf *klp_elf)
}
}
+/* Check for special sections that klp-convert doesn't support */
+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;
+}
+
int main(int argc, const char **argv)
{
const char *klp_in_module, *klp_out_module, *symbols_list;
@@ -649,6 +661,12 @@ int main(int argc, const char **argv)
}
list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
+ if (!is_section_supported(sec->name)) {
+ WARN("Special ELF section: %s not supported",
+ sec->name);
+ return -1;
+ }
+
if (!is_rela_section(sec) ||
is_klp_rela_section(sec->name))
continue;
--
2.20.1
[PATCH v4 09/10] livepatch/selftests: add klp-convert
Add a simple klp-convert livepatch selftest that exercises various
symbol homonym sympos scenarios.
Signed-off-by: Joe Lawrence
---
lib/livepatch/Makefile| 10 ++
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 +++
.../selftests/livepatch/test-livepatch.sh | 64 +++
6 files changed, 321 insertions(+)
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
diff --git a/lib/livepatch/Makefile b/lib/livepatch/Makefile
index 513d200b7942..b19253c72d72 100644
--- a/lib/livepatch/Makefile
+++ b/lib/livepatch/Makefile
@@ -5,6 +5,8 @@
LIVEPATCH_test_klp_atomic_replace := y
LIVEPATCH_test_klp_callbacks_demo := y
LIVEPATCH_test_klp_callbacks_demo2 := y
+LIVEPATCH_test_klp_convert1 := y
+LIVEPATCH_test_klp_convert2 := y
LIVEPATCH_test_klp_livepatch := y
obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
@@ -12,9 +14,17 @@ obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \
test_klp_callbacks_demo2.o \
test_klp_callbacks_busy.o \
test_klp_callbacks_mod.o \
+ test_klp_convert1.o \
+ test_klp_convert2.o \
+ test_klp_convert_mod.o \
test_klp_livepatch.o \
test_klp_shadow_vars.o
+test_klp_convert_mod-y := \
+ test_klp_convert_mod_a.o \
+ test_klp_convert_mod_b.o
+
# Target modules to be livepatched require CC_FLAGS_FTRACE
CFLAGS_test_klp_callbacks_busy.o += $(CC_FLAGS_FTRACE)
CFLAGS_test_klp_callbacks_mod.o+= $(CC_FLAGS_FTRACE)
+CFLAGS_test_klp_convert_mod.o += $(CC_FLAGS_FTRACE)
diff --git a/lib/livepatch/test_klp_convert1.c
b/lib/livepatch/test_klp_convert1.c
new file mode 100644
index ..a359f69a8fdc
--- /dev/null
+++ b/lib/livepatch/test_klp_convert1.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2019 Joe Lawrence
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+
+/* klp-convert symbols - vmlinux */
+extern char *saved_command_line;
+/* klp-convert symbols - test_klp_convert_mod.ko */
+extern char driver_name[];
+extern char homonym_string[];
+extern const char *get_homonym_string(void);
+extern const char *get_driver_name(void);
+
+void print_saved_command_line(void)
+{
+ pr_info("saved_command_line, 0: %s\n", saved_command_line);
+}
+
+void print_driver_name(void)
+{
+ pr_info("driver_name, 0: %s\n", driver_name);
+ pr_info("get_driver_name(), 0: %s\n", get_driver_name());
+}
+
+void print_homonym_string(void)
+{
+ pr_info("homonym_string, 1: %s\n", homonym_string);
+ pr_info("get_homonym_string(), 1: %s\n", get_homonym_string());
+}
+
+/*
+ * saved_command_line is a unique symbol, so the sympos annotation is
+ * optional. Provide to test that sympos=0 works correctly.
+ */
+KLP_MODULE_RELOC(vmlinux) vmlinux_relocs[] = {
+ KLP_SYMPOS(saved_command_line, 0)
+};
+
+/*
+ * driver_name symbols can be found in vmlinux (multiple) and also
+ * test_klp_convert_mod, therefore the annotation is required to
+ * clarify that we want the one from test_klp_convert_mod.
+ *
+ * test_klp_convert_mod contains multiple homonym_string and
+ * get_homonym_string symbols, test resolving the first set here and
+ * the others in test_klp_convert2.c
+ *
+ * get_driver_name is a uniquely named symbol, test that sympos=0
+ * work correctly.
+ */
+KLP_MODULE_RELOC(test_klp_convert_mod) test_klp_convert_mod_relocs_a[] = {
+ KLP_SYMPOS(driver_name, 0),
+ KLP_SYMPOS(homonym_string, 1),
+ KLP_SYMPOS(get_homonym_string, 1),
+ KLP_SYMPOS(get_driver_name, 0),
+};
+
+static struct klp_func funcs[] = {
+ {
+ }, { }
+};
+
+static struct klp_object objs[] = {
+ {
+ /* name being NULL means vmlinux */
+ .funcs = funcs,
+ },
+ {
+ .name = "test_klp_convert_mod",
+ .funcs = funcs,
+ }, { }
+};
+
+static struct klp_patch patch = {
+ .mod = THIS_MODULE,
+ .objs = objs,
+};
+
+static int test_klp_convert_init(void)
+{
+ int ret;
+
+ ret = klp_enable_patch(&patch);
+ if (ret)
+ return ret;
+
+ print_saved_command_line();
+ print_driver_name();
+ print_homonym_string();
+
+ return 0;
+}
+
+static void test_klp_convert_exit(void)
+{
+}
+
+module_init(test_kl
[PATCH v4 02/10] kbuild: Support for Symbols.list creation
From: Joao Moreira For automatic resolution of livepatch relocations, a file called Symbols.list is used. This file maps symbols within every compiled kernel object allowing the identification of symbols whose name is unique, thus relocation can be automatically inferred, or providing information that helps developers when code annotation is required for solving the matter. Add support for creating Symbols.list in the main Makefile. First, ensure that built-in is compiled when CONFIG_LIVEPATCH is enabled (as required to achieve a complete Symbols.list file). Define the command to build Symbols.list (cmd_klp_map) and hook it in the modules rule. As it is undesirable to have symbols from livepatch objects inside Symbols.list, make livepatches discernible by modifying scripts/Makefile.build to create a .livepatch file for each livepatch in $(MODVERDIR). This file then used by cmd_klp_map to identify and bypass livepatches. For identifying livepatches during the build process, a flag variable LIVEPATCH_$(basetarget).o is considered in scripts/Makefile.build. This way, set this flag for the livepatch sample Makefile in samples/livepatch/Makefile. Finally, Add a clean rule to ensure that Symbols.list is removed during clean. Notes: To achieve a correct Symbols.list file, all kernel objects must be considered, thus, its construction require these objects to be priorly built. On the other hand, invoking scripts/Makefile.modpost without having a complete Symbols.list in place would occasionally lead to in-tree livepatches being post-processed incorrectly. To prevent this from becoming a circular dependency, the construction of Symbols.list uses non-post-processed kernel objects and such does not cause harm as the symbols normally referenced from within livepatches are visible at this stage. Also due to these requirements, the spot in-between modules compilation and the invocation of scripts/Makefile.modpost was picked for hooking cmd_klp_map. The approach based on .livepatch files was proposed as an alternative to using MODULE_INFO statements. This approach was originally proposed by Miroslav Benes as a workaround for identifying livepathes without depending on modinfo during the modpost stage. It was moved to this patch as the approach also shown to be useful while building Symbols.list. Signed-off-by: Joao Moreira Signed-off-by: Joe Lawrence --- .gitignore | 1 + Makefile | 30 ++ lib/livepatch/Makefile | 5 + samples/livepatch/Makefile | 4 scripts/Makefile.build | 7 +++ 5 files changed, 43 insertions(+), 4 deletions(-) diff --git a/.gitignore b/.gitignore index a20ac26aa2f5..5cd5758f5ffe 100644 --- a/.gitignore +++ b/.gitignore @@ -45,6 +45,7 @@ *.xz Module.symvers modules.builtin +Symbols.list # # Top-level generic files diff --git a/Makefile b/Makefile index abe13538a8c0..98089f9d44fe 100644 --- a/Makefile +++ b/Makefile @@ -574,10 +574,13 @@ KBUILD_BUILTIN := 1 # If we have only "make modules", don't compile built-in objects. # When we're building modules with modversions, we need to consider # the built-in objects during the descend as well, in order to -# make sure the checksums are up to date before we record them. +# make sure the checksums are up to date before we record them. The +# same applies for building livepatches, as built-in objects may hold +# symbols which are referenced from livepatches and are required by +# klp-convert post-processing tool for resolving these cases. ifeq ($(MAKECMDGOALS),modules) - KBUILD_BUILTIN := $(if $(CONFIG_MODVERSIONS),1) + KBUILD_BUILTIN := $(if $(or $(CONFIG_MODVERSIONS), $(CONFIG_LIVEPATCH)),1) endif # If we have "make modules", compile modules @@ -1261,9 +1264,25 @@ all: modules # duplicate lines in modules.order files. Those are removed # using awk while concatenating to the final file. +quiet_cmd_klp_map = KLP Symbols.list +SLIST = $(objtree)/Symbols.list + +define cmd_klp_map + $(shell echo "klp-convert-symbol-data.0.1" > $(SLIST)) \ + $(shell echo "*vmlinux" >> $(SLIST)) \ + $(shell nm -f posix $(objtree)/vmlinux | cut -d\ -f1 >> $(SLIST)) \ + $(foreach m, $(wildcard $(MODVERDIR)/*.mod), \ + $(eval mod = $(patsubst %.ko,%.o,$(shell head -n1 $(m \ + $(if $(wildcard $(MODVERDIR)/$(shell basename -s .o $(mod)).livepatch),,\ + $(eval fmod = $(subst $(quote),_,$(subst -,_,$(mod \ + $(shell echo "*$(shell basename -s .o $(fmod))" >> $(SLIST))\ + $(shell nm -f posix $(mod) | cut -d\ -f1 >> $(SLIST +endef + PHONY += modules modules: $(vmlinux-dirs) $(if $(KBUILD_BUIL
[PATCH v4 08/10] documentation: Update on livepatch elf format
From: Joao Moreira
Add a section to Documentation/livepatch/module-elf-format.txt
describing how klp-convert works for fixing relocations.
Signed-off-by: Joao Moreira
Signed-off-by: Joe Lawrence
---
Documentation/livepatch/livepatch.txt | 3 ++
Documentation/livepatch/module-elf-format.txt | 50 ---
2 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/Documentation/livepatch/livepatch.txt
b/Documentation/livepatch/livepatch.txt
index 4627b41ff02e..873c11aee038 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -274,6 +274,9 @@ into three levels:
absolute position in the database, but rather the order it has been found
only for a particular object ( vmlinux or a kernel module ). Note that
kallsyms allows for searching symbols according to the object name.
+Uniquely named symbols may use a symbol position of 0. Non-unique
+symbols need to specify their object / kallsyms position, starting
+at position 1.
+ struct klp_object defines an array of patched functions (struct
klp_func) in the same object. Where the object is either vmlinux
diff --git a/Documentation/livepatch/module-elf-format.txt
b/Documentation/livepatch/module-elf-format.txt
index f21a5289a09c..7bef8432352a 100644
--- a/Documentation/livepatch/module-elf-format.txt
+++ b/Documentation/livepatch/module-elf-format.txt
@@ -2,7 +2,8 @@
Livepatch module Elf format
===
-This document outlines the Elf format requirements that livepatch modules must
follow.
+This document outlines the Elf format requirements that livepatch modules must
+follow.
-
Table of Contents
@@ -25,8 +26,9 @@ Table of Contents
3.3.2 Required name format
3.3.3 Example livepatch symbol names
3.3.4 Example `readelf --symbols` output
-4. Architecture-specific sections
-5. Symbol table and Elf section access
+4. Automatic conversion of unresolved relocations
+5. Architecture-specific sections
+6. Symbol table and Elf section access
0. Background and motivation
@@ -270,7 +272,8 @@ Livepatch symbol names must conform to the following format:
[D] The position of the symbol in the object (as according to kallsyms)
This is used to differentiate duplicate symbols within the same
object. The symbol position is expressed numerically (0, 1, 2...).
-The symbol position of a unique symbol is 0.
+The symbol position of a unique symbol is 0. The symbol position of
+the first non-unique symbol is 1, the second is 2, etc.
3.3.3 Example livepatch symbol names:
-
@@ -293,8 +296,43 @@ Symbol table '.symtab' contains 127 entries:
[*] Note that the 'Ndx' (Section index) for these symbols is SHN_LIVEPATCH
(0xff20).
"OS" means OS-specific.
+--
+4. Automatic conversion of unresolved relocations
+--
+Sometimes livepatches may operate on symbols which are not self-contained nor
+exported. When this happens, these symbols remain unresolved in the elf object
+and will trigger an error during the livepatch instantiation.
+
+Whenever possible, the kernel building infrastructure solves this problem
+automatically. First, a symbol database containing information on all compiled
+objects is built. Second, this database - a file named Symbols.list, placed in
+the kernel source root directory - is used to identify targets for unresolved
+relocations, converting them in the livepatch elf accordingly to the
+specifications above-described. While the first stage is fully handled by the
+building system, the second is done by a tool called klp-convert, which can be
+found in "scripts/livepatch".
+
+When an unresolved relocation has as target a symbol whose name is also used by
+different symbols throughout the kernel, the relocation cannot be resolved
+automatically. In these cases, the livepatch developer must add annotations to
+the livepatch, making it possible for the system to identify which is the
+correct target amongst multiple homonymous symbols. Such annotations must be
+done through a data structure as follows:
+
+struct KLP_MODULE_RELOC(object) data_structure_name[] = {
+ KLP_SYMPOS(symbol, pos)
+};
+
+In the above example, object refers to the object file which contains the
+symbol, being vmlinux or a module; symbol refers to the symbol name that will
+be relocated and pos is its position in the object.
+
+When a data structure like this is added to the livepatch, the resulting elf
+will hold symbols that will be identified by klp-convert and used to solve name
+ambiguities.
+
-
-4. Architecture-specific sections
+5. Architecture-specific sections
-
Architectures may override arch_klp_init_obj
[PATCH v4 00/10] klp-convert livepatch build tooling
Hi all,
Thanks for the feedback to v3, I've incorporated a few more bug fixes
and style/spelling nitpicks for the series. v4 is a bit of a resting
place to collect loose ends before considering some heavier future work,
namely arch-specific special section support. See the TODO section
below for more details.
Previous versions
-
RFC:
https://lore.kernel.org/lkml/cover.1477578530.git.jpoim...@redhat.com/
v2:
https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878f...@redhat.com/
v3:
https://lore.kernel.org/lkml/20190410155058.9437-1-joe.lawre...@redhat.com/
Summary
---
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
For review purposes, I posted two branches up to github:
1 - an expanded branch with changes separate from the original
https://github.com/joe-lawrence/linux/tree/klp-convert-v4-expanded
2 - a squashed branch of (1) that comprises v4:
https://github.com/joe-lawrence/linux/tree/klp-convert-v4
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
Summarized from the v3 thread, thanks to Miroslav, Joao and Josh for
feedback and parsing my long braindumps.
- Special (architecture specific) section support:
.altinstructions, .altinst_replacement
.parainstructions
__jump_table
We want to apply livepatch relocations *before* these sections are
processed. Or more precisely, the special section data structures
entries which directly or indirectly involve livepatch relocations.
Those need to be extracted into "klp.arch" sections, a corresponding
rela section generated, and the kernel needs supporting code to handle
deferred processing.
Note that there is already x86-arch code present for handling
.altinstruction and .parainstructions sections in
arch/x86/kernel/livepatch.c.
- Support for multiple homonym symbols with unique
values. Consider a target module with symbol table that
looks like:
29: ... FILELOCAL DEFAULT ABS test_klp_convert_mod_a.c
32: ... FUNCLOCAL DEFAULT3 get_homonym_string
33: ... OBJECT LOCAL DEFAULT5 homonym_string
37: ... FILELOCAL DEFAULT ABS test_klp_convert_mod_b.c
38: ... FUNCLOCAL DEFAULT3 get_homonym_string
39: ... OBJECT LOCAL DEFAULT 11 homonym_string
- The BFD library is incapable of handling two rela sections with
identical sh_info values *as relocation sections*. This affects
binutils and related programs like gdb, objdump, crash utility, etc.
which fail to process klp-converted .ko
[PATCH v4 06/10] modpost: Add modinfo flag to livepatch modules
From: Miroslav Benes
Currently, livepatch infrastructure in the kernel relies on
MODULE_INFO(livepatch, "Y") statement in a livepatch module. Then the
kernel module loader knows a module is indeed livepatch module and can
behave accordingly.
klp-convert, on the other hand relies on LIVEPATCH_* statement in the
module's Makefile for exactly the same reason.
Remove dependency on modinfo and generate MODULE_INFO flag
automatically in modpost when LIVEPATCH_* is defined in the module's
Makefile. Generate a list of all built livepatch modules based on
the .livepatch file and store it in (MODVERDIR)/livepatchmods. Give
this list as an argument for modpost which will use it to identify
livepatch modules.
As MODULE_INFO is no longer needed, remove it.
Signed-off-by: Miroslav Benes
Signed-off-by: Joao Moreira
Signed-off-by: Joe Lawrence
---
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_livepatch.c | 1 -
samples/livepatch/livepatch-callbacks-demo.c | 1 -
samples/livepatch/livepatch-sample.c | 1 -
samples/livepatch/livepatch-shadow-fix1.c| 1 -
samples/livepatch/livepatch-shadow-fix2.c| 1 -
scripts/Makefile.modpost | 8 +-
scripts/mod/modpost.c| 84 ++--
10 files changed, 85 insertions(+), 15 deletions(-)
diff --git a/lib/livepatch/test_klp_atomic_replace.c
b/lib/livepatch/test_klp_atomic_replace.c
index 5af7093ca00c..3bf08a1b7b12 100644
--- a/lib/livepatch/test_klp_atomic_replace.c
+++ b/lib/livepatch/test_klp_atomic_replace.c
@@ -52,6 +52,5 @@ static void test_klp_atomic_replace_exit(void)
module_init(test_klp_atomic_replace_init);
module_exit(test_klp_atomic_replace_exit);
MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
MODULE_AUTHOR("Joe Lawrence ");
MODULE_DESCRIPTION("Livepatch test: atomic replace");
diff --git a/lib/livepatch/test_klp_callbacks_demo.c
b/lib/livepatch/test_klp_callbacks_demo.c
index 3fd8fe1cd1cc..76e2f51a6771 100644
--- a/lib/livepatch/test_klp_callbacks_demo.c
+++ b/lib/livepatch/test_klp_callbacks_demo.c
@@ -116,6 +116,5 @@ static void test_klp_callbacks_demo_exit(void)
module_init(test_klp_callbacks_demo_init);
module_exit(test_klp_callbacks_demo_exit);
MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
MODULE_AUTHOR("Joe Lawrence ");
MODULE_DESCRIPTION("Livepatch test: livepatch demo");
diff --git a/lib/livepatch/test_klp_callbacks_demo2.c
b/lib/livepatch/test_klp_callbacks_demo2.c
index 5417573e80af..76db98fc3071 100644
--- a/lib/livepatch/test_klp_callbacks_demo2.c
+++ b/lib/livepatch/test_klp_callbacks_demo2.c
@@ -88,6 +88,5 @@ static void test_klp_callbacks_demo2_exit(void)
module_init(test_klp_callbacks_demo2_init);
module_exit(test_klp_callbacks_demo2_exit);
MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
MODULE_AUTHOR("Joe Lawrence ");
MODULE_DESCRIPTION("Livepatch test: livepatch demo2");
diff --git a/lib/livepatch/test_klp_livepatch.c
b/lib/livepatch/test_klp_livepatch.c
index aff08199de71..d94d0ae232db 100644
--- a/lib/livepatch/test_klp_livepatch.c
+++ b/lib/livepatch/test_klp_livepatch.c
@@ -46,6 +46,5 @@ static void test_klp_livepatch_exit(void)
module_init(test_klp_livepatch_init);
module_exit(test_klp_livepatch_exit);
MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
MODULE_AUTHOR("Seth Jennings ");
MODULE_DESCRIPTION("Livepatch test: livepatch module");
diff --git a/samples/livepatch/livepatch-callbacks-demo.c
b/samples/livepatch/livepatch-callbacks-demo.c
index 62d97953ad02..e78249d4bff8 100644
--- a/samples/livepatch/livepatch-callbacks-demo.c
+++ b/samples/livepatch/livepatch-callbacks-demo.c
@@ -205,4 +205,3 @@ static void livepatch_callbacks_demo_exit(void)
module_init(livepatch_callbacks_demo_init);
module_exit(livepatch_callbacks_demo_exit);
MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
diff --git a/samples/livepatch/livepatch-sample.c
b/samples/livepatch/livepatch-sample.c
index 01c9cf003ca2..8900a175046b 100644
--- a/samples/livepatch/livepatch-sample.c
+++ b/samples/livepatch/livepatch-sample.c
@@ -79,4 +79,3 @@ static void livepatch_exit(void)
module_init(livepatch_init);
module_exit(livepatch_exit);
MODULE_LICENSE("GPL");
-MODULE_INFO(livepatch, "Y");
diff --git a/samples/livepatch/livepatch-shadow-fix1.c
b/samples/livepatch/livepatch-shadow-fix1.c
index 67a73e5e986e..c5bae813aaab 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -169,4 +169,3 @@ static void livepatch_shadow_fix1_exit(void)
module_init(livepatch_shadow_fix1_init);
module_exit(livepatch_shadow_fix1_exit);
MODULE_LICENSE("
