Groups > linux.kernel > #1713246 > unrolled thread

[PATCH v3] add (un)patch callbacks

Started by	Joe Lawrence <joe.lawrence@redhat.com>
First post	2017-08-16 21:20 +0200
Last post	2017-08-18 16:10 +0200
Articles	3 — 3 participants

Back to article view | Back to linux.kernel

  [PATCH v3] add (un)patch callbacks Joe Lawrence <joe.lawrence@redhat.com> - 2017-08-16 21:20 +0200
    Re: [PATCH v3] add (un)patch callbacks Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-16 22:30 +0200
      Re: [PATCH v3] add (un)patch callbacks Petr Mladek <pmladek@suse.com> - 2017-08-18 16:10 +0200

#1713246 — [PATCH v3] add (un)patch callbacks

From	Joe Lawrence <joe.lawrence@redhat.com>
Date	2017-08-16 21:20 +0200
Subject	[PATCH v3] add (un)patch callbacks
Message-ID	<ufc1b-2Al-3@gated-at.bofh.it>

v3:

- livepatch.h
  - drop obj->patched checks from pre/post-(un)patch funcs,
    add preceding comment and note about obj->patched assumptions
  - move core.c :: klp_is_module() to here

- klp_complete_transition()
  - fix "else if (klp_target_state == KLP_UNPATCHED)" case
  - combine conditional syntax when avoiding module_put for immediate
    patches
  - add check for klp_is_object_loaded to avoid callbacks for any
    unloaded modules (necessary after removing obj->patched checks in
    livepatch.h)

- Documentation
  - added Josh's use-cases blurb in intro
  - s/Callbacks are only executed/A callbacks is only executed/

- livepatch-callbacks-demo.c
  - whitespace cleanup

I also wrote a quick test script (see below) to exercise some of the
load/unload/enable/disable/error status combinations.  I'm not sure
about some of the behaviors, most notably test6 with regard to
post-unpatch-callbacks as executed on a cancelled transition.  (See
results and comments further below.)

Also, maybe it's just my reading of the log, but would it be clearer if
the "(un)patching ... complete" messages indicated that they are
referring to a transaction?  It's a bit confusing to see "unpatching ...
complete" before the pre-unpatch-callbacks ever execute.  Not a big
deal, but I can send a follow up patch if others agree.

-- Joe


Test script
===========

MODULE=samples/livepatch/livepatch-callbacks-mod.ko
LIVEPATCH=samples/livepatch/livepatch-callbacks-demo.ko
DELAY=2s

function load_mod() {
	local mod="$1"
	shift
	local args="$@"
	echo "% insmod $mod $args" > /dev/kmsg
	ret=$(insmod $mod $args 2>&1)
	[[ "$ret" != "" ]] && echo "$ret" > /dev/kmsg
	sleep $DELAY
}

function unload_mod() {
	local mod="$1"
	echo "% rmmod $mod" > /dev/kmsg
	ret=$(rmmod $mod 2>&1)
	[[ "$ret" != "" ]] && echo "$ret" > /dev/kmsg
	sleep $DELAY
}

function disable_lp() {
	echo "% echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled" > /dev/kmsg
	echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
	sleep $DELAY
}

function set_pre_patch_ret {
	local ret="$1"
	echo "% echo $1 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret" > /dev/kmsg
	echo -19 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret
	sleep $DELAY
}

###############################################################
dmesg -C
echo -- test0 - load target module, unload target module > /dev/kmsg

load_mod $MODULE 
unload_mod $MODULE 

dmesg > test0.out
###############################################################
dmesg -C
echo -- test1 - load target module, load livepatch, disable livepatch, unload target module, unload livepatch > /dev/kmsg

load_mod $MODULE
load_mod $LIVEPATCH
disable_lp
unload_mod $LIVEPATCH
unload_mod $MODULE

