LinuxLists.cc - [PATCH v5 0/3] livepatch callbacks

[permalink] [raw]

Subject: [PATCH v5 1/3] livepatch: add (un)patch callbacks

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 transition
starts.

- 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/callbacks.txt for details and
samples/livepatch/ for examples.

Signed-off-by: Joe Lawrence <[email protected]>
---
Documentation/livepatch/callbacks.txt | 594 ++++++++++++++++++++++++
include/linux/livepatch.h | 25 +
kernel/livepatch/core.c | 50 +-
kernel/livepatch/core.h | 37 ++
kernel/livepatch/patch.c | 1 +
kernel/livepatch/transition.c | 21 +-
samples/livepatch/Makefile | 3 +
samples/livepatch/livepatch-callbacks-busymod.c | 72 +++
samples/livepatch/livepatch-callbacks-demo.c | 234 ++++++++++
samples/livepatch/livepatch-callbacks-mod.c | 55 +++
10 files changed, 1079 insertions(+), 13 deletions(-)
create mode 100644 Documentation/livepatch/callbacks.txt
create mode 100644 samples/livepatch/livepatch-callbacks-busymod.c
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..689b3f399696
--- /dev/null
+++ b/Documentation/livepatch/callbacks.txt
@@ -0,0 +1,594 @@
+======================
+(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 callbacks 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 a to-be-patched module from loading.
+
+Callbacks are part of the klp_object structure and their implementation
+is specific to that klp_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 a klp_object is patched
+
+ * Post-patch - after a klp_object has been patched and is active
+ across all tasks
+
+ * Pre-unpatch - before a klp_object is unpatched (ie, patched code is
+ active), used to clean up post-patch callback
+ resources
+
+ * Post-unpatch - after a klp_object has been patched, all code has
+ been restored and no tasks are running patched code,
+ used to cleanup pre-patch callback resources
+
+Each callback action is optional, omitting one does not preclude
+specifying any other. Typical use cases however, pare a pre-patch with
+a post-unpatch handler and a post-patch with a pre-unpatch handler in
+symmetry: the patch handler acquires and configures resources and the
+unpatch handler tears down and releases those same resources.
+
+A callback 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 patch target kernel modules,
+callbacks will only execute if the target module is loaded. When a
+module target is (un)loaded, its callbacks will execute only if the
+livepatch module is enabled.
+
+The pre-patch callback, if specified, is expected to return a status
+code (0 for success, -ERRNO on error). An error status code indicates
+to the livepatching core that patching of the current klp_object is not
+safe and to stop the current patching request. (When no pre-patch
+callback is provided, the transition is assumed to be safe.) If a
+pre-patch callback returns failure, the kernel's module loader will:
+
+ - Refuse to load a livepatch, if the livepatch is loaded after
+ targeted code.
+
+ or:
+
+ - Refuse to load a module, if the livepatch was already successfully
+ loaded.
+
+No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
+for a given klp_object if its pre-patch callback returned non-zero
+status.
+
+
+Example Use-cases
+=================
+
+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.
+
+
+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.)
+
+
+Test cases
+==========
+
+What follows is not an exhaustive test suite of every possible livepatch
+pre/post-(un)patch combination, but a selection that demonstrates a few
+important concepts. Each test case uses the kernel modules located in
+the samples/livepatch/ and assumes that no livepatches are loaded at the
+beginning of the test.
+
+
+Test 1
+------
+
+Test a combination of loading a kernel module and a livepatch that
+patches a function in the first module. (Un)load the target module
+before the livepatch module:
+
+- load target module
+- load livepatch
+- disable livepatch
+- unload target module
+- unload livepatch
+
+First load a target module:
+
+ % insmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 34.475708] livepatch_callbacks_mod: livepatch_callbacks_mod_init
+
+On livepatch enable, before the livepatch transition starts, pre-patch
+callbacks are executed for vmlinux and livepatch_callbacks_mod (those
+klp_objects currently loaded). After klp_objects are patched according
+to the klp_patch, their post-patch callbacks run and the transition
+completes:
+
+ % insmod samples/livepatch/livepatch-callbacks-demo.ko
+ [ 36.503719] livepatch: enabling patch 'livepatch_callbacks_demo'
+ [ 36.504213] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 36.504238] livepatch_callbacks_demo: pre_patch_callback: vmlinux
+ [ 36.504721] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
+ [ 36.505849] livepatch: 'livepatch_callbacks_demo': starting patching transition
+ [ 37.727133] livepatch: 'livepatch_callbacks_demo': completing patching transition
+ [ 37.727232] livepatch_callbacks_demo: post_patch_callback: vmlinux
+ [ 37.727860] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
+ [ 37.728792] livepatch: 'livepatch_callbacks_demo': patching complete
+
+Similarly, on livepatch disable, pre-patch callbacks run before the
+unpatching transition starts. klp_objects are reverted, post-patch
+callbacks execute and the transition completes:
+
+ % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+ [ 38.510209] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 38.510234] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
+ [ 38.510982] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
+ [ 38.512209] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
+ [ 39.711132] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
+ [ 39.711210] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
+ [ 39.711779] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
+ [ 39.712735] livepatch: 'livepatch_callbacks_demo': unpatching complete
+
+ % rmmod samples/livepatch/livepatch-callbacks-demo.ko
+ % rmmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 42.534183] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
+
+
+Test 2
+------
+
+This test is similar to the previous test, but (un)load the livepatch
+module before the target kernel module. This tests the livepatch core's
+module_coming handler:
+
+- load livepatch
+- load target module
+- disable livepatch
+- unload livepatch
+- unload target module
+
+
+On livepatch enable, only pre/post-patch callbacks are executed for
+currently loaded klp_objects, in this case, vmlinux:
+
+ % insmod samples/livepatch/livepatch-callbacks-demo.ko
+ [ 44.553328] livepatch: enabling patch 'livepatch_callbacks_demo'
+ [ 44.553997] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 44.554049] livepatch_callbacks_demo: pre_patch_callback: vmlinux
+ [ 44.554845] livepatch: 'livepatch_callbacks_demo': starting patching transition
+ [ 45.727128] livepatch: 'livepatch_callbacks_demo': completing patching transition
+ [ 45.727212] livepatch_callbacks_demo: post_patch_callback: vmlinux
+ [ 45.727961] livepatch: 'livepatch_callbacks_demo': patching complete
+
+When a targeted module is subsequently loaded, only its pre/post-patch
+callbacks are executed:
+
+ % insmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 46.560845] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
+ [ 46.561988] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
+ [ 46.563452] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
+ [ 46.565495] livepatch_callbacks_mod: livepatch_callbacks_mod_init
+
+On livepatch disable, all currently loaded klp_objects' (vmlinux and
+livepatch_callbacks_mod) pre/post-unpatch callbacks are executed:
+
+ % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+ [ 48.568885] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 48.568910] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
+ [ 48.569441] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
+ [ 48.570502] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
+ [ 49.759091] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
+ [ 49.759171] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
+ [ 49.759742] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
+ [ 49.760690] livepatch: 'livepatch_callbacks_demo': unpatching complete
+
+ % rmmod samples/livepatch/livepatch-callbacks-demo.ko
+ % rmmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 52.592283] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
+
+
+Test 3
+------
+
+Test loading the livepatch after a targeted kernel module, then unload
+the kernel module before disabling the livepatch. This tests the
+livepatch core's module_going handler:
+
+- load target module
+- load livepatch
+- unload target module
+- disable livepatch
+- unload livepatch
+
+First load a target module, then the livepatch:
+
+ % insmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 54.607948] livepatch_callbacks_mod: livepatch_callbacks_mod_init
+
+ % insmod samples/livepatch/livepatch-callbacks-demo.ko
+ [ 56.613919] livepatch: enabling patch 'livepatch_callbacks_demo'
+ [ 56.614411] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 56.614436] livepatch_callbacks_demo: pre_patch_callback: vmlinux
+ [ 56.614818] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
+ [ 56.615656] livepatch: 'livepatch_callbacks_demo': starting patching transition
+ [ 57.759070] livepatch: 'livepatch_callbacks_demo': completing patching transition
+ [ 57.759147] livepatch_callbacks_demo: post_patch_callback: vmlinux
+ [ 57.759621] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
+ [ 57.760307] livepatch: 'livepatch_callbacks_demo': patching complete
+
+When a target module is unloaded, the livepatch is only reverted from
+that klp_object (livepatch_callbacks_mod). As such, only its pre and
+post-unpatch callbacks are executed when this occurs:
+
+ % rmmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 58.623409] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
+ [ 58.623903] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
+ [ 58.624658] livepatch: reverting patch 'livepatch_callbacks_demo' on unloading module 'livepatch_callbacks_mod'
+ [ 58.625305] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
+
+When the livepatch is disabled, pre and post-unpatch callbacks are run
+for the remaining klp_object, vmlinux:
+
+ % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+ [ 60.638420] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 60.638444] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
+ [ 60.638996] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
+ [ 61.727088] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
+ [ 61.727165] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
+ [ 61.727985] livepatch: 'livepatch_callbacks_demo': unpatching complete
+
+ % rmmod samples/livepatch/livepatch-callbacks-demo.ko
+
+
+Test 4
+------
+
+This test is similar to the previous test, however the livepatch is
+loaded first. This tests the livepatch core's module_coming and
+module_going handlers:
+
+- load livepatch
+- load target module
+- unload target module
+- disable livepatch
+- unload livepatch
+
+First load the livepatch:
+
+ % insmod samples/livepatch/livepatch-callbacks-demo.ko
+ [ 64.661552] livepatch: enabling patch 'livepatch_callbacks_demo'
+ [ 64.662147] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 64.662175] livepatch_callbacks_demo: pre_patch_callback: vmlinux
+ [ 64.662850] livepatch: 'livepatch_callbacks_demo': starting patching transition
+ [ 65.695056] livepatch: 'livepatch_callbacks_demo': completing patching transition
+ [ 65.695147] livepatch_callbacks_demo: post_patch_callback: vmlinux
+ [ 65.695561] livepatch: 'livepatch_callbacks_demo': patching complete
+
+When a targeted kernel module is subsequently loaded, only its
+pre/post-patch callbacks are executed:
+
+ % insmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 66.669196] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
+ [ 66.669882] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
+ [ 66.670744] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
+ [ 66.672873] livepatch_callbacks_mod: livepatch_callbacks_mod_init
+
+When the target module is unloaded, the livepatch is only reverted from
+the livepatch_callbacks_mod klp_object. As such, only pre and
+post-unpatch callbacks are executed when this occurs:
+
+ % rmmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 68.680065] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
+ [ 68.680688] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
+ [ 68.681452] livepatch: reverting patch 'livepatch_callbacks_demo' on unloading module 'livepatch_callbacks_mod'
+ [ 68.682094] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
+
+ % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+ [ 70.689225] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 70.689256] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
+ [ 70.689882] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
+ [ 71.711080] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
+ [ 71.711481] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
+ [ 71.711988] livepatch: 'livepatch_callbacks_demo': unpatching complete
+
+ % rmmod samples/livepatch/livepatch-callbacks-demo.ko
+
+
+Test 5
+------
+
+A simple test of loading a livepatch without one of its patch target
+klp_objects ever loaded (livepatch_callbacks_mod):
+
+- load livepatch
+- disable livepatch
+- unload livepatch
+
+Load the livepatch:
+
+ % insmod samples/livepatch/livepatch-callbacks-demo.ko
+ [ 74.711081] livepatch: enabling patch 'livepatch_callbacks_demo'
+ [ 74.711595] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 74.711639] livepatch_callbacks_demo: pre_patch_callback: vmlinux
+ [ 74.712272] livepatch: 'livepatch_callbacks_demo': starting patching transition
+ [ 75.743137] livepatch: 'livepatch_callbacks_demo': completing patching transition
+ [ 75.743219] livepatch_callbacks_demo: post_patch_callback: vmlinux
+ [ 75.743867] livepatch: 'livepatch_callbacks_demo': patching complete
+
+As expected, only pre/post-(un)patch handlers are executed for vmlinux:
+
+ % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+ [ 76.716254] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 76.716278] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
+ [ 76.716666] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
+ [ 77.727089] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
+ [ 77.727194] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
+ [ 77.727907] livepatch: 'livepatch_callbacks_demo': unpatching complete
+
+ % rmmod samples/livepatch/livepatch-callbacks-demo.ko
+
+
+Test 6
+------
+
+Test a scenario where a vmlinux pre-patch callback returns a non-zero
+status (ie, failure):
+
+- load target module
+- load livepatch -ENODEV
+- unload target module
+
+First load a target module:
+
+ % insmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 80.740520] livepatch_callbacks_mod: livepatch_callbacks_mod_init
+
+Load the livepatch module, setting its 'pre_patch_ret' value to -19
+(-ENODEV). When its vmlinux pre-patch callback executed, this status
+code will propagate back to the module-loading subsystem. The result is
+that the insmod command refuses to load the livepatch module:
+
+ % insmod samples/livepatch/livepatch-callbacks-demo.ko pre_patch_ret=-19
+ [ 82.747326] livepatch: enabling patch 'livepatch_callbacks_demo'
+ [ 82.747743] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 82.747767] livepatch_callbacks_demo: pre_patch_callback: vmlinux
+ [ 82.748237] livepatch: pre-patch callback failed for object 'vmlinux'
+ [ 82.748637] livepatch: failed to enable patch 'livepatch_callbacks_demo'
+ [ 82.749059] livepatch: 'livepatch_callbacks_demo': canceling transition, unpatching
+ [ 82.749060] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
+ [ 82.749868] livepatch: 'livepatch_callbacks_demo': unpatching complete
+ [ 82.765809] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-demo.ko: No such device
+
+ % rmmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 84.774238] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
+
+
+Test 7
+------
+
+Similar to the previous test, setup a livepatch such that its vmlinux
+pre-patch callback returns success. However, when a targeted kernel
+module is later loaded, have the livepatch return a failing status code:
+
+- load livepatch
+- setup -ENODEV
+- load target module
+- disable livepatch
+- unload livepatch
+
+Load the livepatch, notice vmlinux pre-patch callback succeeds:
+
+ % insmod samples/livepatch/livepatch-callbacks-demo.ko
+ [ 86.787845] livepatch: enabling patch 'livepatch_callbacks_demo'
+ [ 86.788325] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 86.788427] livepatch_callbacks_demo: pre_patch_callback: vmlinux
+ [ 86.788821] livepatch: 'livepatch_callbacks_demo': starting patching transition
+ [ 87.711069] livepatch: 'livepatch_callbacks_demo': completing patching transition
+ [ 87.711143] livepatch_callbacks_demo: post_patch_callback: vmlinux
+ [ 87.711886] livepatch: 'livepatch_callbacks_demo': patching complete
+
+Set a trap so subsequent pre-patch callbacks to this livepatch will
+return -ENODEV:
+
+ % echo -19 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret
+
+The livepatch pre-patch callback for subsequently loaded target modules
+will return failure, so the module loader refuses to load the kernel
+module. Notice that no post-patch or pre/post-unpatch callbacks are
+executed for this klp_object:
+
+ % insmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 90.796976] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
+ [ 90.797834] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
+ [ 90.798900] livepatch: pre-patch callback failed for object 'livepatch_callbacks_mod'
+ [ 90.799652] livepatch: patch 'livepatch_callbacks_demo' failed for module 'livepatch_callbacks_mod', refusing to load module 'livepatch_callbacks_mod'
+ [ 90.819737] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device
+
+However, pre/post-unpatch callbacks run for the vmlinux klp_object:
+
+ % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+ [ 92.823547] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 92.823573] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
+ [ 92.824331] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
+ [ 93.727128] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
+ [ 93.727327] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
+ [ 93.727861] livepatch: 'livepatch_callbacks_demo': unpatching complete
+
+ % rmmod samples/livepatch/livepatch-callbacks-demo.ko
+
+
+Test 8
+------
+
+Test loading multiple targeted kernel modules. This test-case is
+mainly for comparing with the next test-case.
+
+- load busy target module (0s sleep),
+- load livepatch
+- load target module
+- unload target module
+- disable livepatch
+- unload livepatch
+- unload busy target module
+
+
+Load a target "busy" kernel module which kicks off a worker function
+that immediately exits:
+
+ % insmod samples/livepatch/livepatch-callbacks-busymod.ko sleep_secs=0
+ [ 96.910107] livepatch_callbacks_busymod: livepatch_callbacks_mod_init
+ [ 96.910600] livepatch_callbacks_busymod: busymod_work_func, sleeping 0 seconds ...
+ [ 96.913024] livepatch_callbacks_busymod: busymod_work_func exit
+
+Proceed with loading the livepatch and another ordinary target module,
+notice that the post-patch callbacks are executed and the transition
+completes quickly:
+
+ % insmod samples/livepatch/livepatch-callbacks-demo.ko
+ [ 98.917892] livepatch: enabling patch 'livepatch_callbacks_demo'
+ [ 98.918426] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 98.918453] livepatch_callbacks_demo: pre_patch_callback: vmlinux
+ [ 98.918955] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_busymod -> [MODULE_STATE_LIVE] Normal state
+ [ 98.923835] livepatch: 'livepatch_callbacks_demo': starting patching transition
+ [ 99.743104] livepatch: 'livepatch_callbacks_demo': completing patching transition
+ [ 99.743156] livepatch_callbacks_demo: post_patch_callback: vmlinux
+ [ 99.743679] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_busymod -> [MODULE_STATE_LIVE] Normal state
+ [ 99.744616] livepatch: 'livepatch_callbacks_demo': patching complete
+
+ % insmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 100.930955] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
+ [ 100.931668] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
+ [ 100.932645] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
+ [ 100.934125] livepatch_callbacks_mod: livepatch_callbacks_mod_init
+
+ % rmmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 102.942805] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
+ [ 102.943640] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
+ [ 102.944585] livepatch: reverting patch 'livepatch_callbacks_demo' on unloading module 'livepatch_callbacks_mod'
+ [ 102.945455] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
+
+ % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+ [ 104.953815] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 104.953838] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
+ [ 104.954431] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_busymod -> [MODULE_STATE_LIVE] Normal state
+ [ 104.955426] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
+ [ 106.719073] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
+ [ 106.722633] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
+ [ 106.723282] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_busymod -> [MODULE_STATE_LIVE] Normal state
+ [ 106.724279] livepatch: 'livepatch_callbacks_demo': unpatching complete
+
+ % rmmod samples/livepatch/livepatch-callbacks-demo.ko
+ % rmmod samples/livepatch/livepatch-callbacks-busymod.ko
+ [ 108.975660] livepatch_callbacks_busymod: livepatch_callbacks_mod_exit
+
+
+Test 9
+------
+
+A similar test as the previous one, but force the "busy" kernel module
+to do longer work.
+
+The livepatching core will refuse to patch a task that is currently
+executing a to-be-patched function -- the consistency model stalls the
+current patch transition until this safety-check is met. Test a
+scenario where one of a livepatch's target klp_objects sits on such a
+function for a long time. Meanwhile, load and unload other target
+kernel modules while the livepatch transition is in progress.
+
+- load busy target module (30s sleep)
+- load livepatch
+- load target module
+- unload target module
+- disable livepatch
+- unload livepatch
+- unload busy target module
+
+
+Load the "busy" kernel module, this time make it do 30 seconds worth of
+work:
+
+ % insmod samples/livepatch/livepatch-callbacks-busymod.ko sleep_secs=30
+ [ 110.993362] livepatch_callbacks_busymod: livepatch_callbacks_mod_init
+ [ 110.994059] livepatch_callbacks_busymod: busymod_work_func, sleeping 30 seconds ...
+
+Meanwhile, the livepatch is loaded. Notice that the patch transition
+does not complete as the targeted "busy" module is sitting on a
+to-be-patched function:
+
+ % insmod samples/livepatch/livepatch-callbacks-demo.ko
+ [ 113.000309] livepatch: enabling patch 'livepatch_callbacks_demo'
+ [ 113.000764] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
+ [ 113.000791] livepatch_callbacks_demo: pre_patch_callback: vmlinux
+ [ 113.001289] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_busymod -> [MODULE_STATE_LIVE] Normal state
+ [ 113.005208] livepatch: 'livepatch_callbacks_demo': starting patching transition
+
+Load a second target module (this one is an ordinary idle kernel
+module). Note that *no* post-patch callbacks will be executed while the
+livepatch is still in transition:
+
+ % insmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 115.012740] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
+ [ 115.013406] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
+ [ 115.015315] livepatch_callbacks_mod: livepatch_callbacks_mod_init
+
+Request an unload of the simple kernel module. The patch is still
+transitioning, so its pre-unpatch callbacks are skipped:
+
+ % rmmod samples/livepatch/livepatch-callbacks-mod.ko
+ [ 117.022626] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
+ [ 117.023376] livepatch: reverting patch 'livepatch_callbacks_demo' on unloading module 'livepatch_callbacks_mod'
+ [ 117.024533] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
+
+Finally the livepatch is disabled. Since none of the patch's
+klp_object's post-patch callbacks executed, the remaining klp_object's
+pre-unpatch callbacks are skipped:
+
+ % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+ [ 119.035408] livepatch: 'livepatch_callbacks_demo': reversing transition from patching to unpatching
+ [ 119.035485] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
+ [ 119.711166] livepatch: 'livepatch_callbacks_demo': completing unpatching transition
+ [ 119.714179] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
+ [ 119.714653] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_busymod -> [MODULE_STATE_LIVE] Normal state
+ [ 119.715437] livepatch: 'livepatch_callbacks_demo': unpatching complete
+
+ % rmmod samples/livepatch/livepatch-callbacks-demo.ko
+ % rmmod samples/livepatch/livepatch-callbacks-busymod.ko
+ [ 141.279111] livepatch_callbacks_busymod: busymod_work_func exit
+ [ 141.279760] livepatch_callbacks_busymod: livepatch_callbacks_mod_exit
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 194991ef9347..58403a9af54b 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -87,24 +87,49 @@ struct klp_func {
bool transition;
};

