v3:
- livepatch.h
- drop obj->patched checks from pre/post-(un)patch funcs,
add preceding comment and note about obj->patched assumptions
- move core.c :: klp_is_module() to here
- klp_complete_transition()
- fix "else if (klp_target_state == KLP_UNPATCHED)" case
- combine conditional syntax when avoiding module_put for immediate
patches
- add check for klp_is_object_loaded to avoid callbacks for any
unloaded modules (necessary after removing obj->patched checks in
livepatch.h)
- Documentation
- added Josh's use-cases blurb in intro
- s/Callbacks are only executed/A callbacks is only executed/
- livepatch-callbacks-demo.c
- whitespace cleanup
I also wrote a quick test script (see below) to exercise some of the
load/unload/enable/disable/error status combinations. I'm not sure
about some of the behaviors, most notably test6 with regard to
post-unpatch-callbacks as executed on a cancelled transition. (See
results and comments further below.)
Also, maybe it's just my reading of the log, but would it be clearer if
the "(un)patching ... complete" messages indicated that they are
referring to a transaction? It's a bit confusing to see "unpatching ...
complete" before the pre-unpatch-callbacks ever execute. Not a big
deal, but I can send a follow up patch if others agree.
-- Joe
Test script
===========
MODULE=samples/livepatch/livepatch-callbacks-mod.ko
LIVEPATCH=samples/livepatch/livepatch-callbacks-demo.ko
DELAY=2s
function load_mod() {
local mod="$1"
shift
local args="$@"
echo "% insmod $mod $args" > /dev/kmsg
ret=$(insmod $mod $args 2>&1)
[[ "$ret" != "" ]] && echo "$ret" > /dev/kmsg
sleep $DELAY
}
function unload_mod() {
local mod="$1"
echo "% rmmod $mod" > /dev/kmsg
ret=$(rmmod $mod 2>&1)
[[ "$ret" != "" ]] && echo "$ret" > /dev/kmsg
sleep $DELAY
}
function disable_lp() {
echo "% echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled" > /dev/kmsg
echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
sleep $DELAY
}
function set_pre_patch_ret {
local ret="$1"
echo "% echo $1 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret" > /dev/kmsg
echo -19 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret
sleep $DELAY
}
###############################################################
dmesg -C
echo -- test0 - load target module, unload target module > /dev/kmsg
load_mod $MODULE
unload_mod $MODULE
dmesg > test0.out
###############################################################
dmesg -C
echo -- test1 - load target module, load livepatch, disable livepatch, unload target module, unload livepatch > /dev/kmsg
load_mod $MODULE
load_mod $LIVEPATCH
disable_lp
unload_mod $LIVEPATCH
unload_mod $MODULE
dmesg > test1.out
###############################################################
dmesg -C
echo -- test2 - load livepatch, load target module, disable livepatch, unload livepatch, unload target module > /dev/kmsg
load_mod $LIVEPATCH
load_mod $MODULE
disable_lp
unload_mod $LIVEPATCH
unload_mod $MODULE
dmesg > test2.out
###############################################################
dmesg -C
echo -- test3 - load target module, load livepatch, unload target module, disable livepatch, unload livepatch > /dev/kmsg
load_mod $MODULE
load_mod $LIVEPATCH
unload_mod $MODULE
disable_lp
unload_mod $LIVEPATCH
dmesg > test3.out
###############################################################
dmesg -C
echo -- test4 - load livepatch, load target module, unload target module, disable livepatch, unload livepatch > /dev/kmsg
load_mod $LIVEPATCH
load_mod $MODULE
unload_mod $MODULE
disable_lp
unload_mod $LIVEPATCH
dmesg > test4.out
###############################################################
dmesg -C
echo -- test5 - load livepatch, disable livepatch, unload livepatch > /dev/kmsg
load_mod $LIVEPATCH
disable_lp
unload_mod $LIVEPATCH
dmesg > test5.out
###############################################################
dmesg -C
echo -- test6 - load target module, load livepatch -ENODEV, unload target module > /dev/kmsg
load_mod $MODULE
load_mod $LIVEPATCH pre_patch_ret=-19
unload_mod $LIVEPATCH
unload_mod $MODULE
dmesg > test6.out
###############################################################
dmesg -C
echo -- test7 - load livepatch, setup -ENODEV, load target module, disable livepatch, unload livepatch > /dev/kmsg
load_mod $LIVEPATCH
set_pre_patch_ret -19
load_mod $MODULE
disable_lp
unload_mod $MODULE
unload_mod $LIVEPATCH
dmesg > test7.out
###############################################################
Results
=======
[ 34.504478] -- test0 - load target module, unload target module
[ 34.505137] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[ 34.552726] livepatch_callbacks_mod: module verification failed: signature and/or required key missing - tainting kernel
[ 34.554440] livepatch_callbacks_mod: livepatch_callbacks_mod_init
[ 36.573704] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[ 36.576533] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
A boring test, but as expected, no surprise callbacks were executed.
[ 38.588867] -- test1 - load target module, load livepatch, disable livepatch, unload target module, unload livepatch
[ 38.589910] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[ 38.592337] livepatch_callbacks_mod: livepatch_callbacks_mod_init
[ 40.594840] % insmod samples/livepatch/livepatch-callbacks-demo.ko
[ 40.661270] livepatch_callbacks_demo: tainting kernel with TAINT_LIVEPATCH
[ 40.662666] livepatch: enabling patch 'livepatch_callbacks_demo'
[ 40.663462] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[ 40.664262] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[ 40.665565] livepatch: 'livepatch_callbacks_demo': patching...
[ 41.695061] livepatch: 'livepatch_callbacks_demo': patching complete
[ 41.696024] livepatch_callbacks_demo: post_patch_callback: vmlinux
[ 41.696861] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[ 42.668712] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
[ 42.670354] livepatch: 'livepatch_callbacks_demo': unpatching...
[ 43.743103] livepatch: 'livepatch_callbacks_demo': unpatching complete
[ 43.743760] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
[ 43.744346] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[ 43.745327] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[ 43.745848] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[ 44.672951] % rmmod samples/livepatch/livepatch-callbacks-demo.ko
[ 46.686448] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[ 46.688921] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
Part 1: livepatch loads after the target module, patch callbacks execute for
both vmlinux and the target module.
Part 2: livepatch is disabled while the target module is still loaded, unpatch
callbacks execute for both vmlinux and target module.
[ 48.698388] -- test2 - load livepatch, load target module, disable livepatch, unload livepatch, unload target module
[ 48.699570] % insmod samples/livepatch/livepatch-callbacks-demo.ko
[ 48.702519] livepatch: enabling patch 'livepatch_callbacks_demo'
[ 48.703515] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[ 48.704139] livepatch: 'livepatch_callbacks_demo': patching...
[ 49.695048] livepatch: 'livepatch_callbacks_demo': patching complete
[ 49.695782] livepatch_callbacks_demo: post_patch_callback: vmlinux
[ 50.706813] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[ 50.709855] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
[ 50.710762] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[ 50.711895] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[ 50.713923] livepatch_callbacks_mod: livepatch_callbacks_mod_init
[ 52.716994] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
[ 52.717943] livepatch: 'livepatch_callbacks_demo': unpatching...
[ 53.727092] livepatch: 'livepatch_callbacks_demo': unpatching complete
[ 53.727726] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
[ 53.728307] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[ 53.729428] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[ 53.730002] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[ 54.720253] % rmmod samples/livepatch/livepatch-callbacks-demo.ko
[ 56.735960] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[ 56.738470] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
Part 1: livepatch loads before target module, so only vmlinux patch callbacks
execute. Once target module loads, its patch callbacks run.
Part 2: livepatch is disabled while the target module is still loaded, unpatch
callbacks execute for both vmlinux and target module.
[ 58.747842] -- test3 - load target module, load livepatch, unload target module, disable livepatch, unload livepatch
[ 58.748931] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[ 58.751625] livepatch_callbacks_mod: livepatch_callbacks_mod_init
[ 60.754340] % insmod samples/livepatch/livepatch-callbacks-demo.ko
[ 60.757824] livepatch: enabling patch 'livepatch_callbacks_demo'
[ 60.758466] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[ 60.758969] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[ 60.759923] livepatch: 'livepatch_callbacks_demo': patching...
[ 61.727106] livepatch: 'livepatch_callbacks_demo': patching complete
[ 61.728268] livepatch_callbacks_demo: post_patch_callback: vmlinux
[ 61.728802] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[ 62.762599] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[ 62.765086] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
[ 62.765925] livepatch: reverting patch 'livepatch_callbacks_demo' on unloading module 'livepatch_callbacks_mod'
[ 62.767207] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
[ 62.768179] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
[ 64.776099] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
[ 64.777078] livepatch: 'livepatch_callbacks_demo': unpatching...
[ 65.759068] livepatch: 'livepatch_callbacks_demo': unpatching complete
[ 65.759845] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
[ 65.760444] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[ 66.779280] % rmmod samples/livepatch/livepatch-callbacks-demo.ko
Part 1: livepatch loads after the target module, patch callbacks execute for
both vmlinux and the target module.
Part 2: target module is unloaded, so unpatch callbacks run for the
module. The livepatch is then disabled, vmlinux unpatch callbacks
execute.
[ 68.794346] -- test4 - load livepatch, load target module, unload target module, disable livepatch, unload livepatch
[ 68.795857] % insmod samples/livepatch/livepatch-callbacks-demo.ko
[ 68.799526] livepatch: enabling patch 'livepatch_callbacks_demo'
[ 68.800122] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[ 68.800631] livepatch: 'livepatch_callbacks_demo': patching...
[ 69.727057] livepatch: 'livepatch_callbacks_demo': patching complete
[ 69.727719] livepatch_callbacks_demo: post_patch_callback: vmlinux
[ 70.803162] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[ 70.805853] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
[ 70.806749] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[ 70.807806] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[ 70.809671] livepatch_callbacks_mod: livepatch_callbacks_mod_init
[ 72.812254] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[ 72.814795] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
[ 72.815639] livepatch: reverting patch 'livepatch_callbacks_demo' on unloading module 'livepatch_callbacks_mod'
[ 72.816561] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
[ 72.817615] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
[ 74.831463] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
[ 74.832358] livepatch: 'livepatch_callbacks_demo': unpatching...
[ 75.743119] livepatch: 'livepatch_callbacks_demo': unpatching complete
[ 75.743732] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
[ 75.744469] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[ 76.834520] % rmmod samples/livepatch/livepatch-callbacks-demo.ko
Part 1: livepatch loads before target module, so only vmlinux patch callbacks
execute. Once target module loads, its patch callbacks run.
Part 2: target module is unloaded, so unpatch callbacks run for the
module. The livepatch is then disabled, vmlinux unpatch callbacks
[ 78.851887] -- test5 - load livepatch, disable livepatch, unload livepatch
[ 78.852732] % insmod samples/livepatch/livepatch-callbacks-demo.ko
[ 78.855401] livepatch: enabling patch 'livepatch_callbacks_demo'
[ 78.855966] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[ 78.856799] livepatch: 'livepatch_callbacks_demo': patching...
[ 79.711079] livepatch: 'livepatch_callbacks_demo': patching complete
[ 79.711756] livepatch_callbacks_demo: post_patch_callback: vmlinux
[ 80.859474] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
[ 80.860441] livepatch: 'livepatch_callbacks_demo': unpatching...
[ 81.759137] livepatch: 'livepatch_callbacks_demo': unpatching complete
[ 81.760137] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
[ 81.760994] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[ 82.862596] % rmmod samples/livepatch/livepatch-callbacks-demo.ko
Part 1: livepatch is loaded (no target module), only vmlinux callbacks
run.
Part 2: livepatch is disabled (no target module), only vmlinux callbacks
run.
[ 84.878971] -- test6 - load target module, load livepatch -ENODEV, unload target module
[ 84.879755] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[ 84.882842] livepatch_callbacks_mod: livepatch_callbacks_mod_init
[ 86.885313] % insmod samples/livepatch/livepatch-callbacks-demo.ko pre_patch_ret=-19
[ 86.889259] livepatch: enabling patch 'livepatch_callbacks_demo'
[ 86.890160] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[ 86.890734] livepatch: pre-patch callback failed for object 'vmlinux'
[ 86.891306] livepatch: failed to enable patch 'livepatch_callbacks_demo'
[ 86.891931] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[ 86.892561] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[ 86.908817] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-demo.ko: No such device
[ 88.911655] % rmmod samples/livepatch/livepatch-callbacks-demo.ko
[ 88.914815] rmmod: ERROR: Module livepatch_callbacks_demo is not currently loaded
[ 90.917163] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[ 90.919997] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
Part 1: Livepatch is loaded after the target module, however the
vmlinux-pre-patch-callback returns -ENODEV, so the livepatch module
fails to load.
Note: both vmlinux and target module's post-unpatch-callbacks are
executed as part of the cancelled transition:
klp_enable_patch
__klp_enable_patch
klp_cancel_transition
klp_complete_transition
done:
...
else if (klp_target_state == KLP_UNPATCHED)
klp_post_unpatch_callback(obj);
[ 92.934851] -- test7 - load livepatch, setup -ENODEV, load target module, disable livepatch, unload livepatch
[ 92.935879] % insmod samples/livepatch/livepatch-callbacks-demo.ko
[ 92.938683] livepatch: enabling patch 'livepatch_callbacks_demo'
[ 92.939294] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[ 92.939823] livepatch: 'livepatch_callbacks_demo': patching...
[ 93.727126] livepatch: 'livepatch_callbacks_demo': patching complete
[ 93.727809] livepatch_callbacks_demo: post_patch_callback: vmlinux
[ 94.942396] % echo -19 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret
[ 96.944893] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[ 96.947557] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
[ 96.948816] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[ 96.950416] livepatch: pre-patch callback failed for object 'livepatch_callbacks_mod'
[ 96.951424] livepatch: patch 'livepatch_callbacks_demo' failed for module 'livepatch_callbacks_mod', refusing to load module 'livepatch_callbacks_mod'
[ 96.966586] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device
[ 98.968923] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
[ 98.970179] livepatch: 'livepatch_callbacks_demo': unpatching...
[ 100.703101] livepatch: 'livepatch_callbacks_demo': unpatching complete
[ 100.704057] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
[ 100.704898] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[ 100.972541] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[ 100.975462] rmmod: ERROR: Module livepatch_callbacks_mod is not currently loaded
[ 102.977392] % rmmod samples/livepatch/livepatch-callbacks-demo.ko
Part 1: Livepatch is loaded first, so vmlinux callbacks run
Part 2: The livepatch's pre-patch-callback is setup to now return
-ENODEV
Part 3: When a targetted module is loaded, the pre-patch-callback
returns -ENODEV and the target module fails to load.
Part 4: The livepatch is disabled and only the vmlinux unpatch-callbacks
are executed.
Note: this test should be consistent with the other test which fails a
pre-patch-callback status... ie, the post-unpatch-callback behavior for
said klp_object should be the same.
--
Joe Lawrence (1):
livepatch: add (un)patch callbacks
Documentation/livepatch/callbacks.txt | 87 ++++++++++++
include/linux/livepatch.h | 81 ++++++++++++
kernel/livepatch/core.c | 37 ++++--
kernel/livepatch/patch.c | 5 +-
kernel/livepatch/transition.c | 21 ++-
samples/livepatch/Makefile | 2 +
samples/livepatch/livepatch-callbacks-demo.c | 190 +++++++++++++++++++++++++++
samples/livepatch/livepatch-callbacks-mod.c | 53 ++++++++
8 files changed, 462 insertions(+), 14 deletions(-)
create mode 100644 Documentation/livepatch/callbacks.txt
create mode 100644 samples/livepatch/livepatch-callbacks-demo.c
create mode 100644 samples/livepatch/livepatch-callbacks-mod.c
--
1.8.3.1
Provide livepatch modules a klp_object (un)patching notification
mechanism. Pre and post-(un)patch callbacks allow livepatch modules to
setup or synchronize changes that would be difficult to support in only
patched-or-unpatched code contexts.
Callbacks can be registered for target module or vmlinux klp_objects,
but each implementation is klp_object specific.
- Pre-(un)patch callbacks run before any (un)patching action takes
place.
- Post-(un)patch callbacks run once an object has been (un)patched and
the klp_patch fully transitioned to its target state.
Example use cases include modification of global data and registration
of newly available services/handlers.
See Documentation/livepatch/callback.txt for details.
Signed-off-by: Joe Lawrence <[email protected]>
---
Documentation/livepatch/callbacks.txt | 87 ++++++++++++
include/linux/livepatch.h | 81 ++++++++++++
kernel/livepatch/core.c | 37 ++++--
kernel/livepatch/patch.c | 5 +-
kernel/livepatch/transition.c | 21 ++-
samples/livepatch/Makefile | 2 +
samples/livepatch/livepatch-callbacks-demo.c | 190 +++++++++++++++++++++++++++
samples/livepatch/livepatch-callbacks-mod.c | 53 ++++++++
8 files changed, 462 insertions(+), 14 deletions(-)
create mode 100644 Documentation/livepatch/callbacks.txt
create mode 100644 samples/livepatch/livepatch-callbacks-demo.c
create mode 100644 samples/livepatch/livepatch-callbacks-mod.c
diff --git a/Documentation/livepatch/callbacks.txt b/Documentation/livepatch/callbacks.txt
new file mode 100644
index 000000000000..e18f2678f3bb
--- /dev/null
+++ b/Documentation/livepatch/callbacks.txt
@@ -0,0 +1,87 @@
+(Un)patching Callbacks
+======================
+
+Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
+to execute callback functions when a kernel object is (un)patched. They
+can be considered a "power feature" that extends livepatching abilities
+to include:
+
+ - Safe updates to global data
+
+ - "Patches" to init and probe functions
+
+ - Patching otherwise unpatchable code (i.e. assembly)
+
+In most cases, (un)patch hooks will need to be used in conjunction with
+memory barriers and kernel synchronization primitives, like
+mutexes/spinlocks, or even stop_machine(), to avoid concurrency issues.
+
+Callbacks differ from existing kernel facilities:
+
+ - Module init/exit code doesn't run when disabling and re-enabling a
+ patch.
+
+ - A module notifier can't stop the to-be-patched module from loading.
+
+Callbacks are part of the klp_object structure and their implementation
+is specific to the given object. Other livepatch objects may or may not
+be patched, irrespective of the target klp_object's current state.
+
+Callbacks can be registered for the following livepatch actions:
+
+ * Pre-patch - before klp_object is patched
+
+ * Post-patch - after klp_object has been patched and is active
+ across all tasks
+
+ * Pre-unpatch - before klp_object is unpatched, patched code is active
+
+ * Post-unpatch - after klp_object has been patched, all code has been
+ restored and no tasks are running patched code
+
+A callbacks is only executed if its host klp_object is loaded. For
+in-kernel vmlinux targets, this means that callbacks will always execute
+when a livepatch is enabled/disabled.
+
+For kernel module targets, callbacks will only execute if the target
+module is loaded. When a kernel module target is (un)loaded, its
+callbacks will execute only if the livepatch module is enabled.
+
+The pre-patch callback is expected to return a status code (0 for
+success, -ERRNO on error). An error status code will indicate to the
+livepatching core that patching of the current klp_object is not safe
+and to stop the current patching request. If the problematic klp_object
+is already loaded (i.e. a livepatch is loaded after target code), the
+kernel's module loader will refuse to load the livepatch. On the other
+hand, if the problematic klp_object is already in place (i.e. a target
+module is loaded after a livepatch), then the module loader will refuse
+to load the target kernel module.
+
+
+Example Use-cases
+-----------------
+
+1 - Update global data
+
+A pre-patch callback can be useful to update a global variable. For
+example, 75ff39ccc1bd ("tcp: make challenge acks less predictable")
+changes a global sysctl, as well as patches the tcp_send_challenge_ack()
+function.
+
+In this case, if we're being super paranoid, it might make sense to
+patch the data *after* patching is complete with a post-patch callback,
+so that tcp_send_challenge_ack() could first be changed to read
+sysctl_tcp_challenge_ack_limit with READ_ONCE.
+
+
+2 - Support __init and probe function patches
+
+Although __init and probe functions are not directly livepatch-able, it
+may be possible to implement similar updates via pre/post-patch
+callbacks.
+
+48900cb6af42 ("virtio-net: drop NETIF_F_FRAGLIST") change the way that
+virtnet_probe() initialized its driver's net_device features. A
+pre/post-patch callback could iterate over all such devices, making a
+similar change to their hw_features value. (Client functions of the
+value may need to be updated accordingly.)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991ef9347..500dc9b2b361 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -87,10 +87,25 @@ struct klp_func {
bool transition;
};
+struct klp_object;
+
+/**
+ * struct klp_callback - callback structure for live patching
+ * @callback: function to be executed on callback
+ *
+ */
+struct klp_callbacks {
+ int (*pre_patch)(struct klp_object *obj);
+ void (*post_patch)(struct klp_object *obj);
+ void (*pre_unpatch)(struct klp_object *obj);
+ void (*post_unpatch)(struct klp_object *obj);
+};
+
/**
* struct klp_object - kernel object structure for live patching
* @name: module name (or NULL for vmlinux)
* @funcs: function entries for functions to be patched in the object
+ * @callbacks: functions to be executed pre/post (un)patching
* @kobj: kobject for sysfs resources
* @mod: kernel module associated with the patched object
* (NULL for vmlinux)
@@ -100,6 +115,7 @@ struct klp_object {
/* external */
const char *name;
struct klp_func *funcs;
+ struct klp_callbacks callbacks;
/* internal */
struct kobject kobj;
@@ -138,6 +154,71 @@ struct klp_patch {
func->old_name || func->new_func || func->old_sympos; \
func++)
+/**
+ * klp_is_object_loaded() - is klp_object currently loaded?
+ * @obj: klp_object pointer
+ *
+ * Return: true if klp_object is loaded (always true for vmlinux)
+ */
+static inline bool klp_is_object_loaded(struct klp_object *obj)
+{
+ return !obj->name || obj->mod;
+}
+
+/**
+ * klp_pre_patch_callback - execute before klp_object is patched
+ * @obj: invoke callback for this klp_object
+ *
+ * Return: status from callback
+ *
+ * Callers should ensure obj->patched is *not* set.
+ */
+static inline int klp_pre_patch_callback(struct klp_object *obj)
+{
+ if (obj->callbacks.pre_patch)
+ return (*obj->callbacks.pre_patch)(obj);
+ return 0;
+}
+
+/**
+ * klp_post_patch_callback() - execute after klp_object is patched
+ * @obj: invoke callback for this klp_object
+ *
+ * Callers should ensure obj->patched is set.
+ */
+static inline void klp_post_patch_callback(struct klp_object *obj)
+{
+ if (obj->callbacks.post_patch)
+ (*obj->callbacks.post_patch)(obj);
+}
+
+/**
+ * klp_pre_unpatch_callback() - execute before klp_object is unpatched
+ * and is active across all tasks
+ * @obj: invoke callback for this klp_object
+ *
+ * Callers should ensure obj->patched is set.
+ */
+static inline void klp_pre_unpatch_callback(struct klp_object *obj)
+{
+ if (obj->callbacks.pre_unpatch)
+ (*obj->callbacks.pre_unpatch)(obj);
+}
+
+/**
+ * klp_post_unpatch_callback() - execute after klp_object is unpatched,
+ * all code has been restored and no tasks
+ * are running patched code
+ * @obj: invoke callback for this klp_object
+ *
+ * Callers should ensure obj->patched is *not* set.
+ */
+static inline void klp_post_unpatch_callback(struct klp_object *obj)
+{
+ if (obj->callbacks.post_unpatch)
+ (*obj->callbacks.post_unpatch)(obj);
+}
+
int klp_register_patch(struct klp_patch *);
int klp_unregister_patch(struct klp_patch *);
int klp_enable_patch(struct klp_patch *);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b9628e43c78f..ddb23e18a357 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -54,11 +54,6 @@ static bool klp_is_module(struct klp_object *obj)
return obj->name;
}
-static bool klp_is_object_loaded(struct klp_object *obj)
-{
- return !obj->name || obj->mod;
-}
-
/* sets obj->mod if object is not vmlinux and module is found */
static void klp_find_object_module(struct klp_object *obj)
{
@@ -388,13 +383,18 @@ static int __klp_enable_patch(struct klp_patch *patch)
if (!klp_is_object_loaded(obj))
continue;
- ret = klp_patch_object(obj);
+ ret = klp_pre_patch_callback(obj);
if (ret) {
- pr_warn("failed to enable patch '%s'\n",
- patch->mod->name);
+ pr_warn("pre-patch callback failed for object '%s'\n",
+ klp_is_module(obj) ? obj->name : "vmlinux");
+ goto err;
+ }
- klp_cancel_transition();
- return ret;
+ ret = klp_patch_object(obj);
+ if (ret) {
+ pr_warn("failed to patch object '%s'\n",
+ klp_is_module(obj) ? obj->name : "vmlinux");
+ goto err;
}
}
@@ -403,6 +403,11 @@ static int __klp_enable_patch(struct klp_patch *patch)
patch->enabled = true;
return 0;
+err:
+ pr_warn("failed to enable patch '%s'\n", patch->mod->name);
+
+ klp_cancel_transition();
+ return ret;
}
/**
@@ -871,6 +876,13 @@ int klp_module_coming(struct module *mod)
pr_notice("applying patch '%s' to loading module '%s'\n",
patch->mod->name, obj->mod->name);
+ ret = klp_pre_patch_callback(obj);
+ if (ret) {
+ pr_warn("pre-patch callback failed for object '%s'\n",
+ obj->name);
+ goto err;
+ }
+
ret = klp_patch_object(obj);
if (ret) {
pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
@@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
goto err;
}
+ klp_post_patch_callback(obj);
+
break;
}
}
@@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
if (patch->enabled || patch == klp_transition_patch) {
pr_notice("reverting patch '%s' on unloading module '%s'\n",
patch->mod->name, obj->mod->name);
+
+ klp_pre_unpatch_callback(obj);
klp_unpatch_object(obj);
+ klp_post_unpatch_callback(obj);
}
klp_free_object_loaded(obj);
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 52c4e907c14b..0eed0df6e6d9 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -257,6 +257,7 @@ int klp_patch_object(struct klp_object *obj)
klp_for_each_func(obj, func) {
ret = klp_patch_func(func);
if (ret) {
+ klp_pre_unpatch_callback(obj);
klp_unpatch_object(obj);
return ret;
}
@@ -271,6 +272,8 @@ void klp_unpatch_objects(struct klp_patch *patch)
struct klp_object *obj;
klp_for_each_object(patch, obj)
- if (obj->patched)
+ if (obj->patched) {
+ klp_pre_unpatch_callback(obj);
klp_unpatch_object(obj);
+ }
}
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index b004a1fb6032..7bf55b7f3687 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -109,9 +109,6 @@ static void klp_complete_transition(void)
}
}
- if (klp_target_state == KLP_UNPATCHED && !immediate_func)
- module_put(klp_transition_patch->mod);
-
/* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
if (klp_target_state == KLP_PATCHED)
klp_synchronize_transition();
@@ -130,6 +127,24 @@ static void klp_complete_transition(void)
}
done:
+ klp_for_each_object(klp_transition_patch, obj) {
+ if (!klp_is_object_loaded(obj))
+ continue;
+ if (klp_target_state == KLP_PATCHED)
+ klp_post_patch_callback(obj);
+ else if (klp_target_state == KLP_UNPATCHED)
+ klp_post_unpatch_callback(obj);
+ }
+
+ /*
+ * See complementary comment in __klp_enable_patch() for why we
+ * keep the module reference for immediate patches.
+ */
+ if (!klp_transition_patch->immediate && !immediate_func &&
+ klp_target_state == KLP_UNPATCHED) {
+ module_put(klp_transition_patch->mod);
+ }
+
klp_target_state = KLP_UNDEFINED;
klp_transition_patch = NULL;
}
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
index 10319d7ea0b1..f8e545af5de4 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -1 +1,3 @@
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-demo.o
+obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-callbacks-mod.o
diff --git a/samples/livepatch/livepatch-callbacks-demo.c b/samples/livepatch/livepatch-callbacks-demo.c
new file mode 100644
index 000000000000..a5ae7211f24a
--- /dev/null
+++ b/samples/livepatch/livepatch-callbacks-demo.c
@@ -0,0 +1,190 @@
+/*
+ * Copyright (C) 2017 Joe Lawrence <[email protected]>
+ *
+ * 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/>.
+ */
+
+/*
+ * livepatch-callbacks-demo.c - (un)patching callbacks livepatch demo
+ *
+ *
+ * Purpose
+ * -------
+ *
+ * Demonstration of registering livepatch (un)patching callbacks.
+ *
+ *
+ * Usage
+ * -----
+ *
+ * Step 1 - load the simple module
+ *
+ * insmod samples/livepatch/livepatch-callbacks-mod.ko
+ *
+ *
+ * Step 2 - load the demonstration livepatch (with callbacks)
+ *
+ * insmod samples/livepatch/livepatch-callbacks-demo.ko
+ *
+ *
+ * Step 3 - cleanup
+ *
+ * echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+ * rmmod livepatch_callbacks_demo
+ * rmmod livepatch_callbacks_mod
+ *
+ * Watch dmesg output to see livepatch enablement, callback execution
+ * and patching operations for both vmlinux and module targets.
+ *
+ * NOTE: swap the insmod order of livepatch-callbacks-mod.ko and
+ * livepatch-callbacks-demo.ko to observe what happens when a
+ * target module is loaded after a livepatch with callbacks.
+ *
+ * NOTE: 'pre_patch_ret' is a module parameter that sets the pre-patch
+ * callback return status. Try setting up a non-zero status
+ * such as -19 (-ENODEV):
+ *
+ * # Load demo livepatch, vmlinux is patched
+ * insmod samples/livepatch/livepatch-callbacks-demo.ko
+ *
+ * # Setup next pre-patch callback to return -ENODEV
+ * echo -19 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret
+ *
+ * # Module loader refuses to load the target module
+ * insmod samples/livepatch/livepatch-callbacks-mod.ko
+ * insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+#include <linux/stat.h>
+
+static int pre_patch_ret;
+module_param(pre_patch_ret, int, 0644);
+MODULE_PARM_DESC(pre_patch_ret, "pre_patch_ret (default=0)");
+
+const char *module_state[] = {
+ [MODULE_STATE_LIVE] = "[MODULE_STATE_LIVE] Normal state",
+ [MODULE_STATE_COMING] = "[MODULE_STATE_COMING] Full formed, running module_init",
+ [MODULE_STATE_GOING] = "[MODULE_STATE_GOING] Going away",
+ [MODULE_STATE_UNFORMED] = "[MODULE_STATE_UNFORMED] Still setting it up",
+};
+
+static void callback_info(const char *callback, struct klp_object *obj)
+{
+ if (obj->mod)
+ pr_info("%s: %s -> %s\n", callback, obj->mod->name,
+ module_state[obj->mod->state]);
+ else
+ pr_info("%s: vmlinux\n", callback);
+}
+
+/* Executed on object patching (ie, patch enablement) */
+static int pre_patch_callback(struct klp_object *obj)
+{
+ callback_info(__func__, obj);
+ return pre_patch_ret;
+}
+
+/* Executed on object unpatching (ie, patch disablement) */
+static void post_patch_callback(struct klp_object *obj)
+{
+ callback_info(__func__, obj);
+}
+
+/* Executed on object unpatching (ie, patch disablement) */
+static void pre_unpatch_callback(struct klp_object *obj)
+{
+ callback_info(__func__, obj);
+}
+
+/* Executed on object unpatching (ie, patch disablement) */
+static void post_unpatch_callback(struct klp_object *obj)
+{
+ callback_info(__func__, obj);
+}
+
+static struct klp_func funcs[] = {
+ { }
+};
+
+static struct klp_object objs[] = {
+ {
+ .name = NULL, /* vmlinux */
+ .funcs = funcs,
+ .callbacks = {
+ .pre_patch = pre_patch_callback,
+ .post_patch = post_patch_callback,
+ .pre_unpatch = pre_unpatch_callback,
+ .post_unpatch = post_unpatch_callback,
+ },
+ }, {
+ .name = "livepatch_callbacks_mod",
+ .funcs = funcs,
+ .callbacks = {
+ .pre_patch = pre_patch_callback,
+ .post_patch = post_patch_callback,
+ .pre_unpatch = pre_unpatch_callback,
+ .post_unpatch = post_unpatch_callback,
+ },
+ }, { }
+};
+
+static struct klp_patch patch = {
+ .mod = THIS_MODULE,
+ .objs = objs,
+};
+
+static int livepatch_callbacks_demo_init(void)
+{
+ int ret;
+
+ if (!klp_have_reliable_stack() && !patch.immediate) {
+ /*
+ * WARNING: Be very careful when using 'patch.immediate' in
+ * your patches. It's ok to use it for simple patches like
+ * this, but for more complex patches which change function
+ * semantics, locking semantics, or data structures, it may not
+ * be safe. Use of this option will also prevent removal of
+ * the patch.
+ *
+ * See Documentation/livepatch/livepatch.txt for more details.
+ */
+ patch.immediate = true;
+ pr_notice("The consistency model isn't supported for your architecture. Bypassing safety mechanisms and applying the patch immediately.\n");
+ }
+
+ ret = klp_register_patch(&patch);
+ if (ret)
+ return ret;
+ ret = klp_enable_patch(&patch);
+ if (ret) {
+ WARN_ON(klp_unregister_patch(&patch));
+ return ret;
+ }
+ return 0;
+}
+
+static void livepatch_callbacks_demo_exit(void)
+{
+ WARN_ON(klp_unregister_patch(&patch));
+}
+
+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-callbacks-mod.c b/samples/livepatch/livepatch-callbacks-mod.c
new file mode 100644
index 000000000000..d0f8657e19f3
--- /dev/null
+++ b/samples/livepatch/livepatch-callbacks-mod.c
@@ -0,0 +1,53 @@
+/*
+ * Copyright (C) 2017 Joe Lawrence <[email protected]>
+ *
+ * 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/>.
+ */
+
+/*
+ * livepatch-callbacks-mod.c - (un)patching callbacks demo support module
+ *
+ *
+ * Purpose
+ * -------
+ *
+ * Simple module to demonstrate livepatch (un)patching callbacks.
+ *
+ *
+ * Usage
+ * -----
+ *
+ * This module is not intended to be standalone. See the "Usage"
+ * section of livepatch-callbacks-mod.c.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+
+static int livepatch_callbacks_mod_init(void)
+{
+ pr_info("%s\n", __func__);
+ return 0;
+}
+
+static void livepatch_callbacks_mod_exit(void)
+{
+ pr_info("%s\n", __func__);
+}
+
+module_init(livepatch_callbacks_mod_init);
+module_exit(livepatch_callbacks_mod_exit);
+MODULE_LICENSE("GPL");
--
1.8.3.1
On Wed, Aug 16, 2017 at 03:17:03PM -0400, Joe Lawrence wrote:
> v3:
>
> - livepatch.h
> - drop obj->patched checks from pre/post-(un)patch funcs,
> add preceding comment and note about obj->patched assumptions
> - move core.c :: klp_is_module() to here
>
> - klp_complete_transition()
> - fix "else if (klp_target_state == KLP_UNPATCHED)" case
> - combine conditional syntax when avoiding module_put for immediate
> patches
> - add check for klp_is_object_loaded to avoid callbacks for any
> unloaded modules (necessary after removing obj->patched checks in
> livepatch.h)
>
> - Documentation
> - added Josh's use-cases blurb in intro
> - s/Callbacks are only executed/A callbacks is only executed/
>
> - livepatch-callbacks-demo.c
> - whitespace cleanup
>
> I also wrote a quick test script (see below) to exercise some of the
> load/unload/enable/disable/error status combinations. I'm not sure
> about some of the behaviors, most notably test6 with regard to
> post-unpatch-callbacks as executed on a cancelled transition. (See
> results and comments further below.)
Yeah, that doesn't seem right. Maybe in case of a pre-patch callback
error, we should only call post-unpatch callbacks for those objects
whose pre-patch callbacks were successfully called (and returned zero).
That would mean tracking on a per-object basis which objects had their
pre-patch callbacks called (successfully).
That would give the patch module a post-unpatch chance to tear down
anything it had set up in the pre-patch callback.
And the behavior should be documented.
> Also, maybe it's just my reading of the log, but would it be clearer if
> the "(un)patching ... complete" messages indicated that they are
> referring to a transaction? It's a bit confusing to see "unpatching ...
> complete" before the pre-unpatch-callbacks ever execute. Not a big
> deal, but I can send a follow up patch if others agree.
Hm. I'm thinking this highlights the fact that the pre-unpatch callback
is being called in the wrong place. It should actually be called before
the unpatching transition starts. When called from
klp_unpatch_objects(), the new code is no longer running, so it's
effectively post-patch instead of pre-patch.
Another random thought: maybe we should show the "patching complete"
message *after* the post-patch callback is run. That would be more
honest with the user, as technically, the post-patch callback is part of
the patching process.
And a similar comment for the "unpatching complete" message.
--
Josh
On Wed, Aug 16, 2017 at 03:17:04PM -0400, Joe Lawrence wrote:
> Provide livepatch modules a klp_object (un)patching notification
> mechanism. Pre and post-(un)patch callbacks allow livepatch modules to
> setup or synchronize changes that would be difficult to support in only
> patched-or-unpatched code contexts.
>
> Callbacks can be registered for target module or vmlinux klp_objects,
> but each implementation is klp_object specific.
>
> - Pre-(un)patch callbacks run before any (un)patching action takes
> place.
>
> - Post-(un)patch callbacks run once an object has been (un)patched and
> the klp_patch fully transitioned to its target state.
>
> Example use cases include modification of global data and registration
> of newly available services/handlers.
>
> See Documentation/livepatch/callback.txt for details.
>
> Signed-off-by: Joe Lawrence <[email protected]>
> ---
> Documentation/livepatch/callbacks.txt | 87 ++++++++++++
> include/linux/livepatch.h | 81 ++++++++++++
> kernel/livepatch/core.c | 37 ++++--
> kernel/livepatch/patch.c | 5 +-
> kernel/livepatch/transition.c | 21 ++-
> samples/livepatch/Makefile | 2 +
> samples/livepatch/livepatch-callbacks-demo.c | 190 +++++++++++++++++++++++++++
> samples/livepatch/livepatch-callbacks-mod.c | 53 ++++++++
> 8 files changed, 462 insertions(+), 14 deletions(-)
> create mode 100644 Documentation/livepatch/callbacks.txt
> create mode 100644 samples/livepatch/livepatch-callbacks-demo.c
> create mode 100644 samples/livepatch/livepatch-callbacks-mod.c
>
> diff --git a/Documentation/livepatch/callbacks.txt b/Documentation/livepatch/callbacks.txt
> new file mode 100644
> index 000000000000..e18f2678f3bb
> --- /dev/null
> +++ b/Documentation/livepatch/callbacks.txt
> @@ -0,0 +1,87 @@
> +(Un)patching Callbacks
> +======================
> +
> +Livepatch (un)patch-callbacks provide a mechanism for livepatch modules
> +to execute callback functions when a kernel object is (un)patched. They
> +can be considered a "power feature" that extends livepatching abilities
> +to include:
> +
> + - Safe updates to global data
> +
> + - "Patches" to init and probe functions
> +
> + - Patching otherwise unpatchable code (i.e. assembly)
> +
> +In most cases, (un)patch hooks will need to be used in conjunction with
s/hooks/callbacks/
(though I wouldn't be opposed to changing them back to "hooks" anyway as
it's one fewer syllable :-) but no strong opinion either way on that)
--
Josh
On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> Provide livepatch modules a klp_object (un)patching notification
> mechanism. Pre and post-(un)patch callbacks allow livepatch modules to
> setup or synchronize changes that would be difficult to support in only
> patched-or-unpatched code contexts.
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 194991ef9347..500dc9b2b361 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -138,6 +154,71 @@ struct klp_patch {
> func->old_name || func->new_func || func->old_sympos; \
> func++)
>
> +/**
> + * klp_is_object_loaded() - is klp_object currently loaded?
> + * @obj: klp_object pointer
> + *
> + * Return: true if klp_object is loaded (always true for vmlinux)
> + */
> +static inline bool klp_is_object_loaded(struct klp_object *obj)
> +{
> + return !obj->name || obj->mod;
> +}
> +
> +/**
> + * klp_pre_patch_callback - execute before klp_object is patched
> + * @obj: invoke callback for this klp_object
> + *
> + * Return: status from callback
> + *
> + * Callers should ensure obj->patched is *not* set.
> + */
> +static inline int klp_pre_patch_callback(struct klp_object *obj)
> +{
> + if (obj->callbacks.pre_patch)
> + return (*obj->callbacks.pre_patch)(obj);
> + return 0;
> +}
> +
> +/**
> + * klp_post_patch_callback() - execute after klp_object is patched
> + * @obj: invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is set.
> + */
> +static inline void klp_post_patch_callback(struct klp_object *obj)
> +{
> + if (obj->callbacks.post_patch)
> + (*obj->callbacks.post_patch)(obj);
> +}
> +
> +/**
> + * klp_pre_unpatch_callback() - execute before klp_object is unpatched
> + * and is active across all tasks
> + * @obj: invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is set.
> + */
> +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> +{
> + if (obj->callbacks.pre_unpatch)
> + (*obj->callbacks.pre_unpatch)(obj);
> +}
> +
> +/**
> + * klp_post_unpatch_callback() - execute after klp_object is unpatched,
> + * all code has been restored and no tasks
> + * are running patched code
> + * @obj: invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is *not* set.
> + */
> +static inline void klp_post_unpatch_callback(struct klp_object *obj)
> +{
> + if (obj->callbacks.post_unpatch)
> + (*obj->callbacks.post_unpatch)(obj);
> +}
I guess that we do not want to make these function usable
outside livepatch code. Thefore these inliners should go
to kernel/livepatch/core.h or so.
> +
> int klp_register_patch(struct klp_patch *);
> int klp_unregister_patch(struct klp_patch *);
> int klp_enable_patch(struct klp_patch *);
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..ddb23e18a357 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
> goto err;
> }
>
> + klp_post_patch_callback(obj);
This should be called only if (patch != klp_transition_patch).
Otherwise, it would be called too early.
> +
> break;
> }
> }
> @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
> if (patch->enabled || patch == klp_transition_patch) {
> pr_notice("reverting patch '%s' on unloading module '%s'\n",
> patch->mod->name, obj->mod->name);
> +
> + klp_pre_unpatch_callback(obj);
Also the pre_unpatch() callback should be called only
if (patch != klp_transition_patch). Otherwise, it should have
already been called. It is not the current case but see below.
> klp_unpatch_object(obj);
> + klp_post_unpatch_callback(obj);
> }
>
> klp_free_object_loaded(obj);
> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> index 52c4e907c14b..0eed0df6e6d9 100644
> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -257,6 +257,7 @@ int klp_patch_object(struct klp_object *obj)
> klp_for_each_func(obj, func) {
> ret = klp_patch_func(func);
> if (ret) {
> + klp_pre_unpatch_callback(obj);
This looks strange (somehow asymetric). IMHO, it should not be
needed. klp_pre_unpatch_callback() should revert changes done
by klp_post_patch_callback() that has not run yet.
> klp_unpatch_object(obj);
> return ret;
> }
> @@ -271,6 +272,8 @@ void klp_unpatch_objects(struct klp_patch *patch)
> struct klp_object *obj;
>
> klp_for_each_object(patch, obj)
> - if (obj->patched)
> + if (obj->patched) {
> + klp_pre_unpatch_callback(obj);
This is even more strange. The corresponding klp_post_patch_callback()
is called at the very end of the transaction when the patch is already
used by the entire system. Therefore the operation should be reverted
before we start disabling the patch.
IMHO, klp_pre_unpatch_callback() should get called in
__klp_disable_patch(). I would put it right after
klp_init_transition(patch, KLP_UNPATCHED);
Another advantage of this logic is that we actually do not need
to care about these callbacks in klp_reverse_transition().
But we should probably mention it in the documentation
how the actions done by the patch and unpatch callbacks
are related.
Otherwise, it looks fine to me.
Best Regards,
Petr
On Wed 2017-08-16 15:20:32, Josh Poimboeuf wrote:
> On Wed, Aug 16, 2017 at 03:17:03PM -0400, Joe Lawrence wrote:
> > I also wrote a quick test script (see below) to exercise some of the
> > load/unload/enable/disable/error status combinations. I'm not sure
> > about some of the behaviors, most notably test6 with regard to
> > post-unpatch-callbacks as executed on a cancelled transition. (See
> > results and comments further below.)
>
> Yeah, that doesn't seem right. Maybe in case of a pre-patch callback
> error, we should only call post-unpatch callbacks for those objects
> whose pre-patch callbacks were successfully called (and returned zero).
> That would mean tracking on a per-object basis which objects had their
> pre-patch callbacks called (successfully).
>
> That would give the patch module a post-unpatch chance to tear down
> anything it had set up in the pre-patch callback.
>
> And the behavior should be documented.
All this makes sense.
> > Also, maybe it's just my reading of the log, but would it be clearer if
> > the "(un)patching ... complete" messages indicated that they are
> > referring to a transaction? It's a bit confusing to see "unpatching ...
> > complete" before the pre-unpatch-callbacks ever execute. Not a big
> > deal, but I can send a follow up patch if others agree.
>
> Hm. I'm thinking this highlights the fact that the pre-unpatch callback
> is being called in the wrong place. It should actually be called before
> the unpatching transition starts. When called from
> klp_unpatch_objects(), the new code is no longer running, so it's
> effectively post-patch instead of pre-patch.
>
> Another random thought: maybe we should show the "patching complete"
> message *after* the post-patch callback is run. That would be more
> honest with the user, as technically, the post-patch callback is part of
> the patching process.
>
> And a similar comment for the "unpatching complete" message.
Makes sense as well.
Best Regards,
Petr
On Fri, Aug 18, 2017 at 03:58:16PM +0200, Petr Mladek wrote:
> On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> > Provide livepatch modules a klp_object (un)patching notification
> > mechanism. Pre and post-(un)patch callbacks allow livepatch modules to
> > setup or synchronize changes that would be difficult to support in only
> > patched-or-unpatched code contexts.
> >
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 194991ef9347..500dc9b2b361 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -138,6 +154,71 @@ struct klp_patch {
> > func->old_name || func->new_func || func->old_sympos; \
> > func++)
> >
> > +/**
> > + * klp_is_object_loaded() - is klp_object currently loaded?
> > + * @obj: klp_object pointer
> > + *
> > + * Return: true if klp_object is loaded (always true for vmlinux)
> > + */
> > +static inline bool klp_is_object_loaded(struct klp_object *obj)
> > +{
> > + return !obj->name || obj->mod;
> > +}
> > +
> > +/**
> > + * klp_pre_patch_callback - execute before klp_object is patched
> > + * @obj: invoke callback for this klp_object
> > + *
> > + * Return: status from callback
> > + *
> > + * Callers should ensure obj->patched is *not* set.
> > + */
> > +static inline int klp_pre_patch_callback(struct klp_object *obj)
> > +{
> > + if (obj->callbacks.pre_patch)
> > + return (*obj->callbacks.pre_patch)(obj);
> > + return 0;
> > +}
> > +
> > +/**
> > + * klp_post_patch_callback() - execute after klp_object is patched
> > + * @obj: invoke callback for this klp_object
> > + *
> > + * Callers should ensure obj->patched is set.
> > + */
> > +static inline void klp_post_patch_callback(struct klp_object *obj)
> > +{
> > + if (obj->callbacks.post_patch)
> > + (*obj->callbacks.post_patch)(obj);
> > +}
> > +
> > +/**
> > + * klp_pre_unpatch_callback() - execute before klp_object is unpatched
> > + * and is active across all tasks
> > + * @obj: invoke callback for this klp_object
> > + *
> > + * Callers should ensure obj->patched is set.
> > + */
> > +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> > +{
> > + if (obj->callbacks.pre_unpatch)
> > + (*obj->callbacks.pre_unpatch)(obj);
> > +}
> > +
> > +/**
> > + * klp_post_unpatch_callback() - execute after klp_object is unpatched,
> > + * all code has been restored and no tasks
> > + * are running patched code
> > + * @obj: invoke callback for this klp_object
> > + *
> > + * Callers should ensure obj->patched is *not* set.
> > + */
> > +static inline void klp_post_unpatch_callback(struct klp_object *obj)
> > +{
> > + if (obj->callbacks.post_unpatch)
> > + (*obj->callbacks.post_unpatch)(obj);
> > +}
>
> I guess that we do not want to make these function usable
> outside livepatch code. Thefore these inliners should go
> to kernel/livepatch/core.h or so.
Okay, I can stash them away in an internal header file like core.h.
> > +
> > int klp_register_patch(struct klp_patch *);
> > int klp_unregister_patch(struct klp_patch *);
> > int klp_enable_patch(struct klp_patch *);
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index b9628e43c78f..ddb23e18a357 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
> > goto err;
> > }
> >
> > + klp_post_patch_callback(obj);
>
> This should be called only if (patch != klp_transition_patch).
> Otherwise, it would be called too early.
Can you elaborate a bit on this scenario? When would the transition
patch (as I understand it, a livepatch not quite fully (un)patched) hit
the module coming/going notifier? Is it possible to load or unload a
module like this? I'd like to add this scenario to my test script if
possible.
> > +
> > break;
> > }
> > }
> > @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
> > if (patch->enabled || patch == klp_transition_patch) {
> > pr_notice("reverting patch '%s' on unloading module '%s'\n",
> > patch->mod->name, obj->mod->name);
> > +
> > + klp_pre_unpatch_callback(obj);
>
> Also the pre_unpatch() callback should be called only
> if (patch != klp_transition_patch). Otherwise, it should have
> already been called. It is not the current case but see below.
Ditto.
> > klp_unpatch_object(obj);
> > + klp_post_unpatch_callback(obj);
> > }
> >
> > klp_free_object_loaded(obj);
> > diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
> > index 52c4e907c14b..0eed0df6e6d9 100644
> > --- a/kernel/livepatch/patch.c
> > +++ b/kernel/livepatch/patch.c
> > @@ -257,6 +257,7 @@ int klp_patch_object(struct klp_object *obj)
> > klp_for_each_func(obj, func) {
> > ret = klp_patch_func(func);
> > if (ret) {
> > + klp_pre_unpatch_callback(obj);
>
> This looks strange (somehow asymetric). IMHO, it should not be
> needed. klp_pre_unpatch_callback() should revert changes done
> by klp_post_patch_callback() that has not run yet.
I was skeptical about this call, too. After reading your comment, I
realize it shouldn't be needed here.
> > klp_unpatch_object(obj);
> > return ret;
> > }
> > @@ -271,6 +272,8 @@ void klp_unpatch_objects(struct klp_patch *patch)
> > struct klp_object *obj;
> >
> > klp_for_each_object(patch, obj)
> > - if (obj->patched)
> > + if (obj->patched) {
> > + klp_pre_unpatch_callback(obj);
>
> This is even more strange. The corresponding klp_post_patch_callback()
> is called at the very end of the transaction when the patch is already
> used by the entire system. Therefore the operation should be reverted
> before we start disabling the patch.
>
> IMHO, klp_pre_unpatch_callback() should get called in
> __klp_disable_patch(). I would put it right after
> klp_init_transition(patch, KLP_UNPATCHED);
Okay, looking at the transition code, this makes sense. I'll move the
pre-(un)patch calls into the __enable_patch() / __disable_patch()
functions after initalizing the transition. I think that should clean
up some of the strange ordering of kernel log msgs as well.
> Another advantage of this logic is that we actually do not need
> to care about these callbacks in klp_reverse_transition().
> But we should probably mention it in the documentation
> how the actions done by the patch and unpatch callbacks
> are related.
>
> Otherwise, it looks fine to me.
Thanks for reviewing.
-- Joe
On Mon 2017-08-21 17:18:58, Joe Lawrence wrote:
> On Fri, Aug 18, 2017 at 03:58:16PM +0200, Petr Mladek wrote:
> > On Wed 2017-08-16 15:17:04, Joe Lawrence wrote:
> > > Provide livepatch modules a klp_object (un)patching notification
> > > mechanism. Pre and post-(un)patch callbacks allow livepatch modules to
> > > setup or synchronize changes that would be difficult to support in only
> > > patched-or-unpatched code contexts.
> > >
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index b9628e43c78f..ddb23e18a357 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -878,6 +890,8 @@ int klp_module_coming(struct module *mod)
> > > goto err;
> > > }
> > >
> > > + klp_post_patch_callback(obj);
> >
> > This should be called only if (patch != klp_transition_patch).
> > Otherwise, it would be called too early.
>
> Can you elaborate a bit on this scenario? When would the transition
> patch (as I understand it, a livepatch not quite fully (un)patched) hit
> the module coming/going notifier? Is it possible to load or unload a
> module like this? I'd like to add this scenario to my test script if
> possible.
if (patch == klp_transition_patch) then the transition for this
patch is still running. klp_post_patch_callback() should and is
called at the end of the transition, see klp_complete_transition().
Note that klp_complete_transition() will see the object/module
loaded because obj->mod is set in klp_module_coming().
The (up)patch transition might take a while. It might be even
infinite if a process is blocked in a patched function. klp_mutex
is available most of the time. Therefore it is perfectly fine to
load/remove modules into/from the system and run their
klp_module_coming()/going() callbacks when a livepatch
patch transition is running.
> > > +
> > > break;
> > > }
> > > }
> > > @@ -929,7 +943,10 @@ void klp_module_going(struct module *mod)
> > > if (patch->enabled || patch == klp_transition_patch) {
> > > pr_notice("reverting patch '%s' on unloading module '%s'\n",
> > > patch->mod->name, obj->mod->name);
> > > +
> > > + klp_pre_unpatch_callback(obj);
> >
> > Also the pre_unpatch() callback should be called only
> > if (patch != klp_transition_patch). Otherwise, it should have
> > already been called. It is not the current case but see below.
>
> Ditto.
This is related to the other comment where I and Josh suggested
to call klp_pre_unpatch_callback() in __klp_disable_patch()
before the transition started. It means that the callback
has already been called for klp_transition_patch.
Best Regards,
Petr