[PATCH v4 0/3] livepatch callbacks

[PATCH v4 0/3] livepatch callbacks

[Joe Lawrence]

v4:

- Move callback helpers into core.h

- Move klp_pre_patch_callback() and klp_pre_unpatch_callback()
  invocations into __klp_enable_patch() and __klp_disable_patch()

  - klp_patch_object() and klp_unpatch_objects()
    - Do not run pre-unpatch callbacks from here

- Add a pre_patch_status member to klp_object so when a pre-patch
  callback fails, the helpers can skip any post-patch, pre-unpatch,
  post-unpatch callbacks

- klp_module_coming() and klp_module_going()
  - Do not run post-patch or pre-unpatch callbacks for current
    klp_transition_patch

- Documentation
  - Add various test cases and provide commentary

- Samples
  - Create two target modules: a simple one and another that invokes a
    worker function that sleeps for a long time

- Added two follow-up patches:

  - livepatch: move transition "complete" notice into
    klp_complete_transition() - this pushes the "patching complete" message
    after the post-patch callbacks

  - livepatch: add transition notices - these were helpful during
    debugging of the callback patch.  The transaction annotations were
    also used in the Documentation file tese cases to illustrate the
    order of operations.

  Note that these two patches could be standalone, I include them here
  in this patchset since they affect the content/ordering of kernel logs
  that were included as part of the Documentation.

Joe Lawrence (3):
  livepatch: add (un)patch callbacks
  livepatch: move transition "complete" notice into
    klp_complete_transition()
  livepatch: add transition notices

 Documentation/livepatch/callbacks.txt           | 595 ++++++++++++++++++++++++
 include/linux/livepatch.h                       |  18 +
 kernel/livepatch/core.c                         |  56 ++-
 kernel/livepatch/core.h                         |  78 ++++
 kernel/livepatch/patch.c                        |   1 +
 kernel/livepatch/transition.c                   |  45 +-
 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, 1140 insertions(+), 17 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

-- 
1.8.3.1

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

[Joe Lawrence]

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 <joe.lawrence@redhat.com>
---
 Documentation/livepatch/callbacks.txt           | 595 ++++++++++++++++++++++++
 include/linux/livepatch.h                       |  18 +
 kernel/livepatch/core.c                         |  56 ++-
 kernel/livepatch/core.h                         |  78 ++++
 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, 1120 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..c44825777c28
--- /dev/null
+++ b/Documentation/livepatch/callbacks.txt
@@ -0,0 +1,595 @@
+======================
+(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.749177] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
+  [   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..3f2f8ad9427d 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -87,24 +87,42 @@ struct klp_func {
 	bool transition;
 };
 
+struct klp_object;
+
+/**
+ * struct klp_callbacks - pre/post live-(un)patch callback structure
+ * @callback:	function to be executed on callback
+ *
+ */
+struct klp_callbacks {
+	int (*pre_patch)(struct klp_object *obj);
+	void (*post_patch)(struct klp_object *obj);
+	void (*pre_unpatch)(struct klp_object *obj);
+	void (*post_unpatch)(struct klp_object *obj);
+};
+
 /**
  * struct klp_object - kernel object structure for live patching
  * @name:	module name (or NULL for vmlinux)
  * @funcs:	function entries for functions to be patched in the object
+ * @callbacks:	functions to be executed pre/post (un)patching
  * @kobj:	kobject for sysfs resources
  * @mod:	kernel module associated with the patched object
  *		(NULL for vmlinux)
  * @patched:	the object's funcs have been added to the klp_ops list
+ * @pre_patch_status:	status returned from pre-patch callback
  */
 struct klp_object {
 	/* external */
 	const char *name;
 	struct klp_func *funcs;
+	struct klp_callbacks callbacks;
 
 	/* internal */
 	struct kobject kobj;
 	struct module *mod;
 	bool patched;
+	int pre_patch_callback_status;
 };
 
 /**
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b9628e43c78f..b734316de01c 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,6 +882,13 @@ int klp_module_coming(struct module *mod)
 			pr_notice("applying patch '%s' to loading module '%s'\n",
 				  patch->mod->name, obj->mod->name);
 
+			ret = klp_pre_patch_callback(obj);
+			if (ret) {
+				pr_warn("pre-patch callback failed for object '%s'\n",
+					obj->name);
+				goto err;
+			}
+
 			ret = klp_patch_object(obj);
 			if (ret) {
 				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
@@ -878,6 +896,14 @@ int klp_module_coming(struct module *mod)
 				goto err;
 			}
 
+			/*
+			 * Skip objects in the transition patch for their
+			 * post-patch callbacks will execute as part of
+			 * the transition completion.
+			 */
+			if (patch != klp_transition_patch)
+				klp_post_patch_callback(obj);
+
 			break;
 		}
 	}