dmesg > test1.out
###############################################################
dmesg -C
echo -- test2 - load livepatch, load target module, disable livepatch, unload livepatch, unload target module > /dev/kmsg

load_mod $LIVEPATCH
load_mod $MODULE
disable_lp
unload_mod $LIVEPATCH
unload_mod $MODULE

dmesg > test2.out
###############################################################
dmesg -C
echo -- test3 - load target module, load livepatch, unload target module, disable livepatch, unload livepatch > /dev/kmsg

load_mod $MODULE
load_mod $LIVEPATCH
unload_mod $MODULE
disable_lp
unload_mod $LIVEPATCH

dmesg > test3.out
###############################################################
dmesg -C
echo -- test4 - load livepatch, load target module, unload target module, disable livepatch, unload livepatch > /dev/kmsg
load_mod $LIVEPATCH
load_mod $MODULE
unload_mod $MODULE
disable_lp
unload_mod $LIVEPATCH

dmesg > test4.out
###############################################################
dmesg -C
echo -- test5 - load livepatch, disable livepatch, unload livepatch > /dev/kmsg
load_mod $LIVEPATCH
disable_lp
unload_mod $LIVEPATCH

dmesg > test5.out
###############################################################
dmesg -C
echo -- test6 - load target module, load livepatch -ENODEV, unload target module > /dev/kmsg

load_mod $MODULE
load_mod $LIVEPATCH pre_patch_ret=-19
unload_mod $LIVEPATCH
unload_mod $MODULE

dmesg > test6.out
###############################################################
dmesg -C
echo -- test7 - load livepatch, setup -ENODEV, load target module, disable livepatch, unload livepatch > /dev/kmsg

load_mod $LIVEPATCH
set_pre_patch_ret -19
load_mod $MODULE
disable_lp
unload_mod $MODULE
unload_mod $LIVEPATCH

dmesg > test7.out
###############################################################


Results
=======

[   34.504478] -- test0 - load target module, unload target module
[   34.505137] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[   34.552726] livepatch_callbacks_mod: module verification failed: signature and/or required key missing - tainting kernel
[   34.554440] livepatch_callbacks_mod: livepatch_callbacks_mod_init
[   36.573704] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[   36.576533] livepatch_callbacks_mod: livepatch_callbacks_mod_exit

A boring test, but as expected, no surprise callbacks were executed.

[   38.588867] -- test1 - load target module, load livepatch, disable livepatch, unload target module, unload livepatch
[   38.589910] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[   38.592337] livepatch_callbacks_mod: livepatch_callbacks_mod_init
[   40.594840] % insmod samples/livepatch/livepatch-callbacks-demo.ko
[   40.661270] livepatch_callbacks_demo: tainting kernel with TAINT_LIVEPATCH
[   40.662666] livepatch: enabling patch 'livepatch_callbacks_demo'
[   40.663462] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[   40.664262] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[   40.665565] livepatch: 'livepatch_callbacks_demo': patching...
[   41.695061] livepatch: 'livepatch_callbacks_demo': patching complete
[   41.696024] livepatch_callbacks_demo: post_patch_callback: vmlinux
[   41.696861] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[   42.668712] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
[   42.670354] livepatch: 'livepatch_callbacks_demo': unpatching...
[   43.743103] livepatch: 'livepatch_callbacks_demo': unpatching complete
[   43.743760] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
[   43.744346] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[   43.745327] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[   43.745848] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[   44.672951] % rmmod samples/livepatch/livepatch-callbacks-demo.ko
[   46.686448] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[   46.688921] livepatch_callbacks_mod: livepatch_callbacks_mod_exit

Part 1: livepatch loads after the target module, patch callbacks execute for
both vmlinux and the target module.

Part 2: livepatch is disabled while the target module is still loaded, unpatch
callbacks execute for both vmlinux and target module.

[   48.698388] -- test2 - load livepatch, load target module, disable livepatch, unload livepatch, unload target module
[   48.699570] % insmod samples/livepatch/livepatch-callbacks-demo.ko
[   48.702519] livepatch: enabling patch 'livepatch_callbacks_demo'
[   48.703515] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[   48.704139] livepatch: 'livepatch_callbacks_demo': patching...
[   49.695048] livepatch: 'livepatch_callbacks_demo': patching complete
[   49.695782] livepatch_callbacks_demo: post_patch_callback: vmlinux
[   50.706813] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[   50.709855] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
[   50.710762] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[   50.711895] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[   50.713923] livepatch_callbacks_mod: livepatch_callbacks_mod_init
[   52.716994] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
[   52.717943] livepatch: 'livepatch_callbacks_demo': unpatching...
[   53.727092] livepatch: 'livepatch_callbacks_demo': unpatching complete
[   53.727726] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
[   53.728307] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[   53.729428] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[   53.730002] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[   54.720253] % rmmod samples/livepatch/livepatch-callbacks-demo.ko
[   56.735960] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[   56.738470] livepatch_callbacks_mod: livepatch_callbacks_mod_exit

Part 1: livepatch loads before target module, so only vmlinux patch callbacks
execute.  Once target module loads, its patch callbacks run.

Part 2: livepatch is disabled while the target module is still loaded, unpatch
callbacks execute for both vmlinux and target module.

[   58.747842] -- test3 - load target module, load livepatch, unload target module, disable livepatch, unload livepatch
[   58.748931] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[   58.751625] livepatch_callbacks_mod: livepatch_callbacks_mod_init
[   60.754340] % insmod samples/livepatch/livepatch-callbacks-demo.ko
[   60.757824] livepatch: enabling patch 'livepatch_callbacks_demo'
[   60.758466] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[   60.758969] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[   60.759923] livepatch: 'livepatch_callbacks_demo': patching...
[   61.727106] livepatch: 'livepatch_callbacks_demo': patching complete
[   61.728268] livepatch_callbacks_demo: post_patch_callback: vmlinux
[   61.728802] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[   62.762599] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[   62.765086] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
[   62.765925] livepatch: reverting patch 'livepatch_callbacks_demo' on unloading module 'livepatch_callbacks_mod'
[   62.767207] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
[   62.768179] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
[   64.776099] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
[   64.777078] livepatch: 'livepatch_callbacks_demo': unpatching...
[   65.759068] livepatch: 'livepatch_callbacks_demo': unpatching complete
[   65.759845] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
[   65.760444] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[   66.779280] % rmmod samples/livepatch/livepatch-callbacks-demo.ko

Part 1: livepatch loads after the target module, patch callbacks execute for
both vmlinux and the target module.

Part 2: target module is unloaded, so unpatch callbacks run for the
module.  The livepatch is then disabled, vmlinux unpatch callbacks
execute.

[   68.794346] -- test4 - load livepatch, load target module, unload target module, disable livepatch, unload livepatch
[   68.795857] % insmod samples/livepatch/livepatch-callbacks-demo.ko
[   68.799526] livepatch: enabling patch 'livepatch_callbacks_demo'
[   68.800122] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[   68.800631] livepatch: 'livepatch_callbacks_demo': patching...
[   69.727057] livepatch: 'livepatch_callbacks_demo': patching complete
[   69.727719] livepatch_callbacks_demo: post_patch_callback: vmlinux
[   70.803162] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[   70.805853] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
[   70.806749] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[   70.807806] livepatch_callbacks_demo: post_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[   70.809671] livepatch_callbacks_mod: livepatch_callbacks_mod_init
[   72.812254] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[   72.814795] livepatch_callbacks_mod: livepatch_callbacks_mod_exit
[   72.815639] livepatch: reverting patch 'livepatch_callbacks_demo' on unloading module 'livepatch_callbacks_mod'
[   72.816561] livepatch_callbacks_demo: pre_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
[   72.817615] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_GOING] Going away
[   74.831463] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
[   74.832358] livepatch: 'livepatch_callbacks_demo': unpatching...
[   75.743119] livepatch: 'livepatch_callbacks_demo': unpatching complete
[   75.743732] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
[   75.744469] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[   76.834520] % rmmod samples/livepatch/livepatch-callbacks-demo.ko

Part 1: livepatch loads before target module, so only vmlinux patch callbacks
execute.  Once target module loads, its patch callbacks run.

Part 2: target module is unloaded, so unpatch callbacks run for the
module.  The livepatch is then disabled, vmlinux unpatch callbacks

[   78.851887] -- test5 - load livepatch, disable livepatch, unload livepatch
[   78.852732] % insmod samples/livepatch/livepatch-callbacks-demo.ko
[   78.855401] livepatch: enabling patch 'livepatch_callbacks_demo'
[   78.855966] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[   78.856799] livepatch: 'livepatch_callbacks_demo': patching...
[   79.711079] livepatch: 'livepatch_callbacks_demo': patching complete
[   79.711756] livepatch_callbacks_demo: post_patch_callback: vmlinux
[   80.859474] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
[   80.860441] livepatch: 'livepatch_callbacks_demo': unpatching...
[   81.759137] livepatch: 'livepatch_callbacks_demo': unpatching complete
[   81.760137] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
[   81.760994] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[   82.862596] % rmmod samples/livepatch/livepatch-callbacks-demo.ko

Part 1: livepatch is loaded (no target module), only vmlinux callbacks
run.

Part 2: livepatch is disabled (no target module), only vmlinux callbacks
run.

[   84.878971] -- test6 - load target module, load livepatch -ENODEV, unload target module
[   84.879755] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[   84.882842] livepatch_callbacks_mod: livepatch_callbacks_mod_init
[   86.885313] % insmod samples/livepatch/livepatch-callbacks-demo.ko pre_patch_ret=-19
[   86.889259] livepatch: enabling patch 'livepatch_callbacks_demo'
[   86.890160] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[   86.890734] livepatch: pre-patch callback failed for object 'vmlinux'
[   86.891306] livepatch: failed to enable patch 'livepatch_callbacks_demo'
[   86.891931] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[   86.892561] livepatch_callbacks_demo: post_unpatch_callback: livepatch_callbacks_mod -> [MODULE_STATE_LIVE] Normal state
[   86.908817] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-demo.ko: No such device
[   88.911655] % rmmod samples/livepatch/livepatch-callbacks-demo.ko
[   88.914815] rmmod: ERROR: Module livepatch_callbacks_demo is not currently loaded
[   90.917163] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[   90.919997] livepatch_callbacks_mod: livepatch_callbacks_mod_exit

Part 1: Livepatch is loaded after the target module, however the
vmlinux-pre-patch-callback returns -ENODEV, so the livepatch module
fails to load.

Note: both vmlinux and target module's post-unpatch-callbacks are
executed as part of the cancelled transition:

  klp_enable_patch
    __klp_enable_patch
      klp_cancel_transition
        klp_complete_transition

        done:
          ...
          else if (klp_target_state == KLP_UNPATCHED)
                   klp_post_unpatch_callback(obj);

[   92.934851] -- test7 - load livepatch, setup -ENODEV, load target module, disable livepatch, unload livepatch
[   92.935879] % insmod samples/livepatch/livepatch-callbacks-demo.ko
[   92.938683] livepatch: enabling patch 'livepatch_callbacks_demo'
[   92.939294] livepatch_callbacks_demo: pre_patch_callback: vmlinux
[   92.939823] livepatch: 'livepatch_callbacks_demo': patching...
[   93.727126] livepatch: 'livepatch_callbacks_demo': patching complete
[   93.727809] livepatch_callbacks_demo: post_patch_callback: vmlinux
[   94.942396] % echo -19 > /sys/module/livepatch_callbacks_demo/parameters/pre_patch_ret
[   96.944893] % insmod samples/livepatch/livepatch-callbacks-mod.ko
[   96.947557] livepatch: applying patch 'livepatch_callbacks_demo' to loading module 'livepatch_callbacks_mod'
[   96.948816] livepatch_callbacks_demo: pre_patch_callback: livepatch_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[   96.950416] livepatch: pre-patch callback failed for object 'livepatch_callbacks_mod'
[   96.951424] livepatch: patch 'livepatch_callbacks_demo' failed for module 'livepatch_callbacks_mod', refusing to load module 'livepatch_callbacks_mod'
[   96.966586] insmod: ERROR: could not insert module samples/livepatch/livepatch-callbacks-mod.ko: No such device
[   98.968923] % echo 0 > /sys/kernel/livepatch/livepatch_callbacks_demo/enabled
[   98.970179] livepatch: 'livepatch_callbacks_demo': unpatching...
[  100.703101] livepatch: 'livepatch_callbacks_demo': unpatching complete
[  100.704057] livepatch_callbacks_demo: pre_unpatch_callback: vmlinux
[  100.704898] livepatch_callbacks_demo: post_unpatch_callback: vmlinux
[  100.972541] % rmmod samples/livepatch/livepatch-callbacks-mod.ko
[  100.975462] rmmod: ERROR: Module livepatch_callbacks_mod is not currently loaded
[  102.977392] % rmmod samples/livepatch/livepatch-callbacks-demo.ko

Part 1: Livepatch is loaded first, so vmlinux callbacks run

Part 2: The livepatch's pre-patch-callback is setup to now return
-ENODEV

Part 3: When a targetted module is loaded, the pre-patch-callback
returns -ENODEV and the target module fails to load.

Part 4: The livepatch is disabled and only the vmlinux unpatch-callbacks
are executed.

Note: this test should be consistent with the other test which fails a
pre-patch-callback status... ie, the post-unpatch-callback behavior for
said klp_object should be the same.

--

Joe Lawrence (1):
  livepatch: add (un)patch callbacks

 Documentation/livepatch/callbacks.txt        |  87 ++++++++++++
 include/linux/livepatch.h                    |  81 ++++++++++++
 kernel/livepatch/core.c                      |  37 ++++--
 kernel/livepatch/patch.c                     |   5 +-
 kernel/livepatch/transition.c                |  21 ++-
 samples/livepatch/Makefile                   |   2 +
 samples/livepatch/livepatch-callbacks-demo.c | 190 +++++++++++++++++++++++++++
 samples/livepatch/livepatch-callbacks-mod.c  |  53 ++++++++
 8 files changed, 462 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/livepatch/callbacks.txt
 create mode 100644 samples/livepatch/livepatch-callbacks-demo.c
 create mode 100644 samples/livepatch/livepatch-callbacks-mod.c

-- 
1.8.3.1

[toc] | [next] | [standalone]

#1713292

From	Josh Poimboeuf <jpoimboe@redhat.com>
Date	2017-08-16 22:30 +0200
Message-ID	<ufd6V-3gr-1@gated-at.bofh.it>
In reply to	#1713246

On Wed, Aug 16, 2017 at 03:17:03PM -0400, Joe Lawrence wrote:
> v3:
> 
> - livepatch.h
>   - drop obj->patched checks from pre/post-(un)patch funcs,
>     add preceding comment and note about obj->patched assumptions
>   - move core.c :: klp_is_module() to here
> 
> - klp_complete_transition()
>   - fix "else if (klp_target_state == KLP_UNPATCHED)" case
>   - combine conditional syntax when avoiding module_put for immediate
>     patches
>   - add check for klp_is_object_loaded to avoid callbacks for any
>     unloaded modules (necessary after removing obj->patched checks in
>     livepatch.h)
> 
> - Documentation
>   - added Josh's use-cases blurb in intro
>   - s/Callbacks are only executed/A callbacks is only executed/
> 
> - livepatch-callbacks-demo.c
>   - whitespace cleanup
> 
> I also wrote a quick test script (see below) to exercise some of the
> load/unload/enable/disable/error status combinations.  I'm not sure
> about some of the behaviors, most notably test6 with regard to
> post-unpatch-callbacks as executed on a cancelled transition.  (See
> results and comments further below.)

Yeah, that doesn't seem right.  Maybe in case of a pre-patch callback
error, we should only call post-unpatch callbacks for those objects
whose pre-patch callbacks were successfully called (and returned zero).
That would mean tracking on a per-object basis which objects had their
pre-patch callbacks called (successfully).

That would give the patch module a post-unpatch chance to tear down
anything it had set up in the pre-patch callback.

And the behavior should be documented.

> Also, maybe it's just my reading of the log, but would it be clearer if
> the "(un)patching ... complete" messages indicated that they are
> referring to a transaction?  It's a bit confusing to see "unpatching ...
> complete" before the pre-unpatch-callbacks ever execute.  Not a big
> deal, but I can send a follow up patch if others agree.

Hm.  I'm thinking this highlights the fact that the pre-unpatch callback
is being called in the wrong place.  It should actually be called before
the unpatching transition starts.  When called from
klp_unpatch_objects(), the new code is no longer running, so it's
effectively post-patch instead of pre-patch.

Another random thought: maybe we should show the "patching complete"
message *after* the post-patch callback is run.  That would be more
honest with the user, as technically, the post-patch callback is part of
the patching process.

And a similar comment for the "unpatching complete" message.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1715111

From	Petr Mladek <pmladek@suse.com>
Date	2017-08-18 16:10 +0200
Message-ID	<ufQ8j-4Qn-33@gated-at.bofh.it>
In reply to	#1713292

On Wed 2017-08-16 15:20:32, Josh Poimboeuf wrote:
> On Wed, Aug 16, 2017 at 03:17:03PM -0400, Joe Lawrence wrote:
> > I also wrote a quick test script (see below) to exercise some of the
> > load/unload/enable/disable/error status combinations.  I'm not sure
> > about some of the behaviors, most notably test6 with regard to
> > post-unpatch-callbacks as executed on a cancelled transition.  (See
> > results and comments further below.)
> 
> Yeah, that doesn't seem right.  Maybe in case of a pre-patch callback
> error, we should only call post-unpatch callbacks for those objects
> whose pre-patch callbacks were successfully called (and returned zero).
> That would mean tracking on a per-object basis which objects had their
> pre-patch callbacks called (successfully).
> 
> That would give the patch module a post-unpatch chance to tear down
> anything it had set up in the pre-patch callback.
> 
> And the behavior should be documented.

All this makes sense.

> > Also, maybe it's just my reading of the log, but would it be clearer if
> > the "(un)patching ... complete" messages indicated that they are
> > referring to a transaction?  It's a bit confusing to see "unpatching ...
> > complete" before the pre-unpatch-callbacks ever execute.  Not a big
> > deal, but I can send a follow up patch if others agree.
> 
> Hm.  I'm thinking this highlights the fact that the pre-unpatch callback
> is being called in the wrong place.  It should actually be called before
> the unpatching transition starts.  When called from
> klp_unpatch_objects(), the new code is no longer running, so it's
> effectively post-patch instead of pre-patch.
> 
> Another random thought: maybe we should show the "patching complete"
> message *after* the post-patch callback is run.  That would be more
> honest with the user, as technically, the post-patch callback is part of
> the patching process.
> 
> And a similar comment for the "unpatching complete" message.

Makes sense as well.

Best Regards,
Petr

[toc] | [prev] | [standalone]

csiph-web

[PATCH v3] add (un)patch callbacks

Contents

#1713246 — [PATCH v3] add (un)patch callbacks

#1713292

#1715111