+struct klp_object;
+
+/**
+ * struct klp_callbacks - pre/post live-(un)patch callback structure
+ * @pre_patch: executed before code patching
+ * @post_patch: executed after code patching
+ * @pre_unpatch: executed before code unpatching
+ * @post_unpatch: executed after code unpatching
+ *
+ * All callbacks are optional. Only the pre-patch callback, if provided,
+ * will be unconditionally executed. If the parent klp_object fails to
+ * patch for any reason, including a non-zero error status returned from
+ * the pre-patch callback, no further callbacks will be executed.
+ */
+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)
* @patched: the object's funcs have been added to the klp_ops list
+ * @callbacks_enabled: flag indicating if callbacks should be run
*/
struct klp_object {
/* external */
const char *name;
struct klp_func *funcs;
+ struct klp_callbacks callbacks;

/* internal */
struct kobject kobj;
struct module *mod;
bool patched;
+ bool callbacks_enabled;
};

/**
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b9628e43c78f..aca62c4b8616 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)
{
@@ -285,6 +280,8 @@ static int klp_write_object_relocations(struct module *pmod,

static int __klp_disable_patch(struct klp_patch *patch)
{
+ struct klp_object *obj;
+
if (klp_transition_patch)
return -EBUSY;

@@ -295,6 +292,10 @@ static int __klp_disable_patch(struct klp_patch *patch)

klp_init_transition(patch, KLP_UNPATCHED);

+ klp_for_each_object(patch, obj)
+ if (patch->enabled && obj->patched)
+ klp_pre_unpatch_callback(obj);
+
/*
* Enforce the order of the func->transition writes in
* klp_init_transition() and the TIF_PATCH_PENDING writes in
@@ -388,13 +389,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 +409,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,13 +882,27 @@ 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",
patch->mod->name, obj->mod->name, ret);
+
+ if (patch != klp_transition_patch)
+ klp_post_unpatch_callback(obj);
+
goto err;
}

+ if (patch != klp_transition_patch)
+ klp_post_patch_callback(obj);
+
break;
}
}
@@ -927,9 +952,14 @@ void klp_module_going(struct module *mod)
* is in transition.
*/
if (patch->enabled || patch == klp_transition_patch) {
+
+ if (patch != klp_transition_patch)
+ klp_pre_unpatch_callback(obj);
+
pr_notice("reverting patch '%s' on unloading module '%s'\n",
patch->mod->name, obj->mod->name);
klp_unpatch_object(obj);
+ klp_post_unpatch_callback(obj);
}

klp_free_object_loaded(obj);
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index c74f24c47837..230675b7b3d4 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -1,6 +1,43 @@
#ifndef _LIVEPATCH_CORE_H
#define _LIVEPATCH_CORE_H

+#include <linux/livepatch.h>
+
extern struct mutex klp_mutex;

+static inline bool klp_is_object_loaded(struct klp_object *obj)
+{
+ return !obj->name || obj->mod;
+}
+
+static inline int klp_pre_patch_callback(struct klp_object *obj)
+{
+ int ret;
+
+ ret = (obj->callbacks.pre_patch) ?
+ (*obj->callbacks.pre_patch)(obj) : 0;
+
+ obj->callbacks_enabled = !ret;
+
+ return ret;
+}
+
+static inline void klp_post_patch_callback(struct klp_object *obj)
+{
+ if (obj->callbacks.post_patch)
+ (*obj->callbacks.post_patch)(obj);
+}
+
+static inline void klp_pre_unpatch_callback(struct klp_object *obj)
+{
+ if (obj->callbacks.pre_unpatch)
+ (*obj->callbacks.pre_unpatch)(obj);
+}
+
+static inline void klp_post_unpatch_callback(struct klp_object *obj)
+{
+ if (obj->callbacks_enabled && obj->callbacks.post_unpatch)
+ (*obj->callbacks.post_unpatch)(obj);
+}
+
#endif /* _LIVEPATCH_CORE_H */
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 52c4e907c14b..82d584225dc6 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -28,6 +28,7 @@
#include <linux/slab.h>
#include <linux/bug.h>
#include <linux/printk.h>
+#include "core.h"
#include "patch.h"
#include "transition.h"

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..d1e7fded7597 100644
--- a/samples/livepatch/Makefile
+++ b/samples/livepatch/Makefile
@@ -1 +1,4 @@
obj-$(CONFIG_SAMPLE_LIVEPATCH) += livepatch-sample.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
diff --git a/samples/livepatch/livepatch-callbacks-busymod.c b/samples/livepatch/livepatch-callbacks-busymod.c
new file mode 100644
index 000000000000..80d06e103f1b
--- /dev/null
+++ b/samples/livepatch/livepatch-callbacks-busymod.c
@@ -0,0 +1,72 @@
+/*
+ * 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-busymod.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>
+#include <linux/workqueue.h>
+#include <linux/delay.h>
+
+static int sleep_secs;
+module_param(sleep_secs, int, 0644);
+MODULE_PARM_DESC(sleep_secs, "sleep_secs (default=0)");
+
+static void busymod_work_func(struct work_struct *work);
+static DECLARE_DELAYED_WORK(work, busymod_work_func);
+
+static void busymod_work_func(struct work_struct *work)
+{
+ pr_info("%s, sleeping %d seconds ...\n", __func__, sleep_secs);
+ msleep(sleep_secs * 1000);
+ pr_info("%s exit\n", __func__);
+}
+
+static int livepatch_callbacks_mod_init(void)
+{
+ pr_info("%s\n", __func__);
+ schedule_delayed_work(&work,
+ msecs_to_jiffies(1000 * 0));
+ return 0;
+}
+
+static void livepatch_callbacks_mod_exit(void)
+{
+ cancel_delayed_work_sync(&work);
+ pr_info("%s\n", __func__);
+}
+
+module_init(livepatch_callbacks_mod_init);
+module_exit(livepatch_callbacks_mod_exit);
+MODULE_LICENSE("GPL");
diff --git a/samples/livepatch/livepatch-callbacks-demo.c b/samples/livepatch/livepatch-callbacks-demo.c
new file mode 100644
index 000000000000..3d115bd68442
--- /dev/null
+++ b/samples/livepatch/livepatch-callbacks-demo.c
@@ -0,0 +1,234 @@
+/*
+ * 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
+ *
+ * NOTE: There is a second target module,
+ * livepatch-callbacks-busymod.ko, available for experimenting
+ * with livepatch (un)patch callbacks. This module contains
+ * a 'sleep_secs' parameter that parks the module on one of the
+ * functions that the livepatch demo module wants to patch.
+ * Modifying this value and tweaking the order of module loads can
+ * effectively demonstrate stalled patch transitions:
+ *
+ * # Load a target module, let it park on 'busymod_work_func' for
+ * # thirty seconds
+ * insmod samples/livepatch/livepatch-callbacks-busymod.ko sleep_secs=30
+ *
+ * # Meanwhile load the livepatch
+ * insmod samples/livepatch/livepatch-callbacks-demo.ko
+ *
+ * # ... then load and unload another target module while the
+ * # transition is in progress
+ * insmod samples/livepatch/livepatch-callbacks-mod.ko
+ * rmmod samples/livepatch/livepatch-callbacks-mod.ko
+ *
+ * # Finally cleanup
+ * echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
+ * rmmod samples/livepatch/livepatch-callbacks-demo.ko
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/livepatch.h>
+
+static int pre_patch_ret;
+module_param(pre_patch_ret, int, 0644);
+MODULE_PARM_DESC(pre_patch_ret, "pre_patch_ret (default=0)");
+
+static const char *const 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 void patched_work_func(struct work_struct *work)
+{
+ pr_info("%s\n", __func__);
+}
+
+static struct klp_func no_funcs[] = {
+ { }
+};
+
+static struct klp_func busymod_funcs[] = {
+ {
+ .old_name = "busymod_work_func",
+ .new_func = patched_work_func,
+ }, { }
+};
+
+static struct klp_object objs[] = {
+ {
+ .name = NULL, /* vmlinux */
+ .funcs = no_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 = no_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_busymod",
+ .funcs = busymod_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..508fcfba3f22
--- /dev/null
+++ b/samples/livepatch/livepatch-callbacks-mod.c
@@ -0,0 +1,55 @@
+/*
+ * 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-demo.c.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/workqueue.h>
+#include <linux/delay.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

2017-08-31 14:54:13

[permalink] [raw]

Subject: [PATCH v5 3/3] livepatch: add transition notices

Log a few kernel debug messages at the beginning of the following livepatch
transition functions:

klp_complete_transition()
klp_cancel_transition()
klp_init_transition()
klp_reverse_transition()

Also update the log notice message in klp_start_transition() for similar
verbiage as the above messages.

Suggested-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
---
kernel/livepatch/transition.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 53887f0bca10..3d44a3cf27be 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -82,6 +82,10 @@ static void klp_complete_transition(void)
unsigned int cpu;
bool immediate_func = false;

+ pr_debug("'%s': completing %s transition\n",
+ klp_transition_patch->mod->name,
+ klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+
if (klp_target_state == KLP_UNPATCHED) {
/*
* All tasks have transitioned to KLP_UNPATCHED so we can now
@@ -163,6 +167,9 @@ void klp_cancel_transition(void)
if (WARN_ON_ONCE(klp_target_state != KLP_PATCHED))
return;

+ pr_debug("'%s': canceling transition, unpatching\n",
+ klp_transition_patch->mod->name);
+
klp_target_state = KLP_UNPATCHED;
klp_complete_transition();
}
@@ -441,7 +448,8 @@ void klp_start_transition(void)

WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);

- pr_notice("'%s': %s...\n", klp_transition_patch->mod->name,
+ pr_notice("'%s': starting %s transition\n",
+ klp_transition_patch->mod->name,
klp_target_state == KLP_PATCHED ? "patching" : "unpatching");

/*
@@ -489,6 +497,9 @@ void klp_init_transition(struct klp_patch *patch, int state)

WARN_ON_ONCE(klp_target_state != KLP_UNDEFINED);

+ pr_debug("'%s': initializing %s transition\n", patch->mod->name,
+ klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+
klp_transition_patch = patch;

/*
@@ -562,6 +573,11 @@ void klp_reverse_transition(void)
unsigned int cpu;
struct task_struct *g, *task;

+ pr_debug("'%s': reversing transition from %s\n",
+ klp_transition_patch->mod->name,
+ klp_target_state == KLP_PATCHED ? "patching to unpatching" :
+ "unpatching to patching");
+
klp_transition_patch->enabled = !klp_transition_patch->enabled;

klp_target_state = !klp_target_state;
--
1.8.3.1

2017-08-31 14:54:09

[permalink] [raw]

Subject: [PATCH v5 2/3] livepatch: move transition "complete" notice into klp_complete_transition()

klp_complete_transition() performs a bit of housework before a
transition to KLP_PATCHED or KLP_UNPATCHED is actually completed
(including post-(un)patch callbacks). To be consistent, move the
transition "complete" kernel log notice out of
klp_try_complete_transition() and into klp_complete_transition().

Suggested-by: Josh Poimboeuf <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
---
kernel/livepatch/transition.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 7bf55b7f3687..53887f0bca10 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -136,6 +136,9 @@ static void klp_complete_transition(void)
klp_post_unpatch_callback(obj);
}

+ pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
+ klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+
/*
* See complementary comment in __klp_enable_patch() for why we
* keep the module reference for immediate patches.
@@ -423,9 +426,6 @@ void klp_try_complete_transition(void)
}

success:
- pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
- klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
-
/* we're done, now cleanup the data structures */
klp_complete_transition();
}
--
1.8.3.1

2017-09-12 08:53:04

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

> diff --git a/Documentation/livepatch/callbacks.txt b/Documentation/livepatch/callbacks.txt
> new file mode 100644
> index 000000000000..689b3f399696
> --- /dev/null
> +++ b/Documentation/livepatch/callbacks.txt
> @@ -0,0 +1,594 @@
> +======================
> +(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 callbacks 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 a to-be-patched module from loading.
> +
> +Callbacks are part of the klp_object structure and their implementation
> +is specific to that klp_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 a klp_object is patched
> +
> + * Post-patch - after a klp_object has been patched and is active
> + across all tasks
> +
> + * Pre-unpatch - before a klp_object is unpatched (ie, patched code is
> + active), used to clean up post-patch callback
> + resources
> +
> + * Post-unpatch - after a klp_object has been patched, all code has
> + been restored and no tasks are running patched code,
> + used to cleanup pre-patch callback resources
> +
> +Each callback action is optional, omitting one does not preclude
> +specifying any other. Typical use cases however, pare a pre-patch with

s/pare/pair/ ?

> +a post-unpatch handler and a post-patch with a pre-unpatch handler in
> +symmetry: the patch handler acquires and configures resources and the
> +unpatch handler tears down and releases those same resources.

I think it is more than a typical use case. Test 9 shows that. Pre-unpatch
callbacks are skipped if a transition is reversed. I don't have a problem
with that per se, because it seems like a good approach, but maybe we
should describe it properly here. Am I right?

Anyway, it relates to the next remark just below, which is another rule.
So it is not totally arbitrary.

> +A callback 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 patch target kernel modules,
> +callbacks will only execute if the target module is loaded. When a
> +module target is (un)loaded, its callbacks will execute only if the
> +livepatch module is enabled.
> +
> +The pre-patch callback, if specified, is expected to return a status
> +code (0 for success, -ERRNO on error). An error status code indicates
> +to the livepatching core that patching of the current klp_object is not
> +safe and to stop the current patching request. (When no pre-patch
> +callback is provided, the transition is assumed to be safe.) If a
> +pre-patch callback returns failure, the kernel's module loader will:
> +
> + - Refuse to load a livepatch, if the livepatch is loaded after
> + targeted code.
> +
> + or:
> +
> + - Refuse to load a module, if the livepatch was already successfully
> + loaded.
> +
> +No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
> +for a given klp_object if its pre-patch callback returned non-zero
> +status.

Shouldn't this be changed to what Josh proposed? That is

No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
for a given klp_object if the object failed to patch, due to a failed
pre_patch callback or for any other reason.

If the object did successfully patch, but the patch transition never
started for some reason (e.g., if another object failed to patch),
only the post-unpatch callback will be called.

> +Test 1
> +------
> +
> +Test a combination of loading a kernel module and a livepatch that
> +patches a function in the first module. (Un)load the target module
> +before the livepatch module:
> +
> +- load target module
> +- load livepatch
> +- disable livepatch
> +- unload target module
> +- unload livepatch
> +
> +First load a target module:
> +
> + % insmod samples/livepatch/livepatch-callbacks-mod.ko
> + [ 34.475708] livepatch_callbacks_mod: livepatch_callbacks_mod_init
> +
> +On livepatch enable, before the livepatch transition starts, pre-patch
> +callbacks are executed for vmlinux and livepatch_callbacks_mod (those
> +klp_objects currently loaded). After klp_objects are patched according
> +to the klp_patch, their post-patch callbacks run and the transition
> +completes:
> +
> + % insmod samples/livepatch/livepatch-callbacks-demo.ko
> + [ 36.503719] livepatch: enabling patch 'livepatch_callbacks_demo'
> + [ 36.504213] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition

s/unpatching/patching/

I guess it is a copy&paste error and you can find it elsewhere too.

Apart from these, the documentation is great!

> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 194991ef9347..58403a9af54b 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -87,24 +87,49 @@ struct klp_func {
> bool transition;
> };
>
> +struct klp_object;
> +
> +/**
> + * struct klp_callbacks - pre/post live-(un)patch callback structure
> + * @pre_patch: executed before code patching
> + * @post_patch: executed after code patching
> + * @pre_unpatch: executed before code unpatching
> + * @post_unpatch: executed after code unpatching
> + *
> + * All callbacks are optional. Only the pre-patch callback, if provided,
> + * will be unconditionally executed. If the parent klp_object fails to
> + * patch for any reason, including a non-zero error status returned from
> + * the pre-patch callback, no further callbacks will be executed.
> + */
> +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)
> * @patched: the object's funcs have been added to the klp_ops list
> + * @callbacks_enabled: flag indicating if callbacks should be run

"flag indicating if post-unpatch callback should be run" ?

and then we could change the name to something like
'pre-patch_callback_enabled' (but that's really ugly).

> */
> struct klp_object {
> /* external */
> const char *name;
> struct klp_func *funcs;
> + struct klp_callbacks callbacks;
>
> /* internal */
> struct kobject kobj;
> struct module *mod;
> bool patched;
> + bool callbacks_enabled;
> };

How about moving callbacks_enabled to klp_callbacks structure? It belongs
there. It is true, that we'd mix internal and external members with that.

[...]

> @@ -871,13 +882,27 @@ 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;
> + }

There is a problem here. We cycle through all enabled patches (or
klp_transition_patch) and call klp_pre_patch_callback() everytime an
enabled patch contains a patch for a coming module. Now, it can easily
happen that klp_pre_patch_callback() fails. And not the first one from the
first relevant patch, but the next one. In that case we need to call
klp_post_unpatch_callback() for all already processed relevant patches in
the error path.

Unfortunately, we need to do the same for klp_patch_object() below,
because there is the same problem and we missed it.

> +
> ret = klp_patch_object(obj);
> if (ret) {
> pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> patch->mod->name, obj->mod->name, ret);
> +
> + if (patch != klp_transition_patch)
> + klp_post_unpatch_callback(obj);
> +
> goto err;

Here.

Could you do it as a part of the patch set (or send it separately),
please?

> diff --git a/samples/livepatch/livepatch-callbacks-mod.c b/samples/livepatch/livepatch-callbacks-mod.c
> new file mode 100644
> index 000000000000..508fcfba3f22
> --- /dev/null
> +++ b/samples/livepatch/livepatch-callbacks-mod.c
> @@ -0,0 +1,55 @@
> +/*
> + * 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-demo.c.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>

> +#include <linux/workqueue.h>
> +#include <linux/delay.h>

At least these two headers files are not needed here.

> +
> +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");

Miroslav

2017-09-12 09:09:06

[permalink] [raw]

Subject: Re: [PATCH v5 2/3] livepatch: move transition "complete" notice into klp_complete_transition()

On Thu, 31 Aug 2017, Joe Lawrence wrote:

> klp_complete_transition() performs a bit of housework before a
> transition to KLP_PATCHED or KLP_UNPATCHED is actually completed
> (including post-(un)patch callbacks). To be consistent, move the
> transition "complete" kernel log notice out of
> klp_try_complete_transition() and into klp_complete_transition().
>
> Suggested-by: Josh Poimboeuf <[email protected]>
> Signed-off-by: Joe Lawrence <[email protected]>

Acked-by: Miroslav Benes <[email protected]>

Miroslav

2017-09-12 09:29:24

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] livepatch: add transition notices

On Thu, 31 Aug 2017, Joe Lawrence wrote:

> Log a few kernel debug messages at the beginning of the following livepatch
> transition functions:
>
> klp_complete_transition()
> klp_cancel_transition()
> klp_init_transition()
> klp_reverse_transition()
>
> Also update the log notice message in klp_start_transition() for similar
> verbiage as the above messages.
>
> Suggested-by: Josh Poimboeuf <[email protected]>
> Signed-off-by: Joe Lawrence <[email protected]>
> ---
> kernel/livepatch/transition.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 53887f0bca10..3d44a3cf27be 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -82,6 +82,10 @@ static void klp_complete_transition(void)
> unsigned int cpu;
> bool immediate_func = false;
>
> + pr_debug("'%s': completing %s transition\n",
> + klp_transition_patch->mod->name,
> + klp_target_state == KLP_PATCHED ? "patching" : "unpatching");

The only downside here is that you can get a message about starting
patching transition and then a message about completing unpatching
transition, because the patching failed and klp_cancel_transition() was
called. I think we can live with that, because there would be a message
from klp_cancel_transition() as well and hopefully no one would be
confused.

> @@ -163,6 +167,9 @@ void klp_cancel_transition(void)
> if (WARN_ON_ONCE(klp_target_state != KLP_PATCHED))
> return;
>
> + pr_debug("'%s': canceling transition, unpatching\n",
> + klp_transition_patch->mod->name);
> +

This one.

Acked-by: Miroslav Benes <[email protected]>

Miroslav

2017-09-12 15:48:53

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

On 09/12/2017 04:53 AM, Miroslav Benes wrote:
>
>> diff --git a/Documentation/livepatch/callbacks.txt b/Documentation/livepatch/callbacks.txt
>> ...
>> +Each callback action is optional, omitting one does not preclude
>> +specifying any other. Typical use cases however, pare a pre-patch with
>
> s/pare/pair/ ?

Yup. At least I didn't use "pear" :)

>> +a post-unpatch handler and a post-patch with a pre-unpatch handler in
>> +symmetry: the patch handler acquires and configures resources and the
>> +unpatch handler tears down and releases those same resources.
>
> I think it is more than a typical use case. Test 9 shows that. Pre-unpatch
> callbacks are skipped if a transition is reversed. I don't have a problem
> with that per se, because it seems like a good approach, but maybe we
> should describe it properly here. Am I right?

I think the text was a little fuzzy in regard to what "typical" was
referring to. How about this edit:

--
Each callback is optional, omitting one does not preclude specifying any
other. However, the livepatching core executes the handlers in
symmetry: pre-patch callbacks have a post-patch counterpart and
post-patch callbacks have a pre-unpatch counterpart. An unpatch
callback will only be executed if its corresponding patch callback was
executed. Typical use cases pair a patch handler that acquires and
configures resources with an unpatch handler tears down and releases
those same resources.
--

Does that clarify that the execution symmetry is fixed and that
implementing callbacks with that property in mind is up to the caller?

More on the reversed transition comment below ...

> Anyway, it relates to the next remark just below, which is another rule.
> So it is not totally arbitrary.
>
>> +A callback 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 patch target kernel modules,
>> +callbacks will only execute if the target module is loaded. When a
>> +module target is (un)loaded, its callbacks will execute only if the
>> +livepatch module is enabled.
>> +
>> +The pre-patch callback, if specified, is expected to return a status
>> +code (0 for success, -ERRNO on error). An error status code indicates
>> +to the livepatching core that patching of the current klp_object is not
>> +safe and to stop the current patching request. (When no pre-patch
>> +callback is provided, the transition is assumed to be safe.) If a
>> +pre-patch callback returns failure, the kernel's module loader will:
>> +
>> + - Refuse to load a livepatch, if the livepatch is loaded after
>> + targeted code.
>> +
>> + or:
>> +
>> + - Refuse to load a module, if the livepatch was already successfully
>> + loaded.
>> +
>> +No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
>> +for a given klp_object if its pre-patch callback returned non-zero
>> +status.
>
> Shouldn't this be changed to what Josh proposed? That is
>
> No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
> for a given klp_object if the object failed to patch, due to a failed
> pre_patch callback or for any other reason.
>
> If the object did successfully patch, but the patch transition never
> started for some reason (e.g., if another object failed to patch),
> only the post-unpatch callback will be called.

Yeah, I thought I added to the doc, but apparently only coded it. In
between these two sentences I'd like to include your suggestion about a
reversed-transition:

--
If a patch transition is reversed, no pre-unpatch handlers will be run
(this follows the previously mentioned symmetry -- pre-unpatch callbacks
will only occur if their corresponding post-patch callback executed).
--

I think it fits better down here with the collection of misc. rules and
notes.

>> +Test 1
>> +------
>> +
>> +Test a combination of loading a kernel module and a livepatch that
>> +patches a function in the first module. (Un)load the target module
>> +before the livepatch module:
>> +
>> +- load target module
>> +- load livepatch
>> +- disable livepatch
>> +- unload target module
>> +- unload livepatch
>> +
>> +First load a target module:
>> +
>> + % insmod samples/livepatch/livepatch-callbacks-mod.ko
>> + [ 34.475708] livepatch_callbacks_mod: livepatch_callbacks_mod_init
>> +
>> +On livepatch enable, before the livepatch transition starts, pre-patch
>> +callbacks are executed for vmlinux and livepatch_callbacks_mod (those
>> +klp_objects currently loaded). After klp_objects are patched according
>> +to the klp_patch, their post-patch callbacks run and the transition
>> +completes:
>> +
>> + % insmod samples/livepatch/livepatch-callbacks-demo.ko
>> + [ 36.503719] livepatch: enabling patch 'livepatch_callbacks_demo'
>> + [ 36.504213] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
>
> s/unpatching/patching/
>
> I guess it is a copy&paste error and you can find it elsewhere too.

Oh no! This is a actually a bug from patch 3:

void klp_init_transition(struct klp_patch *patch, int state)
{
...

WARN_ON_ONCE(klp_target_state != KLP_UNDEFINED);

pr_debug("'%s': initializing %s transition\n", patch->mod->name,
klp_target_state == KLP_PATCHED ? "patching" : "unpatching");

Wow, that debug msg is going to be very confusing. I can move this
down, or just print the target "state" as passed into the function.

>
> Apart from these, the documentation is great!

Thanks, I find the test cases / doc more work than actually writing the
code. So many combinations and corner cases to such a simple idea.

>
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 194991ef9347..58403a9af54b 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -87,24 +87,49 @@ struct klp_func {
>> bool transition;
>> };
>>
>> +struct klp_object;
>> +
>> +/**
>> + * struct klp_callbacks - pre/post live-(un)patch callback structure
>> + * @pre_patch: executed before code patching
>> + * @post_patch: executed after code patching
>> + * @pre_unpatch: executed before code unpatching
>> + * @post_unpatch: executed after code unpatching
>> + *
>> + * All callbacks are optional. Only the pre-patch callback, if provided,
>> + * will be unconditionally executed. If the parent klp_object fails to
>> + * patch for any reason, including a non-zero error status returned from
>> + * the pre-patch callback, no further callbacks will be executed.
>> + */
>> +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)
>> * @patched: the object's funcs have been added to the klp_ops list
>> + * @callbacks_enabled: flag indicating if callbacks should be run
>
> "flag indicating if post-unpatch callback should be run" ?
>
> and then we could change the name to something like
> 'pre-patch_callback_enabled' (but that's really ugly).

Since we removed all the extraneous checks (for post-patch and
pre-unpatch) against this value, it's probably clearest to rename it
"post_unpatch_callback_enabled".

Initially I preferred leaving the callbacks_enabled check in every
callback execution wrapper, but if those callers will be guaranteed not
to ever invoke these routines in the wrong contexts, then it's probably
clearest to call out "post-unpatch" in its name.

>> */
>> struct klp_object {
>> /* external */
>> const char *name;
>> struct klp_func *funcs;
>> + struct klp_callbacks callbacks;
>>
>> /* internal */
>> struct kobject kobj;
>> struct module *mod;
>> bool patched;
>> + bool callbacks_enabled;
>> };
>
> How about moving callbacks_enabled to klp_callbacks structure? It belongs
> there. It is true, that we'd mix internal and external members with that.
>
> [...]

No strong preferences here. It's simple enough to change. And it would
reduce the enable flag above to "post_unpatch_enabled"

>> @@ -871,13 +882,27 @@ 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;
>> + }
>
> There is a problem here. We cycle through all enabled patches (or
> klp_transition_patch) and call klp_pre_patch_callback() everytime an
> enabled patch contains a patch for a coming module. Now, it can easily
> happen that klp_pre_patch_callback() fails. And not the first one from the
> first relevant patch, but the next one. In that case we need to call
> klp_post_unpatch_callback() for all already processed relevant patches in
> the error path.

Good test case, if I understand you correctly:

- Load target modules mod1 and mod2
- Load a livepatch that targets mod1 and mod2
- pre-patch succeeds for mod1
- pre-patch fails for mod2

and then we should:

- NOT run post-patch or pre/post-unpatch handlers for mod2
- NOT run post-patch or pre-unpatch handlers for mod1
- do run post-unpatch handler for mod1
- Refuse to load the livepatch

Does that sound right?

> Unfortunately, we need to do the same for klp_patch_object() below,
> because there is the same problem and we missed it.
>
>> +
>> ret = klp_patch_object(obj);
>> if (ret) {
>> pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
>> patch->mod->name, obj->mod->name, ret);
>> +
>> + if (patch != klp_transition_patch)
>> + klp_post_unpatch_callback(obj);
>> +
>> goto err;
>
> Here.
>
> Could you do it as a part of the patch set (or send it separately),
> please?

I can spin a v6... hopefully it's getting close to merge-able :)

>> diff --git a/samples/livepatch/livepatch-callbacks-mod.c b/samples/livepatch/livepatch-callbacks-mod.c
>> ...
>
>> +#include <linux/workqueue.h>
>> +#include <linux/delay.h>
>
> At least these two headers files are not needed here.

Removed, thanks.

-- Joe

2017-09-12 22:05:49

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

On Tue, Sep 12, 2017 at 11:48:48AM -0400, Joe Lawrence wrote:
> On 09/12/2017 04:53 AM, Miroslav Benes wrote:
> >> @@ -871,13 +882,27 @@ 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;
> >> + }
> >
> > There is a problem here. We cycle through all enabled patches (or
> > klp_transition_patch) and call klp_pre_patch_callback() everytime an
> > enabled patch contains a patch for a coming module. Now, it can easily
> > happen that klp_pre_patch_callback() fails. And not the first one from the
> > first relevant patch, but the next one. In that case we need to call
> > klp_post_unpatch_callback() for all already processed relevant patches in
> > the error path.
>
> Good test case, if I understand you correctly:
>
> - Load target modules mod1 and mod2
> - Load a livepatch that targets mod1 and mod2
> - pre-patch succeeds for mod1
> - pre-patch fails for mod2
>
> and then we should:
>
> - NOT run post-patch or pre/post-unpatch handlers for mod2
> - NOT run post-patch or pre-unpatch handlers for mod1
> - do run post-unpatch handler for mod1
> - Refuse to load the livepatch
>
> Does that sound right?

Erm, probably not...

> > Unfortunately, we need to do the same for klp_patch_object() below,
> > because there is the same problem and we missed it.
> >
> >> +
> >> ret = klp_patch_object(obj);
> >> if (ret) {
> >> pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> >> patch->mod->name, obj->mod->name, ret);
> >> +
> >> + if (patch != klp_transition_patch)
> >> + klp_post_unpatch_callback(obj);
> >> +
> >> goto err;
> >
> > Here.
> >
> > Could you do it as a part of the patch set (or send it separately),
> > please?

I've re-read this a few times, and I think I might have been originally
off-base with what I thought you were concerned about. But I think I
grok it now: the problem you pointed out arises because
klp_module_coming() iterates like so:

for each klp_patch
for each kobj in klp_patch

which means that we may have made pre-patch callbacks and patched a
given kobj for an earlier klp_patch that now fails for a later
klp_patch.

What should be the defined behavior in this case? I would expect that
we need to unpatch all similar kobjs across klp_patches which have
already been successfully patched. In turn, their post-unpatch
callbacks should be invoked.

If that's true, maybe this would make a better follow-on patch.

-- Joe

2017-09-12 22:22:51

by Josh Poimboeuf

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

On Tue, Sep 12, 2017 at 06:05:44PM -0400, Joe Lawrence wrote:
> On Tue, Sep 12, 2017 at 11:48:48AM -0400, Joe Lawrence wrote:
> > On 09/12/2017 04:53 AM, Miroslav Benes wrote:
> > >> @@ -871,13 +882,27 @@ 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;
> > >> + }
> > >
> > > There is a problem here. We cycle through all enabled patches (or
> > > klp_transition_patch) and call klp_pre_patch_callback() everytime an
> > > enabled patch contains a patch for a coming module. Now, it can easily
> > > happen that klp_pre_patch_callback() fails. And not the first one from the
> > > first relevant patch, but the next one. In that case we need to call
> > > klp_post_unpatch_callback() for all already processed relevant patches in
> > > the error path.
> >
> > Good test case, if I understand you correctly:
> >
> > - Load target modules mod1 and mod2
> > - Load a livepatch that targets mod1 and mod2
> > - pre-patch succeeds for mod1
> > - pre-patch fails for mod2
> >
> > and then we should:
> >
> > - NOT run post-patch or pre/post-unpatch handlers for mod2
> > - NOT run post-patch or pre-unpatch handlers for mod1
> > - do run post-unpatch handler for mod1
> > - Refuse to load the livepatch
> >
> > Does that sound right?
>
> Erm, probably not...
>
> > > Unfortunately, we need to do the same for klp_patch_object() below,
> > > because there is the same problem and we missed it.
> > >
> > >> +
> > >> ret = klp_patch_object(obj);
> > >> if (ret) {
> > >> pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > >> patch->mod->name, obj->mod->name, ret);
> > >> +
> > >> + if (patch != klp_transition_patch)
> > >> + klp_post_unpatch_callback(obj);
> > >> +
> > >> goto err;
> > >
> > > Here.
> > >
> > > Could you do it as a part of the patch set (or send it separately),
> > > please?
>
> I've re-read this a few times, and I think I might have been originally
> off-base with what I thought you were concerned about. But I think I
> grok it now: the problem you pointed out arises because
> klp_module_coming() iterates like so:
>
> for each klp_patch
> for each kobj in klp_patch
>
> which means that we may have made pre-patch callbacks and patched a
> given kobj for an earlier klp_patch that now fails for a later
> klp_patch.
>
> What should be the defined behavior in this case? I would expect that
> we need to unpatch all similar kobjs across klp_patches which have
> already been successfully patched. In turn, their post-unpatch
> callbacks should be invoked.
>
> If that's true, maybe this would make a better follow-on patch.

The rabbit hole seems to be getting deeper, is it really worth it? I'd
rather we just make the pre-patch handler return void and be done with
it, as Joe originally proposed.

So far, allowing the pre-patch handler to halt patching is a purely
theoretical feature, nobody even knows if we need it yet, and whether
it's worth the pain. So I'd vote to just simplify this mess and let
whoever wants the feature try to implement it :-)

--
Josh

2017-09-13 07:22:09

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

On Tue, 12 Sep 2017, Joe Lawrence wrote:

> On 09/12/2017 04:53 AM, Miroslav Benes wrote:
>
> >> +a post-unpatch handler and a post-patch with a pre-unpatch handler in
> >> +symmetry: the patch handler acquires and configures resources and the
> >> +unpatch handler tears down and releases those same resources.
> >
> > I think it is more than a typical use case. Test 9 shows that. Pre-unpatch
> > callbacks are skipped if a transition is reversed. I don't have a problem
> > with that per se, because it seems like a good approach, but maybe we
> > should describe it properly here. Am I right?
>
> I think the text was a little fuzzy in regard to what "typical" was
> referring to. How about this edit:
>
> --
> Each callback is optional, omitting one does not preclude specifying any
> other. However, the livepatching core executes the handlers in
> symmetry: pre-patch callbacks have a post-patch counterpart and

s/post-patch/post-unpatch/

> post-patch callbacks have a pre-unpatch counterpart. An unpatch
> callback will only be executed if its corresponding patch callback was
> executed. Typical use cases pair a patch handler that acquires and
> configures resources with an unpatch handler tears down and releases
> those same resources.
> --
>
> Does that clarify that the execution symmetry is fixed and that
> implementing callbacks with that property in mind is up to the caller?

Yes, thank you.

> More on the reversed transition comment below ...
>
> > Anyway, it relates to the next remark just below, which is another rule.
> > So it is not totally arbitrary.
> >
> >> +A callback 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 patch target kernel modules,
> >> +callbacks will only execute if the target module is loaded. When a
> >> +module target is (un)loaded, its callbacks will execute only if the
> >> +livepatch module is enabled.
> >> +
> >> +The pre-patch callback, if specified, is expected to return a status
> >> +code (0 for success, -ERRNO on error). An error status code indicates
> >> +to the livepatching core that patching of the current klp_object is not
> >> +safe and to stop the current patching request. (When no pre-patch
> >> +callback is provided, the transition is assumed to be safe.) If a
> >> +pre-patch callback returns failure, the kernel's module loader will:
> >> +
> >> + - Refuse to load a livepatch, if the livepatch is loaded after
> >> + targeted code.
> >> +
> >> + or:
> >> +
> >> + - Refuse to load a module, if the livepatch was already successfully
> >> + loaded.
> >> +
> >> +No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
> >> +for a given klp_object if its pre-patch callback returned non-zero
> >> +status.
> >
> > Shouldn't this be changed to what Josh proposed? That is
> >
> > No post-patch, pre-unpatch, or post-unpatch callbacks will be executed
> > for a given klp_object if the object failed to patch, due to a failed
> > pre_patch callback or for any other reason.
> >
> > If the object did successfully patch, but the patch transition never
> > started for some reason (e.g., if another object failed to patch),
> > only the post-unpatch callback will be called.
>
> Yeah, I thought I added to the doc, but apparently only coded it. In
> between these two sentences I'd like to include your suggestion about a
> reversed-transition:
>
> --
> If a patch transition is reversed, no pre-unpatch handlers will be run
> (this follows the previously mentioned symmetry -- pre-unpatch callbacks
> will only occur if their corresponding post-patch callback executed).
> --
>
> I think it fits better down here with the collection of misc. rules and
> notes.

Yes.

> >> +Test 1
> >> +------
> >> +
> >> +Test a combination of loading a kernel module and a livepatch that
> >> +patches a function in the first module. (Un)load the target module
> >> +before the livepatch module:
> >> +
> >> +- load target module
> >> +- load livepatch
> >> +- disable livepatch
> >> +- unload target module
> >> +- unload livepatch
> >> +
> >> +First load a target module:
> >> +
> >> + % insmod samples/livepatch/livepatch-callbacks-mod.ko
> >> + [ 34.475708] livepatch_callbacks_mod: livepatch_callbacks_mod_init
> >> +
> >> +On livepatch enable, before the livepatch transition starts, pre-patch
> >> +callbacks are executed for vmlinux and livepatch_callbacks_mod (those
> >> +klp_objects currently loaded). After klp_objects are patched according
> >> +to the klp_patch, their post-patch callbacks run and the transition
> >> +completes:
> >> +
> >> + % insmod samples/livepatch/livepatch-callbacks-demo.ko
> >> + [ 36.503719] livepatch: enabling patch 'livepatch_callbacks_demo'
> >> + [ 36.504213] livepatch: 'livepatch_callbacks_demo': initializing unpatching transition
> >
> > s/unpatching/patching/
> >
> > I guess it is a copy&paste error and you can find it elsewhere too.
>
> Oh no! This is a actually a bug from patch 3:
>
> void klp_init_transition(struct klp_patch *patch, int state)
> {
> ...
>
> WARN_ON_ONCE(klp_target_state != KLP_UNDEFINED);
>
> pr_debug("'%s': initializing %s transition\n", patch->mod->name,
> klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>
> Wow, that debug msg is going to be very confusing. I can move this
> down, or just print the target "state" as passed into the function.

Oh, it is a bug. You're right. I'd move it down. Target state could
cause confusion. User shouldn't need to know anything about live patching
internals.

> >
> > Apart from these, the documentation is great!
>
> Thanks, I find the test cases / doc more work than actually writing the
> code. So many combinations and corner cases to such a simple idea.
>
> >
> >> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> >> index 194991ef9347..58403a9af54b 100644
> >> --- a/include/linux/livepatch.h
> >> +++ b/include/linux/livepatch.h
> >> @@ -87,24 +87,49 @@ struct klp_func {
> >> bool transition;
> >> };
> >>
> >> +struct klp_object;
> >> +
> >> +/**
> >> + * struct klp_callbacks - pre/post live-(un)patch callback structure
> >> + * @pre_patch: executed before code patching
> >> + * @post_patch: executed after code patching
> >> + * @pre_unpatch: executed before code unpatching
> >> + * @post_unpatch: executed after code unpatching
> >> + *
> >> + * All callbacks are optional. Only the pre-patch callback, if provided,
> >> + * will be unconditionally executed. If the parent klp_object fails to
> >> + * patch for any reason, including a non-zero error status returned from
> >> + * the pre-patch callback, no further callbacks will be executed.
> >> + */
> >> +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)
> >> * @patched: the object's funcs have been added to the klp_ops list
> >> + * @callbacks_enabled: flag indicating if callbacks should be run
> >
> > "flag indicating if post-unpatch callback should be run" ?
> >
> > and then we could change the name to something like
> > 'pre-patch_callback_enabled' (but that's really ugly).
>
> Since we removed all the extraneous checks (for post-patch and
> pre-unpatch) against this value, it's probably clearest to rename it
> "post_unpatch_callback_enabled".
>
> Initially I preferred leaving the callbacks_enabled check in every
> callback execution wrapper, but if those callers will be guaranteed not
> to ever invoke these routines in the wrong contexts, then it's probably
> clearest to call out "post-unpatch" in its name.
>
> >> */
> >> struct klp_object {
> >> /* external */
> >> const char *name;
> >> struct klp_func *funcs;
> >> + struct klp_callbacks callbacks;
> >>
> >> /* internal */
> >> struct kobject kobj;
> >> struct module *mod;
> >> bool patched;
> >> + bool callbacks_enabled;
> >> };
> >
> > How about moving callbacks_enabled to klp_callbacks structure? It belongs
> > there. It is true, that we'd mix internal and external members with that.
> >
> > [...]
>
> No strong preferences here. It's simple enough to change. And it would
> reduce the enable flag above to "post_unpatch_enabled"

If everyone agrees, I'd move it to klp_callbacks structure and call it as
you propose.

Thanks,
Miroslav

2017-09-13 07:29:48

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

On Tue, 12 Sep 2017, Josh Poimboeuf wrote:

> On Tue, Sep 12, 2017 at 06:05:44PM -0400, Joe Lawrence wrote:
> > On Tue, Sep 12, 2017 at 11:48:48AM -0400, Joe Lawrence wrote:
> > > On 09/12/2017 04:53 AM, Miroslav Benes wrote:
> > > >> @@ -871,13 +882,27 @@ 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;
> > > >> + }
> > > >
> > > > There is a problem here. We cycle through all enabled patches (or
> > > > klp_transition_patch) and call klp_pre_patch_callback() everytime an
> > > > enabled patch contains a patch for a coming module. Now, it can easily
> > > > happen that klp_pre_patch_callback() fails. And not the first one from the
> > > > first relevant patch, but the next one. In that case we need to call
> > > > klp_post_unpatch_callback() for all already processed relevant patches in
> > > > the error path.
> > >
> > > Good test case, if I understand you correctly:
> > >
> > > - Load target modules mod1 and mod2
> > > - Load a livepatch that targets mod1 and mod2
> > > - pre-patch succeeds for mod1
> > > - pre-patch fails for mod2
> > >
> > > and then we should:
> > >
> > > - NOT run post-patch or pre/post-unpatch handlers for mod2
> > > - NOT run post-patch or pre-unpatch handlers for mod1
> > > - do run post-unpatch handler for mod1
> > > - Refuse to load the livepatch
> > >
> > > Does that sound right?
> >
> > Erm, probably not...
> >
> > > > Unfortunately, we need to do the same for klp_patch_object() below,
> > > > because there is the same problem and we missed it.
> > > >
> > > >> +
> > > >> ret = klp_patch_object(obj);
> > > >> if (ret) {
> > > >> pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > > >> patch->mod->name, obj->mod->name, ret);
> > > >> +
> > > >> + if (patch != klp_transition_patch)
> > > >> + klp_post_unpatch_callback(obj);
> > > >> +
> > > >> goto err;
> > > >
> > > > Here.
> > > >
> > > > Could you do it as a part of the patch set (or send it separately),
> > > > please?
> >
> > I've re-read this a few times, and I think I might have been originally
> > off-base with what I thought you were concerned about. But I think I
> > grok it now: the problem you pointed out arises because
> > klp_module_coming() iterates like so:
> >
> > for each klp_patch
> > for each kobj in klp_patch
> >
> > which means that we may have made pre-patch callbacks and patched a
> > given kobj for an earlier klp_patch that now fails for a later
> > klp_patch.

Yes, that's the scenario.

> > What should be the defined behavior in this case? I would expect that
> > we need to unpatch all similar kobjs across klp_patches which have
> > already been successfully patched. In turn, their post-unpatch
> > callbacks should be invoked.
> >
> > If that's true, maybe this would make a better follow-on patch.

Yes, you'd need to loop back, unpatch everything and call post-unpatch
callbacks too. Probably too much for this patch set, so we can deal with
the problem later.

> The rabbit hole seems to be getting deeper, is it really worth it? I'd
> rather we just make the pre-patch handler return void and be done with
> it, as Joe originally proposed.
>
> So far, allowing the pre-patch handler to halt patching is a purely
> theoretical feature, nobody even knows if we need it yet, and whether
> it's worth the pain. So I'd vote to just simplify this mess and let
> whoever wants the feature try to implement it :-)

Unfortunately, the problem is there even without Joe's callbacks. If it
was only a problem of callbacks, I'd go along with you. I see two options.

1. we'll fix this for klp_patch_object(). Then callbacks' problem would be
simple to solve, because the infrastructure would be already there.

2. we'll remove any error handling from klp_coming_module and we'll allow
target modules to load even with a patching failure. This doesn't seem to
be the right approach...

Miroslav

2017-09-13 13:53:23

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

On 09/13/2017 03:29 AM, Miroslav Benes wrote:
> On Tue, 12 Sep 2017, Josh Poimboeuf wrote:
>
>> On Tue, Sep 12, 2017 at 06:05:44PM -0400, Joe Lawrence wrote:
>>> On Tue, Sep 12, 2017 at 11:48:48AM -0400, Joe Lawrence wrote:
>>> I've re-read this a few times, and I think I might have been originally
>>> off-base with what I thought you were concerned about. But I think I
>>> grok it now: the problem you pointed out arises because
>>> klp_module_coming() iterates like so:
>>>
>>> for each klp_patch
>>> for each kobj in klp_patch
>>>
>>> which means that we may have made pre-patch callbacks and patched a
>>> given kobj for an earlier klp_patch that now fails for a later
>>> klp_patch.
>
> Yes, that's the scenario.
>
>>> What should be the defined behavior in this case? I would expect that
>>> we need to unpatch all similar kobjs across klp_patches which have
>>> already been successfully patched. In turn, their post-unpatch
>>> callbacks should be invoked.
>>>
>>> If that's true, maybe this would make a better follow-on patch.
>
> Yes, you'd need to loop back, unpatch everything and call post-unpatch
> callbacks too. Probably too much for this patch set, so we can deal with
> the problem later.

Completely untested/compiled, but something like (sans callbacks)?

--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -894,6 +894,29 @@ int klp_module_coming(struct module *mod)
pr_warn("patch '%s' failed for module '%s', refusing to load module
'%s'\n",
patch->mod->name, obj->mod->name, obj->mod->name);
+
+ /*
+ * Run back through the patch list and unpatch any klp_object
+ * that matches the one we errored on above.
+ */
+ list_for_each_entry(patch, &klp_patches, list) {
+
+ if (!patch->enabled || patch == klp_transition_patch)
+ continue;
+
+ klp_for_each_object(patch, obj) {
+
+ if (!obj->patched ||
+ !klp_is_module(obj) ||
+ strcmp(obj->name, mod->name))
+ continue;
+
+ klp_unpatch_object(obj);
+ /* post-unpatch callback would go here */
+
+ break;
+ }
+ }
+
mod->klp_alive = false;
klp_free_object_loaded(obj);
mutex_unlock(&klp_mutex);

>> The rabbit hole seems to be getting deeper, is it really worth it? I'd
>> rather we just make the pre-patch handler return void and be done with
>> it, as Joe originally proposed.
>>
>> So far, allowing the pre-patch handler to halt patching is a purely
>> theoretical feature, nobody even knows if we need it yet, and whether
>> it's worth the pain. So I'd vote to just simplify this mess and let
>> whoever wants the feature try to implement it :-)

"Rabbit hole" is an apt description :) the question is whether this is
the last hurdle, or just one of many more coming our way. I'd like to
think the former, but I'm the guy down in the rabbit hole, so my
perspective is tainted.

Ripping this out of the code would be relatively easy. Re-doing the
tests/documentation/comments will be a bit of work to reword everything.

> Unfortunately, the problem is there even without Joe's callbacks. If it
> was only a problem of callbacks, I'd go along with you. I see two options.
>
> 1. we'll fix this for klp_patch_object(). Then callbacks' problem would be
> simple to solve, because the infrastructure would be already there.

If the bugfix is like the above, then it's not too bad a diversion. I
can run some tests this afternoon to try and tackle this.

> 2. we'll remove any error handling from klp_coming_module and we'll allow
> target modules to load even with a patching failure. This doesn't seem to
> be the right approach...
I agree.

-- Joe

2017-09-13 21:37:31

[permalink] [raw]

Subject: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails

Hi Miroslav,

I worked out the code that I posted earlier today and I think this could
address the multiple-patch module_coming() issue you pointed out.

Note that this was tacked onto the end of the "[PATCH v5 0/3] livepatch
callbacks" patchset, so it includes unpatching callbacks. I can easily
strip those out (and remove the additional debugging pr_'s) and make
this a stand-alone patch that would apply before the callback patchset.

See the test case below.

-- Joe

Test X
------

Multiple livepatches targeting the same klp_objects may be loaded at
the same time. If a target module loads and any of the livepatch's
pre-patch callbacks fail, then the module is not allowed to load.
Furthermore, any livepatches that that did succeed will be reverted
(only the incoming module / klp_object) and their pre/post-unpatch
callbacks executed.

- load livepatch
- load livepatch2
- load livepatch3
- setup livepatch3 pre-patch return of -ENODEV
- load target module (should fail)
- disable livepatch3
- disable livepatch2
- disable livepatch
- unload livepatch3
- unload livepatch2
- unload livepatch

Load three livepatches, each target a livepatch_callbacks_mod module and
vmlinux:

% insmod samples/livepatch/livepatch-callbacks-demo.ko
[ 26.032048] livepatch_callbacks_demo: module verification failed: signature and/or required key missing - tainting kernel
[ 26.033701] livepatch: enabling patch 'livepatch_callbacks_demo'
[ 26.034294] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[ 26.034850] livepatch: 'livepatch_callbacks_demo': starting patching transition
[ 27.743212] livepatch_callbacks_demo: post_patch_callback: vmlinux
[ 27.744130] livepatch: 'livepatch_callbacks_demo': patching complete

% insmod samples/livepatch/livepatch-callbacks-demo2.ko
[ 29.120553] livepatch: enabling patch 'livepatch_callbacks_demo2'
[ 29.121077] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
[ 29.121610] livepatch: 'livepatch_callbacks_demo2': starting patching transition
[ 30.751215] livepatch_callbacks_demo2: post_patch_callback: vmlinux
[ 30.751786] livepatch: 'livepatch_callbacks_demo2': patching complete

% insmod samples/livepatch/livepatch-callbacks-demo3.ko
[ 32.144285] livepatch: enabling patch 'livepatch_callbacks_demo3'
[ 32.144779] livepatch_callbacks_demo3: pre_patch_callback: vmlinux
[ 32.145360] livepatch: 'livepatch_callbacks_demo3': starting patching transition
[ 33.695211] livepatch_callbacks_demo3: post_patch_callback: vmlinux
[ 33.695739] livepatch: 'livepatch_callbacks_demo3': patching complete

Setup the third livepatch to fail its pre-patch callback when the target
module is loaded:

% echo samples/livepatch/livepatch-callbacks-demo3.ko > /sys/module/livepatch_callbacks_demo3/parameters/pre_patch_ret

Load the target module:

% insmod samples/livepatch/livepatch-callbacks-mod.ko

The first livepatch pre-patch callback succeeds, the klp_object is
patched, and its post-patch callback is executed:

[ 38.210512] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
[ 38.211430] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[ 38.212426] livepatch: JL: klp_patch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
[ 38.213243] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init

Likewise for the second livepatch:

[ 38.214578] livepatch: applying patch 'livepatch_callbacks_demo2' to loading module 'livepatch_callbacks_mod'
[ 38.215754] livepatch_callbacks_demo2: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[ 38.217066] livepatch: JL: klp_patch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
[ 38.218072] livepatch_callbacks_demo2: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init

But the third livepatch fails its pre-patch callback:

[ 38.219290] livepatch: applying patch 'livepatch_callbacks_demo3' to loading module 'livepatch_callbacks_mod'
[ 38.220182] livepatch_callbacks_demo3: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[ 38.221256] livepatch: pre-patch callback failed for object 'livepatch_callbacks_mod'

We refuse to load the target module:

[ 38.221906] livepatch: patch 'livepatch_callbacks_demo3' failed for module 'livepatch_callbacks_mod', refusing to load module 'livepatch_callbacks_mod'

So we double back and unpatch (including pre-unpatch and post-unpatch
callbacks) the first livepatch, then the second:

[ 38.223080] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[ 38.223966] livepatch: JL: klp_unpatch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
[ 38.224980] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[ 38.226174] livepatch_callbacks_demo2: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[ 38.227127] livepatch: JL: klp_unpatch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
[ 38.228231] livepatch_callbacks_demo2: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init

Finally the module loader reports an error:

[ 38.242684] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device

Clean it all up:

% echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo3/enabled
[ 41.248198] livepatch_callbacks_demo3: pre_unpatch_callback: vmlinux
[ 41.248799] livepatch: 'livepatch_callbacks_demo3': starting unpatching transition
[ 42.719135] livepatch_callbacks_demo3: post_unpatch_callback: vmlinux
[ 42.719622] livepatch: 'livepatch_callbacks_demo3': unpatching complete

% echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
[ 47.269103] livepatch_callbacks_demo2: pre_unpatch_callback: vmlinux
[ 47.269682] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
[ 48.735253] livepatch_callbacks_demo2: post_unpatch_callback: vmlinux
[ 48.735928] livepatch: 'livepatch_callbacks_demo2': unpatching complete

% echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
[ 53.289287] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
[ 53.289987] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
[ 54.751146] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[ 54.751656] livepatch: 'livepatch_callbacks_demo': unpatching complete

% rmmod samples/livepatch/livepatch-callbacks-demo3.ko
% rmmod samples/livepatch/livepatch-callbacks-demo2.ko
% rmmod samples/livepatch/livepatch-callbacks-demo.ko

-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--

>From b80b90cb54b498d2b1165d409ce4b0ca47610b36 Mon Sep 17 00:00:00 2001
From: Joe Lawrence <[email protected]>
Date: Wed, 13 Sep 2017 16:51:13 -0400
Subject: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails

When an incoming module is considered for livepatching by
klp_module_coming(), it iterates over multiple patches and multiple
kernel objects in this order:

list_for_each_entry(patch, &klp_patches, list) {
klp_for_each_object(patch, obj) {

which means that if one of the kernel objects fail to patch for whatever
reason, klp_module_coming()'s error path should double back and unpatch
any previous kernel object that was patched for a previous patch.

Reported-by: Miroslav Benes <[email protected]>
Signed-off-by: Joe Lawrence <[email protected]>
---
kernel/livepatch/core.c | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index aca62c4b8616..7f5192618cc8 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -889,6 +889,8 @@ int klp_module_coming(struct module *mod)
goto err;
}

+pr_err("JL: klp_patch_object(%p) patch=%p obj->name: %s\n", obj, patch, obj->name);
+
ret = klp_patch_object(obj);
if (ret) {
pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
@@ -919,7 +921,33 @@ int klp_module_coming(struct module *mod)
pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
patch->mod->name, obj->mod->name, obj->mod->name);
mod->klp_alive = false;
- klp_free_object_loaded(obj);
+
+ /*
+ * Run back through the patch list and unpatch any klp_object that
+ * was patched before hitting an error above.
+ */
+
+ list_for_each_entry(patch, &klp_patches, list) {
+
+ if (!patch->enabled || patch == klp_transition_patch)
+ continue;
+
+ klp_for_each_object(patch, obj) {
+
+ if (!obj->patched || !klp_is_module(obj) ||
+ strcmp(obj->name, mod->name))
+ continue;
+
+ klp_pre_unpatch_callback(obj);
+pr_err("JL: klp_unpatch_object(%p) patch=%p obj->name: %s\n", obj, patch, obj->name);
+ klp_unpatch_object(obj);
+ klp_post_unpatch_callback(obj);
+ klp_free_object_loaded(obj);
+
+ break;
+ }
+ }
+
mutex_unlock(&klp_mutex);

return ret;
--
2.7.5

2017-09-20 11:19:09

[permalink] [raw]

Subject: Re: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails

On Wed, 13 Sep 2017, Joe Lawrence wrote:

> Hi Miroslav,

Hi,

sorry for the late response. I'm also travelling now and we have SUSECon
conference next week, so just a quick answer. It looks ok at first glance,
but I need to take a proper look.

> I worked out the code that I posted earlier today and I think this could
> address the multiple-patch module_coming() issue you pointed out.
>
> Note that this was tacked onto the end of the "[PATCH v5 0/3] livepatch
> callbacks" patchset, so it includes unpatching callbacks. I can easily
> strip those out (and remove the additional debugging pr_'s) and make
> this a stand-alone patch that would apply before the callback patchset.

I think this would be better. Strip callbacks out and send this either
separately (and base callbacks patch set on this), or make it 1/n of the
series.

> See the test case below.
>
> -- Joe
>
> Test X
> ------
>
> Multiple livepatches targeting the same klp_objects may be loaded at
> the same time. If a target module loads and any of the livepatch's
> pre-patch callbacks fail, then the module is not allowed to load.
> Furthermore, any livepatches that that did succeed will be reverted
> (only the incoming module / klp_object) and their pre/post-unpatch
> callbacks executed.
>
> - load livepatch
> - load livepatch2
> - load livepatch3
> - setup livepatch3 pre-patch return of -ENODEV
> - load target module (should fail)
> - disable livepatch3
> - disable livepatch2
> - disable livepatch
> - unload livepatch3
> - unload livepatch2
> - unload livepatch
>
>
> Load three livepatches, each target a livepatch_callbacks_mod module and
> vmlinux:
>
> % insmod samples/livepatch/livepatch-callbacks-demo.ko
> [ 26.032048] livepatch_callbacks_demo: module verification failed: signature and/or required key missing - tainting kernel
> [ 26.033701] livepatch: enabling patch 'livepatch_callbacks_demo'
> [ 26.034294] livepatch_callbacks_demo: pre_patch_callback: vmlinux
> [ 26.034850] livepatch: 'livepatch_callbacks_demo': starting patching transition
> [ 27.743212] livepatch_callbacks_demo: post_patch_callback: vmlinux
> [ 27.744130] livepatch: 'livepatch_callbacks_demo': patching complete
>
> % insmod samples/livepatch/livepatch-callbacks-demo2.ko
> [ 29.120553] livepatch: enabling patch 'livepatch_callbacks_demo2'
> [ 29.121077] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
> [ 29.121610] livepatch: 'livepatch_callbacks_demo2': starting patching transition
> [ 30.751215] livepatch_callbacks_demo2: post_patch_callback: vmlinux
> [ 30.751786] livepatch: 'livepatch_callbacks_demo2': patching complete
>
> % insmod samples/livepatch/livepatch-callbacks-demo3.ko
> [ 32.144285] livepatch: enabling patch 'livepatch_callbacks_demo3'
> [ 32.144779] livepatch_callbacks_demo3: pre_patch_callback: vmlinux
> [ 32.145360] livepatch: 'livepatch_callbacks_demo3': starting patching transition
> [ 33.695211] livepatch_callbacks_demo3: post_patch_callback: vmlinux
> [ 33.695739] livepatch: 'livepatch_callbacks_demo3': patching complete
>
> Setup the third livepatch to fail its pre-patch callback when the target
> module is loaded:
>
> % echo samples/livepatch/livepatch-callbacks-demo3.ko > /sys/module/livepatch_callbacks_demo3/parameters/pre_patch_ret
>
> Load the target module:
>
> % insmod samples/livepatch/livepatch-callbacks-mod.ko
>
> The first livepatch pre-patch callback succeeds, the klp_object is
> patched, and its post-patch callback is executed:
>
> [ 38.210512] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
> [ 38.211430] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> [ 38.212426] livepatch: JL: klp_patch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
> [ 38.213243] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>
> Likewise for the second livepatch:
>
> [ 38.214578] livepatch: applying patch 'livepatch_callbacks_demo2' to loading module 'livepatch_callbacks_mod'
> [ 38.215754] livepatch_callbacks_demo2: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> [ 38.217066] livepatch: JL: klp_patch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
> [ 38.218072] livepatch_callbacks_demo2: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>
> But the third livepatch fails its pre-patch callback:
>
> [ 38.219290] livepatch: applying patch 'livepatch_callbacks_demo3' to loading module 'livepatch_callbacks_mod'
> [ 38.220182] livepatch_callbacks_demo3: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> [ 38.221256] livepatch: pre-patch callback failed for object 'livepatch_callbacks_mod'
>
> We refuse to load the target module:
>
> [ 38.221906] livepatch: patch 'livepatch_callbacks_demo3' failed for module 'livepatch_callbacks_mod', refusing to load module 'livepatch_callbacks_mod'
>
> So we double back and unpatch (including pre-unpatch and post-unpatch
> callbacks) the first livepatch, then the second:
>
> [ 38.223080] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> [ 38.223966] livepatch: JL: klp_unpatch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
> [ 38.224980] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> [ 38.226174] livepatch_callbacks_demo2: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> [ 38.227127] livepatch: JL: klp_unpatch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
> [ 38.228231] livepatch_callbacks_demo2: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
>
> Finally the module loader reports an error:
>
> [ 38.242684] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device
>
> Clean it all up:
>
> % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo3/enabled
> [ 41.248198] livepatch_callbacks_demo3: pre_unpatch_callback: vmlinux
> [ 41.248799] livepatch: 'livepatch_callbacks_demo3': starting unpatching transition
> [ 42.719135] livepatch_callbacks_demo3: post_unpatch_callback: vmlinux
> [ 42.719622] livepatch: 'livepatch_callbacks_demo3': unpatching complete
>
> % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
> [ 47.269103] livepatch_callbacks_demo2: pre_unpatch_callback: vmlinux
> [ 47.269682] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
> [ 48.735253] livepatch_callbacks_demo2: post_unpatch_callback: vmlinux
> [ 48.735928] livepatch: 'livepatch_callbacks_demo2': unpatching complete
>
> % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
> [ 53.289287] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
> [ 53.289987] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
> [ 54.751146] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
> [ 54.751656] livepatch: 'livepatch_callbacks_demo': unpatching complete
>
> % rmmod samples/livepatch/livepatch-callbacks-demo3.ko
> % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
> % rmmod samples/livepatch/livepatch-callbacks-demo.ko
>
>
> -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
>
> >From b80b90cb54b498d2b1165d409ce4b0ca47610b36 Mon Sep 17 00:00:00 2001
> From: Joe Lawrence <[email protected]>
> Date: Wed, 13 Sep 2017 16:51:13 -0400
> Subject: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails
>
> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
>
> list_for_each_entry(patch, &klp_patches, list) {
> klp_for_each_object(patch, obj) {
>
> which means that if one of the kernel objects fail to patch for whatever
> reason, klp_module_coming()'s error path should double back and unpatch
> any previous kernel object that was patched for a previous patch.
>
> Reported-by: Miroslav Benes <[email protected]>
> Signed-off-by: Joe Lawrence <[email protected]>
> ---
> kernel/livepatch/core.c | 30 +++++++++++++++++++++++++++++-
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index aca62c4b8616..7f5192618cc8 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -889,6 +889,8 @@ int klp_module_coming(struct module *mod)
> goto err;
> }
>
> +pr_err("JL: klp_patch_object(%p) patch=%p obj->name: %s\n", obj, patch, obj->name);
> +
> ret = klp_patch_object(obj);
> if (ret) {
> pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> @@ -919,7 +921,33 @@ int klp_module_coming(struct module *mod)
> pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
> patch->mod->name, obj->mod->name, obj->mod->name);
> mod->klp_alive = false;
> - klp_free_object_loaded(obj);
> +
> + /*
> + * Run back through the patch list and unpatch any klp_object that
> + * was patched before hitting an error above.
> + */
> +
> + list_for_each_entry(patch, &klp_patches, list) {

I think it would be safer to use
list_for_each_entry_{continue,from}_reverse() iterator (probably
_continue_reverse(), because the current patch failed). That would unpatch
the objects in the correct order (see your test case above) and it is
also an optimization because you'd process only those patches which were
walked through during the first loop.

> +
> + if (!patch->enabled || patch == klp_transition_patch)
> + continue;

Is the second part with klp_transition_patch correct? Yes, we need to skip
disabled patches. No question about that. But klp_transition_patch seems
odd. It is true, that (if I am not mistaken) klp_transition_patch is the
last patch in patches list which is relevant (because we cannot
enable/disable any random patch in the list). If that failed to patch,
you wouldn't need to worry about it anyway, because you need to process
previous patches only. Am I missing something? So I think it can stay,
yes. But I'd like to understand it.

Miroslav

> + klp_for_each_object(patch, obj) {
> +
> + if (!obj->patched || !klp_is_module(obj) ||
> + strcmp(obj->name, mod->name))
> + continue;
> +
> + klp_pre_unpatch_callback(obj);
> +pr_err("JL: klp_unpatch_object(%p) patch=%p obj->name: %s\n", obj, patch, obj->name);
> + klp_unpatch_object(obj);
> + klp_post_unpatch_callback(obj);
> + klp_free_object_loaded(obj);
> +
> + break;
> + }
> + }
> +
> mutex_unlock(&klp_mutex);
>
> return ret;
> --
> 2.7.5
>

2017-09-20 15:17:23

[permalink] [raw]

Subject: Re: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails

On Wed, Sep 20, 2017 at 01:19:05PM +0200, Miroslav Benes wrote:
> On Wed, 13 Sep 2017, Joe Lawrence wrote:
>
> > Hi Miroslav,
>
> Hi,
>
> sorry for the late response. I'm also travelling now and we have SUSECon
> conference next week, so just a quick answer. It looks ok at first glance,
> but I need to take a proper look.

No problem, thanks for the update and safe travels.

> > I worked out the code that I posted earlier today and I think this could
> > address the multiple-patch module_coming() issue you pointed out.
> >
> > Note that this was tacked onto the end of the "[PATCH v5 0/3] livepatch
> > callbacks" patchset, so it includes unpatching callbacks. I can easily
> > strip those out (and remove the additional debugging pr_'s) and make
> > this a stand-alone patch that would apply before the callback patchset.
>
> I think this would be better. Strip callbacks out and send this either
> separately (and base callbacks patch set on this), or make it 1/n of the
> series.

Agreed.

> > See the test case below.
> >
> > -- Joe
> >
> > Test X
> > ------
> >
> > Multiple livepatches targeting the same klp_objects may be loaded at
> > the same time. If a target module loads and any of the livepatch's
> > pre-patch callbacks fail, then the module is not allowed to load.
> > Furthermore, any livepatches that that did succeed will be reverted
> > (only the incoming module / klp_object) and their pre/post-unpatch
> > callbacks executed.
> >
> > - load livepatch
> > - load livepatch2
> > - load livepatch3
> > - setup livepatch3 pre-patch return of -ENODEV
> > - load target module (should fail)
> > - disable livepatch3
> > - disable livepatch2
> > - disable livepatch
> > - unload livepatch3
> > - unload livepatch2
> > - unload livepatch
> >
> >
> > Load three livepatches, each target a livepatch_callbacks_mod module and
> > vmlinux:
> >
> > % insmod samples/livepatch/livepatch-callbacks-demo.ko
> > [ 26.032048] livepatch_callbacks_demo: module verification failed: signature and/or required key missing - tainting kernel
> > [ 26.033701] livepatch: enabling patch 'livepatch_callbacks_demo'
> > [ 26.034294] livepatch_callbacks_demo: pre_patch_callback: vmlinux
> > [ 26.034850] livepatch: 'livepatch_callbacks_demo': starting patching transition
> > [ 27.743212] livepatch_callbacks_demo: post_patch_callback: vmlinux
> > [ 27.744130] livepatch: 'livepatch_callbacks_demo': patching complete
> >
> > % insmod samples/livepatch/livepatch-callbacks-demo2.ko
> > [ 29.120553] livepatch: enabling patch 'livepatch_callbacks_demo2'
> > [ 29.121077] livepatch_callbacks_demo2: pre_patch_callback: vmlinux
> > [ 29.121610] livepatch: 'livepatch_callbacks_demo2': starting patching transition
> > [ 30.751215] livepatch_callbacks_demo2: post_patch_callback: vmlinux
> > [ 30.751786] livepatch: 'livepatch_callbacks_demo2': patching complete
> >
> > % insmod samples/livepatch/livepatch-callbacks-demo3.ko
> > [ 32.144285] livepatch: enabling patch 'livepatch_callbacks_demo3'
> > [ 32.144779] livepatch_callbacks_demo3: pre_patch_callback: vmlinux
> > [ 32.145360] livepatch: 'livepatch_callbacks_demo3': starting patching transition
> > [ 33.695211] livepatch_callbacks_demo3: post_patch_callback: vmlinux
> > [ 33.695739] livepatch: 'livepatch_callbacks_demo3': patching complete
> >
> > Setup the third livepatch to fail its pre-patch callback when the target
> > module is loaded:
> >
> > % echo samples/livepatch/livepatch-callbacks-demo3.ko > /sys/module/livepatch_callbacks_demo3/parameters/pre_patch_ret
> >
> > Load the target module:
> >
> > % insmod samples/livepatch/livepatch-callbacks-mod.ko
> >
> > The first livepatch pre-patch callback succeeds, the klp_object is
> > patched, and its post-patch callback is executed:
> >
> > [ 38.210512] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
> > [ 38.211430] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > [ 38.212426] livepatch: JL: klp_patch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
> > [ 38.213243] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> >
> > Likewise for the second livepatch:
> >
> > [ 38.214578] livepatch: applying patch 'livepatch_callbacks_demo2' to loading module 'livepatch_callbacks_mod'
> > [ 38.215754] livepatch_callbacks_demo2: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > [ 38.217066] livepatch: JL: klp_patch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
> > [ 38.218072] livepatch_callbacks_demo2: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> >
> > But the third livepatch fails its pre-patch callback:
> >
> > [ 38.219290] livepatch: applying patch 'livepatch_callbacks_demo3' to loading module 'livepatch_callbacks_mod'
> > [ 38.220182] livepatch_callbacks_demo3: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > [ 38.221256] livepatch: pre-patch callback failed for object 'livepatch_callbacks_mod'
> >
> > We refuse to load the target module:
> >
> > [ 38.221906] livepatch: patch 'livepatch_callbacks_demo3' failed for module 'livepatch_callbacks_mod', refusing to load module 'livepatch_callbacks_mod'
> >
> > So we double back and unpatch (including pre-unpatch and post-unpatch
> > callbacks) the first livepatch, then the second:
> >
> > [ 38.223080] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > [ 38.223966] livepatch: JL: klp_unpatch_object(ffffffffc02a9128) patch=ffffffffc02a9000 obj->name: livepatch_callbacks_mod
> > [ 38.224980] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > [ 38.226174] livepatch_callbacks_demo2: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> > [ 38.227127] livepatch: JL: klp_unpatch_object(ffffffffc02ae128) patch=ffffffffc02ae000 obj->name: livepatch_callbacks_mod
> > [ 38.228231] livepatch_callbacks_demo2: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
> >
> > Finally the module loader reports an error:
> >
> > [ 38.242684] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device
> >
> > Clean it all up:
> >
> > % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo3/enabled
> > [ 41.248198] livepatch_callbacks_demo3: pre_unpatch_callback: vmlinux
> > [ 41.248799] livepatch: 'livepatch_callbacks_demo3': starting unpatching transition
> > [ 42.719135] livepatch_callbacks_demo3: post_unpatch_callback: vmlinux
> > [ 42.719622] livepatch: 'livepatch_callbacks_demo3': unpatching complete
> >
> > % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo2/enabled
> > [ 47.269103] livepatch_callbacks_demo2: pre_unpatch_callback: vmlinux
> > [ 47.269682] livepatch: 'livepatch_callbacks_demo2': starting unpatching transition
> > [ 48.735253] livepatch_callbacks_demo2: post_unpatch_callback: vmlinux
> > [ 48.735928] livepatch: 'livepatch_callbacks_demo2': unpatching complete
> >
> > % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
> > [ 53.289287] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
> > [ 53.289987] livepatch: 'livepatch_callbacks_demo': starting unpatching transition
> > [ 54.751146] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
> > [ 54.751656] livepatch: 'livepatch_callbacks_demo': unpatching complete
> >
> > % rmmod samples/livepatch/livepatch-callbacks-demo3.ko
> > % rmmod samples/livepatch/livepatch-callbacks-demo2.ko
> > % rmmod samples/livepatch/livepatch-callbacks-demo.ko
> >
> >
> > -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
> >
> > >From b80b90cb54b498d2b1165d409ce4b0ca47610b36 Mon Sep 17 00:00:00 2001
> > From: Joe Lawrence <[email protected]>
> > Date: Wed, 13 Sep 2017 16:51:13 -0400
> > Subject: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails
> >
> > When an incoming module is considered for livepatching by
> > klp_module_coming(), it iterates over multiple patches and multiple
> > kernel objects in this order:
> >
> > list_for_each_entry(patch, &klp_patches, list) {
> > klp_for_each_object(patch, obj) {
> >
> > which means that if one of the kernel objects fail to patch for whatever
> > reason, klp_module_coming()'s error path should double back and unpatch
> > any previous kernel object that was patched for a previous patch.
> >
> > Reported-by: Miroslav Benes <[email protected]>
> > Signed-off-by: Joe Lawrence <[email protected]>
> > ---
> > kernel/livepatch/core.c | 30 +++++++++++++++++++++++++++++-
> > 1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index aca62c4b8616..7f5192618cc8 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -889,6 +889,8 @@ int klp_module_coming(struct module *mod)
> > goto err;
> > }
> >
> > +pr_err("JL: klp_patch_object(%p) patch=%p obj->name: %s\n", obj, patch, obj->name);
> > +
> > ret = klp_patch_object(obj);
> > if (ret) {
> > pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > @@ -919,7 +921,33 @@ int klp_module_coming(struct module *mod)
> > pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
> > patch->mod->name, obj->mod->name, obj->mod->name);
> > mod->klp_alive = false;
> > - klp_free_object_loaded(obj);
> > +
> > + /*
> > + * Run back through the patch list and unpatch any klp_object that
> > + * was patched before hitting an error above.
> > + */
> > +
> > + list_for_each_entry(patch, &klp_patches, list) {
>
> I think it would be safer to use
> list_for_each_entry_{continue,from}_reverse() iterator (probably
> _continue_reverse(), because the current patch failed). That would unpatch
> the objects in the correct order (see your test case above) and it is
> also an optimization because you'd process only those patches which were
> walked through during the first loop.

I had originally tested with list_for_each_entry_reverse(), but then
noticed that klp_module_going() iterates through the patches using
list_for_each_entry(). Strictly speaking, there is also the matter of
the klp_objects, but we don't have a klp_for_each_object_reverse() macro
that would complete the mirrored-symmetry.

For pre/post-(un)patch callbacks, they are supposed to be independent
from each other anyway, so theoretically their execution order shouldn't
matter.

That said, maybe we can compromise on list_for_each_entry_reverse() for
both klp_module_going() and the klp_module_coming() error path above?

> > +
> > + if (!patch->enabled || patch == klp_transition_patch)
> > + continue;
>
> Is the second part with klp_transition_patch correct? Yes, we need to skip
> disabled patches. No question about that. But klp_transition_patch seems
> odd. It is true, that (if I am not mistaken) klp_transition_patch is the
> last patch in patches list which is relevant (because we cannot
> enable/disable any random patch in the list). If that failed to patch,
> you wouldn't need to worry about it anyway, because you need to process
> previous patches only. Am I missing something? So I think it can stay,
> yes. But I'd like to understand it.

You might be correct here, I basically copy/pasted it from the code
above with the understanding that the klp_transition_patch was handled
by klp_complete_transition(). If it is an unneeded check, then I can
remove it.

Thanks,

-- Joe

2017-09-26 14:49:15

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

On Thu 2017-08-31 10:53:51, Joe Lawrence wrote:
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e43c78f..aca62c4b8616 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)
> {
> @@ -285,6 +280,8 @@ static int klp_write_object_relocations(struct module *pmod,
>
> static int __klp_disable_patch(struct klp_patch *patch)
> {
> + struct klp_object *obj;
> +
> if (klp_transition_patch)
> return -EBUSY;
>
> @@ -295,6 +292,10 @@ static int __klp_disable_patch(struct klp_patch *patch)
>
> klp_init_transition(patch, KLP_UNPATCHED);
>
> + klp_for_each_object(patch, obj)
> + if (patch->enabled && obj->patched)
> + klp_pre_unpatch_callback(obj);
> +
> /*
> * Enforce the order of the func->transition writes in
> * klp_init_transition() and the TIF_PATCH_PENDING writes in
> @@ -388,13 +389,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");

We should call klp_post_unpatch_callback(obj) here to make it
synchronous.

Well, what about calling:

klp_pre_patch_callback() inside klp_patch_object() and
klp_post_unpatch_callback() inside klp_unpatch_object()

By other words, we would do the two operations. It would have
two advantages:

+ error handling for free
+ no need for the strange callbacks_enabled flag

It would require the more strict consistency model if there
is a dependency between the callbacks and patches from various
modules. But we would probably need the consistency model
in this case anyway.

> + goto err;

> }
> }
>

Otherwise I think that we are getting close.

Best Regards,
Petr

PS: I hope that the above problem and solution has not been mentioned
yet. I am sorry if it was. I am a bit lost in many mails after
vacation, sickness, and conference.

2017-09-26 19:01:56

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

On 09/26/2017 10:49 AM, Petr Mladek wrote:
> On Thu 2017-08-31 10:53:51, Joe Lawrence wrote:
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index b9628e43c78f..aca62c4b8616 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)
>> {
>> @@ -285,6 +280,8 @@ static int klp_write_object_relocations(struct module *pmod,
>>
>> static int __klp_disable_patch(struct klp_patch *patch)
>> {
>> + struct klp_object *obj;
>> +
>> if (klp_transition_patch)
>> return -EBUSY;
>>
>> @@ -295,6 +292,10 @@ static int __klp_disable_patch(struct klp_patch *patch)
>>
>> klp_init_transition(patch, KLP_UNPATCHED);
>>
>> + klp_for_each_object(patch, obj)
>> + if (patch->enabled && obj->patched)
>> + klp_pre_unpatch_callback(obj);
>> +
>> /*
>> * Enforce the order of the func->transition writes in
>> * klp_init_transition() and the TIF_PATCH_PENDING writes in
>> @@ -388,13 +389,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");
>
> We should call klp_post_unpatch_callback(obj) here to make it
> synchronous.

Are you talking about the error path? As its coded here,
klp_cancel_transition() will call klp_complete_transition() with
klp_target_state = KLP_UNPATCHED and then klp_complete_transition()'s
done: code will call klp_post_unpatch_callback() on all the necessary
kobj's. Is there something asynchronous about that?

> Well, what about calling:
>
> klp_pre_patch_callback() inside klp_patch_object() and
> klp_post_unpatch_callback() inside klp_unpatch_object()

v1 started out that way, but we migrated to placing these around the
callers of klp_(un)patch_object() to try and better line up the
locations of the pre- hooks with the post- hook locations.

I can take a second look at reversing this decision, but that may take a
little time while I page all the testing corner cases back into my brain :)

> By other words, we would do the two operations. It would have
> two advantages:
>
> + error handling for free
> + no need for the strange callbacks_enabled flag

Indeed, it would be nice to ditch that callbacks_enabled wart.

> It would require the more strict consistency model if there
> is a dependency between the callbacks and patches from various
> modules. But we would probably need the consistency model
> in this case anyway.
>
>> + goto err;
>
>> }
>> }
>>
>
> Otherwise I think that we are getting close.
>
> Best Regards,
> Petr
>
> PS: I hope that the above problem and solution has not been mentioned
> yet. I am sorry if it was. I am a bit lost in many mails after
> vacation, sickness, and conference.

I think the only other outstanding issue before rolling a v6 is the one
that Miroslav raised about the error path in klp_module_coming():

https://marc.info/?l=linux-kernel&m=150590635602784&w=2
https://marc.info/?l=linux-kernel&m=150592065007463&w=2

Thanks,

-- Joe

2017-09-27 08:02:45

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] livepatch: add (un)patch callbacks

On Tue 2017-09-26 15:01:52, Joe Lawrence wrote:
> On 09/26/2017 10:49 AM, Petr Mladek wrote:
> > On Thu 2017-08-31 10:53:51, Joe Lawrence wrote:
> >> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> >> index b9628e43c78f..aca62c4b8616 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)
> >> {
> >> @@ -285,6 +280,8 @@ static int klp_write_object_relocations(struct module *pmod,
> >>
> >> static int __klp_disable_patch(struct klp_patch *patch)
> >> {
> >> + struct klp_object *obj;
> >> +
> >> if (klp_transition_patch)
> >> return -EBUSY;
> >>
> >> @@ -295,6 +292,10 @@ static int __klp_disable_patch(struct klp_patch *patch)
> >>
> >> klp_init_transition(patch, KLP_UNPATCHED);
> >>
> >> + klp_for_each_object(patch, obj)
> >> + if (patch->enabled && obj->patched)
> >> + klp_pre_unpatch_callback(obj);
> >> +
> >> /*
> >> * Enforce the order of the func->transition writes in
> >> * klp_init_transition() and the TIF_PATCH_PENDING writes in
> >> @@ -388,13 +389,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");
> >
> > We should call klp_post_unpatch_callback(obj) here to make it
> > synchronous.
>
> Are you talking about the error path? As its coded here,
> klp_cancel_transition() will call klp_complete_transition() with
> klp_target_state = KLP_UNPATCHED and then klp_complete_transition()'s
> done: code will call klp_post_unpatch_callback() on all the necessary
> kobj's. Is there something asynchronous about that?

Ah, I have missed it. It is a bit tricky ;-)

> > Well, what about calling:
> >
> > klp_pre_patch_callback() inside klp_patch_object() and
> > klp_post_unpatch_callback() inside klp_unpatch_object()
>
> v1 started out that way, but we migrated to placing these around the
> callers of klp_(un)patch_object() to try and better line up the
> locations of the pre- hooks with the post- hook locations.

I guess that the move was mainly motivated by introducing 4 callbacks
instead of only two of them.

On one hand, it is fine to see a symmetric code like, for example,
in klp_module_going():

klp_pre_unpatch_callback(obj);
klp_unpatch_object(obj);
klp_post_unpatch_callback(obj);

On the other hand, it adds yet another asymmetry between
__klp_enable_patch()/__klp_disable_patch() and
klp_finish_transition(), see my confusion above.

I know that that the asymmetry was already there because of the
klp_patch_object() and klp_unpatch_object().

I mean that klp_patch_object() calls klp_unpatch_object() in case
of errors. But this handles only the current object. We still
rely on calling klp_cancel_transition()->klp_complete_transition()
to call klp_unpatch_object() for the other already proceed objects.

> I can take a second look at reversing this decision, but that may take a
> little time while I page all the testing corner cases back into my brain :)

I am sorry for the late reply. Heh, I needed to refresh a lot of
things as well. The advantage is that one could see things from
new perspective when the head was cleaned in between ;-)

> > By other words, we would do the two operations. It would have
> > two advantages:
> >
> > + error handling for free
> > + no need for the strange callbacks_enabled flag
>
> Indeed, it would be nice to ditch that callbacks_enabled wart.

Yup, I hope that in this case the less states would mean
the easier logic. And handling of klp_patch_object()/klp_unpatch()
object is already tricky enough. It would be lovely to just reuse
it if we can.

> I think the only other outstanding issue before rolling a v6 is the one
> that Miroslav raised about the error path in klp_module_coming():
>
> https://marc.info/?l=linux-kernel&m=150590635602784&w=2
> https://marc.info/?l=linux-kernel&m=150592065007463&w=2

I am going to look at it.

Best Regards,
Petr

2017-09-27 11:35:25

[permalink] [raw]

Subject: Re: [PATCH v5 2/3] livepatch: move transition "complete" notice into klp_complete_transition()

On Thu 2017-08-31 10:53:52, Joe Lawrence wrote:
> klp_complete_transition() performs a bit of housework before a
> transition to KLP_PATCHED or KLP_UNPATCHED is actually completed
> (including post-(un)patch callbacks). To be consistent, move the
> transition "complete" kernel log notice out of
> klp_try_complete_transition() and into klp_complete_transition().
>
> Suggested-by: Josh Poimboeuf <[email protected]>
> Signed-off-by: Joe Lawrence <[email protected]>

Looks fine.

Reviewed-by: Petr Mladek <[email protected]>

Best Regards,
Petr

2017-09-27 11:49:11

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] livepatch: add transition notices

On Thu 2017-08-31 10:53:53, Joe Lawrence wrote:
> Log a few kernel debug messages at the beginning of the following livepatch
> transition functions:
>
> klp_complete_transition()
> klp_cancel_transition()
> klp_init_transition()
> klp_reverse_transition()
>
> Also update the log notice message in klp_start_transition() for similar
> verbiage as the above messages.
>
> Suggested-by: Josh Poimboeuf <[email protected]>
> Signed-off-by: Joe Lawrence <[email protected]>
> ---
> kernel/livepatch/transition.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 53887f0bca10..3d44a3cf27be 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -82,6 +82,10 @@ static void klp_complete_transition(void)
> unsigned int cpu;
> bool immediate_func = false;
>
> + pr_debug("'%s': completing %s transition\n",
> + klp_transition_patch->mod->name,
> + klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
> +
> if (klp_target_state == KLP_UNPATCHED) {
> /*
> * All tasks have transitioned to KLP_UNPATCHED so we can now
> @@ -163,6 +167,9 @@ void klp_cancel_transition(void)
> if (WARN_ON_ONCE(klp_target_state != KLP_PATCHED))
> return;
>
> + pr_debug("'%s': canceling transition, unpatching\n",

This sentence is a bit confusing. It is related to the Mirek's concern
about that the following message would be "completing unpatching transition".

What about using something like:

pr_debug("'%s': canceling patching transition, going to unpatch\n",

Best Regards,
Petr

2017-09-27 12:17:24

[permalink] [raw]

Subject: Re: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails

On Wed 2017-09-13 17:36:38, Joe Lawrence wrote:
> >From b80b90cb54b498d2b1165d409ce4b0ca47610b36 Mon Sep 17 00:00:00 2001
> From: Joe Lawrence <[email protected]>
> Date: Wed, 13 Sep 2017 16:51:13 -0400
> Subject: [RFC] livepatch: unpatch all klp_objects if klp_module_coming fails
>
> When an incoming module is considered for livepatching by
> klp_module_coming(), it iterates over multiple patches and multiple
> kernel objects in this order:
>
> list_for_each_entry(patch, &klp_patches, list) {
> klp_for_each_object(patch, obj) {
>
> which means that if one of the kernel objects fail to patch for whatever
> reason, klp_module_coming()'s error path should double back and unpatch
> any previous kernel object that was patched for a previous patch.
>
> Reported-by: Miroslav Benes <[email protected]>
> Signed-off-by: Joe Lawrence <[email protected]>
> ---
> kernel/livepatch/core.c | 30 +++++++++++++++++++++++++++++-
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index aca62c4b8616..7f5192618cc8 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -889,6 +889,8 @@ int klp_module_coming(struct module *mod)
> goto err;
> }
>
> +pr_err("JL: klp_patch_object(%p) patch=%p obj->name: %s\n", obj, patch, obj->name);
> +
> ret = klp_patch_object(obj);
> if (ret) {
> pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> @@ -919,7 +921,33 @@ int klp_module_coming(struct module *mod)
> pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
> patch->mod->name, obj->mod->name, obj->mod->name);
> mod->klp_alive = false;
> - klp_free_object_loaded(obj);
> +
> + /*
> + * Run back through the patch list and unpatch any klp_object that
> + * was patched before hitting an error above.
> + */
> +
> + list_for_each_entry(patch, &klp_patches, list) {
> +
> + if (!patch->enabled || patch == klp_transition_patch)
> + continue;

klp_init_object_loaded() is called even the patch is not enabled.
Therefore we need to call klp_free_object_loaded() for all
objects where this was called.

> + klp_for_each_object(patch, obj) {
> +
> + if (!obj->patched || !klp_is_module(obj) ||
> + strcmp(obj->name, mod->name))
> + continue;
> +
> + klp_pre_unpatch_callback(obj);
> +pr_err("JL: klp_unpatch_object(%p) patch=%p obj->name: %s\n", obj, patch, obj->name);
> + klp_unpatch_object(obj);
> + klp_post_unpatch_callback(obj);
> + klp_free_object_loaded(obj);
> +
> + break;
> + }
> + }

Well, we basically need to do the same as we do in
__klp_module_coming(). I would suggest to somehow
reuse the code.

The question is how to detect the patches that need
the revert. I would suggest to use the same trick that
we use in klp_free_funcs_limited() and do something like:

void __klp_module_going_limited(struct module *mod,
struct klp_patch *limit)
{
struct klp_patch *patch;
struct klp_object *obj;

/*
* Each module has to know that klp_module_going()
* has been called. We never know what module will
* get patched by a new patch.
*/
mod->klp_alive = false;

list_for_each_entry(patch, &klp_patches, list) {
if (patch == limit)
break;

klp_for_each_object(patch, obj) {
if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
continue;

/*
* Only unpatch the module if the patch is enabled or
* is in transition.
*/
if (patch->enabled || patch == klp_transition_patch) {

if (patch != klp_transition_patch)
klp_pre_unpatch_callback(obj);

pr_notice("reverting patch '%s' on unloading module '%s'\n",
patch->mod->name, obj->mod->name);
klp_unpatch_object(obj);
klp_post_unpatch_callback(obj);
}

klp_free_object_loaded(obj);
break;
}
}
}

Also please make this the first patch in the series. It fixes
an existing problem. We might want to get this in earlier
than the rest of the patchset.

Best Regards,
Petr

2017-09-27 15:45:29