@@ -927,9 +953,19 @@ void klp_module_going(struct module *mod)
 			 * is in transition.
 			 */
 			if (patch->enabled || patch == klp_transition_patch) {
+
+				/*
+				 * Skip objects in the transition patch for
+				 * their pre-unpatch callbacks executed
+				 * before the transition started.
+				 */
+				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..78c041902116 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -1,6 +1,84 @@
 #ifndef _LIVEPATCH_CORE_H
 #define _LIVEPATCH_CORE_H
 
+#include <linux/livepatch.h>
+
 extern struct mutex klp_mutex;
 
+/**
+ * klp_is_object_loaded() - is klp_object currently loaded?
+ * @obj:	klp_object pointer
+ *
+ * Return: true if klp_object is loaded (always true for vmlinux)
+ */
+static inline bool klp_is_object_loaded(struct klp_object *obj)
+{
+	return !obj->name || obj->mod;
+}
+
+/**
+ * klp_pre_patch_callback - executed before klp_object is patched
+ * @obj:	invoke callback for this klp_object
+ *
+ * Return: status from callback
+ *
+ * Callers should ensure obj->patched is *not* set.
+ */
+static inline int klp_pre_patch_callback(struct klp_object *obj)
+{
+	obj->pre_patch_callback_status =
+		(obj->callbacks.pre_patch) ?
+			(*obj->callbacks.pre_patch)(obj) : 0;
+
+	return obj->pre_patch_callback_status;
+}
+
+/**
+ * klp_post_patch_callback() - executed after klp_object is patched
+ * @obj:	invoke callback for this klp_object
+ *
+ * Callers should ensure obj->patched is set.
+ */
+static inline void klp_post_patch_callback(struct klp_object *obj)
+{
+	if (obj->callbacks.post_patch)
+		(*obj->callbacks.post_patch)(obj);
+}
+
+/**
+ * klp_pre_unpatch_callback() - executed before klp_object is unpatched
+ *                              and is active across all tasks
+ * @obj:	invoke callback for this klp_object
+ *
+ * This callback will not be run if the pre-patch callback status was
+ * non-zero.
+ *
+ * Callers should ensure obj->patched is set.
+ */
+static inline void klp_pre_unpatch_callback(struct klp_object *obj)
+{
+	if (!obj->pre_patch_callback_status &&
+	    obj->callbacks.pre_unpatch)
+		(*obj->callbacks.pre_unpatch)(obj);
+}
+
+/**
+ * klp_post_unpatch_callback() - executed after klp_object is unpatched,
+ *                               all code has been restored and no tasks
+ *                               are running patched code
+ * @obj:	invoke callback for this klp_object
+ *
+ * This callback will not be run if the pre-patch callback status was
+ * non-zero.
+ *
+ * Callers should ensure obj->patched is *not* set.
+ */
+static inline void klp_post_unpatch_callback(struct klp_object *obj)
+{
+	if (!obj->pre_patch_callback_status &&
+	    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 <joe.lawrence@redhat.com>
+ *
+ * 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 <joe.lawrence@redhat.com>
+ *
+ * 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 <joe.lawrence@redhat.com>
+ *
+ * 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

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

[Josh Poimboeuf]

On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote:
> +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.749177] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
> +  [   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

First off, this documentation is very nice because it clarifies all the
callback scenarios and edge cases.

The above situation still seems a little odd to me.  If I understand
correctly, the target module was never patched, and its pre_patch
callback was never called.  But its post_unpatch callback *was* called.
That doesn't seem right.

Maybe we should change the condition a little bit.  Currently it's:

  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.

I think that might have been my idea, but seeing the above case makes it
clear that it's not quite right.  Maybe it should instead be:

  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.

So then, instead of tracking whether the pre-patch callback succeeded,
we just need to track whether the object was patched (which we already
do, with obj->patched).

What do you think?

-- 
Josh

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

[Joe Lawrence]

On 08/29/2017 11:49 AM, Josh Poimboeuf wrote:
> On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote:
>> +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.749177] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
>> +  [   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
> 
> First off, this documentation is very nice because it clarifies all the
> callback scenarios and edge cases.
> 
> The above situation still seems a little odd to me.  If I understand
> correctly, the target module was never patched, and its pre_patch
> callback was never called.  But its post_unpatch callback *was* called.
> That doesn't seem right.

Ah, this does look to be a bug.

> Maybe we should change the condition a little bit.  Currently it's:
> 
>   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.
> 
> I think that might have been my idea, but seeing the above case makes it
> clear that it's not quite right.

It could have been correct if the code differentiated between a
never-run pre_patch_status of 0 (by kzalloc) and a successful
pre_patch_status of 0 (by callback return), I think.

>                                   Maybe it should instead be:
> 
>   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.

That description sounds correct...

> So then, instead of tracking whether the pre-patch callback succeeded,
> we just need to track whether the object was patched (which we already
> do, with obj->patched).
> 
> What do you think?

I think this would only work if there was a sticky
"obj->was_ever_patched" variable.  We moved the post-unpatch-callback to
the very end of klp_complete_transition()... by that point, obj->patched
will have already been cleared by klp_unpatch_objects.

We could maybe move obj->patched assignments out to encapsulate the pre
and post callbacks... but I would need to think about that a while.  It
seems pretty clear and symmetric as it is today (immediately set in
klp_(un)patch_object().

Perhaps a more careful checking of obj->pre_patch_callback_status is all
we need?  (I can't think of anything more succinct than adding a
obj->pre_patch_callback_done variable to the mix.)

-- Joe

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

[Josh Poimboeuf]

On Tue, Aug 29, 2017 at 03:22:06PM -0400, Joe Lawrence wrote:
> On 08/29/2017 11:49 AM, Josh Poimboeuf wrote:
> > On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote:
> >> +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.749177] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
> >> +  [   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
> > 
> > First off, this documentation is very nice because it clarifies all the
> > callback scenarios and edge cases.
> > 
> > The above situation still seems a little odd to me.  If I understand
> > correctly, the target module was never patched, and its pre_patch
> > callback was never called.  But its post_unpatch callback *was* called.
> > That doesn't seem right.
> 
> Ah, this does look to be a bug.
> 
> > Maybe we should change the condition a little bit.  Currently it's:
> > 
> >   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.
> > 
> > I think that might have been my idea, but seeing the above case makes it
> > clear that it's not quite right.
> 
> It could have been correct if the code differentiated between a
> never-run pre_patch_status of 0 (by kzalloc) and a successful
> pre_patch_status of 0 (by callback return), I think.
> 
> >                                   Maybe it should instead be:
> > 
> >   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.
> 
> That description sounds correct...
> 
> > So then, instead of tracking whether the pre-patch callback succeeded,
> > we just need to track whether the object was patched (which we already
> > do, with obj->patched).
> > 
> > What do you think?
> 
> I think this would only work if there was a sticky
> "obj->was_ever_patched" variable.  We moved the post-unpatch-callback to
> the very end of klp_complete_transition()... by that point, obj->patched
> will have already been cleared by klp_unpatch_objects.
> 
> We could maybe move obj->patched assignments out to encapsulate the pre
> and post callbacks... but I would need to think about that a while.  It
> seems pretty clear and symmetric as it is today (immediately set in
> klp_(un)patch_object().
> 
> Perhaps a more careful checking of obj->pre_patch_callback_status is all
> we need?  (I can't think of anything more succinct than adding a
> obj->pre_patch_callback_done variable to the mix.)

Makes sense.  I think you're right that obj->patched wouldn't work.

But there's one more weird case I didn't mention.  If the patch has a
post-unpatch callback, but it doesn't have a pre-patch callback, then
'obj->pre_patch_callback_done' would never get set and the post-unpatch
callback would never get called, even if the patch was successful.

So instead of 'obj->pre_patch_callback_done', how about
'obj->callbacks_enabled'?

It could be set in the following cases:

  a) if the object has a pre_patch callback, set obj->callbacks_enabled
     after the pre_patch callback succeeds; 

  b) else, if the patch does *not* have a pre_patch callback, set
     obj->callbacks_enabled after klp_patch_object() succeeds.

And the variable would need to be cleared after the post_unpatch
callback was run.

It's a bit complicated, but that seems to be the most logicial behavior
as far as I can tell.

Thoughts?

-- 
Josh

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

[Joe Lawrence]

On Tue, Aug 29, 2017 at 02:59:12PM -0500, Josh Poimboeuf wrote:
> On Tue, Aug 29, 2017 at 03:22:06PM -0400, Joe Lawrence wrote:
> > On 08/29/2017 11:49 AM, Josh Poimboeuf wrote:
> > > On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote:
> > >> +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.749177] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
> > >> +  [   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
> > > 
> > > First off, this documentation is very nice because it clarifies all the
> > > callback scenarios and edge cases.
> > > 
> > > The above situation still seems a little odd to me.  If I understand
> > > correctly, the target module was never patched, and its pre_patch
> > > callback was never called.  But its post_unpatch callback *was* called.
> > > That doesn't seem right.
> > 
> > Ah, this does look to be a bug.
> > 
> > > Maybe we should change the condition a little bit.  Currently it's:
> > > 
> > >   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.
> > > 
> > > I think that might have been my idea, but seeing the above case makes it
> > > clear that it's not quite right.
> > 
> > It could have been correct if the code differentiated between a
> > never-run pre_patch_status of 0 (by kzalloc) and a successful
> > pre_patch_status of 0 (by callback return), I think.
> > 
> > >                                   Maybe it should instead be:
> > > 
> > >   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.
> > 
> > That description sounds correct...
> > 
> > > So then, instead of tracking whether the pre-patch callback succeeded,
> > > we just need to track whether the object was patched (which we already
> > > do, with obj->patched).
> > > 
> > > What do you think?
> > 
> > I think this would only work if there was a sticky
> > "obj->was_ever_patched" variable.  We moved the post-unpatch-callback to
> > the very end of klp_complete_transition()... by that point, obj->patched
> > will have already been cleared by klp_unpatch_objects.
> > 
> > We could maybe move obj->patched assignments out to encapsulate the pre
> > and post callbacks... but I would need to think about that a while.  It
> > seems pretty clear and symmetric as it is today (immediately set in
> > klp_(un)patch_object().
> > 
> > Perhaps a more careful checking of obj->pre_patch_callback_status is all
> > we need?  (I can't think of anything more succinct than adding a
> > obj->pre_patch_callback_done variable to the mix.)
> 
> Makes sense.  I think you're right that obj->patched wouldn't work.
> 
> But there's one more weird case I didn't mention.  If the patch has a
> post-unpatch callback, but it doesn't have a pre-patch callback, then
> 'obj->pre_patch_callback_done' would never get set and the post-unpatch
> callback would never get called, even if the patch was successful.

Interesting case.  I didn't code anything up, but the idea was that the
other callbacks would only run iff pre_patch_done && status == 0 ||
!pre_patch_callback.  But the following suggestion is clearer IMHO ...

> So instead of 'obj->pre_patch_callback_done', how about
> 'obj->callbacks_enabled'?
> 
> It could be set in the following cases:
> 
>   a) if the object has a pre_patch callback, set obj->callbacks_enabled
>      after the pre_patch callback succeeds; 
> 
>   b) else, if the patch does *not* have a pre_patch callback, set
>      obj->callbacks_enabled after klp_patch_object() succeeds.
> 
> And the variable would need to be cleared after the post_unpatch
> callback was run.
> 
> It's a bit complicated, but that seems to be the most logicial behavior
> as far as I can tell.
> 
> Thoughts?

What if we flip it around as "callbacks_disabled"?  By default, kzalloc
would init as false.  It would only be set to true if the pre-patch
callback is provided and if it returns failure.  Would that reduce the
number of conditions when we need to set this var?

Also, as you noted, I think it would need to reset/cleared after the
post-patch callback.  (For the livepatch-already-loaded cases.)

-- Joe

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

[Josh Poimboeuf]

On Wed, Aug 30, 2017 at 09:27:16AM -0400, Joe Lawrence wrote:
> > So instead of 'obj->pre_patch_callback_done', how about
> > 'obj->callbacks_enabled'?
> > 
> > It could be set in the following cases:
> > 
> >   a) if the object has a pre_patch callback, set obj->callbacks_enabled
> >      after the pre_patch callback succeeds; 
> > 
> >   b) else, if the patch does *not* have a pre_patch callback, set
> >      obj->callbacks_enabled after klp_patch_object() succeeds.
> > 
> > And the variable would need to be cleared after the post_unpatch
> > callback was run.
> > 
> > It's a bit complicated, but that seems to be the most logicial behavior
> > as far as I can tell.
> > 
> > Thoughts?
> 
> What if we flip it around as "callbacks_disabled"?  By default, kzalloc
> would init as false.  It would only be set to true if the pre-patch
> callback is provided and if it returns failure.  Would that reduce the
> number of conditions when we need to set this var?

Yeah, 'callbacks_disabled' sounds better.

> Also, as you noted, I think it would need to reset/cleared after the
> post-patch callback.  (For the livepatch-already-loaded cases.)

Since it can only be set when the pre-patch fails, I think it would only
need to be cleared after post-unpatch?

Or another alternative would be to unconditionally clear and/or set it
in klp_pre_patch_callback() so that its previous value doesn't matter.

-- 
Josh

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

[Josh Poimboeuf]

On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote:
> @@ -871,6 +882,13 @@ int klp_module_coming(struct module *mod)
>  			pr_notice("applying patch '%s' to loading module '%s'\n",
>  				  patch->mod->name, obj->mod->name);
>  
> +			ret = klp_pre_patch_callback(obj);
> +			if (ret) {
> +				pr_warn("pre-patch callback failed for object '%s'\n",
> +					obj->name);
> +				goto err;
> +			}
> +
>  			ret = klp_patch_object(obj);
>  			if (ret) {
>  				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",

If klp_pre_patch_callback() succeeds but klp_patch_object() fails, the
post-unpatch callback needs to be called in the error path.

> +/**
> + * klp_pre_patch_callback - executed before klp_object is patched
> + * @obj:	invoke callback for this klp_object
> + *
> + * Return: status from callback
> + *
> + * Callers should ensure obj->patched is *not* set.

Can this comment be removed since it no longer checks obj->patched?

> +static inline int klp_pre_patch_callback(struct klp_object *obj)
> +{
> +	obj->pre_patch_callback_status =
> +		(obj->callbacks.pre_patch) ?
> +			(*obj->callbacks.pre_patch)(obj) : 0;
> +
> +	return obj->pre_patch_callback_status;
> +}
> +
> +/**
> + * klp_post_patch_callback() - executed after klp_object is patched
> + * @obj:	invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is set.

Ditto here and below.

> +static inline void klp_post_patch_callback(struct klp_object *obj)
> +{
> +	if (obj->callbacks.post_patch)
> +		(*obj->callbacks.post_patch)(obj);
> +}
> +
> +/**
> + * klp_pre_unpatch_callback() - executed before klp_object is unpatched
> + *                              and is active across all tasks
> + * @obj:	invoke callback for this klp_object
> + *
> + * This callback will not be run if the pre-patch callback status was
> + * non-zero.

I think this comment should be a little broader.  The callback won't be
called if the object fails to patch for *any* reason.

Also, I think all the comments about when the callbacks are run or not
run would be better placed in a paragraph above the 'struct
klp_callbacks' definition in include/linux/livepatch.h.

And actually, IMO, all the functions in core.h are straightforward
enough that they don't need *any* function header comments.  And the
same for klp_is_object_loaded().  I would just remove all the core.h
comments, and just add a short explanation in livepatch.h about when the
callbacks are called and not called.

> + * Callers should ensure obj->patched is set.
> + */
> +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> +{
> +	if (!obj->pre_patch_callback_status &&
> +	    obj->callbacks.pre_unpatch)
> +		(*obj->callbacks.pre_unpatch)(obj);
> +}

I think pre_patch_callback_status (or callbacks_disabled) doesn't need
to be checked here, right?  Since if we got this far, the patching was
successful and callbacks will be enabled by definition?

-- 
Josh

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

[Joe Lawrence]

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().

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 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

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

[Josh Poimboeuf]

On Fri, Aug 25, 2017 at 03:10:01PM -0400, 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().
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>

I think the Signed-off-by should always be the last line.  Otherwise:

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

[PATCH v4 3/3] livepatch: add transition notices

[Joe Lawrence]

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.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 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

Re: [PATCH v4 3/3] livepatch: add transition notices

[Josh Poimboeuf]

On Fri, Aug 25, 2017 at 03:10:02PM -0400, 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.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>

Same Signed-off-by comment here.  Otherwise:

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh